Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-12-06T00:12:45Z

Mgfiles: /* Relevant Links */

== Problem Description ==
In Expertiza, both students and instructors are able to view a student's teammate reviews. On the reviews page, only a single heatmap is shown which does not provide a full representation of the student's performance. In addition, it is unclear if the heatmap is displaying information about the reviews the student has written or received. The heatmaps on the instructor and student views are not labeled and do not provide clarity on the information they are displaying.

== Proposed Changes ==

=== Calculate Composite Review Score ===

==== Problem ====
It would be useful for the instructor to see a composite score derived from 1) all of the reviews a team has received and 2) all of the reviews a student has received from teammates and 3) all of the reviews a student has given to teammates. This would be calculated in a similar fashion to the average peer review score shown at the top of a student's review page. Currently, all review tables only display the average score for each row as shown on the figure below. As each row hosts a unique question, the average row score is the average score given/received for each question. The image below shows where we would like to display the composite score.

[[File:Composite score.png|center|border|1000px|Instructor view of a team's reviews does not show a composite score]]

==== Solution ====
The previous semester's team calculated the composite value within the HTML view page which does not follow the design principles of this course. We plan to move this mathematical operation to the appropriate model where it where will be accessible to any view that requires a composite score for its heatmap.

The problem will have to be solved through changes in the two files listed below:

===== vm_question_response.rb =====
This file hosts the methods required to build a review table. It has methods for linking an assignment to a questionnaire, adding reviews and score with color coding, displaying team members, etc. It also contains more computational based functions that calculate the number of comments greater than 20 words, for example. This file made sense to add a <code>composite_score</code> method that loops through each question and calculates the average score given/received for that specific question. It returns an array containing the average review score and the maximum possible score which is further discussed in the below section. The method is smart in the sense that it can filter out questions that do not have a numeric response.

===== _view_heatgrid.html.erb =====
This file will call the <code>_view_compompsite_score.html.erb</code> partial view which calls the <code>composite_score</code> method to populate a string tag that displays the average score for all reviews out of the maximum possible score. The maximum possible score value sets a reference for how well the student performed. This was a requirement for showing a composite score with a reference point because not all questions are required to have the same maximum score. For example, take a review table with two questions with maximum scores of 1 and 5. If the student received a 1 / 1 and 5 / 5, the composite score would show 3 as calculated by the scores received divided by the number of questions. This would be confusing so a reference point must be added. This is done by summing the maximum scores possible 1 and 5 and again dividing by the number of questions. The view will now show 3 / 3 which are perfect marks!

===== view_team.html.erb =====
This file uses the <code>_view_compompsite_score.html.erb</code> partial to display the composite score for student views. It uses the same logic as the above description.

===== _view_compompsite_score.html.erb =====
This file was created to add the <code>composite_score</code> method call. This was broken out into a separate partial for any future view changes that may go into the composite score. This was also done to adhere to the DRY principle as several views are required to call this method.

The following changes can be seen in the output of the image below.
[[File:Composite score fixes.png|center|border|1000px|Instructor view of a team's reviews showing a composite score]]

 

'''The composite score calculation can also be seen on the student view here. A bug was received on the previous semesters work that their composite score calculation was breaking the average peer review score by setting it to zero. This issue has been resolved as you can see you can see both scores are being displayed in a correct manner.'''

[[File:Working peer review.PNG|center|border|1000px|Functional peer review score]]

<hr>

=== Improved Display of Review Heatmap ===

==== Problem ====
This problem contains many parts. Firstly, the student scores view does not even show a heatmap of reviews at all as seen below.

[[File:no_student_heatmap.png|center|border|1000px|The my scores view page for the student doesn't show any teammate review heatmaps]]

This will have to be included with the condition that the instructor has selected that the student can view the reviews in the assignment options. Additionally, in the display of the heatmap itself there is no indication if the reviews displayed are reviews about the student signed in or made by the student signed in as seen below.

[[File:no_description_on_heatmap.png|center|border|1000px|The heatmap printout doesn't specify if showing reviews by or for a certain student]]

A more descriptive label will have to be added to show this. Lastly, a composite score will have to be displayed under each heatmap to average the scores that the student received.

==== Solution ====
The problem will have to be solved through changes in two erb files listed below:

===== _view_heatgrid.html.erb =====

This file contains the logic to display the actual heatgrid to the user. It will have to be modified in two ways. Firstly, we will have to add a conditional header that will either display that the heatgrid shows reviews for the student or by the student depending on the type of questionnaire that it is displaying. Secondly, we will have to add a place under the heatgrid to display the composite score of that heatgrid. In the previous attempt at this project, the group added the calculation logic for the composite score to this file which is incorrect because it is an erb file. Instead, this file will contain a call to a funtion in the model that will calculate the value and return it for display.

===== view_team.html.erb =====

This file contains the logic to display the page where the student views the questionnaires. We will have to add logic to display the reviews of that student if the assignment is set to have show teammate reviews be true.

<hr>

=== Preservation of Anonymized View Naming Convention ===

==== Problem ====
Student names are visible in the teammate reviews tab, even when the user is in the anonymized view. So the display when in an anonymized view, where no names are to be shown, but only “Student nnn”,“Instructor mmm”, etc. (Anonymized view is intended to be used in demos, so we can use live data to demo the system without showing anyone’s name.)

[[File:Anonnymized.jpg|center|border|1000px|]]
==== Solution ====
We plan to remove the team member name from being displayed when using anonymized view by adding some changes to the following files.

=====user.rb=====
In user file the anonymized user function is defined and when called it checks and returns True if a particular user is in anonymized view or else returns False. We will be improving the conditional logic to display the user name only when not present in anonymized view.

=====_view_heatgrid.html.erb=====
The file contains the logic to display the heatgrid to the user. When we make the changes to user file we will need to make some changes in this file to not display the user name in anonymized view.

==== Use Case Flow diagram for Anonymized View ====
[[File:uml_1.PNG|center|border|500px|UML for Anonymized]]

== Bugs ==
The following bugs were discovered and still need to be fixed. These bugs were well outside of the scope of our project, however, we wanted to document them for future reference.

These bugs can be found by logging in as an instructor, going to the Manage->Assignments tab, clicking View Scores of Final project (and design doc) and then selecting any team.

As a temporary debugging feature, we added column averages in an attempt to solve the issues we were seeing the review tables. You can see these averages in the bottom row of the image. Up until student 9020 all scores and averages are correct. It is theorized that the average score for student 9020 is being pulled from student 8977's scores on table 1. Student 9020 is showing review scores of student 9022 so the cells have become shifted by 1. The student 9020 review has essentially disappeared from the view which we could not determine the root cause of. The average of each row is being populated in student 9042 due to this shift by 1.

The top image shows the student 9022 review which you can see is populated in student 9020 column from the bottom image.

[[File:9022_student_review.PNG|center|border|1000px|Student 9022 review]]

[[File:Reviews page.png|center|border|1000px|Instructors reviews page should bugs on the table]]

== Testing ==

=== Flow to access affected views ===
==== _view_heatgrid.html.erb ====
This file can be accessed through the GUI differently when logged in as an instructor or a student. When logged in as a student:

# Click "Assignments" at the top of the screen
# Click desired assignment to view reviews from
# Click on "Your scores" button
# Scroll down past assignment score heatgrid
# You will see the reviews that others have made about you if the instructor has enabled that setting on this assignment

If you are logged in as an instructor:

# Click "Manage..." then "Assignments" at the top of the screen
# Click on the "view scores" button on the desired assignment
# Click the plus next to the team that you are interested in
# Click on the "Teammate Reviews" tab at the top of the panel
# Select the student that you wish to see the reviews about
# The relevant heatgrid will display with a descriptive title

=== Test Login Credentials ===
==== Instructor ====
* username: instructor6
* password: password
==== Student ====
* username: student7185
* password: password

=== Added Unit Tests ===
==== view_team_spec.rb ====
This test file was at first failing even without making any changes so first we fixed the failing test by filling in some incomplete factory creations in the set up of the test. Then, a negative check was added to make sure the teammate review scores are not displayed to the student while the assignment has show teammate reviews set to false. We had to do a negative case instead of positive because it was overly complicated to create a teammate review submission and a vm with that teammate review for the html file to register that there was a review to display. The added negative check looks like the code segment below:
 
<code>
expect(page).to_not have_text 'Teammate Review'
</code>

=== Added System Tests ===
For calculating the composite score, we built Rspec tests to ensure the correct calculation was being performed. We implemented factories in the <code>vm_question_response_spec.rb</code> test file to improve modularity and code readability. Factories provide more flexibility in generating models to meet the requirements of the test, as opposed to fixtures. As you can see from the below image, the code uses a before section to streamline the creation of the many objects required to build a review table. Without these factories, each individual test would have required the reuse of object creation code which would have violated DRY principles.

[[File:Spec factories.PNG|center|border|1000px|Use of RSpec testing factories]]

Many variations of score arrays were passed into the <code>composite_score</code> method for evaluation. All tests were passed after running the spec file. The score arrays included nil values, strings, etc. and ensured that the return value was correct. These tests can be seen here.

[[File:Spec tests.PNG|center|border|1000px|Rspec tests for the composite_score method]]

=== Added System Tests ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View student reviews received from teammates and given to teammates from a student view ====

---Code still being developed---

==== 2) View teammate reviews as a student when enabled ====

The teammate reviews will be shown at the bottom of the screen because it has been enabled by the instructor:

[[File:yes_student_heatmap.png|center|border|1000px|Student view shows teammate reviews of student if allowed]]

==== 3) View composite score of reviews from a student view ====

---Code still being developed---

The following tests were conducted as follows:
 
- Sign in as instructor above
 
- Click Manage -> Assignments
 
- Click on the View Scores of Final Project assignment
 
- Click into any team

==== 1) View student reviews received from teammates and given to teammates from an instructor view ====

---Code still being developed---

==== 2) View composite score of reviews from an instructor view ====

---Code still being developed---

==== 3) Show descriptive name for heatmaps from an instructor view ====
The displayed heatgrid has a new, descriptive title describing exactly what reviews are being displayed:

[[File:yes_description_on_heatmap.png|center|border|1000px|Teammate Review Heatmap has descriptive title]]

=== Manual Testing for Anonymized User View: ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View your teammates names in the Team members section of review scores ====
In Normal user view the Users name or ID is displayed.
 
[[File:student_view_scores.png|center|border|900px|Anonymized View]]

==== 2) Switch to Anonymized View and view your teammates names in the Team members section of review scores ====
In Anonymized view the Users name or ID is not displayed.
 
[[File:anonymized_view.png|center|border|900px|Anonymized View]]

== Relevant Links ==
* '''Github Repository:''' https://github.com/colin-odowd/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2476
* '''VCL Server:''' http://152.7.178.100:8080/
* '''Video:''' https://youtu.be/Hcqr4uLzU38

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-12-04T15:17:05Z

Mgfiles: /* view_team_spec.rb */

== Problem Description ==
In Expertiza, both students and instructors are able to view a student's teammate reviews. On the reviews page, only a single heatmap is shown which does not provide a full representation of the student's performance. In addition, it is unclear if the heatmap is displaying information about the reviews the student has written or received. The heatmaps on the instructor and student views are not labeled and do not provide clarity on the information they are displaying.

== Proposed Changes ==

=== Calculate Composite Review Score ===

==== Problem ====
It would be useful for the instructor to see a composite score derived from all of the reviews similar to the average score shown on the students review page. Currently, the code only displays the average score for each row of the review table on the instructors view. The image below shows where we would like to display the composite score.

[[File:Composite score.png|center|border|1000px|Instructor view of a student's reviews does not show a composite score]]

==== Solution ====
The previous semester's team calculated the composite value within the view page which does not follow the design principles of this course. We plan to move this mathematical operation to the appropriate model where it where will be accessible to any view that requires a composite score for its heatmap.

The problem will have to be solved through changes in the two files listed below:

===== rscore.rb =====
This file contains the logic for doing mathematical functions for review tables. In the init function, the average score for each table is calculated. A new method can push the average score for each review into an array that can later be used to calculate the average across all reviews. Another method can reference this array to calculate the composite score across each review.

===== _view_heatgrid.html.erb =====
This file will call the composite score method described above to populate a string tag that displays the average score for all reviews on a view.

<hr>

=== Improved Handling of Relevant Reviews ===

==== Problem ====
The heatmap displayed on the Student view does not give the full representation of the group performance. The view should show both the reviews received from a students teammates as well as external reviews. The image below shows that the students can not see their teammates reviews even when the instructor checkbox for viewing teammate reviews is selected.

[[File:TeammateReviewsMissing.png|center|border|1000px|The "Your Scores" page for students does not show teammate reviews]]

==== Solution ====
We will need to improve how the review table(s) intended for display are generated.

The problem will have to be solved through changes in file listed below:

===== grades_controller.rb =====
This file contains the logic for orchestrating the build of the heatmap tables. The logic is this controller will need to be updated to populate the table array with reviews from each team participant.

===== view_team.html.erb =====
This file controls what tables are displayed when an instructor views a team. This view will needed to be updated to include the display the missing tables.

<hr>

=== Improved Display of Review Heatmap ===

==== Problem ====
This problem contains many parts. Firstly, the student scores view does not even show a heatmap of reviews at all as seen below.

[[File:no_student_heatmap.png|center|border|1000px|The my scores view page for the student doesn't show any teammate review heatmaps]]

This will have to be included with the condition that the instructor has selected that the student can view the reviews in the assignment options. Additionally, in the display of the heatmap itself there is no indication if the reviews displayed are reviews about the student signed in or made by the student signed in as seen below.

[[File:no_description_on_heatmap.png|center|border|1000px|The heatmap printout doesn't specify if showing reviews by or for a certain student]]

A more descriptive label will have to be added to show this. Lastly, a composite score will have to be displayed under each heatmap to average the scores that the student received.

==== Solution ====
The problem will have to be solved through changes in two erb files listed below:

===== _view_heatgrid.html.erb =====

This file contains the logic to display the actual heatgrid to the user. It will have to be modified in two ways. Firstly, we will have to add a conditional header that will either display that the heatgrid shows reviews for the student or by the student depending on the type of questionnaire that it is displaying. Secondly, we will have to add a place under the heatgrid to display the composite score of that heatgrid. In the previous attempt at this project, the group added the calculation logic for the composite score to this file which is incorrect because it is an erb file. Instead, this file will contain a call to a funtion in the model that will calculate the value and return it for display.

===== view_team.html.erb =====

This file contains the logic to display the page where the student views the questionnaires. We will have to add logic to display the reviews of that student if the assignment is set to have show teammate reviews be true.

<hr>

=== Preservation of Anonymized View Naming Convention ===

==== Problem ====
Student names are visible in the teammate reviews tab, even when the user is in the anonymized view. So the display when in an anonymized view, where no names are to be shown, but only “Student nnn”,“Instructor mmm”, etc. (Anonymized view is intended to be used in demos, so we can use live data to demo the system without showing anyone’s name.)

[[File:Anonnymized.jpg|center|border|1000px|]]
==== Solution ====
We plan to remove the team member name from being displayed when using anonymized view by adding some changes to the following files.

=====user.rb=====
In user file the anonymized user function is defined and when called it checks and returns True if a particular user is in anonymized view or else returns False. We will be improving the conditional logic to display the user name only when not present in anonymized view.

=====_view_heatgrid.html.erb=====
The file contains the logic to display the heatgrid to the user. When we make the changes to user file we will need to make some changes in this file to not display the user name in anonymized view.

==== Use Case Flow diagram for Anonymized View ====
[[File:uml_1.PNG|center|border|500px|UML for Anonymized]]

== Bugs ==
In addition to our required changes, we plan to explore the following bugs that we have found on the reviews page for the instructor:

Up until student 9020 all scores and averages are correct. It is theorized that the average score for student 9020 is being pulled from student 8977's scores on table 1. Student 9020 is showing review scores of student 9022 so the cells have become shifted by 1. The student 9020 review has essentially disappeared from the view. The average of each row is being populated in student 9042 due to this shift.

The top image shows the student 9022 review which you can see is populated in student 9020 column from the bottom image.

[[File:9022_student_review.PNG|center|border|1000px|Student 9022 review]]

[[File:Reviews page.png|center|border|1000px|Instructors reviews page should bugs on the table]]

== Testing ==

=== Flow to access affected views ===
==== _view_heatgrid.html.erb ====
This file can be accessed through the GUI differently when logged in as an instructor or a student. When logged in as a student:

# Click "Assignments" at the top of the screen
# Click desired assignment to view reviews from
# Click on "Your scores" button
# Scroll down past assignment score heatgrid
# You will see the reviews that others have made about you if the instructor has enabled that setting on this assignment

If you are logged in as an instructor:

# Click "Manage..." then "Assignments" at the top of the screen
# Click on the "view scores" button on the desired assignment
# Click the plus next to the team that you are interested in
# Click on the "Teammate Reviews" tab at the top of the panel
# Select the student that you wish to see the reviews about
# The relevant heatgrid will display with a descriptive title

=== Test Login Credentials ===
==== Instructor ====
* username: instructor6
* password: password
==== Student ====
* username: student7185
* password: password

=== Added Unit Tests ===
==== view_team_spec.rb ====
This test file was at first failing even without making any changes so first we fixed the failing test by filling in some incomplete factory creations in the set up of the test. Then, a negative check was added to make sure the teammate review scores are not displayed to the student while the assignment has show teammate reviews set to false. We had to do a negative case instead of positive because it was overly complicated to create a teammate review submission and a vm with that teammate review for the html file to register that there was a review to display. The added negative check looks like the code segment below:
 
<code>
expect(page).to_not have_text 'Teammate Review'
</code>

=== Added System Tests ===
RSpec tests will be added as a component of the first code submission.

=== Added System Tests ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View student reviews received from teammates and given to teammates from a student view ====

---Code still being developed---

==== 2) View teammate reviews as a student when enabled ====

The teammate reviews will be shown at the bottom of the screen because it has been enabled by the instructor:

[[File:yes_student_heatmap.png|center|border|1000px|Student view shows teammate reviews of student if allowed]]

==== 3) View composite score of reviews from a student view ====

---Code still being developed---

The following tests were conducted as follows:
 
- Sign in as instructor above
 
- Click Manage -> Assignments
 
- Click on the View Scores of Final Project assignment
 
- Click into any team

==== 1) View student reviews received from teammates and given to teammates from an instructor view ====

---Code still being developed---

==== 2) View composite score of reviews from an instructor view ====

---Code still being developed---

==== 3) Show descriptive name for heatmaps from an instructor view ====
The displayed heatgrid has a new, descriptive title describing exactly what reviews are being displayed:

[[File:yes_description_on_heatmap.png|center|border|1000px|Teammate Review Heatmap has descriptive title]]

=== Manual Testing for Anonymized User View: ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View your teammates names in the Team members section of review scores ====
In Normal user view the Users name or ID is displayed.
 
[[File:student_view_scores.png|center|border|900px|Anonymized View]]

==== 2) Switch to Anonymized View and view your teammates names in the Team members section of review scores ====
In Anonymized view the Users name or ID is not displayed.
 
[[File:anonymized_view.png|center|border|900px|Anonymized View]]

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-12-04T15:16:45Z

Mgfiles: Added description of changes to view_team_spec.rb

== Problem Description ==
In Expertiza, both students and instructors are able to view a student's teammate reviews. On the reviews page, only a single heatmap is shown which does not provide a full representation of the student's performance. In addition, it is unclear if the heatmap is displaying information about the reviews the student has written or received. The heatmaps on the instructor and student views are not labeled and do not provide clarity on the information they are displaying.

== Proposed Changes ==

=== Calculate Composite Review Score ===

==== Problem ====
It would be useful for the instructor to see a composite score derived from all of the reviews similar to the average score shown on the students review page. Currently, the code only displays the average score for each row of the review table on the instructors view. The image below shows where we would like to display the composite score.

[[File:Composite score.png|center|border|1000px|Instructor view of a student's reviews does not show a composite score]]

==== Solution ====
The previous semester's team calculated the composite value within the view page which does not follow the design principles of this course. We plan to move this mathematical operation to the appropriate model where it where will be accessible to any view that requires a composite score for its heatmap.

The problem will have to be solved through changes in the two files listed below:

===== rscore.rb =====
This file contains the logic for doing mathematical functions for review tables. In the init function, the average score for each table is calculated. A new method can push the average score for each review into an array that can later be used to calculate the average across all reviews. Another method can reference this array to calculate the composite score across each review.

===== _view_heatgrid.html.erb =====
This file will call the composite score method described above to populate a string tag that displays the average score for all reviews on a view.

<hr>

=== Improved Handling of Relevant Reviews ===

==== Problem ====
The heatmap displayed on the Student view does not give the full representation of the group performance. The view should show both the reviews received from a students teammates as well as external reviews. The image below shows that the students can not see their teammates reviews even when the instructor checkbox for viewing teammate reviews is selected.

[[File:TeammateReviewsMissing.png|center|border|1000px|The "Your Scores" page for students does not show teammate reviews]]

==== Solution ====
We will need to improve how the review table(s) intended for display are generated.

The problem will have to be solved through changes in file listed below:

===== grades_controller.rb =====
This file contains the logic for orchestrating the build of the heatmap tables. The logic is this controller will need to be updated to populate the table array with reviews from each team participant.

===== view_team.html.erb =====
This file controls what tables are displayed when an instructor views a team. This view will needed to be updated to include the display the missing tables.

<hr>

=== Improved Display of Review Heatmap ===

==== Problem ====
This problem contains many parts. Firstly, the student scores view does not even show a heatmap of reviews at all as seen below.

[[File:no_student_heatmap.png|center|border|1000px|The my scores view page for the student doesn't show any teammate review heatmaps]]

This will have to be included with the condition that the instructor has selected that the student can view the reviews in the assignment options. Additionally, in the display of the heatmap itself there is no indication if the reviews displayed are reviews about the student signed in or made by the student signed in as seen below.

[[File:no_description_on_heatmap.png|center|border|1000px|The heatmap printout doesn't specify if showing reviews by or for a certain student]]

A more descriptive label will have to be added to show this. Lastly, a composite score will have to be displayed under each heatmap to average the scores that the student received.

==== Solution ====
The problem will have to be solved through changes in two erb files listed below:

===== _view_heatgrid.html.erb =====

This file contains the logic to display the actual heatgrid to the user. It will have to be modified in two ways. Firstly, we will have to add a conditional header that will either display that the heatgrid shows reviews for the student or by the student depending on the type of questionnaire that it is displaying. Secondly, we will have to add a place under the heatgrid to display the composite score of that heatgrid. In the previous attempt at this project, the group added the calculation logic for the composite score to this file which is incorrect because it is an erb file. Instead, this file will contain a call to a funtion in the model that will calculate the value and return it for display.

===== view_team.html.erb =====

This file contains the logic to display the page where the student views the questionnaires. We will have to add logic to display the reviews of that student if the assignment is set to have show teammate reviews be true.

<hr>

=== Preservation of Anonymized View Naming Convention ===

==== Problem ====
Student names are visible in the teammate reviews tab, even when the user is in the anonymized view. So the display when in an anonymized view, where no names are to be shown, but only “Student nnn”,“Instructor mmm”, etc. (Anonymized view is intended to be used in demos, so we can use live data to demo the system without showing anyone’s name.)

[[File:Anonnymized.jpg|center|border|1000px|]]
==== Solution ====
We plan to remove the team member name from being displayed when using anonymized view by adding some changes to the following files.

=====user.rb=====
In user file the anonymized user function is defined and when called it checks and returns True if a particular user is in anonymized view or else returns False. We will be improving the conditional logic to display the user name only when not present in anonymized view.

=====_view_heatgrid.html.erb=====
The file contains the logic to display the heatgrid to the user. When we make the changes to user file we will need to make some changes in this file to not display the user name in anonymized view.

==== Use Case Flow diagram for Anonymized View ====
[[File:uml_1.PNG|center|border|500px|UML for Anonymized]]

== Bugs ==
In addition to our required changes, we plan to explore the following bugs that we have found on the reviews page for the instructor:

Up until student 9020 all scores and averages are correct. It is theorized that the average score for student 9020 is being pulled from student 8977's scores on table 1. Student 9020 is showing review scores of student 9022 so the cells have become shifted by 1. The student 9020 review has essentially disappeared from the view. The average of each row is being populated in student 9042 due to this shift.

The top image shows the student 9022 review which you can see is populated in student 9020 column from the bottom image.

[[File:9022_student_review.PNG|center|border|1000px|Student 9022 review]]

[[File:Reviews page.png|center|border|1000px|Instructors reviews page should bugs on the table]]

== Testing ==

=== Flow to access affected views ===
==== _view_heatgrid.html.erb ====
This file can be accessed through the GUI differently when logged in as an instructor or a student. When logged in as a student:

# Click "Assignments" at the top of the screen
# Click desired assignment to view reviews from
# Click on "Your scores" button
# Scroll down past assignment score heatgrid
# You will see the reviews that others have made about you if the instructor has enabled that setting on this assignment

If you are logged in as an instructor:

# Click "Manage..." then "Assignments" at the top of the screen
# Click on the "view scores" button on the desired assignment
# Click the plus next to the team that you are interested in
# Click on the "Teammate Reviews" tab at the top of the panel
# Select the student that you wish to see the reviews about
# The relevant heatgrid will display with a descriptive title

=== Test Login Credentials ===
==== Instructor ====
* username: instructor6
* password: password
==== Student ====
* username: student7185
* password: password

=== Added Unit Tests ===
==== view_team_spec.rb ====
This test file was at first failing even without making any changes so first we fixed the failing test by filling in some incomplete factory creations in the set up of the test. Then, a negative check was added to make sure the teammate review scores are not displayed to the student while the assignment has show teammate reviews set to false. We had to do a negative case instead of positive because it was overly complicated to create a teammate review submission and a vm with that teammate review for the html file to register that there was a review to display. The added negative check look like the code segment below:
 
<code>
expect(page).to_not have_text 'Teammate Review'
</code>

=== Added System Tests ===
RSpec tests will be added as a component of the first code submission.

=== Added System Tests ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View student reviews received from teammates and given to teammates from a student view ====

---Code still being developed---

==== 2) View teammate reviews as a student when enabled ====

The teammate reviews will be shown at the bottom of the screen because it has been enabled by the instructor:

[[File:yes_student_heatmap.png|center|border|1000px|Student view shows teammate reviews of student if allowed]]

==== 3) View composite score of reviews from a student view ====

---Code still being developed---

The following tests were conducted as follows:
 
- Sign in as instructor above
 
- Click Manage -> Assignments
 
- Click on the View Scores of Final Project assignment
 
- Click into any team

==== 1) View student reviews received from teammates and given to teammates from an instructor view ====

---Code still being developed---

==== 2) View composite score of reviews from an instructor view ====

---Code still being developed---

==== 3) Show descriptive name for heatmaps from an instructor view ====
The displayed heatgrid has a new, descriptive title describing exactly what reviews are being displayed:

[[File:yes_description_on_heatmap.png|center|border|1000px|Teammate Review Heatmap has descriptive title]]

=== Manual Testing for Anonymized User View: ===
The following tests were conducted as follows:
 
- Sign in as test student7185 above
 
- Navigate to assignments
 
- Click on the Final Project assignment
 
- Click on "Your scores" button
 

==== 1) View your teammates names in the Team members section of review scores ====
In Normal user view the Users name or ID is displayed.
 
[[File:student_view_scores.png|center|border|900px|Anonymized View]]

==== 2) Switch to Anonymized View and view your teammates names in the Team members section of review scores ====
In Anonymized view the Users name or ID is not displayed.
 
[[File:anonymized_view.png|center|border|900px|Anonymized View]]

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-21T19:22:05Z

Mgfiles: /* Added System Tests */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:37:59Z

Mgfiles: /* Added System Tests */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:34:17Z

Mgfiles: /* Added System Tests */

File:Yes student heatmap.png

2022-11-20T18:31:31Z

Mgfiles:

File:Yes description on heatmap.png

2022-11-20T18:31:18Z

Mgfiles:

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:25:56Z

Mgfiles: /* Flow to access affected views */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:18:47Z

Mgfiles: /* Test Login Credentials */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:18:19Z

Mgfiles: /* Test Login Credentials */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-20T18:17:07Z

Mgfiles: /* Flow to access affected views */

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-14T17:33:24Z

Mgfiles: Edited the size of images for readability

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-14T17:32:29Z

Mgfiles: Added images to describe my problem section

File:No description on heatmap.png

2022-11-14T17:28:12Z

Mgfiles:

File:No student heatmap.png

2022-11-14T17:28:01Z

Mgfiles:

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-11T14:37:05Z

Mgfiles: Added description of changes to fix the improved display of heatmap problem

== Problem Description ==

== Proposed Changes ==

=== Calculate Composite Review Score ===

=== Improved Handling of Relevant Reviews ===

=== Improved Display of Review Heatmap ===

==== Problem ====
This problem contains many parts. Firstly, the student scores view does not even show a heatmap of reviews at all. This will have to be included with the condition that the instructor has selected that the student can view the reviews in the assignment options. Additionally, in the display of the heatmap itself there is no indication if the reviews displayed are reviews about the student signed in or made by the student signed in. A more descriptive label will have to be added to show this. Lastly, a composite score will have to be displayed under each heatmap to average the scores that the student received.

==== Solution ====
The problem will have to be solved through changes in two erb files listed below:

===== _view_heatgrid.html.erb =====

This file contains the logic to display the actual heatgrid to the user. It will have to be modified in two ways. Firstly, we will have to add a conditional header that will either display that the heatgrid shows reviews for the student or by the student depending on the type of questionnaire that it is displaying. Secondly, we will have to add a place under the heatgrid to display the composite score of that heatgrid. In the previous attempt at this project, the group added the calculation logic for the composite score to this file which is incorrect because it is an erb file. Instead, this file will contain a call to a funtion in the model that will calculate the value and return it for display.

===== view_team.html.erb =====

This file contains the logic to display the page where the student views the questionnaires. We will have to add logic to display the reviews of that student if the assignment is set to have show teammate reviews be true.

=== Preservation of Anonymized View Naming Convention ===

== Testing ==

=== Flow to access affected views ===

=== Test Login Credentials ===

=== Added Unit Tests ===

=== Added System Tests ===

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-11T00:06:21Z

Mgfiles: Establish Section Headings

== Problem Description ==

== Proposed Changes ==

=== Calculate Composite Review Score ===

=== Improved Haling of Relevant Reviews ===

=== Improved Display of Review Heatmap ===

=== Preservation of Anonymized View Naming Convention ===

== Testing ==

=== Flow to access affected views ===

=== Test Login Credentials ===

=== Added Unit Tests ===

=== Added System Tests ===

CSC/ECE 517 Fall 2022 - E2282. Fix teammate-review view

2022-11-10T23:56:15Z

Mgfiles: beginning edit

Todo: Write page

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-11-01T18:32:52Z

Mgfiles: Added pictures for the completed manual/system tests

== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updated at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.

== Files Modified ==
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Bugfix: Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze. This was beyond the scope of our work but we wanted to improve the page functionality.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}

=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Added two new RSpec tests for the <code>update_password</code> method
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
|-
|2
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit] [https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
|-
|3
|Added <code>User</code> and <code>PasswordReset</code> factories and removed hardcoded variables
|Cleaned up hardcoded <code>User</code> and <code>PasswordReset</code> models with premade factories to improve readability of the code
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit] [https://github.com/expertiza/expertiza/commit/96376e0b633e6b6e08d472a4cb4600d20e0c024f Commit]
|-
|}

=== Changes to <code>spec/factories/password_retrieval_factory.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Created a file to host the <code>password_retrieval_controller_spec.rb</code> fixtures
|We implemented factories in the rspec test file to improve modularity and code readability.
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]
|-
|}

=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}

=== Changes to <code>app/controllers/auth_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}

=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|2
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}

== Testing ==

'''Everything in the testing segment was beyond the scope of our work. However, we wanted to validate our code through RSpec testing before merging into the <code>main</code> Expertiza branch. In the second submission phase, we plan to further enhance the testing suite of the <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> through factories and fixtures.'''

=== Testing <code>password_retrieval_controller.rb</code> ===
Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]

After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]

We implemented factories in the rspec test file to improve modularity and code readability. Factories provide more flexibility in generating models to meet the requirements of the test, as opposed to fixtures. As you can see from the below images, the code is significantly cleaner as many lines of hardcoded strings have been removed.
[[File:prefactory.png|center|frame|rspec tests before factories were implemented]] 
[[File:postfactory.png|center|frame|rspec tests after factories were implemented]]

The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]

=== Testing <code>auth_controller_.rb</code> ===
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]

We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_improving_auth_controller_spec.png|center|frame|rspec tests continuing to all pass after improving and adding to the auth_controller tests]]

Tests prior to the changes covered 91.94% of <code>auth_controller.rb</code>.
[[File:coverage_before.png|center|frame|rspec test coverage report before the improvements to the auth_controller unit tests]]

This is already a good coverage but after my changes I wanted to ensure my changes were also tested thoroughly. After adding tests, the tests covered 95.24% of <code>auth_controller.rb</code>. It improved slightly to be an even better coverage.
[[File:coverage_after.png|center|frame|rspec test coverage report after the improvements to the auth_controller unit tests]]

=== Test Plan - Additional Manual/System Test Cases ===
Along with the unit tests that we have written to test our files, we conducted a few system tests manually to verify the functionality works as expected. The <code>password_retrieval_controller.rb</code> is difficult for us to test because we don't have access to the email used to make the sample account, but the <code>auth_controller.rb</code> is very testable. Below is the tests we completed listed in the order of completion.

==== 1) Test instructor login & redirect to home page ====
When navigating to the expertiza website, our branch correctly displayed the login page as shown below.
[[File:before_login.png|center|border|Form shown to the user before logging in to expertiza]]
After typing in the sample login credentials of "instructor6" and "password," the user is then correctly logged in and redirected to the home page for instructors shown below.
[[File:after_login.png|center|border|1400px|Page shown to the user after logging in to expertiza]]
==== 2) Test instructor logout & redirect to login page ====
Then, when the user clicks the logout button, they are correctly logged out and redirected once again to the login page as shown below.
[[File:after_logout.png|center|border|Form shown to the user after logging out of expertiza]]
==== 3) Test incorrect login ====
When the user attempts to login but enters the incorrect login information of "instructor6" and "incorrect," the user is shown the error message and then correctly redirected to the forgot password page shown below.
[[File:after_incorrect_login.png|center|border|1400px|Form shown to the user after an incorrect login attempt to expertiza]]
== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/

== Contributors to this project ==
* Grey Files (unityid: mgfiles, github: greyfiles)
* Colin O'Dowd (unityid: cdodowd, github: colin-odowd)
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)

File:Before login.png

2022-11-01T18:25:33Z

Mgfiles: Screen shown before the user logs in

Screen shown before the user logs in

File:After logout.png

2022-11-01T18:25:19Z

Mgfiles: Screen shown after the user logs out

Screen shown after the user logs out

File:After login.png

2022-11-01T18:25:00Z

Mgfiles: Screen shown after a correct login

Screen shown after a correct login

File:After incorrect login.png

2022-11-01T18:24:42Z

Mgfiles: Screen shown after an incorrect login

Screen shown after an incorrect login

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-11-01T18:24:16Z

Mgfiles: Added explanation of manual system tests completed on the branch

== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updated at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.

== Files Modified ==
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Bugfix: Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze. This was beyond the scope of our work but we wanted to improve the page functionality.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}

=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Added two new RSpec tests for the <code>update_password</code> method
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
|-
|2
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit] [https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
|-
|3
|Added <code>User</code> and <code>PasswordReset</code> factories and removed hardcoded variables
|Cleaned up hardcoded <code>User</code> and <code>PasswordReset</code> models with premade factories to improve readability of the code
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit] [https://github.com/expertiza/expertiza/commit/96376e0b633e6b6e08d472a4cb4600d20e0c024f Commit]
|-
|}

=== Changes to <code>spec/factories/password_retrieval_factory.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Created a file to host the <code>password_retrieval_controller_spec.rb</code> fixtures
|We implemented factories in the rspec test file to improve modularity and code readability.
|[https://github.com/expertiza/expertiza/commit/e946533235846a853ee42908b15e75d628552f9e Commit]
|-
|}

=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}

=== Changes to <code>app/controllers/auth_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}

=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|2
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}

== Testing ==

'''Everything in the testing segment was beyond the scope of our work. However, we wanted to validate our code through RSpec testing before merging into the <code>main</code> Expertiza branch. In the second submission phase, we plan to further enhance the testing suite of the <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> through factories and fixtures.'''

=== Testing <code>password_retrieval_controller.rb</code> ===
Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]

After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]

We implemented factories in the rspec test file to improve modularity and code readability. Factories provide more flexibility in generating models to meet the requirements of the test, as opposed to fixtures. As you can see from the below images, the code is significantly cleaner as many lines of hardcoded strings have been removed.
[[File:prefactory.png|center|frame|rspec tests before factories were implemented]] 
[[File:postfactory.png|center|frame|rspec tests after factories were implemented]]

The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]

=== Testing <code>auth_controller_.rb</code> ===
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]

We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_improving_auth_controller_spec.png|center|frame|rspec tests continuing to all pass after improving and adding to the auth_controller tests]]

Tests prior to the changes covered 91.94% of <code>auth_controller.rb</code>.
[[File:coverage_before.png|center|frame|rspec test coverage report before the improvements to the auth_controller unit tests]]

This is already a good coverage but after my changes I wanted to ensure my changes were also tested thoroughly. After adding tests, the tests covered 95.24% of <code>auth_controller.rb</code>. It improved slightly to be an even better coverage.
[[File:coverage_after.png|center|frame|rspec test coverage report after the improvements to the auth_controller unit tests]]

=== Test Plan - Additional Manual/System Test Cases ===
Along with the unit tests that we have written to test our files, we conducted a few system tests manually to verify the functionality works as expected. The <code>password_retrieval_controller.rb</code> is difficult for us to test because we don't have access to the email used to make the sample account, but the <code>auth_controller.rb</code> is very testable. Below is the tests we completed listed in the order of completion.

==== 1) Test instructor login & redirect to home page ====
When navigating to the expertiza website, our branch correctly displayed the login page as shown below.
//before_login.png
After typing in the sample login credentials of "instructor6" and "password," the user is then correctly logged in and redirected to the home page for instructors shown below.
//after_login.png
==== 2) Test instructor logout & redirect to login page ====
Then, when the user clicks the logout button, they are correctly logged out and redirected once again to the login page as shown below.
//after_logout.png
==== 3) Test incorrect login ====
When the user attempts to login but enters the incorrect login information of "instructor6" and "incorrect," the user is shown the error message and then correctly redirected to the forgot password page shown below.
//after_incorrect_login.png
== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/

== Contributors to this project ==
* Grey Files (unityid: mgfiles, github: greyfiles)
* Colin O'Dowd (unityid: cdodowd, github: colin-odowd)
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-26T20:37:43Z

Mgfiles: Added code coverage numbers for auth_controller.rb

== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updates at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.

== Files Modified ==
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}

=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Added two new RSpec tests for the <code>update_password</code> method
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
|-
|2
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit] [https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
|-
|}

=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}

=== Changes to <code>app/controllers/auth_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}

=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|2
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}

== Testing ==

=== Testing <code>password_retrieval_controller.rb</code> ===
Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]

After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]

The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]

=== Testing <code>auth_controller_.rb</code> ===
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]

We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_improving_auth_controller_spec.png|center|frame|rspec tests continuing to all pass after improving and adding to the auth_controller tests]]

Tests prior to the changes covered 91.94% of <code>auth_controller.rb</code>.
[[File:coverage_before.png|center|frame|rspec test coverage report before the improvements to the auth_controller unit tests]]

This is already a good coverage but after my changes I wanted to ensure my changes were also tested thoroughly. After adding tests, the tests covered 95.24% of <code>auth_controller.rb</code>. It improved slightly to be an even better coverage.
[[File:coverage_after.png|center|frame|rspec test coverage report after the improvements to the auth_controller unit tests]]

== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/

== Contributors to this project ==
* Grey Files (unityid: mgfiles, github: greyfiles)
* Colin Odowd (unityid: cdodowd, github: colin-odowd)
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)

File:Coverage after.png

2022-10-26T20:34:05Z

Mgfiles: Code coverage of auth_controller.rb after adding tests

Code coverage of auth_controller.rb after adding tests

File:Coverage before.png

2022-10-26T20:33:41Z

Mgfiles: Code coverage of auth_controller.rb before adding tests

Code coverage of auth_controller.rb before adding tests

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-26T19:35:05Z

Mgfiles: Added the image showing the improved auth_controller_spec.rb running and passing

== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updates at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.

== Files Modified ==
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}

=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Added two new RSpec tests for the <code>update_password</code> method
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
|-
|2
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit] [https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
|-
|}

=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}

=== Changes to <code>app/controllers/auth_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}

=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Improved existing tests for <code>after_login</code> to explicitly test for a redirect
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|2
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}

== Testing ==

=== Testing <code>password_retrieval_controller.rb</code> ===
Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]

After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]

The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]

=== Testing <code>auth_controller_.rb</code> ===
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]

We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_improving_auth_controller_spec.png|center|frame|rspec tests continuing to all pass after improving and adding to the auth_controller tests]]

== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/

== Contributors to this project ==
* Grey Files (unityid: mgfiles, github: greyfiles)
* Colin Odowd (unityid: cdodowd, github: colin-odowd)
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)

File:After improving auth controller spec.png

2022-10-26T19:33:08Z

Mgfiles: Tests running for auth_controller.rb after improving and adding more tests

Tests running for auth_controller.rb after improving and adding more tests

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-26T19:31:50Z

Mgfiles: Added description of auth_controller part of project and auth_controller_spec changes

== Overview of Expertiza ==
Expertiza is an open source software written using Ruby on Rails which functions as a learning management software system. It has man different functions and abilities including the ability to create assignments, quizzes, assignment groups and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system for user authentication with different user roles and permissions that determine how each user interacts with the content. The <code>auth_controller.rb</code> and <code>password_retrieval_controller.rb</code> which are the files primarily addressed in this project are both critical controllers in providing this functionality.
== Description of Project ==
<code>auth_controller</code> is used for authentication purposes. The controller completes a variety of tasks including handling the correct user logins, incorrect passwords, unknown users, and making sure the session and role information is updates at all points in that process. The original problem description listed three issues, two of which had since been corrected by other code changes since the document was written. The remaining problem was that some logger messages were included in methods that could be placed more cleanly in <code>before_action</code> methods. Also, I personally noticed that there was a few lines of repeated code involved in resetting the role cache that needed to be combined into a shared private method.

The <code>password_retrieval_controller</code> deals with the process of updating and resetting a user password. The <code>send_password</code> method generates a token and appends it to a password reset URL. If a user submits a valid email address on the <code>password_retrieval/forgotten</code> view, the URL is sent to the user's email. When a user goes to the password reset URL, the token parameter is decrypted and checked for expiration. Next, the <code>password_retrieval/reset_password</code> view is loaded where a user enters an updated password and is sent back to the home page. In this project, the method was refactored in the following ways: to adhere to DRY principles, removal of hardwired constants, renaming of methods and variables, and enhanced comments. In addition, RSpec testing coverage of the controller was improved from 63.33% to 91.8% through a series of new tests that validate the functionality of the <code>update_password</code> and <code>send_password</code> methods.

== Files Modified ==
=== Changes to <code>app/controllers/password_retrieval_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated <code>check_reset_url</code> method name to <code>check_token_validity</code>
|The method validates that the password reset token is valid and present. The updated method name provides a more functionally descriptive name.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|2
|Replaced repeated code in lines 35-36 and 62-63
|The use of repeated code violates the DRY principle and so it was moved to a new method.
|[https://github.com/expertiza/expertiza/commit/5429abd6fcb39f7bdbb0aaa1813f19c8101d7e25 Commit]
|-
|3
|Change token expiration time to constant in line 41
|This time should not be hardwired; it should be a constant or a parameter.
|[https://github.com/expertiza/expertiza/commit/3f0b51f6f2f106df8338483396a95d4068e39c7f Commit]
|-
|4
|Reload page if email is nil or empty on <code>password_retrieval/forgotten</code> view
|An empty email parameter was causing the send password button to freeze.
|[https://github.com/expertiza/expertiza/commit/70f77ac851b234f840709859dc1ee9d6725c34fc Commit]
|-
|5
|Improve overall comments and rewrite error messages
|The comments and error messages in the controller need to be more meaningful, specific and clear.
|[https://github.com/expertiza/expertiza/commit/1af745dc3b59641cb0266ebe49ee996718381fd0 Commit]
|}

=== Changes to <code>spec/controllers/password_retrieval_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Added two new RSpec tests for the <code>update_password</code> method
|There were no tests for the <code>update_password</code> method. We wanted to enhance the test suite of this controller by increasing the coverage of its Rspec tests.
|[https://github.com/expertiza/expertiza/commit/a90b1aada9878c7cdf7319dd022432cca8eadd2f Commit]
|-
|2
|Added two new RSpec tests for the <code>send_password</code> method to check nil or blank input for email
|There were no tests for the <code>send_password</code> method pertaining to checking invalid inputs in the request params
|[https://github.com/greyfiles/expertiza/commit/1d2d2d94ef730ab427bb326049bc5ec800a0dfc9 Commit] [https://github.com/greyfiles/expertiza/commit/781d6f42ca37829e0e514de8bcef1c85b2a035a2 Commit]
|-
|}

=== Changes to <code>config/routes.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Updated URL path and controller action to updated method name <code>check_token_validity</code>
|The action and URL path must be renamed to generate pathing to the controller method and views.
|[https://github.com/expertiza/expertiza/commit/3f9f63ab51e90743dfab0b860574aa9b673f2717 Commit]
|-
|}

=== Changes to <code>app/controllers/auth_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Move logger messages to <code>before_action</code> blocks wherever possible
|Logger messages are inserted to log important events occurring in the code and do not relate directly to the logic. When possible, moving them to either <code>before_action</code> or <code>after_action</code> blocks makes the code more readable and easier to understand. It also separates the functionality of the method itself and the logging functionality.
|[https://github.com/greyfiles/expertiza/commit/7069f5d3cbfa2b7259e85e39dbfbf6fb41a0ce1d Commit]
|-
|2
|*Not in required chagnes* Replaced repeated code for re-caching the user role
|We noticed that although not listed on the recommended changes, this action involved exactly repeated code in the controller. The use of repeated code violates the DRY principle and so it was moved to a new method called <code>self.rebuild_role_cache</code>.
|[https://github.com/greyfiles/expertiza/commit/9ef20cffa0fe7b8440b97856a6db4b5351eece35 Commit]
|-
|3
|Improved helper function names
|Originally we made the new helper functions used in logging have unhelpful, confusing names. Making them more clear helps the code to be more understandable.
|[https://github.com/greyfiles/expertiza/commit/32f8435255add7b44b38fd747f81f435d331d14d Commit]
|}

=== Changes to <code>spec/controllers/auth_controller_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! Commit Link
|-
|1
|Improved existing tests for <code>after_login</code> to explicitly tests for a redirect
|When looking over the existing test cases, I noticed that the test that was verifying that the <code>after_login</code> method would redirect the user only "allowed" the redirect and did not "expect" it. I changed it to "expect" the redirect to ensure that functionality is working.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|2
|Added a test for <code>set_current_role</code> when the role is found to make sure it rebuilds the role cache
|In the unit tests for the <code>set_current_role</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the current role was set.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|-
|3
|Added a test for <code>clear_user_info</code> to make sure it rebuilds the role cache
|In the unit tests for the <code>clear_user_info</code> method, my new addition of a private helper method allowed us to ensure that the role cache was being rebuilt when the user's info was cleared.
|[https://github.com/greyfiles/expertiza/commit/23b3eaa888810f93a4af18c2ca6904ab21580a4b Commit]
|}

== Testing ==

=== Testing <code>password_retrieval_controller.rb</code> ===
Before any refactoring to auth_controller.rb was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:before_refactor_password_retrieval_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

Tests prior to the changes covered 63.3% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_before.png|center|frame|rspec test coverage report before the refactoring was completed]]

After adding tests, the tests covered 91.1% of <code>password_retrieval_controller.rb</code>.
[[File:E2252_password_retrieval_coverage_report_after.png|center|frame|rspec test coverage report after the refactoring was completed]]

The below image shows the output of the following command after all tests were added: <code>rspec spec/controllers/password_retrieval_controller_spec.rb</code>

[[File:After refactor password retrieval controller.png|center|frame|rspec tests all passing after the refactoring was completed]]

=== Testing <code>auth_controller_.rb</code> ===
Before any refactoring to <code>auth_controller.rb</code> was done, we ran the rspec tests created for the controller with the following command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:before_refactor_auth_controller.png|center|frame|rspec tests all passing before the refactoring was completed]]

After making all of the above changes to <code>auth_controller.rb</code>, we ran the rspec tests for the controller again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

[[File:after_refactor_auth_controller.png|center|frame|rspec tests continuing to all pass after completing the refactoring]]

We have successfully preserved the passing tests after the improvements we made to the <code>auth_controller.rb</code>. Next we looked at the existing tests to see if they could be improved. As listed in the files changed above, there were a few improvements to be made to <code>auth_controller_spec.rb</code>. We improved a check for redirecting the user after logging in and added two tests to make sure the role cache was being rebuilt after both setting the current role and clearing the user info. We ran the tests again with the command: <code>rspec spec/controllers/auth_controller_spec.rb</code>

== Relevant Links ==
* '''Github Repository:''' https://github.com/greyfiles/expertiza
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2460
* '''VCL Server:''' http://152.7.98.115:8080/

== Contributors to this project ==
* Grey Files (unityid: mgfiles, github: greyfiles)
* Colin Odowd (unityid: cdodowd, github: colin-odowd)
* Pradyumna Khawas (unityid: ppkhawas, github: therealppk)

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-26T02:15:09Z

Mgfiles: Added information of the participants in the group

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-25T22:35:27Z

Mgfiles: Added testing description for the auth_controller.rb existing tests

File:After refactor auth controller.png

2022-10-25T22:22:21Z

Mgfiles: rspec tests for auth_controller after refactoring

rspec tests for auth_controller after refactoring

File:Before refactor auth controller.png

2022-10-25T22:21:36Z

Mgfiles: rspec tests for auth_controller.rb before refactoring

rspec tests for auth_controller.rb before refactoring

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-25T22:20:17Z

Mgfiles: Added description of expertiza and changes to auth_controller.rb

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-24T22:29:44Z

Mgfiles: Added descriptions of each section to guide group members when contributing

== Overview of Expertiza ==
TODO: Add description of general expertiza system and how our controllers relate to the overall functionality.
== Description of Project ==
TODO: Describe the objectives of the project and what issues we were attempting to address.
== Files Modified ==
TODO: List all files that were modified and the changes made to them. Include what principle was violated/what the problem was and how the change fixed it.
== Testing ==
TODO: Show how the existing testing suite was passing before and after our refactoring - preserving functionality.

CSC/ECE 517 Fall 2022 - E2252. Refactor auth controller.rb & password retrieval controller.rb

2022-10-24T22:23:44Z

Mgfiles: Establish basic outline of page

Overview of Expertiza
Description of Project
Files Modified
Testing