Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-12-05T21:37:01Z

Jrlanois: /* Issue 4: Remove Methods */

==Introduction==
===Background===
In Expertiza, teams are used to allow students to work together in group projects. The teams controller is responsible for assigning students to teams, as well as performing CRUD operations. In this project we will refactor teams_controller.rb and add tests to increase test coverage. We aim to follow the DRY principle in our refactoring by removing code that exists in multiple places. This project is a continuation of our previous work as described below, and we will build our existing code changes.

===Previous Work===
A team in spring 2022 attempted to refactor the controller as shown [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2214:_Refactor_teams_controller here]. This revision modified a few methods such as delete, inherit, and bequeath_all. It also added new methods like init_team_type, get_parent_by_id, get_parent_from_child.

More recently, we began a new refactor which can be found [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2022_-_E2254:_Refactor_teams_controller.rb here]. This revision involved renaming methods, improving how teams were removed from the waitlist, and drying code. Some code was also moved to the Team model (team.rb).

==Project Overview==
There are several issues we plan to address as part of this project.

* More comments need to be added to the controller to briefly explain functionality of custom methods.
* Find and fix any code where the DRY principle can be applied. Two places that will be taken a look at for this are the create and update methods.
* Try to refactor the update method to make it cleaner.
* There are few methods such as team_size, team_parent which will be removed.
* Fix the code and Hindi text in <code>config/locales/hi_IN.yml</code>.
* Add tests to increase test coverage by 10-20% if able.

===Issue 1: Add Comments===
We plan to add comments describing the functionality of the following methods.

* list

* new

* update

* edit

* delete_all

* delete

===Issue 2: Dry Code===
Here's one fix that needs to be done that can be used to enfore the DRY principle in these the list, create, and delete_all methods in teams_controller.rb. To fix this, we can extract the <code>Object.const_get(session[:team_type] + ... )</code> to its own method that can be used in all instances. By moving this code to its own method and replacing with the previous code, it will be following the DRY princple because there's less code being repeated.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes todo for list create delete all.png|700px]]

To address this code, we added a new method:

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - get team type const method.png]]

This method replaced the repeating code from in the program, ending up with the result seen below. This code is cleaner and properly applies the DRY principle.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes.png|700px]]

===Issue 3: Refactor Update===
We will analyze the update function in teams_controller.rb to see how we could refactor it to improve functionality. For example, the update method current checks to see if there is an existing team with the same id as highlighted below, but this check is not needed when updating as the team should in fact already exist.

[[File:Issue 3 redline.jpg|700px]]

The code to rescue a TeamExistsError can also be removed since there will no longer be a check that could potentially throw such an error.

===Issue 4: Remove Methods===
We will analyze teams_controller to find unused or one-line methods such as team_size and team_parent and remove or refactor them to clean up the code. The goal of this task is to remove unused or extra code to make the controller easier to read and understand. After analyzing the code, we could see that the team_size method only had one line, and was being used only in one place. So, the method was removed and the call to it (boxed in blue) was replaced with the line of code from the method. The code for team_parent was found to be redundant, and so every call made for team_parent was replaced with what would be the result from it (boxedin red). Also, because the ID of the team parent is the same as the params[:id], all instances of team_parent.id were replaced with params[:id] (boxed in green).

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - Removed Methods Changes.png | 700px]]

===Issue 5: Update Translations===
The code that was changed is highlighted below. The translation needs to be ensured that it is proper and the frontend needs to make sure that it's mapped to the correct phrase. The English file was also updated to reflect new language introduced in our last project, E2254, which is linked above.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - hi IN.yml things to change.png|700px]]

===Issue 6: Test Coverage===
Before starting this project, the teams controller was 79.8% covered. If all of the below tests were added, the controller would reach at 100% coverage. We added some of these tests as part of this project and brought coverage up to 94.9%, which is a 15.1% increase. We also discovered a couple bugs when implementing the tests.

====Create team when team exists====

Parts of an existing test were uncommented and fixed to test this case and show coverage.

====Update team when team exists====

A new test was added to ensure the code properly handles the TeamExistsError which occurs if an existing team name is used.

====Update team when team doesn't exist====

Parts of an existing test were uncommented and fixed to test this case and show coverage.

====Delete all when child nodes exist====

Attempting to write a test for this method revealed that the delete_all method is currently broken in Expertiza. The code before we made any changes
called Team.destroy_all to delete the teams for an assignment. However, this will delete all teams, not just the teams for a assignment or course.
This code was commented out and a note was added to document the broken method.
A skipped test stub was added, but the delete_all method will need to implemented properly before the full test is written.

====Copy to assignment when no course====

An existing test case was fixed to test this code. The test should have been failing, but it passed because of improper checking.
The test code was updated to check the flash message to ensure an error is shown when an invalid course id is given.
The code was updated to try to actually find a course with the given id rather than just checking if the id is present.

====Create when team parent is a Course====

This test was not added, but adding it should d cover an additional 2 lines in the team_type method.

==Diagrams==
===Overview Diagram===
The following diagram shows an overview of the different routes in the controller and some of their basic functionality.

[[File:E2274_teams_controller_overview.png|700px]]

===Use Case Diagram===
The teams controller is mainly responsible for dealing with the Team model. It can perform CRUD operations with teams and manage them on assignments and courses. It also has methods that can move the teams between them.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - use case diagram.png|700px]]

==Test Plan==
===RSpec Tests===
As mentioned for Issue 6, we plan to write several RSpec tests to increase test coverage of the teams_controller file by 10-20%. Specific test cases are list in the issue's section.

===Manual Testing===
====Randomize Teams Testing====

Given you are logged in as an instructor

* Username: ''instructor6'' Password ''password''

And you are on the Manage Assignments page

[[File:E2254_AssignmentsPage.png|700px]]

When you click on create teams for an assignment with participants

[[File:E2254_CreateTeams.png|700px]]

And you click "Create Team"

[[File:E2254_Toolbar.png|700px]]

And enter appropriate information

Then teams should be randomized and created

====Bequeath all Testing====

Given a team has been created for an assignment

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png|700px]]

And I am on the create teams page for an assignment

When I click "Bequeath All Teams"

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png|700px]]

Then I should be able to move the team from an assignment to a course

Course teams before bequeath all:

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png|700px]]

Course teams after bequeath all:

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png|700px]]

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-12-05T21:32:43Z

Jrlanois: /* Issue 2: Dry Code */

==Introduction==
===Background===
In Expertiza, teams are used to allow students to work together in group projects. The teams controller is responsible for assigning students to teams, as well as performing CRUD operations. In this project we will refactor teams_controller.rb and add tests to increase test coverage. We aim to follow the DRY principle in our refactoring by removing code that exists in multiple places. This project is a continuation of our previous work as described below, and we will build our existing code changes.

===Previous Work===
A team in spring 2022 attempted to refactor the controller as shown [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2214:_Refactor_teams_controller here]. This revision modified a few methods such as delete, inherit, and bequeath_all. It also added new methods like init_team_type, get_parent_by_id, get_parent_from_child.

More recently, we began a new refactor which can be found [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2022_-_E2254:_Refactor_teams_controller.rb here]. This revision involved renaming methods, improving how teams were removed from the waitlist, and drying code. Some code was also moved to the Team model (team.rb).

==Project Overview==
There are several issues we plan to address as part of this project.

* More comments need to be added to the controller to briefly explain functionality of custom methods.
* Find and fix any code where the DRY principle can be applied. Two places that will be taken a look at for this are the create and update methods.
* Try to refactor the update method to make it cleaner.
* There are few methods such as team_size, team_parent which will be removed.
* Fix the code and Hindi text in <code>config/locales/hi_IN.yml</code>.
* Add tests to increase test coverage by 10-20% if able.

===Issue 1: Add Comments===
We plan to add comments describing the functionality of the following methods.

* list

* new

* update

* edit

* delete_all

* delete

===Issue 2: Dry Code===
Here's one fix that needs to be done that can be used to enfore the DRY principle in these the list, create, and delete_all methods in teams_controller.rb. To fix this, we can extract the <code>Object.const_get(session[:team_type] + ... )</code> to its own method that can be used in all instances. By moving this code to its own method and replacing with the previous code, it will be following the DRY princple because there's less code being repeated.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes todo for list create delete all.png|700px]]

To address this code, we added a new method:

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - get team type const method.png]]

This method replaced the repeating code from in the program, ending up with the result seen below. This code is cleaner and properly applies the DRY principle.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes.png|700px]]

===Issue 3: Refactor Update===
We will analyze the update function in teams_controller.rb to see how we could refactor it to improve functionality. For example, the update method current checks to see if there is an existing team with the same id as highlighted below, but this check is not needed when updating as the team should in fact already exist.

[[File:Issue 3 redline.jpg|700px]]

The code to rescue a TeamExistsError can also be removed since there will no longer be a check that could potentially throw such an error.

===Issue 4: Remove Methods===
We will analyze teams_controller to find unused or one-line methods such as team_size and team_parent and remove or refactor them to clean up the code. The goal of this task is to remove unused or extra code to make the controller easier to read and understand.

===Issue 5: Update Translations===
The code that was changed is highlighted below. The translation needs to be ensured that it is proper and the frontend needs to make sure that it's mapped to the correct phrase. The English file was also updated to reflect new language introduced in our last project, E2254, which is linked above.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - hi IN.yml things to change.png|700px]]

===Issue 6: Test Coverage===
Before starting this project, the teams controller was 79.8% covered. If all of the below tests were added, the controller would reach at 100% coverage. We added some of these tests as part of this project and brought coverage up to 94.9%, which is a 15.1% increase. We also discovered a couple bugs when implementing the tests.

====Create team when team exists====

Parts of an existing test were uncommented and fixed to test this case and show coverage.

====Update team when team exists====

A new test was added to ensure the code properly handles the TeamExistsError which occurs if an existing team name is used.

====Update team when team doesn't exist====

Parts of an existing test were uncommented and fixed to test this case and show coverage.

====Delete all when child nodes exist====

Attempting to write a test for this method revealed that the delete_all method is currently broken in Expertiza. The code before we made any changes
called Team.destroy_all to delete the teams for an assignment. However, this will delete all teams, not just the teams for a assignment or course.
This code was commented out and a note was added to document the broken method.
A skipped test stub was added, but the delete_all method will need to implemented properly before the full test is written.

====Copy to assignment when no course====

An existing test case was fixed to test this code. The test should have been failing, but it passed because of improper checking.
The test code was updated to check the flash message to ensure an error is shown when an invalid course id is given.
The code was updated to try to actually find a course with the given id rather than just checking if the id is present.

====Create when team parent is a Course====

This test was not added, but adding it should d cover an additional 2 lines in the team_type method.

==Diagrams==
===Overview Diagram===
The following diagram shows an overview of the different routes in the controller and some of their basic functionality.

[[File:E2274_teams_controller_overview.png|700px]]

===Use Case Diagram===
The teams controller is mainly responsible for dealing with the Team model. It can perform CRUD operations with teams and manage them on assignments and courses. It also has methods that can move the teams between them.

[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - use case diagram.png|700px]]

==Test Plan==
===RSpec Tests===
As mentioned for Issue 6, we plan to write several RSpec tests to increase test coverage of the teams_controller file by 10-20%. Specific test cases are list in the issue's section.

===Manual Testing===
====Randomize Teams Testing====

Given you are logged in as an instructor

* Username: ''instructor6'' Password ''password''

And you are on the Manage Assignments page

[[File:E2254_AssignmentsPage.png|700px]]

When you click on create teams for an assignment with participants

[[File:E2254_CreateTeams.png|700px]]

And you click "Create Team"

[[File:E2254_Toolbar.png|700px]]

And enter appropriate information

Then teams should be randomized and created

====Bequeath all Testing====

Given a team has been created for an assignment

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png|700px]]

And I am on the create teams page for an assignment

When I click "Bequeath All Teams"

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png|700px]]

Then I should be able to move the team from an assignment to a course

Course teams before bequeath all:

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png|700px]]

Course teams after bequeath all:

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png|700px]]

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - get team type const method.png

2022-12-05T21:29:29Z

Jrlanois: New method added for get_team_type_const to address DRY principle for E2274

New method added for get_team_type_const to address DRY principle for E2274

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - Removed Methods Changes.png

2022-12-05T21:27:01Z

Jrlanois: Methods removed for E2274

Methods removed for E2274

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes.png

2022-12-05T21:26:34Z

Jrlanois: DRY Fixes for E2274

DRY Fixes for E2274

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-18T23:00:56Z

Jrlanois: /* Manual Testing */

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-18T22:50:17Z

Jrlanois: /* Issue 2: Dry Code */

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T21:31:20Z

Jrlanois: /* Issue 2: Dry Code */

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T21:31:08Z

Jrlanois: /* Issue 3: Refactor Update */

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T21:27:54Z

Jrlanois: Added use case diagram.

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - use case diagram.png

2022-11-14T21:24:32Z

Jrlanois: Use case diagram for Teams_controller.rb

Use case diagram for Teams_controller.rb

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T20:57:43Z

Jrlanois: /* Issue 2 Dry Code */

==Introduction==
In this project we refactored teams_controller.rb and added tests associated with its functionality. Before we worked on it, teams_controller.rb had some minor parts of a few methods that could be improved. More comments can be added to improve the readability of methods in the file as well as code more understandable for what custom methods do. Furthermore, test coverage could be improved.

==Previous Work==
A team in spring 2022 attempted to refactor the controller as shown [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2214:_Refactor_teams_controller here]. This revision modified a few methods such as delete, inherit, and bequeath_all. It also added new methods like init_team_type, get_parent_by_id, get_parent_from_child.

More recently, we began a new refactor which can be found [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2022_-_E2254:_Refactor_teams_controller.rb here]. This revision involved renaming methods, improving how teams were removed from the waitlist, and drying code. Some code was also moved to the Team model (team.rb).

This project is a continuation of our previous work and we will build upon our existing code changes.

==Issues Being Worked on==
* More comments need to be added to the controller to briefly explain functionality of custom methods.
* Find and fix any code where the DRY principle can be applied. Two places that will be taken a look at for this are the create and update methods.
* Try to refactor the update method to make it cleaner.
* There are few methods such as team_size, team_parent which will be removed.
* Fix the code and Hindi text in <code>config/locales/hi_IN.yml</code>.
* Add tests to increase test coverage by 10-20% if able.

===Issue 1 Add Comments===
We plan to add comments describing the functionality of the following methods.

* list

* new

* update

* edit

* delete_all

* delete

===Issue 2 Dry Code===
Here's one fix that needs to be done that can be used to enfore the DRY principle in these the list, create, and delete_all methods in teams_controller.rb. To fix this, we can extract the <code>Object.const_get(session[:team_type] + ... )</code> to it's own method that can be used in all instances.
[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes todo for list create delete all.png]]

===Issue 3 Refactor Update===
We will analyze the update function in teams_controller.rb to see how we could refactor it to improve functionality. For example, the update method current checks to see if there is an existing team with the same id as highlighted below, but this check is not needed when updating as the team should in fact already exist.
[[File:Issue 3 redline.jpg]]

===Issue 4 Remove Methods===
We will analyze teams_controller to find unused or one-line methods such as team_size and team_parent and remove or refactor them to clean up the code. The goal of this task is to remove unused or extra code to make the controller easier to read and understand.

===Issue 5 Update Translations===
The code that may need to changed is highlighted below. The translation needs to be ensured that it is proper and the frontend needs to make sure that it's mapped to the correct phrase.
[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - hi IN.yml things to change.png]]

===Issue 6 Test Coverage===
Before starting this project, the teams controller is 79.8% covered. If all of the below tests are added, the controller reach at 100% coverage. We plan to at least add some of the tests as part of this project.

* Create team when team exists

* Update team when team exists

* Update team when team doesn't exist

* Delete all when child nodes exist

* Copy to assignment when no course

* List when team parent is a Course

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - DRY fixes todo for list create delete all.png

2022-11-14T20:55:24Z

Jrlanois: Here's one fix that needs to be done that can be used to enfore the DRY principle in these the list, create, and delete_all methods in teams_controller.rb.

Here's one fix that needs to be done that can be used to enfore the DRY principle in these the list, create, and delete_all methods in teams_controller.rb.

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T20:24:36Z

Jrlanois: /* Issue 5 Update Translations */

==Introduction==
In this project we refactored teams_controller.rb and added tests associated with its functionality. Before we worked on it, teams_controller.rb had some minor parts of a few methods that could be improved. More comments can be added to improve the readability of methods in the file as well as code more understandable for what custom methods do. Furthermore, test coverage could be improved.

==Previous Work==
Prior work can be found [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2214:_Refactor_teams_controller here]. This work was done in spring 2022, and it mainly consisted of refactoring a few methods such as delete, inherit, and bequeath_all. It also added new methods like init_team_type, get_parent_by_id, get_parent_from_child.

More recently, work was done by us from first part of the OSS project, which can be found [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2022_-_E2254:_Refactor_teams_controller.rb here]. This revision involved refactoring the methods like create_teams method, which involved renaming it to randomize_teams. The delete method was refactored to improve how teams were removed from the waitlist. The inherit method was also refactored to be renamed to copy_to_assignment and part of the code was moved to the custom method copy_teams_to_collection in Team model (team.rb). Furthermore, bequeath_all was also refactored to remove nested if statements and other returns so the method is easier to follow from looking at it.

==Issues Being Worked on==
* More comments need to be added to the controller to briefly explain functionality of custom methods.
* Find and fix any code where the DRY principle can be applied. Two places that will be taken a look at for this are the create and update methods.
* Try to refactor the update method to make it cleaner.
* There are few methods such as team_size, team_parent which will be removed.
* Fix the code and Hindi text in <code>config/locales/hi_IN.yml</code>.
* Add tests to increase test coverage by 10-20% if able.

===Issue 1 Add Comments===
We plan to add a comment describing the functionality of the following methods.

* list

* new

* update

* edit

* delete_all

* delete

===Issue 2 Dry Code===

===Issue 3 Refactor Update===

===Issue 4 Remove Methods===
We will analyze teams_controller to find unused or one-line methods such as team_size and team_parent and remove or refactor them to clean up the code. The goal of this task is to remove unused or extra code to make the controller easier to read and understand.

===Issue 5 Update Translations===
The code that may need to changed is highlighted below. The translation needs to be ensured that it is proper and the frontend needs to make sure that it's mapped to the correct phrase.
[[File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - hi IN.yml things to change.png]]

===Issue 6 Test Coverage===

File:CSC-ECE 517 Fall 2022 - E2274 Refactor teams controller.rb - hi IN.yml things to change.png

2022-11-14T20:22:43Z

Jrlanois: The highlighted code may need to be changed in on these lines in <code>config/locales/hi_IN.yml</code>.

The highlighted code may need to be changed in on these lines in <code>config/locales/hi_IN.yml</code>.

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T15:36:36Z

Jrlanois: Updated current text

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-14T14:50:53Z

Jrlanois: /* Introduction */

==Introduction==
In this project we refactored teams_controller.rb and added tests associated with its functionality. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Previous Work==
https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2214:_Refactor_teams_controller

https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2022_-_E2254:_Refactor_teams_controller.rb

==Issues Being Worked on==
* Add more comments to the controller, briefly explain the functionality of custom methods
* Find out areas in the code where DRY principle can be applied (For eg. look at create and update methods to find similarity)
* Try to look at the waitlist functionality and simplify it.
* Try to look and modify the update function
* There are few methods team_size, team_parent which look unnecessary
* Try to update the config/locales/hi_IN.yml hindi text.
* Try to increase the test coverage.

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-13T22:41:26Z

Jrlanois: /* Issues Being Worked on */

CSC/ECE 517 Fall 2022 - E2274: Refactor teams controller.rb

2022-11-13T22:40:56Z

Jrlanois: Added initial things

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-31T23:26:43Z

Jrlanois: /* Test Plan */

==Introduction==
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Issues Fixed==
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

==Files Changed==
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

==Changes==
===Fix #2===
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function. This also needed to be updated in the rspec test <code>teams_controller_spec.rb</code>:
<nowiki> describe 'create teams method' do
context 'when correct parameters are passed' do
it 'creates teams with random names' do
allow(ExpertizaLogger).to receive(:info).and_return(nil)
...
user_session = { user: instructor, team_type: 'Assignment' }
result = get :randomize_teams, params: request_params, session: user_session
# status code 302: Redirect url
...
end
end
end</nowiki>

===Fix #3===
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params</code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model.

<nowiki>
# Gets the model representing the parent of the team
def team_type
if session[:team_type] == 'Assignment'
Assignment
elsif session[:team_type] == 'Course'
Course
end
end

# Finds the object containing the students
# which the team will be generated from
def team_parent
@team_parent ||= team_type.find(params[:id])
end</nowiki>

===Fixes #4 and #6===
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>.
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
# # if there is another team in waitlist, make this team hold this topic
# topic_id = @signed_up_team.first.topic_id
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
# end</nowiki>
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well.
The change to being able to delete from the waitlist needed to be accounted for in the <code>teams_controller_spec.rb</code> rspec test, which can be seen here:
<nowiki>context 'when called and team is not nil and it does not hold a topic' do
it 'deletes the team' do
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
...
end
end</nowiki>
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed:
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do
...
controller.session[:team_type] = 'Assignment'

allow(SignedUpTeam).to receive(:destroy).and_return(nil)
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
allow(Team).to receive(:find).with(any_args).and_return(team)
allow(Team).to receive(:find_by).with(any_args).and_return(team)
...
end</nowiki>

===Fixes #7 and #8===
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

===Fix #9===
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

==Test Plan==

===Test Coverage===
Rspec test coverage of <code>app/controllers/teams_controller.rb</code> fromt the main branch of expertiza:
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage before refactoring.png]]
Rspec test coverage of <code>app/controllers/teams_controller.rb</code> after our refactoring:
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage after refactoring.png]]

===Randomize Teams===

Given you are logged in as an instructor

* Username: ''instructor6'' Password ''password''

And you are on the Manage Assignments page

[[File:E2254_AssignmentsPage.png]]

When you click on create teams for an assignment with participants

[[File:E2254_CreateTeams.png]]

And you click "Create Team"

[[File:E2254_Toolbar.png]]

And enter appropriate information

Then teams should be randomized and created

===Bequeath all===

Given a team has been created for an assignment
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png]]

And I am on the create teams page for an assignment

When I click "Bequeath All Teams"
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png]]

Then I should be able to move the team from an assignment to a course

Course teams before bequeath all:
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png]]

Course teams after bequeath all:
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png]]

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-31T22:43:20Z

Jrlanois: /* Test Plan */

==Introduction==
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Issues Fixed==
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

==Files Changed==
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

==Changes==
===Fix #2===
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function. This also needed to be updated in the rspec test <code>teams_controller_spec.rb</code>:
<nowiki> describe 'create teams method' do
context 'when correct parameters are passed' do
it 'creates teams with random names' do
allow(ExpertizaLogger).to receive(:info).and_return(nil)
...
user_session = { user: instructor, team_type: 'Assignment' }
result = get :randomize_teams, params: request_params, session: user_session
# status code 302: Redirect url
...
end
end
end</nowiki>

===Fix #3===
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params</code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model.

<nowiki>
# Gets the model representing the parent of the team
def team_type
if session[:team_type] == 'Assignment'
Assignment
elsif session[:team_type] == 'Course'
Course
end
end

# Finds the object containing the students
# which the team will be generated from
def team_parent
@team_parent ||= team_type.find(params[:id])
end</nowiki>

===Fixes #4 and #6===
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>.
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
# # if there is another team in waitlist, make this team hold this topic
# topic_id = @signed_up_team.first.topic_id
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
# end</nowiki>
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well.
The change to being able to delete from the waitlist needed to be accounted for in the <code>teams_controller_spec.rb</code> rspec test, which can be seen here:
<nowiki>context 'when called and team is not nil and it does not hold a topic' do
it 'deletes the team' do
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
...
end
end</nowiki>
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed:
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do
...
controller.session[:team_type] = 'Assignment'

allow(SignedUpTeam).to receive(:destroy).and_return(nil)
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
allow(Team).to receive(:find).with(any_args).and_return(team)
allow(Team).to receive(:find_by).with(any_args).and_return(team)
...
end</nowiki>

===Fixes #7 and #8===
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

===Fix #9===
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

==Test Plan==

===Randomize Teams===

Given you are logged in as an instructor

* Username: ''instructor6'' Password ''password''

And you are on the Manage Assignments page

[[File:E2254_AssignmentsPage.png]]

When you click on create teams for an assignment with participants

[[File:E2254_CreateTeams.png]]

And you click "Create Team"

[[File:E2254_Toolbar.png]]

And enter appropriate information

Then teams should be randomized and created

===Bequeath all===

Given a team has been created for an assignment

And I am on the create teams page for an assignment

When I click "Bequeath All Teams"

Then I should be able to move the team from an assignment to a course

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage before refactoring.png]]
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage after refactoring.png]]

[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png]]
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png]]
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png]]
[[File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png]]

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - teams for test assignment.png

2022-10-31T22:41:40Z

Jrlanois:

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage before refactoring.png

2022-10-31T22:40:40Z

Jrlanois:

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - coverage after refactoring.png

2022-10-31T22:40:23Z

Jrlanois:

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams before bequeathing all.png

2022-10-31T22:39:59Z

Jrlanois:

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - course teams after bequeathing all.png

2022-10-31T22:36:17Z

Jrlanois:

File:CSC-ECE 517 Fall 2022 - E2254 Refactor teams controller.rb - bequeath all success message.png

2022-10-31T22:34:31Z

Jrlanois:

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-26T20:01:53Z

Jrlanois: /* Fix #2 */

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-26T20:01:28Z

Jrlanois: /* Fixes #4 and #6 */

==Introduction==
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Issues Fixed==
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

==Files Changed==
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

==Changes==
===Fix #2===
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function. This also needed to be updated in the rspec test <code>teams_controller_spec.rb</code>:
<nowiki> describe 'create teams method' do
context 'when correct parameters are passed' do
it 'creates teams with random names' do
...
user_session = { user: instructor, team_type: 'Assignment' }
result = get :randomize_teams, params: request_params, session: user_session
# status code 302: Redirect url
...
end
end
end</nowiki>

===Fix #3===
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params</code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model.

<nowiki>
# Gets the model representing the parent of the team
def team_type
if session[:team_type] == 'Assignment'
Assignment
elsif session[:team_type] == 'Course'
Course
end
end

# Finds the object containing the students
# which the team will be generated from
def team_parent
@team_parent ||= team_type.find(params[:id])
end</nowiki>

===Fixes #4 and #6===
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>.
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
# # if there is another team in waitlist, make this team hold this topic
# topic_id = @signed_up_team.first.topic_id
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
# end</nowiki>
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well.
The change to being able to delete from the waitlist needed to be accounted for in the <code>teams_controller_spec.rb</code> rspec test, which can be seen here:
<nowiki>context 'when called and team is not nil and it does not hold a topic' do
it 'deletes the team' do
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
...
end
end</nowiki>
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed:
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do
...
controller.session[:team_type] = 'Assignment'

allow(SignedUpTeam).to receive(:destroy).and_return(nil)
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
allow(Team).to receive(:find).with(any_args).and_return(team)
allow(Team).to receive(:find_by).with(any_args).and_return(team)
...
end</nowiki>

===Fixes #7 and #8===
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

===Fix #9===
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-26T19:58:35Z

Jrlanois: /* Fix #2 */

==Introduction==
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Issues Fixed==
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

==Files Changed==
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

==Changes==
===Fix #2===
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function. This also needed to be updated in the rspec test <code>teams_controller_spec.rb</code>:
<nowiki> describe 'create teams method' do
context 'when correct parameters are passed' do
it 'creates teams with random names' do
...
user_session = { user: instructor, team_type: 'Assignment' }
result = get :randomize_teams, params: request_params, session: user_session
# status code 302: Redirect url
...
end
end
end</nowiki>

===Fix #3===
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params</code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model.

<nowiki>
# Gets the model representing the parent of the team
def team_type
if session[:team_type] == 'Assignment'
Assignment
elsif session[:team_type] == 'Course'
Course
end
end

# Finds the object containing the students
# which the team will be generated from
def team_parent
@team_parent ||= team_type.find(params[:id])
end</nowiki>

===Fixes #4 and #6===
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>.
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
# # if there is another team in waitlist, make this team hold this topic
# topic_id = @signed_up_team.first.topic_id
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
# end</nowiki>
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well.
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed:
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do
...
controller.session[:team_type] = 'Assignment'

allow(SignedUpTeam).to receive(:destroy).and_return(nil)
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
allow(Team).to receive(:find).with(any_args).and_return(team)
allow(Team).to receive(:find_by).with(any_args).and_return(team)
...
end</nowiki>

===Fixes #7 and #8===
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

===Fix #9===
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-26T19:53:47Z

Jrlanois: /* Changes */

==Introduction==
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

==Issues Fixed==
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>randomize_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

==Files Changed==
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

==Changes==
===Fix #2===
The <code>create_teams</code> method's name poorly reflected it's actual function. The function takes a group of students from an assignment or course and creates randomized teams. To reflect this, the method was renamed to <code>randomize_teams</code> and a comment was added briefly describing the function.

===Fix #3===
Several places used a complicated line to find the parent of a team. The parent is either a <code>Course</code> or an <code>Assignment</code>. The type of parent is stored in the <code>session</code> and the <code>id</code> of the parent in the <code>params<code>. We created two new private methods, <code>team_type</code> and <code>team_parent</code> to DRY out the code. The <code>team_type</code> method returns either <code>Course</code> or <code>Assignment</code> and the <code>team_parent</code> method returns the the actual parent rather than the model.

===Fixes #4 and #6===
Removed the following if statement editing a waitlist in a controller and replaced it with <code>Waitlist.remove_from_waitlists(@team.id)</code>.
<nowiki># if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
# # if there is another team in waitlist, make this team hold this topic
# topic_id = @signed_up_team.first.topic_id
# next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
# # if slot exist, then confirm the topic for this team and delete all waitlists for this team
# SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
# end</nowiki>
In the process of making this change, the unused <code>@signUps</code> variable is removed, taking care of Fix #6 as well.
The change in how things were deleted from waitlists and how things were deleted from signed up teams led to issues with rspec testing in <code>airbrake_exception_errors_controller_tests_spec.rb</code>. To fix this, we allowed functionality to account for what was changed:
<nowiki> it 'will delete the team if current team did not involve in any other reviews' do
...
controller.session[:team_type] = 'Assignment'

allow(SignedUpTeam).to receive(:destroy).and_return(nil)
allow(Waitlist).to receive(:remove_from_waitlists).and_return(nil)
allow(Team).to receive(:find).with(any_args).and_return(team)
allow(Team).to receive(:find_by).with(any_args).and_return(team)
...
end</nowiki>

===Fixes #7 and #8===
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

===Fix #9===
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-26T19:14:30Z

Jrlanois: added info for fixes 8 and 9

=Introduction=
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.

=Issues Fixed=
# Added more comments and briefly explained the functionality of custom methods like <code>action_allowed</code>, and <code>list</code>
# Changed method name of <code>create_teams</code> to a better name: <code>random_teams</code>
# Found areas in the code where the DRY principle can be applied and did so
# The delete method manipulates a waitlist! This needs to be done in a model class, e.g., SignedUpTeam. And also check if this code is already present in the model or not. Explore the code base to find where else waitlist is manipulated.
#* Changed the delete method to use the existing Waitlist.remove_from_waitlists method to handle removing the deleted team from all waitlists
# Removed all extra added gems
# Renamed variable signUps to signups because it's a noun
#* Actually ended up removing this variable in the process of doing fix 4 above
# Moved list copying code from the <code>inherit</code> method to be a class method of the Team model: <code>copy_teams_to_collection</code>. Also replaced identical code in bequeath_all to also use this class method.
# Changed the name of the <code>inherit</code> method to <code>copy_to_assignment</code> so the name is more descriptive of it's purpose
# Removed nested if statments and removed other return points from bequeath_all
# Removed unnecessary comments for methods like <code>update</code> and added meaningful comments to other methods

=Files Changed=
* app/controllers/teams_controller.rb
* app/models/team.rb
* app/views/teams/new.html.erb
* config/locales/en_US.yml
* config/locales/en_IN.yml
* config/routes.rb
* spec/controllers/teams_controller_spec.rb
* spec/controllers/airbrake_exception_errors_controller_tests_spec.rb

=Changes=
* Fixes #7 and #8:
One of the main changes made to implement this was in <code>Team.rb</code> where the class method <code>copy_teams_to_collection</code> was added. 
<nowiki># E2254 : This method copies a list of teams to another collection. It is mainly
# used in the Team model for copy_to_assignment and bequeath_all for the purpose of copying
# teams from assignments to courses and vice versa.
def self.copy_teams_to_collection(teams, recipient_collection_id)
teams.each do |team|
team.copy(recipient_collection_id)
end
end</nowiki>
This method was called in <code>teams_controller.rb</code> in the <code>copy_to_assignment</code> (method used to be named <code>inherit</code>) and <code>bequeath_all</code> methods.
<nowiki>def copy_to_assignment
...
if teams.empty?
flash[:note] = 'No teams were found when trying to copy to assignment.'
else
Team.copy_teams_to_collection(teams, assignment.id)
end
...
end</nowiki>
<nowiki>def bequeath_all
...
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
...
end</nowiki>

* Fix #9:
Refactored bequeath_all to get rid of all the nested if statements
<nowiki> # Copies existing teams from an assignment to a
# course if the course doesn't already have teams
def bequeath_all
assignment = Assignment.find(params[:id])
course = Course.find(assignment.course_id) if assignment.course_id

if !assignment.course_id
flash[:error] = 'No course was found for this assignment.'
elsif course.course_teams.any?
flash[:error] = 'The course already has associated teams'
else
teams = assignment.teams
Team.copy_teams_to_collection(teams, course.id)
flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
end
redirect_to controller: 'teams', action: 'list', id: assignment.id
end</nowiki>
Also, the multiple returns have been removed from the method. The redirect and return for the check to ensure
a course team was being used was moved to it's own method:
<nowiki> # Redirects if the team is not a CourseTeam
def ensure_course_team
if session[:team_type] == 'Course'
flash[:error] = 'Invalid team type for bequeathal'
redirect_to controller: 'teams', action: 'list', id: params[:id]
end
end</nowiki>
and specified to run at before the <code>bequeath_all</code> method at the start of the file:
<nowiki>before_action :ensure_course_team, only: %i[bequeath_all]</nowiki>

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-25T19:57:28Z

Jrlanois: Started adding in changed code

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-25T18:27:07Z

Jrlanois: Added files changed bullets

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-25T18:18:34Z

Jrlanois: /* Introduction */

CSC/ECE 517 Fall 2022 - E2254: Refactor teams controller.rb

2022-10-25T17:36:36Z

Jrlanois: In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles.

===Introduction===
In this project we refactored teams_controller.rb and some tests associated with it. Before we worked on it, teams_controller.rb had some problems that violated some essential Rails design principles. Some methods have been moved to the teams.rb model class. Some duplicate code was removed. We added some comments for readability. And, we fixed tests to be more consistent with the program and execution.