Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-12-06T01:44:47Z

Wchen37: /* Code Modifications ( Work Completed) */

== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

==Problem Statement==
*Provide extensive comments for all the code you view/refactor/modify.
*Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

*Address and test effects of add_signup_topics_staggered
:* Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
:* Currently in RSpec, it is only implicitly tested in the method create.

*Refactor participants variable in load_add_signup_topics
:* [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]

*There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
:* Rename the attribute.

*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb

* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.

==Prior Work section with last years and last projects background==
* [[CSC/ECE 517 Fall 2022 - E2259. Refactor signup_sheet_controller]]

: We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:

:* Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

:* Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.

:* Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().

:* Removed load_add_signup_topics() since it does nothing for add_signup_topics().

:* Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.

:* List method was too long and has been refactored.

: We have also added two new test cases for the controller:

:* Destroy signup_sheet when there are other topics in the assignment
:* General test case added for rendering list view

* [[CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb]]
:* DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
:* The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
:* Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to create/delete new topics for an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== General Design Goals ==

* Remove violations of Single Responsibility principle by refactoring the design of the controller.

* Magic Tricks of Testing
:* Test incoming command messages by making assertions about direct public side effects.

* Increase Cohesion and Minimize Coupling

* DRY out the code wherever possible

* Improve code clarity by naming methods and variables appropriately

* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable

==Plan of Work==

* '''Controller Re-design'''

The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -

[[File:SUPCLASS.PNG|300px|Image: 300 pixels]]

[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]]

The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

* '''@Participants variable'''

References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.

[[File:participantsVar.png]]

There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.

def delete_signup
participant = AssignmentParticipant.find(params[:id])
...
...
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
...

* '''Increase Test Coverage'''
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.

*''' Address and test effects of add_signup_topics_staggered '''
:* Work done
::* RSpec
:::* When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.

context 'when topic can be found' do
context 'when assignment.staggered_deadline is True' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
context 'when assignment.staggered_deadline is False' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
end

:* Plan
::* RSpec
:::* Test add_signup_topics explicitly with different assignments as the input argument.
::* Mandatory
:::* Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.

* '''Assignment is_intelligent attribute is a source of confusion'''

There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It
simply checks for whether bidding is being used to assign topics.

Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose.
If this attribute were to be renamed, it would involve a lot of changes across the project -

:* The schema for Assignment would have to be updated
t.boolean "is_intelligent"
:* All creation of Assignment objects across the code and specs would have to be changed

context 'when current assignment is not intelligent assignment' do
it 'renders signup_sheet#list page' do
assignment.is_intelligent = false
allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)])
request_params = { id: 1 }
get :list, params: request_params
expect(response).to render_template(:list)
end
end
:* Existing test or sample DB data for Assignments would also have to be updated accordingly.

:* Views including tree_display would also be affected as they contain checks like this -

(props) =>
props.is_intelligent
? {
title: 'Intelligent Assignment',
href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`,
src: '/assets/tree_view/run-lottery.png'
}

* ''' Code Climate Issues '''
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#

Some issues observed on Code Climate for this controller -
:* Class SignUpSheetController has 32 methods (exceeds 20 allowed).
:* Class has too many lines. [393/100]

We plan to move some methods that are not directly involves in CRUD to the helper modules:
update_waitlist
compute_signed_up_topics
add_signup_topics,
set_values_for_new_topic,
set_priority,
switch_original_topic_to_approved_suggested_topic

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
:* rspec spec/controllers/signup_sheet_controller_spec.rb
: We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
:* rspec spec/helpers/signup_sheet_helper_spec.rb
:* rspec spec/models/signup_sheet_spec.rb

[[File:F22_Signup_rspec.png]]

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Add New Topic or Delete topic using links in the form below

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

; Verify setting up staggered deadlines of signup topics in assignments
# Login as instructor
# Hover on "Manage" Tab
# Click on "Assignments" Tab
# Click on Edit Button for "Wikipedia contribution" assignment
# Select "Staggered deadline assignnment?" checkbox
# Click "OK" at the popping up dialog
# Click "Save" button
# Click on "Topics" Tab
# Scroll down to the bottom and click "Show start/due date"
# Find out 2 topics, which are selected by at least 1 student, respectively
# Setup different deadlines for them and make sure one topic is in submission stage and the other one is in review stage
# Click "Save" button
# The updates should be saved without any error
# Sign in as one of the students for each topic, you should see different stages of the assignment "Wikipedia contribution" in the assignment list

== Code Modifications ( Work Completed) ==

* '''Is_intelligent (Swetha)'''

---------------------------------------------------
* '''Participants variable'''
References to participants (plural) have been replaced with @team_members for clarity, consistency as team_members was used in a similar fashion elsewhere in the controller and to avoid clashing with @participant (singular), which is used in many areas.

Example Old:
<% @participants = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %>
Example New:
<% @team_members = SignedUpTeam.find_team_participants(@assignment.id, session[:ip]) %>

Commit : https://github.com/weichic-ncsu/expertiza/commit/7b8a230f8d0ddb3953ec3a9db8506caa2bb691e4
---------------------------------------------------

* '''Staggered Deadlines of Topics in Assignment'''

:* Added back views rendered for staggered deadlines in the signup sheet

:* Fixed controller error of this feature

:* Tested this feature with demo video

Example Old:

<%= form_tag controller: 'assignment', action: 'edit', assignment_id: @assignment.id do %>
<% end %>

Example New:

<%= form_tag controller: 'assignments', action: 'edit', assignment_id: @assignment.id do %>
<% end %>

Commits:

# https://github.com/weichic-ncsu/expertiza/commit/6dd51b6629ac020d6dd65ef9994a638041106f87
# https://github.com/weichic-ncsu/expertiza/commit/2853eaf37f36f6e1291a14faaeca715bcbfe15e8
# https://github.com/weichic-ncsu/expertiza/commit/e38c3cba2625c613b836f056273492c53f9bbad8

---------------------------------------------------

* '''Add_signup_topics'''

Previously we encountered an error page when attempting to create a topic with the same name as an existing topic. The functionality was set up to update the attributes of the existing topic and then redirect the user to add_signup_topics. However this route was broken ( from an error in a different controller class) and the functionality was very unclear from reading the code.

We have changed this so that when the user attempts to create a new topic with the same name as an existing topic, the topic attributes are updated as before, but the user is instead redirected back to the topics tab of the assignment edit page.

Old ( in def update_existing_topic )
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
New:
topic.update_attributes(topic_params)
redirect_to edit_assignment_path(params[:id], anchor: 'tabs-2')

With this change we were also able to remove the below method:

def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id) :
(redirect_to action: 'add_signup_topics', id: assignment_id)
end

We also updated the corresponding rspec tests to reflect the change:

old:
expect(response).to redirect_to('/signup_sheet/add_signup_topics?id=1')
new:
expect(response).to redirect_to('/assignments/1/edit#tabs-2')

commits:

https://github.com/weichic-ncsu/expertiza/commit/5dec776b7355290efde7e2249de83d89fd0dd259

https://github.com/weichic-ncsu/expertiza/commit/8ab175b02cf825f5f414e04e33f1790da0f1bd93

https://github.com/weichic-ncsu/expertiza/commit/1dcd1b8107657fbeaaea60a586e6705dc0cf3404
---------------------------------------------------

* '''Methods removed (Swetha)'''
---------------------------------------------------

* '''Methods moved (Swetha)'''

==Relevant Links==
*https://github.com/weichic-ncsu/expertiza
*Link to Pull Request
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-12-05T22:52:07Z

Wchen37: /* Code Modifications ( Work Completed) */

== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

==Problem Statement==
*Provide extensive comments for all the code you view/refactor/modify.
*Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

*Address and test effects of add_signup_topics_staggered
:* Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
:* Currently in RSpec, it is only implicitly tested in the method create.

*Refactor participants variable in load_add_signup_topics
:* [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]

*There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
:* Rename the attribute.

*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb

* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.

==Prior Work section with last years and last projects background==
* [[CSC/ECE 517 Fall 2022 - E2259. Refactor signup_sheet_controller]]

: We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:

:* Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

:* Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.

:* Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().

:* Removed load_add_signup_topics() since it does nothing for add_signup_topics().

:* Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.

:* List method was too long and has been refactored.

: We have also added two new test cases for the controller:

:* Destroy signup_sheet when there are other topics in the assignment
:* General test case added for rendering list view

* [[CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb]]
:* DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
:* The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
:* Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to create/delete new topics for an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== General Design Goals ==

* Remove violations of Single Responsibility principle by refactoring the design of the controller.

* Magic Tricks of Testing
:* Test incoming command messages by making assertions about direct public side effects.

* Increase Cohesion and Minimize Coupling

* DRY out the code wherever possible

* Improve code clarity by naming methods and variables appropriately

* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable

==Plan of Work==

* '''Controller Re-design'''

The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -

[[File:SUPCLASS.PNG|300px|Image: 300 pixels]]

[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]]

The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

* '''@Participants variable'''

References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.

[[File:participantsVar.png]]

There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.

def delete_signup
participant = AssignmentParticipant.find(params[:id])
...
...
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
...

* '''Increase Test Coverage'''
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.

*''' Address and test effects of add_signup_topics_staggered '''
:* Work done
::* RSpec
:::* When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.

context 'when topic can be found' do
context 'when assignment.staggered_deadline is True' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
context 'when assignment.staggered_deadline is False' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
end

:* Plan
::* RSpec
:::* Test add_signup_topics explicitly with different assignments as the input argument.
::* Mandatory
:::* Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.

* '''Assignment is_intelligent attribute is a source of confusion'''

There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It
simply checks for whether bidding is being used to assign topics.

Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose.
If this attribute were to be renamed, it would involve a lot of changes across the project -

:* The schema for Assignment would have to be updated
t.boolean "is_intelligent"
:* All creation of Assignment objects across the code and specs would have to be changed

context 'when current assignment is not intelligent assignment' do
it 'renders signup_sheet#list page' do
assignment.is_intelligent = false
allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)])
request_params = { id: 1 }
get :list, params: request_params
expect(response).to render_template(:list)
end
end
:* Existing test or sample DB data for Assignments would also have to be updated accordingly.

:* Views including tree_display would also be affected as they contain checks like this -

(props) =>
props.is_intelligent
? {
title: 'Intelligent Assignment',
href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`,
src: '/assets/tree_view/run-lottery.png'
}

* ''' Code Climate Issues '''
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#

Some issues observed on Code Climate for this controller -
:* Class SignUpSheetController has 32 methods (exceeds 20 allowed).
:* Class has too many lines. [393/100]

We plan to move some methods that are not directly involves in CRUD to the helper modules:
update_waitlist
compute_signed_up_topics
add_signup_topics,
set_values_for_new_topic,
set_priority,
switch_original_topic_to_approved_suggested_topic

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
:* rspec spec/controllers/signup_sheet_controller_spec.rb
: We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
:* rspec spec/helpers/signup_sheet_helper_spec.rb
:* rspec spec/models/signup_sheet_spec.rb

[[File:F22_Signup_rspec.png]]

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Add New Topic or Delete topic using links in the form below

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

; Verify setting up staggered deadlines of signup topics in assignments
# Login as instructor
# Hover on "Manage" Tab
# Click on "Assignments" Tab
# Click on Edit Button for "Wikipedia contribution" assignment
# Select "Staggered deadline assignnment?" checkbox
# Click "OK" at the popping up dialog
# Click "Save" button
# Click on "Topics" Tab
# Scroll down to the bottom and click "Show start/due date"
# Find out 2 topics, which are selected by at least 1 student, respectively
# Setup different deadlines for them and make sure one topic is in submission stage and the other one is in review stage
# Click "Save" button
# The updates should be saved without any error
# Sign in as one of the students for each topic, you should see different stages of the assignment "Wikipedia contribution" in the assignment list

== Code Modifications ( Work Completed) ==

* Is_intelligent (Swetha)

* Participants variable (Juan)

* Staggered Deadlines of Topics in Assignment

:* Added back views rendered for staggered deadlines in the signup sheet

:* Fixed controller error of this feature

:* Tested this feature with demo video

* Add_signup_topics (Juan)

* Methods removed (Swetha)

* Methods moved (Swetha)

==Relevant Links==
*https://github.com/weichic-ncsu/expertiza
*Link to Pull Request
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-12-04T20:38:54Z

Wchen37: /* Mandatory */

== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

==Problem Statement==
*Provide extensive comments for all the code you view/refactor/modify.
*Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

*Address and test effects of add_signup_topics_staggered
:* Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
:* Currently in RSpec, it is only implicitly tested in the method create.

*Refactor participants variable in load_add_signup_topics
:* [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]

*There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
:* Rename the attribute.

*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb

* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.

==Prior Work section with last years and last projects background==
* [[CSC/ECE 517 Fall 2022 - E2259. Refactor signup_sheet_controller]]

: We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:

:* Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

:* Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.

:* Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().

:* Removed load_add_signup_topics() since it does nothing for add_signup_topics().

:* Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.

:* List method was too long and has been refactored.

: We have also added two new test cases for the controller:

:* Destroy signup_sheet when there are other topics in the assignment
:* General test case added for rendering list view

* [[CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb]]
:* DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
:* The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
:* Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to create/delete new topics for an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== General Design Goals ==

* Remove violations of Single Responsibility principle by refactoring the design of the controller.

* Magic Tricks of Testing
:* Test incoming command messages by making assertions about direct public side effects.

* Increase Cohesion and Minimize Coupling

* DRY out the code wherever possible

* Improve code clarity by naming methods and variables appropriately

* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable

==Plan of Work==

* '''Controller Re-design'''

The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -

[[File:SUPCLASS.PNG|300px|Image: 300 pixels]]

[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]]

The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

* '''@Participants variable'''

References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.

[[File:participantsVar.png]]

There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.

def delete_signup
participant = AssignmentParticipant.find(params[:id])
...
...
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
...

* '''Increase Test Coverage'''
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.

*''' Address and test effects of add_signup_topics_staggered '''
:* Work done
::* RSpec
:::* When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.

context 'when topic can be found' do
context 'when assignment.staggered_deadline is True' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
context 'when assignment.staggered_deadline is False' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
end

:* Plan
::* RSpec
:::* Test add_signup_topics explicitly with different assignments as the input argument.
::* Mandatory
:::* Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.

* '''Assignment is_intelligent attribute is a source of confusion'''

There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It
simply checks for whether bidding is being used to assign topics.

Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose.
If this attribute were to be renamed, it would involve a lot of changes across the project -

:* The schema for Assignment would have to be updated
t.boolean "is_intelligent"
:* All creation of Assignment objects across the code and specs would have to be changed

context 'when current assignment is not intelligent assignment' do
it 'renders signup_sheet#list page' do
assignment.is_intelligent = false
allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)])
request_params = { id: 1 }
get :list, params: request_params
expect(response).to render_template(:list)
end
end
:* Existing test or sample DB data for Assignments would also have to be updated accordingly.

:* Views including tree_display would also be affected as they contain checks like this -

(props) =>
props.is_intelligent
? {
title: 'Intelligent Assignment',
href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`,
src: '/assets/tree_view/run-lottery.png'
}

* ''' Code Climate Issues '''
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#

Some issues observed on Code Climate for this controller -
:* Class SignUpSheetController has 32 methods (exceeds 20 allowed).
:* Class has too many lines. [393/100]

We plan to move some methods that are not directly involves in CRUD to the helper modules:
update_waitlist
compute_signed_up_topics
add_signup_topics,
set_values_for_new_topic,
set_priority,
switch_original_topic_to_approved_suggested_topic

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
:* rspec spec/controllers/signup_sheet_controller_spec.rb
: We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
:* rspec spec/helpers/signup_sheet_helper_spec.rb
:* rspec spec/models/signup_sheet_spec.rb

[[File:F22_Signup_rspec.png]]

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Add New Topic or Delete topic using links in the form below

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

; Verify setting up staggered deadlines of signup topics in assignments
# Login as instructor
# Hover on "Manage" Tab
# Click on "Assignments" Tab
# Click on Edit Button for "Wikipedia contribution" assignment
# Select "Staggered deadline assignnment?" checkbox
# Click "OK" at the popping up dialog
# Click "Save" button
# Click on "Topics" Tab
# Scroll down to the bottom and click "Show start/due date"
# Find out 2 topics, which are selected by at least 1 student, respectively
# Setup different deadlines for them and make sure one topic is in submission stage and the other one is in review stage
# Click "Save" button
# The updates should be saved without any error
# Sign in as one of the students for each topic, you should see different stages of the assignment "Wikipedia contribution" in the assignment list

== Code Modifications==

==Relevant Links==
*https://github.com/weichic-ncsu/expertiza
*Link to Pull Request
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-12-01T20:01:05Z

Wchen37: /* Mandatory */

== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

==Problem Statement==
*Provide extensive comments for all the code you view/refactor/modify.
*Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

*Address and test effects of add_signup_topics_staggered
:* Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
:* Currently in RSpec, it is only implicitly tested in the method create.

*Refactor participants variable in load_add_signup_topics
:* [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]

*There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
:* Rename the attribute.

*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb

* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.

==Prior Work section with last years and last projects background==
* [[CSC/ECE 517 Fall 2022 - E2259. Refactor signup_sheet_controller]]

: We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:

:* Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

:* Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.

:* Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().

:* Removed load_add_signup_topics() since it does nothing for add_signup_topics().

:* Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.

:* List method was too long and has been refactored.

: We have also added two new test cases for the controller:

:* Destroy signup_sheet when there are other topics in the assignment
:* General test case added for rendering list view

* [[CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb]]
:* DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
:* The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
:* Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to create/delete new topics for an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== General Design Goals ==

* Remove violations of Single Responsibility principle by refactoring the design of the controller.

* Magic Tricks of Testing
:* Test incoming command messages by making assertions about direct public side effects.

* Increase Cohesion and Minimize Coupling

* DRY out the code wherever possible

* Improve code clarity by naming methods and variables appropriately

* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable

==Plan of Work==

* '''Controller Re-design'''

The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -

[[File:SUPCLASS.PNG|300px|Image: 300 pixels]]

[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]]

The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

* '''@Participants variable'''

References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.

[[File:participantsVar.png]]

There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.

def delete_signup
participant = AssignmentParticipant.find(params[:id])
...
...
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
...

* '''Increase Test Coverage'''
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.

*''' Address and test effects of add_signup_topics_staggered '''
:* Work done
::* RSpec
:::* When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.

context 'when topic can be found' do
context 'when assignment.staggered_deadline is True' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
context 'when assignment.staggered_deadline is False' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
end

:* Plan
::* RSpec
:::* Test add_signup_topics explicitly with different assignments as the input argument.
::* Mandatory
:::* Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.

* '''Assignment is_intelligent attribute is a source of confusion'''

There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It
simply checks for whether bidding is being used to assign topics.

Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose.
If this attribute were to be renamed, it would involve a lot of changes across the project -

:* The schema for Assignment would have to be updated
t.boolean "is_intelligent"
:* All creation of Assignment objects across the code and specs would have to be changed

context 'when current assignment is not intelligent assignment' do
it 'renders signup_sheet#list page' do
assignment.is_intelligent = false
allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)])
request_params = { id: 1 }
get :list, params: request_params
expect(response).to render_template(:list)
end
end
:* Existing test or sample DB data for Assignments would also have to be updated accordingly.

:* Views including tree_display would also be affected as they contain checks like this -

(props) =>
props.is_intelligent
? {
title: 'Intelligent Assignment',
href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`,
src: '/assets/tree_view/run-lottery.png'
}

* ''' Code Climate Issues '''
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#

Some issues observed on Code Climate for this controller -
:* Class SignUpSheetController has 32 methods (exceeds 20 allowed).
:* Class has too many lines. [393/100]

We plan to move some methods that are not directly involves in CRUD to the helper modules:
update_waitlist
compute_signed_up_topics
add_signup_topics,
set_values_for_new_topic,
set_priority,
switch_original_topic_to_approved_suggested_topic

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
:* rspec spec/controllers/signup_sheet_controller_spec.rb
: We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
:* rspec spec/helpers/signup_sheet_helper_spec.rb
:* rspec spec/models/signup_sheet_spec.rb

[[File:F22_Signup_rspec.png]]

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Add New Topic or Delete topic using links in the form below

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

; Verify setting up staggered deadlines of signup topics in assignments
# Login as instructor
# Hover on "Manage" Tab
# Click on "Assignments" Tab
# Click on Edit Button for "Wikipedia contribution" assignment
# Select "Staggered deadline assignnment?" checkbox
# Click "OK" at the popping up dialog
# Click "Save" button
# Click on "Topics" Tab
# Scroll down to the bottom and click "Show start/due date"
# Setup submission deadlines and review deadlines for at least 2 topics, and make sure the deadlines in these 2 topics are different
# Click "Save" button
# The updates should be saved without any error

Normal signup topics
[[File:Topics_No_Staggered.png]]

Set staggered deadlines for topics
[[File:Topics_Set_Staggered.png]]
[[File:Topics_Staggered.png]]
[[File:Topics_Staggered_Set_Due_Date.png]]

Results
[[File:Set_Staggered.png]]

== Code Modifications==

==Relevant Links==
*https://github.com/weichic-ncsu/expertiza
*Link to Pull Request
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

File:Set Staggered.png

2022-12-01T19:56:05Z

Wchen37:

File:Topics Staggered Set Due Date.png

2022-12-01T19:55:53Z

Wchen37:

File:Topics Staggered.png

2022-12-01T19:55:39Z

Wchen37:

File:Topics Set Staggered.png

2022-12-01T19:55:17Z

Wchen37:

File:Topics No Staggered.png

2022-12-01T19:54:49Z

Wchen37:

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-12-01T19:54:02Z

Wchen37: /* Mandatory */

== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

==Problem Statement==
*Provide extensive comments for all the code you view/refactor/modify.
*Ensure that any code changes do not break any functionality. Refactoring method names might cause cascaded updates in other files.

*Address and test effects of add_signup_topics_staggered
:* Add_signup_topics_staggered was supposed to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]. However, it was removed in the last version since it did not do anything different from add_signup_topics.
:* Currently in RSpec, it is only implicitly tested in the method create.

*Refactor participants variable in load_add_signup_topics
:* [In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.] [Has this been fixed?]

*There are nine checks for @assignment.is_intelligent. This attribute is atrociously named. It simply checks for whether bidding is being used to assign topics.
:* Rename the attribute.

*Also see Code Climate issues. To reduce the size of the file, you could move some methods to sign_up_sheet_helper.rb

* The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller. It is also indicative of poor class design and violation of the Single Responsibility Principle.

==Prior Work section with last years and last projects background==
* [[CSC/ECE 517 Fall 2022 - E2259. Refactor signup_sheet_controller]]

: We ( @sswetha08 @weichic-ncsu @juanbenavid ) have performed the following refactoring to fix the issues specified for this refactoring project:

:* Renamed "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

:* Update method refactored by removing explicit update of every attribute of topic and instead performing this task through update_attributes function provided by rails. Further, update_max_choosers method changed to update_waitlist to support this.

:* Removed add_signup_topics_staggered() and replaced it with add_signup_topics() since it does nothing but call add_signup_topics().

:* Removed load_add_signup_topics() since it does nothing for add_signup_topics().

:* Removed the feature to sign up for a topic as an instructor as there seems to be no way to access this feature from the views.

:* List method was too long and has been refactored.

: We have also added two new test cases for the controller:

:* Destroy signup_sheet when there are other topics in the assignment
:* General test case added for rendering list view

* [[CSC/ECE 517 Spring 2022 - E2223. Refactor sign up sheet controller.rb]]
:* DRYed out code used to check for certain errors; however, that was done using an array of messages and then selecting one of them using a constant subscript, whereas it would have been better to pass the messages as parameters.
:* The compute_signup_topics method returns results by side-effects (in instance variables); probably this could have been done more cleanly.
:* Most of the changes involve regularizing use of "sign_up" vs. "signup"; this should ideally be done in a separate commit so it would be mergeable without merging the whole project.

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to create/delete new topics for an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== General Design Goals ==

* Remove violations of Single Responsibility principle by refactoring the design of the controller.

* Magic Tricks of Testing
:* Test incoming command messages by making assertions about direct public side effects.

* Increase Cohesion and Minimize Coupling

* DRY out the code wherever possible

* Improve code clarity by naming methods and variables appropriately

* Improve code clarity of the controller by moving non-CRUD methods to helper functions wherever applicable

==Plan of Work==

* '''Controller Re-design'''

The signup_sheet controller currently has 30 methods, which is a violation of the upper limit for methods in a controller.Upon analysis, we found that signup_sheet controller has methods related to creating/updating and deleting topics as well as methods to create and delete a sign-up for those topics in an assignment. This is certainly a violation of the Single Responsibility principle as this controller handles responsibilties of both topic CRUD and signup CRUD operations. To fix this, we have proposed to split this controller and create a new controller for Topics to manage its responsibilities. We plan to split up the methods as per below Class Diagram -

[[File:SUPCLASS.PNG|300px|Image: 300 pixels]]

[[File:SUPCLASSNEW.PNG|500px|Image: 500 pixels]]

The new design follows '''Single Responsibility''', and the methods were split up on the basis of '''Information Expert''' pattern. Using the principle of Information Expert, a general approach to assigning responsibilities is to look at a given responsibility, determine the information needed to fulfill it, and then determine where that information is stored.

* '''@Participants variable'''

References to @participants is scattered throughout the project, including some references in signup sheet views. This variable was removed in the signup sheet controller in the previous pull request for the Fall 2022 OSS project, but references in views have not been removed. In the OSS project we noticed that test coverage for some of the methods/views in this controller were incomplete, so we should make sure the removal of this variable does not cause problems. The naming could also use improvement, as only teams are allowed to signup for projects.

[[File:participantsVar.png]]

There is also naming confusion because the controller has many references to the (singular) participant variable, and it is not clear how the two are supposed to be different. So better comments/naming is important.

def delete_signup
participant = AssignmentParticipant.find(params[:id])
...
...
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
...

* '''Increase Test Coverage'''
We noticed that in general the controller could use more/better rspec tests as some error pages we encountered while manually testing were not captured by the current rspec. Further, new tests will have to added/ old tests will have to be repurposed to check the newly designed Topic controller.

*''' Address and test effects of add_signup_topics_staggered '''
:* Work done
::* RSpec
:::* When creating a topic which could be found, test the availability in different values of assignment.staggered_deadline.

context 'when topic can be found' do
context 'when assignment.staggered_deadline is True' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
context 'when assignment.staggered_deadline is False' do
it 'updates the existing topic and redirects to signup_sheet#add_signup_topics page' do
...
end
end
end

:* Plan
::* RSpec
:::* Test add_signup_topics explicitly with different assignments as the input argument.
::* Mandatory
:::* Test the route from app/views/assignments/edit/_topics.html.erb with different assignment_form.

* '''Assignment is_intelligent attribute is a source of confusion'''

There are several checks for @assignment.is_intelligent across the controllers and views. This attribute is atrociously named. It
simply checks for whether bidding is being used to assign topics.

Proposed changes include renaming the attribute to bid_for_topics which more accurately captures its purpose.
If this attribute were to be renamed, it would involve a lot of changes across the project -

:* The schema for Assignment would have to be updated
t.boolean "is_intelligent"
:* All creation of Assignment objects across the code and specs would have to be changed

context 'when current assignment is not intelligent assignment' do
it 'renders signup_sheet#list page' do
assignment.is_intelligent = false
allow(Bid).to receive(:where).with(team_id: 1).and_return([double('Bid', topic_id: 1)])
request_params = { id: 1 }
get :list, params: request_params
expect(response).to render_template(:list)
end
end
:* Existing test or sample DB data for Assignments would also have to be updated accordingly.

:* Views including tree_display would also be affected as they contain checks like this -

(props) =>
props.is_intelligent
? {
title: 'Intelligent Assignment',
href: '/lottery/run_intelligent_assignment/' + `${parseInt(props.id) / 2}`,
src: '/assets/tree_view/run-lottery.png'
}

* ''' Code Climate Issues '''
https://codeclimate.com/github/expertiza/expertiza/app/controllers/sign_up_sheet_controller.rb#

Some issues observed on Code Climate for this controller -
:* Class SignUpSheetController has 32 methods (exceeds 20 allowed).
:* Class has too many lines. [393/100]

We plan to move some methods that are not directly involves in CRUD to the helper modules:
update_waitlist
compute_signed_up_topics
add_signup_topics,
set_values_for_new_topic,
set_priority,
switch_original_topic_to_approved_suggested_topic

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
:* rspec spec/controllers/signup_sheet_controller_spec.rb
: We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
:* rspec spec/helpers/signup_sheet_helper_spec.rb
:* rspec spec/models/signup_sheet_spec.rb

[[File:F22_Signup_rspec.png]]

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Add New Topic or Delete topic using links in the form below

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

; Verify setting up staggered deadlines of signup topics in assignments
# Login as instructor
# Hover on "Manage" Tab
# Click on "Assignments" Tab
# Click on Edit Button for "Wikipedia contribution" assignment
# Select "Staggered deadline assignnment?" checkbox
# Click "OK" at the popping up dialog
# Click "Save" button
# Click on "Topics" Tab
# Scroll down to the bottom and click "Show start/due date"
# Setup submission deadlines and review deadlines for at least 2 topics, and make sure the deadlines in these 2 topics are different
# Click "Save" button
# The updates should be saved without any error

== Code Modifications==

==Relevant Links==
*https://github.com/weichic-ncsu/expertiza
*Link to Pull Request
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-16T00:11:41Z

Wchen37: /* Plan of Work */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-16T00:06:27Z

Wchen37: /* Plan of Work */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-16T00:02:50Z

Wchen37: /* Plan of Work */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T11:05:34Z

Wchen37: /* Plan and List of Work done (specific tasks planned) */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T11:04:56Z

Wchen37: /* Prior Work section with last years and last projects background */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T11:01:18Z

Wchen37: /* RSpec */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T10:59:35Z

Wchen37: /* Plan and List of Work done (specific tasks planned) */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T10:38:53Z

Wchen37: /* General Design Goals */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T10:33:43Z

Wchen37: /* General Design Goals */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:43:10Z

Wchen37: /* Plan and List of Work done (specific tasks planned) */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:42:35Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:40:43Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:40:25Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:38:55Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:04:32Z

Wchen37: /* Prior Work section with last years and last projects background */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:02:06Z

Wchen37: /* Prior Work section with last years and last projects background */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:01:23Z

Wchen37: /* Prior Work section with last years and last projects background */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T09:00:24Z

Wchen37: /* Prior Work section with last years and last projects background */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T08:58:33Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-15T08:55:16Z

Wchen37: /* Problem Statement */

CSC/ECE 517 Fall 2022 - E2279. Further refactoring and improvement of signup sheet controller

2022-11-14T23:06:24Z

Wchen37:

==Problem Statement==
*Prior Work section with last years and last projects background
*Plan and List of Work done (General Design goals and specific tasks planned)
*Test Plan
*Testing Scenarios (Basic + Edge cases)
*RSpec Testing
*Manual Testing
*Modifications (probably after working on them)
*Team
*Members
*Mentor
*Relevant Links
*Link to GitHub Repo for our Expertiza
*Link to Pull Request
*Link to Expertiza Server

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T08:20:02Z

Wchen37: /* Edge Cases and Pre-Conditions */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
# Destroy signup_sheet when other topics with the same assignment exists.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

; New test case
: Destroy signup_sheet when there are other topics in the assignment

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T08:18:58Z

Wchen37: /* RSpec */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

; New test case
: Destroy signup_sheet when there are other topics in the assignment

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T07:35:50Z

Wchen37: /* Test Plan */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Test Plan ==
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
:

=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T07:35:13Z

Wchen37: /* Test Plan */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Test Plan ==
=== RSpec ===
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

; Edge Cases and Pre-Conditions
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
:

=== Mandatory ===
; Login as instructor
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

; Login as student
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T07:33:20Z

Wchen37: /* Testing */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Test Plan ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-11-01T07:30:55Z

Wchen37: /* Edge Cases and Pre-Conditions */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
# Create signup_sheet when sign_up_topic cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-26T21:27:17Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2259 OSS assignment for Fall 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (ssairam)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project at http://152.7.99.122:8080/
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller performs the following functions:
* Allows an instructor to add/remove topics to an assignment.
* Allows an instructor to assign/remove students to topics (This has been removed as it is unused/impersonation of student is used instead)
* Allows a student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
*Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

1. Rename "sign_up" to be "signup" when it is used as adjective or noun in the names of variables/controller/helper functions.

'''Before changing variable names'''

def new
@id = params[:id]
@sign_up_topic = SignUpTopic.new
@sign_up_topic.assignment = Assignment.find(params[:id])
@topic = @sign_up_topic
end

def set_values_for_new_topic
@sign_up_topic = SignUpTopic.new
@sign_up_topic.topic_identifier = params[:topic][:topic_identifier]
@sign_up_topic.topic_name = params[:topic][:topic_name]
@sign_up_topic.max_choosers = params[:topic][:max_choosers]
@sign_up_topic.category = params[:topic][:category]
@sign_up_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''After changing variable names'''

def new
@id = params[:id]
@signup_topic = SignUpTopic.new
@signup_topic.assignment = Assignment.find(params[:id])
@topic = @signup_topic
end

def set_values_for_new_topic
@signup_topic = SignUpTopic.new
@signup_topic.topic_identifier = params[:topic][:topic_identifier]
@signup_topic.topic_name = params[:topic][:topic_name]
@signup_topic.max_choosers = params[:topic][:max_choosers]
@signup_topic.category = params[:topic][:category]
@signup_topic.assignment_id = params[:id]
@assignment = Assignment.find(params[:id])
end

'''Before changing controller name'''

class SignUpSheetController < ApplicationController
...
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''After changing controller name'''

class SignupSheetController < ApplicationController
...
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
...

'''Before changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_sign_up_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
sign_up_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

'''After changing helper function names'''

<%= link_to image_tag('signup.png', :border => 0, :title => 'Sign Up Student', :align => 'middle'),
signup_as_instructor_signup_sheet_index_path( assignment_id: params[:id], topic_id: topic.id) %>
...
<%= link_to image_tag('delete_icon.png', :border => 0, :title => 'Delete Topic', :align => 'middle'),
signup_sheet_path(:id=>topic.id, :assignment_id => params[:id]), method: :delete, confirm: 'Are you sure?' %>

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Update method has been refactored by removing explicit update of every attribute of topic and instead performing this task through
update_attributes function provided by rails. To filter out topic specific params from the hash, topic_params method has been defined.

Previous version of update:
def update
@topic = SignUpTopic.find(params[:id])
if @topic
@topic.topic_identifier = params[:topic][:topic_identifier]
update_max_choosers @topic
@topic.category = params[:topic][:category]
@topic.topic_name = params[:topic][:topic_name]
@topic.micropayment = params[:topic][:micropayment]
@topic.description = params[:topic][:description]
@topic.link = params[:topic][:link]
@topic.save
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Refactored update:

def update
@topic = SignUpTopic.find(params[:id])
if @topic
update_waitlist @topic
@topic.update_attributes(topic_params)
undo_link("The topic: \"#{@topic.topic_name}\" has been successfully updated. ")
else
flash[:error] = 'The topic could not be updated.'
end
# Akshay - correctly changing the redirection url to topics tab in edit assignment view.
redirect_to edit_assignment_path(params[:assignment_id]) + '#tabs-2'
end

Further, update_max_choosers method has been refactored so as to not set the value of max_choosers separately. It now simply checks whether the given value of max_choosers is valid and if yes, updates the waitlist accordingly. For instance, if max_choosers increase from 1 to 2, 1 team on the waitlist can be allotted the topic.The name of update_max_choosers has been changed to update_waitlist to reflect this change. update_existing_topic method also was refactored similar to update.

def update_existing_topic(topic)
update_waitlist topic
topic.update_attributes(topic_params)
redirect_to_sign_up(params[:id])
end

def update_waitlist(topic)
# While saving the max choosers you should be careful; if there are users who have signed up for this particular
# topic and are on waitlist, then they have to be converted to confirmed topic based on the availability. But if
# there are choosers already and if there is an attempt to decrease the max choosers, as of now I am not allowing
# it.
if SignedUpTeam.find_by(topic_id: topic.id).nil? || topic.max_choosers == topic_params[:max_choosers]
return
elsif topic.max_choosers.to_i < topic_params[:max_choosers].to_i
topic.update_waitlisted_users topic_params[:max_choosers]
else
flash[:error] = 'The value of the maximum number of choosers can only be increased! No change has been made to maximum choosers.'
end
end

https://github.com/weichic-ncsu/expertiza/commit/aab4361285e53f440bff1818ee8acce8c8a33648

3. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

'''Before'''

def add_signup_topics_staggered
add_signup_topics
end

# simple function that redirects ti the /add_signup_topics or the /add_signup_topics_staggered page depending on assignment type
# staggered means that different topics can have different deadlines.
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
assignment.staggered_deadline == true ? (redirect_to action: 'add_signup_topics_staggered', id: assignment_id)
: (redirect_to action: 'add_signup_topics', id: assignment_id)
end

'''After'''

# simple function that redirects ti the /add_signup_topics page
def redirect_to_sign_up(assignment_id)
assignment = Assignment.find(assignment_id)
redirect_to action: 'add_signup_topics', id: assignment_id
end

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

4. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

'''Before'''

def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

# retrieves all the data associated with the given assignment. Includes all topics,
def load_add_signup_topics(assignment_id)
@id = assignment_id
@sign_up_topics = SignUpTopic.where('assignment_id = ?', assignment_id)
@slots_filled = SignUpTopic.find_slots_filled(assignment_id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)

@assignment = Assignment.find(assignment_id)
# ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
# to treat all assignments as team assignments
# Though called participants, @participants are actually records in signed_up_teams table, which
# is a mapping table between teams and topics (waitlisted recorded are also counted)
@participants = SignedUpTeam.find_team_participants(assignment_id, session[:ip])
end

'''After'''
def add_signup_topics
load_add_signup_topics(params[:id])
SignUpSheet.add_signup_topic(params[:id])
end

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

5. The feature to sign up for a topic as an instructor has been removed as it didn't appear to have been used in many years. More importantly, there seems to be no way to access this feature from the views. Hence sign_up_as_instructor, sign_up_as_instructor_action and delete_sign_up_as_instructor methods have been removed along with its associated views and tests in the controller spec for signup_sheet.

https://github.com/weichic-ncsu/expertiza/commit/7b3f68ad5da1bc24e1f58a4c815aeb5985156aa8

https://github.com/weichic-ncsu/expertiza/commit/4d3baceb5a7135c89fad7660fd342eb49b2a20df

6. List method refactored by moving computation of sign_up topics to new method, using values directly in views rather than copying to instance variables, and new method for finding topic deadlines.

https://github.com/weichic-ncsu/expertiza/commit/4684f83e388d7605a742ac85ffc90917c5864dd5

https://github.com/weichic-ncsu/expertiza/commit/b5636bb109258c7cddb6d4199ad5141c14a5dda8

https://github.com/weichic-ncsu/expertiza/commit/ed995ba8d68c5d4767a80a555114a2dd40f9b131

https://github.com/weichic-ncsu/expertiza/commit/7d81153839fbd87783a1cdaee2b797b2b46c3aa5

https://github.com/weichic-ncsu/expertiza/commit/7f9720680a7d13db168de1009dcd54273a184286

''' Before '''
<pre>
def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
@sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@max_team_size = @assignment.max_team_size
team_id = @participant.team.try(:id)
@use_bookmark = @assignment.use_bookmark

if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
signed_up_topics = []
@bids.each do |bid|
sign_up_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << sign_up_topic if sign_up_topic
end
signed_up_topics &= @sign_up_topics
@sign_up_topics -= signed_up_topics
@bids = signed_up_topics
end

@num_of_topics = @sign_up_topics.size
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('sign_up_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end
</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @max_team_size) %>
...
</pre>

<pre>
...
<% if @use_bookmark %>
...
</pre>

''' After '''
<pre>
# method to return a list of topics for which a bid has been made by a team
def compute_signed_up_topics
signed_up_topics = []
@bids.each do |bid|
signup_topic = SignUpTopic.find_by(id: bid.topic_id)
signed_up_topics << signup_topic if signup_topic
end
signed_up_topics &= @signup_topics
return signed_up_topics
end

def find_topic_deadlines
@signup_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 7)
@drop_topic_deadline = @assignment.due_dates.find_by(deadline_type_id: 6)
end

def list
@participant = AssignmentParticipant.find(params[:id].to_i)
@assignment = @participant.assignment
@signup_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
@slots_filled = SignUpTopic.find_slots_filled(@assignment.id)
@slots_waitlisted = SignUpTopic.find_slots_waitlisted(@assignment.id)
@show_actions = true
@priority = 0
team_id = @participant.team.try(:id)

# if assignment is intelligent, want to know which topics the team has already bid on
if @assignment.is_intelligent
@bids = team_id.nil? ? [] : Bid.where(team_id: team_id).order(:priority)
@bids = compute_signed_up_topics()
@signup_topics -= @bids
end

@num_of_topics = @signup_topics.size
@student_bids = team_id.nil? ? [] : Bid.where(team_id: team_id)
find_topic_deadlines()

unless @assignment.due_dates.find_by(deadline_type_id: 1).nil?
@show_actions = false if !@assignment.staggered_deadline? && (@assignment.due_dates.find_by(deadline_type_id: 1).due_at < Time.now)

# Find whether the user has signed up for any topics; if so the user won't be able to
# sign up again unless the former was a waitlisted topic
# if team assignment, then team id needs to be passed as parameter else the user's id
users_team = SignedUpTeam.find_team_users(@assignment.id, session[:user].id)
@selected_topics = if users_team.empty?
nil
else
SignedUpTeam.find_user_signup_topics(@assignment.id, users_team.first.t_id)
end
end
render('signup_sheet/intelligent_topic_selection') && return if @assignment.is_intelligent
end

</pre>

In views:

<pre>
...
<%= get_intelligent_topic_row(topic, @selected_topics, @assignment.max_team_size) %>
...
</pre>

<pre>
...
<% if @assignment.use_bookmark %>
...
</pre>

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb
We also ran the rspecs for signup_sheet model and helper just to make sure no functionality was broken.
: rspec spec/helpers/signup_sheet_helper_spec.rb
: rspec spec/models/signup_sheet_spec.rb

=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:20:58Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi

1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

3. Remove load_add_signup_topics() since it does nothing for add_signup_topics().

https://github.com/weichic-ncsu/expertiza/commit/e648df810a171a5b5b0756d91dd2c3c8d692b7cb

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:19:27Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi

1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

2. Remove add_signup_topics_staggered() and replace it with add_signup_topics() since it does nothing but call add_signup_topics().

https://github.com/weichic-ncsu/expertiza/commit/14059f8ee9b846bc4f9830d16b000c99d3f5c71c

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:17:51Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi

1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

https://github.com/weichic-ncsu/expertiza/commit/12f5e58ddb7782ec0ac7a4455489be8ecd167071

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:17:33Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi

1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12

https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed

https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:17:18Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi

1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12
https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed
https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T21:17:04Z

Wchen37: /* Solutions */

This wiki page is for the information regarding the changes made for the E2223 OSS assignment for Spring 2022, CSC/ECE 517.
== Team ==
=== Mentor ===
* Ed Gehringer (efg)

=== Team Members ===
* Juan Benavides (jdbenavi)
* Wei-Chi Chen (wchen37)
* Swetha Sairamakrishnan (jkini)

== Introduction ==
Expertiza is a open source project developed on Ruby on Rails. This web application is maintained by the student and faculty at NC State. This application gives complete control to the instructor to maintain the assignments in their class. With multiple functionalities such as adding topics, creating groups, and peer reviews, Expertiza is a well developed application that can handle all types of assignments. To learn more about the full functionality Expertiza has to offer, visit the Expertiza wiki.

== Peer Review Information ==

For users intending to view the deployed Expertiza associated with this assignment, the details are below:
* We have deployed the project -------------------
* Instructor login: username -> instructor6, password -> password
* Student login: username -> student5899, password -> password

== About Controller ==
The sign up sheet controller
* Allows an instructor to add/remove topics
* Allows an instructor to assign/remove students to topics
* Allows an student to see the list of available topics which can be bid on for given OSS assignment.

== Issues ==
*Naming is inconsistent. When used as a verb, “sign up” is two words. When an adjective or a noun, it should be one word, e.g., “signup_sheet_controller.” Cf. “sign_up_as_instructor”. Please make the naming consistent. Of course, this will result in changes in calling methods as well. [Mostly fixed by previous version of project.]
*Update method has a plethora of instance variables defined before updating. These might not be necessary (e.g., look at update method of bookmarks_controller). Decide whether so many instance variables are really needed. Refactor the variables not needed out.
*Add_signup_topics_staggered does not do anything different from add_signup_topics. Separate functions are needed, because add_signup_topics_staggered needs to make sure that the deadlines are set. [Assignment 1042 has staggered deadlines]
*Several method names can be improved (including: load_add_signup_topics, list, ad_info etc.)
*What are differences between signup_as_instructor and signup_as_instructor_action methods? Investigate if they are needed and improve the method names if both are needed. Provide comments as to what each method does.
*The list method is too long and is sparsely commented. Provide comments and identify if the method can be split or made cleaner by moving code to models or helper methods.
*Refactor participants variable in load_add_signup_topics
[In retrospect, the meaning of this is not clear. The @participants variable is used in a way that is very obscure, with code spread across several views, and no comments saying what it is doing. It used to be that participants (individual students) signed up for topics. Now, only teams can sign up for topics. So @participants does not make sense.
Signup_as_instructor_action has if-else ladder. It can be made more elegant. [If it is worth keeping this method at all.]
*Delete_signup and delete_signup_as_instructor have much in common and violate the DRY principle. delete_signup_as_instructor

== Solutions ==

-- Swetha

-- Wei-Chi
1. Rename "sign_up" to be "signup" when it is used as adjective or noun for variable/method/controller name.

https://github.com/weichic-ncsu/expertiza/commit/7fb2cceacbe459e5847141baea501bbd71952f12
https://github.com/weichic-ncsu/expertiza/commit/1152bac5c77cb9cf07f7de820e8677ba9a2a47ed
https://github.com/weichic-ncsu/expertiza/commit/3a5c6131c169812e455c20406809650a8a512646

-- Juan

== Testing ==
; RSpec
: As such no functionality changed in any function, only refactoring done. All the refactoring was carefully verified by running rspec tests. To run the spec file, run:
: rspec spec/controllers/signup_sheet_controller_spec.rb

; Physical Testing

:

#edit this stuff:
=== Edge Cases and Pre-Conditions ===
# When dropping topic if submission already done.
# When deadline from dropping topic has passed.
# Deleting topic when topic cannot be found.
# Signup in case when user cannot be found.
:

=== Login as instructor ===
# Hover on Manage Tab
# Click on Assignments Tab
# Click on Edit Button for "OSS project & documentation" assignment
# Select the Topic Tab
# Click the Green Check for any topic
# Enter a student UnityID and click submit

=== Login as student ===
# Click on OSS project/Writing assignment 2
# Click on Signup sheet
# The list must be visible, which indicates functionality is as before after refactoring list function.

CSC/ECE 517 Fall 2022 - E2259. Refactor signup sheet controller

2022-10-25T20:35:33Z

Wchen37: /* Team Members */