Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T04:07:31Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before (this image is out of date, the return functionality for the varying rubric report rounds has since been generalized, please see the repo for the updated version) (The updated version does not include the old version comments, which is why this screenshot is used):

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Files ===

The files that were added or modified to our repository are shown below.

==== Models ====

The following files were copied from the expertiza/expertiza repository:
* analytic/assignment_team_analytic.rb
* assignment_team.rb
* feedback_response_map.rb

Added assignment_team.rb and assignment_team_analytic.rb as the existing spec tests depended on those 2 files. No changes were made to these files other than commenting out include Scoring in assignment_team.rb

feedback_response_map.rb

Many comments were placed throughout the document in order to make code more readable.

The main changes were taking feedback_response_report method and scanning it, to find different functionalities that could be taken out of it and turned into the following private helper methods: get_feedback_authors, varying_rubrics_report, static_rubrics_report.

Additionally, the variable @temp_response_map_ids was changed into response_map_ids both inside and outside the helper methods that used this variable. The variable @temp_review_responses was not changed except for being called review_responses in the helper methods.

==== Database ====

migrate/20241203083135_add_missing_reponse_map_fields.rb

One class is added, called AddMissingResponseMapFields, with a change of adding columns to response_maps and type and calibrate_to.

db/schema.rb

The only things changed in this file were a version change, as well as the addition of a t.string of "type" and a t.integer of "calibrate_to".

==== Spec ====

factories.rb

This file was modified to use single quotation marks instead of double quotation marks to adhere to RuboCop.

factories/factories.rb

We populated the factory with necessary factories such as response, course, institution, and role, as well as fixing the factory formatting to function properly.

feedback_response_map.rb

The code was updated to adhere to RuboCop guidelines. Secondly, tests and parts of tests were commented out if they were reliant on functionality that was not yet implemented in reimplementation-back-end.

=== Video ===

https://youtu.be/SbmDoJNm4Q4

=== Conclusion ===

By refactoring feedback_response_map.rb, this project enhances Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T03:58:23Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before (this image is out of date, the return functionality for the varying rubric report rounds has since been generalized, please see the repo for the updated version) (The updated version does not include the old version comments, which is why this screenshot is used):

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Files ===

The files that were added or modified to our repository are shown below.

==== Models ====

The following files were copied from the expertiza/expertiza repository:
* analytic/assignment_team_analytic.rb
* assignment_team.rb
* feedback_response_map.rb

feedback_response_map.rb

Many comments were placed throughout the document in order to make code more readable.

The main changes were taking feedback_response_report method and scanning it, to find different functionalities that could be taken out of it and turned into the following private helper methods: get_feedback_authors, varying_rubrics_report, static_rubrics_report.

Additionally, the variable @temp_response_map_ids was changed into response_map_ids both inside and outside the helper methods that used this variable. The variable @temp_review_responses was not changed except for being called review_responses in the helper methods.

==== Database ====

migrate/20241203083135_add_missing_reponse_map_fields.rb

One class is added, called AddMissingResponseMapFields, with a change of adding columns to response_maps and type and calibrate_to.

db/schema.rb

The only things changed in this file were a version change, as well as the addition of a t.string of "type" and a t.integer of "calibrate_to".

==== Spec ====

factories.rb

This file was modified to use single quotation marks instead of double quotation marks to adhere to RuboCop.

factories/factories.rb

We populated the factory with necessary factories to run our tests.

feedback_response_map.rb

The code was updated to adhere to RuboCop guidelines. Secondly, tests and parts of tests were commented out if they were reliant on functionality that was not yet implemented in reimplementation-back-end.

=== Video ===

https://youtu.be/SbmDoJNm4Q4

=== Conclusion ===

By refactoring feedback_response_map.rb, this project enhances Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T03:53:05Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before (this image is out of date, the return functionality for the varying rubric report rounds has since been generalized, please see the repo for the updated version) (The updated version does not include the old version comments, which is why this screenshot is used):

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Files ===

The files that were added or modified to our repository are shown below.

==== Models ====

The following files were copied from the expertiza/expertiza repository:
* analytic/assignment_team_analytic.rb
* assignment_team.rb
* feedback_response_map.rb

feedback_response_map.rb

Many comments were placed throughout the document in order to make code more readable.

The main changes were taking feedback_response_report method and scanning it, to find different functionalities that could be taken out of it and turned into the following private helper methods: get_feedback_authors, varying_rubrics_report, static_rubrics_report.

Additionally, the variable @temp_response_map_ids was changed into response_map_ids both inside and outside the helper methods that used this variable. The variable @temp_review_responses was not changed except for being called review_responses in the helper methods.

==== Database ====

migrate/20241203083135_add_missing_reponse_map_fields.rb

One class is added, called AddMissingResponseMapFields, with a change of adding columns to response_maps and type and calibrate_to.

db/schema.rb

The only things changed in this file were a version change, as well as the addition of a t.string of "type" and a t.integer of "calibrate_to".

==== Spec ====

factories.rb

This file was modified to use single quotation marks instead of double quotation marks.

factories/factories.rb

We populated the factory with necessary factories to run our tests.

feedback_response_map.rb

=== Video ===

https://youtu.be/SbmDoJNm4Q4

=== Conclusion ===

By refactoring feedback_response_map.rb, this project enhances Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T03:31:53Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before (this image is out of date, the return functionality for the varying rubric report rounds has since been generalized, please see the repo for the updated version) (The updated version does not include the old version comments, which is why this screenshot is used):

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Files ===

==== Models ====

The following files were copied from the expertiza/expertiza repository:
* analytic/assignment_team_analytic.rb
* assignment_team.rb
* feedback_response_map.rb

feedback_response_map.rb

fds

==== Database ====

migrate/20241203083135_add_missing_reponse_map_fields.rb

db/schema.rb

==== Spec ====

factories.rb

factories/factories.rb

feedback_response_map.rb

=== Video ===

https://youtu.be/SbmDoJNm4Q4

=== Conclusion ===

By refactoring feedback_response_map.rb, this project enhances Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

File:E2451-UpdatedArraysMain.png

2024-12-04T02:32:14Z

Sdaniel8: Sdaniel8 uploaded a new version of File:E2451-UpdatedArraysMain.png

File:E2451-UpdatedArraysMain.png

2024-12-04T02:30:16Z

Sdaniel8: Sdaniel8 uploaded a new version of File:E2451-UpdatedArraysMain.png

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T01:51:54Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to access id, since it was a parameter given to feedback_response_report. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before:

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Conclusion ===

By refactoring feedback_response_map.rb, this project will enhance Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-04T01:29:19Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

The following are the original arrays used to statically represent the review rounds.

[[File:E2451-OriginalArrayRounds.png]]

We can take these multiple arrays, and use dictionaries instead, where each round is called by its key value (integer value).

[[File:E2451-UpdatedArraysMain.png | 600px]]

The code found in the feedback_response_report method that use the arrays were moved to the two helper methods, seen as follows:

[[File:E2451-UpdatedArrays1.png | 600px]]

[[File:E2451-UpdatedArrays2.png | 600px]]

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to ____. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

Final UML for FeedbackResponseMap

[[File:E2451_FeedbackResponseMap_final.drawio.png | 600px]]

Authors

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before_1.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after_1.png]]

Report

This image contains comments showing what the implementation looked like before:

[[File:E2451_reports_before_1.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying_1.png]]

[[File:E2451_report_static_1.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

Complexity changes

Previously, Rubocop reported the following failures on behalf of the self.feedback_response_report method:

Metrics/PerceivedComplexity: Perceived complexity for feedback_response_report is too high. [14/8]
Metrics/CyclomaticComplexity: Cyclomatic complexity for feedback_response_report is too high. [12/7]
Metrics/AbcSize: Assignment Branch Condition size for feedback_response_report is too high. [<14, 46, 16> 50.68/20]

Now, neither self.feedback_response_report nor any of its associated private methods trigger any of these Metric violations!
(Note that the email method does trigger a AbcSize violation when the protective comments are removed, but adjusting that method is beyond the scope of this project for reasons listed in the Implementation Plan)

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Conclusion ===

By refactoring feedback_response_map.rb, this project will enhance Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

File:E2451-UpdatedArrays2.png

2024-12-04T01:27:48Z

Sdaniel8:

File:E2451-UpdatedArrays1.png

2024-12-04T01:27:14Z

Sdaniel8:

File:E2451-UpdatedArraysMain.png

2024-12-04T01:26:43Z

Sdaniel8:

File:E2451-OriginalArrayRounds.png

2024-12-04T00:52:02Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-03T23:47:22Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

We can take the multiple arrays found to represent rounds, and use dictionaries instead, where each round is called by its key value (integer value).

INSERT PIC OF BEFORE AND AFTER OF ARRAYS/DICTS HERE

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to refactor the code. All of the offenses can be seen below:

[[File:E2451-Rubocop1.png | 600px]]

[[File:E2451-Rubocop2.png | 600px]]

[[File:E2451-Rubocop3.png | 600px]]

[[File:E2451-Rubocop4.png | 600px]]

All of these offenses were fixed. The following three offenses were the original ones. For the first and third offenses, the colon after the method headers were simply removed. For the second offense, the “return” keyword caused errors, thus, it was removed, because a ruby method will return its last statement. The two other private methods containing “return” also had that keyword removed.

[[File:E2451-RubocopOriginal.png | 600px]]

Lastly, commenting was added throughout the files, to improve readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to ____. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

INSERT UPDATED UML DIAGRAM AND NEW IMAGES OF THE FILES

4. Research future refactoring for email method

5. Improve variable naming and structure

====Authors====

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after.png]]

====Report====

This image contains comments showing what the implementation looked like before:

[[File:E2451_reports_before.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying.png]]

[[File:E2451_report_static.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

[MAYBE ADD COMPLEXITY METRICS BEFORE AND AFTER HERE?]

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Conclusion ===

By refactoring feedback_response_map.rb, this project will enhance Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

File:E2451-RubocopOriginal.png

2024-12-03T23:47:17Z

Sdaniel8:

File:E2451-Rubocop4.png

2024-12-03T23:42:05Z

Sdaniel8:

File:E2451-Rubocop3.png

2024-12-03T23:41:43Z

Sdaniel8:

File:E2451-Rubocop2.png

2024-12-03T23:41:00Z

Sdaniel8:

File:E2451-Rubocop1.png

2024-12-03T23:38:06Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-12-03T21:26:48Z

Sdaniel8:

This page contains the details of OSS project ''E2451: Reimplement feedback response map'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions, among other tasks. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

=== Project Overview ===

The goal of this project is to refactor the feedback_response_map.rb file from the Expertiza repository and integrate it into the reimplementation-back-end repository. Within Expertiza, when a user submits a review, an instance of the Response class is created, associated with a ResponseMap. Each ResponseMap links a reviewer (reviewer_id), reviewee (reviewee_id), and the reviewed item (reviewed_object_id). FeedbackResponseMap, a subclass of ResponseMap, represents the feedback provided by a reviewee on the feedback received for their submission.

This class has significant room for improvement, especially with respect to object-oriented principles. Refactoring feedback_response_map.rb will streamline functionality, enhance maintainability, and improve adherence to coding standards.

=== Current Implementation ===

The current feedback_response_map.rb in the Expertiza repository has several issues. It includes the following problems:

* Poor adherence to SOLID and DRY principles: The current code structure introduces unnecessary repetition and complexity.
* feedback_response_report Method Issues: This critical method is highly complex, relying on custom data structures and separate containers for each review round, resulting in a DRY violation.
* Improper Responsibility Assignment: The email method does not belong within this class and would benefit from refactoring out into a separate class using the Visitor pattern. This pattern is suggested to streamline functionality, though alternative design patterns may be considered if more suitable.
* Memory Inefficiencies: The method currently creates unnecessary memory through inefficient structures, including multiple separate arrays for different rounds of review, which could be simplified into a 2D array structure.
* Code Smells: The code has redundant methods, unclear variable names, inefficient loops, and overly long method names.

=== Objectives & Requirements ===
1. Refactor the feedback_response_report Method

* Address the DRY violation by consolidating the containers for review rounds.
* Generalize the code to handle both cases—when rubrics vary by round and when they do not—without duplicating code.
* Introduce efficient data structures, like a 2D array, to replace multiple arrays and reduce memory usage.

2. Separate Responsibilities Using Visitor Pattern (stretch goal)

* Extract the email method using the Visitor pattern to modularize email functionality. Alternatively, select a design pattern better suited to organizing this method if available. Note that the email method is not properly used in its current location and is better suited to the Response class rather than ResponseMap.

3. Eliminate Redundant Methods

* Remove methods that do not serve a clear purpose and improve method naming for clarity.
* Aim to address any redundant meta-review functionality, as indicated by the professor's preference to remove these from response_map.rb.

4. Refactor for Code Clarity and Efficiency

* Improve variable names, reduce unnecessary loops, and replace long method names with concise, intuitive alternatives.
* Ensure that each method has a single, clearly defined purpose, improving code readability.

5. Testing and Documentation

* Write comprehensive tests for all newly implemented and modified methods to ensure reliability.
* Add detailed comments to facilitate code maintenance and understanding for future developers.

== Design ==

=== Design Principles ===

* Single Responsibility Principle (SRP): Each method and class should have a single, well-defined purpose.
* Don’t Repeat Yourself (DRY): Eliminate redundancy by creating general solutions that handle multiple scenarios.
* Ruby Readability Guidelines (Week 9 Notes):
** Guideline: Variable names should be neither too short nor too long.
** Guideline: Names should not be redundant.
** Guideline: A method should do only one thing and do it well.
* Visitor Pattern: Utilize this pattern to separate emailing functionalities from the main class.

=== Implementation Plan ===

The refactoring will proceed as follows:

1. Analyze and Refactor Current Code Structure

* Review and understand the existing structure and dependencies within feedback_response_map.rb.
* Identify areas of redundancy, complexity, and inefficiency, particularly in feedback_response_report, to create a clear plan for refactoring.
* Try to reduce character count and improve readability with aliasing (this way we do not break any code that relies on being able to call specific methods).

2. Refactor the feedback_response_report Method

* Consolidate containers for each review round to address DRY violations. By using a single structure to store review data, we will eliminate hard-coded repetition.
* Generalize logic to accommodate both scenarios where rubrics vary by round and where they do not, without duplicating code paths.
* Replace inefficient structures, such as multiple arrays, with more flexible and memory-efficient options, like a 2D array or a hash, to reduce memory usage and improve scalability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

* Extract frequently used or complex code blocks into helper methods, reducing the size of primary methods and enhancing reusability.
* Use helper methods for specific actions, like handling rounds or retrieving assignment data, to make the code cleaner and better organized.
* Emphasis on adhering to the guideline: A method should do only one thing and do it well.

4. Research Future Refactoring for email Method

* The email method is convoluted and the functionality should be refactored into a different class in the future.
* This is because this visitor pattern violation is present in many classes, and is beyond the scope of a one-class re-implementation.

5. Improve Variable Naming and Structure

* Replace ambiguous variable names with clearer alternatives that convey purpose, such as renaming @temp_review_responses and @temp_response_map_ids to more meaningful names.
* Simplify long method names to make the code more readable and approachable for new contributors.

=== Implementation ===

1. Analyze and Refactor Current Code Structure

Before refactoring, the FeedbackResponseMap class contains 6 methods, with the majority of the code in self.feedback_response_report:

[[File:FeedbackResponseMap_before.png |250px]]

We aim to change that by splitting up this method into three helper methods, as seen in the following:

[[File:FeedbackResponseMap_after.png |350px]]

To go into more detail, here is the code for the feedback_response_report method, and how we intend to split it up:

[[File:E2451_after_code1.png |600px]]

[[File:E2451_after_code2.png |600px]]

The red section of code will be self.get_feedback_authors, yellow will be self.varying_rubrics_report, and green will be self.static_rubrics_report. Each separated group will be its own method, and self.varying_rubrics_report and self.static_rubrics_report (the yellow and green methods) will be returned at the end of the new feedback_response_report.

2. Refactor the feedback_response_report Method

We can take the multiple arrays found to represent rounds, and use dictionaries instead, where each round is called by its key value (integer value).

INSERT PIC OF BEFORE AND AFTER OF ARRAYS/DICTS HERE

Additionally, Rubocop was used to analyze the code and adhere to proper Ruby Guidelines. The command below was used to search for any code smells that we missed.

rubocop app/models/feedback_response_map.rb

Originally, there were only three offenses, but this largely grew as we continued to clean up the code. All of the offenses can be seen below:

INSERT PICS OF RUBOCOP

Lastly, commenting was added throughout the files, to increase readability.

3. Introduce Helper Methods for Reusable Logic (for feedback_response_report)

As planned in step 1, the feedback_response_report method was split up into 3 private helper methods: get_feedback_authors(id), varying_rubrics_report(review_responses, response_map_ids), and static_rubrics_report(review_responses, response_map_ids). One change we made that was different from the UML diagram was the addition of an id parameter in the get_feedback_authors method, in order to ____. Each helper method was planned in order to stick to the guideline of “A method should do only one thing and do it well.”

INSERT UPDATED UML DIAGRAM AND NEW IMAGES OF THE FILES

4. Research future refactoring for email method

5. Improve variable naming and structure

====Authors====

This image contains comments showing what the implementation looked like before:

[[File:E2451_authors_before.png]]

This image shows the construction of the private method we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_authors_after.png]]

====Report====

This image contains comments showing what the implementation looked like before:

[[File:E2451_reports_before.png]]

These images show the construction of the private methods we implemented to replace it, reducing complexity and improving readability in the process:

[[File:E2451_reports_varying.png]]

[[File:E2451_report_static.png]]

Notice that in both the original self.feedback_response_report method and the private methods, comments are included to explain each non-obvious step of the process.

[MAYBE ADD COMPLEXITY METRICS BEFORE AND AFTER HERE?]

4. Research Future Refactoring for email Method

As discussed in the implementation plan, the email method was left as is. Any changes would require the addition of a new class and/or major refactoring for the functionality.

5. Improve Variable Naming and Structure

The two sets of variables discussed in the plan, @temp_response_map_ids and @temp_review_responses were modified as needed. First, the @temp_response_map_ids was moved to the helper methods varying_rubrics_report and static_rubrics_report, to be initialized, and changed to response_map_ids.

Secondly, @temp_review_responses was changed to review_responses in the helper methods that call it: varying_rubrics_report and static_rubrics_report

=== Testing ===

1. Adapt Existing Tests for Refactored Methods

* Any new helper methods will be private, and so by the "Magic Tricks of Testing" guide, should not be tested (both query and command messages sent to self are ignored in the test plan).
* Use mocks to isolate components during testing, especially for external dependencies like Mailer, to ensure that email functionality works as expected without direct integration.

2. Document Code for Clarity

* Write detailed comments explaining the purpose of each method, input and output, and any critical logic that might be complex for future developers to understand.
* Create or update documentation and diagrams to illustrate the newly designed structure, making it easier for new developers to contribute or modify functionality.

=== Documentation ===

Ensure all code is well-documented, with comments explaining purpose, inputs, outputs, and any complex logic. Maintain updated documentation for all modified and newly added classes and methods, allowing for ease of understanding and maintenance.

=== Conclusion ===

By refactoring feedback_response_map.rb, this project will enhance Expertiza’s backend maintainability and readability, ensuring a modular, efficient, and more secure implementation.

== Team Information ==

=== Mentor ===

* Chaitanya Srusti (crsrusti@ncsu.edu)

=== Members ===

* Sarah Daniel (sdaniel8@ncsu.edu)

* William Grochocinski (wagrocho@ncsu.edu)

* Tipton Middleton (tgmiddl2@ncsu.edu)

* ChatGPT (Honorary Member)

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-29T18:52:19Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-29T18:43:01Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:20:29Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:19:47Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:17:42Z

Sdaniel8:

File:E2451 after code2.png

2024-11-18T04:14:48Z

Sdaniel8:

File:E2451 after code1.png

2024-11-18T04:14:01Z

Sdaniel8:

File:FeedbackResponseMap after.png

2024-11-18T04:11:15Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:05:26Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:05:08Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-18T04:04:42Z

Sdaniel8:

File:FeedbackResponseMap before.png

2024-11-18T04:00:38Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-12T03:39:11Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2451. Reimplement feedback response map.rb

2024-11-12T03:28:37Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:45:58Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

==== Models & Controllers ====

app/models/bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

app/controllers/api/v1/bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

app/helpers/authorization_helper.rb

This file was imported from the main Expertiza/Expertiza repo and functionality was added to integrate with the action_allowed? method to determine if users had valid privileges when trying to perform certain actions. However, the functionality in this file depends on session[:user], something that is not implemented in Expertiza/reimplementation-back-end. Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

app/models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

spec/requests/api/v1/bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Load files in /spec/support/
|Files in /spec/support/ and its subfolders need to be loaded in for certain functionality to work. In particular, factory_bot.rb and authentication_helper.rb are found in this folder.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Seed the database if it isn't already seeded
|After running into issues with the database not being seeded when it should be, rails_helper was modified to check if seeding has occurred and to seed it if that had not already been done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added SimpleCov configuration
|SimpleCov needs configuration files in spec_helper in order to load and be configured properly. Since SimpleCov was required for this project, this configuration information needed to be added to spec_helper.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Initial creation
|This helper file was created to facilitate the automatic login process. When called with a username and password, the system would try to login as that user and return the configuration header that needed to be send with each future CRUD request. If the password field is not provided, the helper attempts to use the default password. If the user is also left blank, an administrator login will be returned.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Removal of Admin Login
|After fully implementing the five separate users, we could not think of a use-case where we would want to log-in as an administrator by default since any user seeded in the DB could easily be loaded and logged in with. Due to this, we removed the internal login_admin functionality, thus requiring all calls to AuthenticationHelper to specify a user.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|}

===== Spec Factories =====

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/institution.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/join_team_requests.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/roles.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/student_tasks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/users.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

==== Database files ====

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below, as well as number of times each line was executed:

[[File:BookmarksController1To33Coverage.PNG |1000px]]

[[File:BookmarksController34To63Coverage.PNG |1000px]]

[[File:BookmarksController64To98Coverage.PNG |1000px]]

[[File:BookmarksController99To130Coverage.PNG |1000px]]

[[File:BookmarksController130To154Coverage.PNG |1000px]]

As we can see, many of the lines were executed 10 times or more. Also, many lines in the beginning were executed only once. However, substantial code is executed at least more than once.

See the summary of coverage for both files below:

Bookmark.rb

[[File:BookmarkCoverageE2480.png |1000px]]

BookmarksController.rb

[[File:BookmarksControllerCoverageE2480.png |1000px]]

As mentioned before, both files have 100% coverage, with an average of 20 hits (executions) per line for BookmarksController.rb.

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:44:43Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

==== Models & Controllers ====

app/models/bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

app/controllers/api/v1/bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

app/helpers/authorization_helper.rb

This file was imported from the main Expertiza/Expertiza repo and functionality was added to integrate with the action_allowed? method to determine if users had valid privileges when trying to perform certain actions. However, the functionality in this file depends on session[:user], something that is not implemented in Expertiza/reimplementation-back-end. Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

app/models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

spec/requests/api/v1/bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Load files in /spec/support/
|Files in /spec/support/ and its subfolders need to be loaded in for certain functionality to work. In particular, factory_bot.rb and authentication_helper.rb are found in this folder.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Seed the database if it isn't already seeded
|After running into issues with the database not being seeded when it should be, rails_helper was modified to check if seeding has occurred and to seed it if that had not already been done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added SimpleCov configuration
|SimpleCov needs configuration files in spec_helper in order to load and be configured properly. Since SimpleCov was required for this project, this configuration information needed to be added to spec_helper.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Initial creation
|This helper file was created to facilitate the automatic login process. When called with a username and password, the system would try to login as that user and return the configuration header that needed to be send with each future CRUD request. If the password field is not provided, the helper attempts to use the default password. If the user is also left blank, an administrator login will be returned.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Removal of Admin Login
|After fully implementing the five separate users, we could not think of a use-case where we would want to log-in as an administrator by default since any user seeded in the DB could easily be loaded and logged in with. Due to this, we removed the internal login_admin functionality, thus requiring all calls to AuthenticationHelper to specify a user.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|}

===== Spec Factories =====

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/institution.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/join_team_requests.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/roles.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/student_tasks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/users.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

==== Database files ====

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below, as well as number of times each line was executed:

[[File:BookmarksController1To33Coverage.PNG |1000px]]

[[File:BookmarksController34To63Coverage.PNG |1000px]]

[[File:BookmarksController64To98Coverage.PNG |1000px]]

[[File:BookmarksController99To130Coverage.PNG |1000px]]

[[File:BookmarksController130To154Coverage.PNG |1000px]]

As we can see, many of the lines were executed 10 times or more. Also, many lines in the beginning were executed only once. However, substantial code is executed at least more than once.

See the summary of coverage for both files below:

Bookmark.rb

[[File:BookmarkCoverageE2480.png |1000px]]

BookmarksController.rb

[[File:BookmarksControllerCoverageE2480.png |1000px]]

As mentioned before, both files have 100% coverage, with around 20 average hits per line for BookmarksController.rb.

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

File:BookmarksController130To154Coverage.PNG

2024-11-01T02:37:59Z

Sdaniel8:

File:BookmarksController99To130Coverage.PNG

2024-11-01T02:37:38Z

Sdaniel8:

File:BookmarksController64To98Coverage.PNG

2024-11-01T02:37:17Z

Sdaniel8:

File:BookmarksController34To63Coverage.PNG

2024-11-01T02:36:56Z

Sdaniel8:

File:BookmarksController1To33Coverage.PNG

2024-11-01T02:36:33Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:36:10Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

==== Models & Controllers ====

app/models/bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

app/controllers/api/v1/bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

app/helpers/authorization_helper.rb

This file was imported from the main Expertiza/Expertiza repo and functionality was added to integrate with the action_allowed? method to determine if users had valid privileges when trying to perform certain actions. However, the functionality in this file depends on session[:user], something that is not implemented in Expertiza/reimplementation-back-end. Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

app/models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

spec/requests/api/v1/bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Load files in /spec/support/
|Files in /spec/support/ and its subfolders need to be loaded in for certain functionality to work. In particular, factory_bot.rb and authentication_helper.rb are found in this folder.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Seed the database if it isn't already seeded
|After running into issues with the database not being seeded when it should be, rails_helper was modified to check if seeding has occurred and to seed it if that had not already been done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added SimpleCov configuration
|SimpleCov needs configuration files in spec_helper in order to load and be configured properly. Since SimpleCov was required for this project, this configuration information needed to be added to spec_helper.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

===== Spec Factories =====

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/institution.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/join_team_requests.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/roles.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/student_tasks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/users.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

==== Database files ====

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below, as well as number of times each line was executed:

[[File:BookmarksController1To33Coverage.png]]

[[File:BookmarksController34To63Coverage.png]]

[[File:BookmarksController64To98Coverage.png]]

[[File:BookmarksController99To130Coverage.png]]

[[File:BookmarksController130To154Coverage.png]]

See the coverage for both files below:

Bookmark.rb

[[File:BookmarkCoverageE2480.png]]

BookmarksController.rb

[[File:BookmarksControllerCoverageE2480.png]]

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:28:23Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

==== Models & Controllers ====

app/models/bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

app/controllers/api/v1/bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

app/helpers/authorization_helper.rb

This file was imported from the main Expertiza/Expertiza repo and functionality was added to integrate with the action_allowed? method to determine if users had valid privileges when trying to perform certain actions. However, the functionality in this file depends on session[:user], something that is not implemented in Expertiza/reimplementation-back-end. Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

app/models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

spec/requests/api/v1/bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

===== Spec Factories =====

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/institution.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/join_team_requests.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/roles.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/student_tasks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/users.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

==== Database files ====

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below:

See the coverage for both files below:

Bookmark.rb

[[File:BookmarkCoverageE2480.png]]

BookmarksController.rb

[[File:BookmarksControllerCoverageE2480.png]]

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

File:BookmarksControllerCoverageE2480.png

2024-11-01T02:26:01Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:25:37Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

app/models/bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

app/controllers/api/v1/bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

app/helpers/authorization_helper.rb

This file was imported from the main Expertiza/Expertiza repo and functionality was added to integrate with the action_allowed? method to determine if users had valid privileges when trying to perform certain actions. However, the functionality in this file depends on session[:user], something that is not implemented in Expertiza/reimplementation-back-end. Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

app/models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

spec/requests/api/v1/bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

===== Spec Factories =====

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/institution.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/join_team_requests.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/roles.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/student_tasks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

spec/factories/users.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|
|
|
|}

==== Database files ====

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below:

See the coverage for both files below:

Bookmark.rb

[[BookmarkCoverageE2480.png]]

BookmarksController.rb

[[BookmarksControllerCoverageE2480.png]]

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

File:BookmarkCoverageE2480.png

2024-11-01T02:23:19Z

Sdaniel8:

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-11-01T02:22:42Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

"Access" for different user types (Students are omitted, as they are only allowed to modify/delete bookmarks they created) is defined as follows: TAs have access if there exists a TaMapping between them and the course the bookmark is under. Intructors have access if they are the instructor for the course the bookmark is under. Admins have access if they are the parent of (created) the instructor of the course the bookmark is under. Super Admins always have access.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

authorization_helper.rb

This file is used to aid tests in logging in to different users. The action_allowed? method in bookmarks_controller.rb uses this file.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Removed details from login_admin
|If there are no users in the database, there is something wrong, because there should always be an admin in the database
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Tests & Helpers ====

===== Spec Files =====

bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

===== Spec Helpers =====

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/support/authentication_helper.rb

===== Spec Factories =====

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/roles.rb

This file was renamed from spec/factories/role.rb to spec/factories/roles.rb (originally taken from Authentication functionality from Expertiza).

spec/factories/bookmarks.rb

spec/factories/institution.rb

spec/factories/join_team_requests.rb

spec/factories/role.rb

spec/factories/student_tasks.rb

spec/factories/topics.rb

spec/factories/users.rb

==== Database files ====

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

|}

==== Deleted Files ====

Two files regarding factories were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

* spec/factories.rb - the factories in this file were split into their own factory files in spec/factories/
* spec/factories/factories.rb - this was an empty file, deleted to clean up the repo

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:
rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]
Where we would include
-A
If we wanted it to auto-correct any automatically correctable violations, and
--out ./spec/requests/<temporary file>.txt
If we wanted output to be delivered to a file for improved readability

==== What Changed ====

Lots of small violations like trailing whitespace, unnecessary blank lines, using " == nil" instead of ".nil?", and using double instead of single quotes were corrected automatically.

Some bigger violations like brace indent alignment and inner field indentation were also able to be fixed automatically, although many of those came about after manual fixes to other violations, like:

Line length violations were plentiful, most of them being caused by the long API commands. When we broke those down into their chunks, often times the `params:` argument would have its contents spread across multiple lines for readability (which caused some of the indent-related violations mentioned above). Other line length violations were on test descriptor lines (`it '<...>'`). The descriptors that triggered them covered complex tests, and so were intentionally detailed to inform future developers of what the test does, so shortening them was not ideal. These violations were resolved by splitting the descriptor across multiple lines.

Overall, the readability of the test file improved substantially!

==== What did not change, and why? ====

There were some violations that were intentionally not corrected to preserve readability and consistency:
* Block length violations were dismissed because adhering to them would undermine the structure of our test file. Our test file is laid out in segments defined by the `describe` blocks. At the uppermost level there is a describe block encapsulating all of the tests in the file. From there tests are divided by user type, this reduces redundancy in data preparation, improving test efficiency by having separate `before(:each)` blocks for each user. Within users, tests are further divided by API call type. This way it is easy to determine the location of a failing test, improving the speed of debugging. Because our structure relies on nested `describe` blocks, those at the uppermost levels are prone to triggering block length violations. With this in mind, we made the conscious decision to disable rubocop block length metrics with the following comments at the start and end of our test file respectively:
# rubocop:disable Metrics/BlockLength

# rubocop:enable Metrics/BlockLength

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

Tests for both Bookmark_Controller.rb and Bookmark.rb both have 100% coverage! Previously, tests were not comprehensive, and tests would not run at all. We recreated all of the tests and made sure coverage was at least above 90% (as expected in the Requirements).

View the code for Bookmark_Controller.rb below:

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-10-31T22:31:45Z

Sdaniel8:

This page contains the details of OSS project ''E2480: Implement testing for new Bookmarks Controller'' done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

authorization_helper.rb

This file is used to aid tests in logging in to different users. The action_allowed? method in bookmarks_controller.rb uses this file.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Removed details from login_admin
|If there are no users in the database, there is something wrong, because there should always be an admin in the database
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Spec files ====

bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/roles.rb

This file was renamed from spec/factories/role.rb to spec/factories/roles.rb (originally taken from Authentication functionality from Expertiza).

===== Authentication functionality from Expertiza=====

The following were taken from the Expertiza repo and added into our repo for functionality purposes in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

*''spec/factories/bookmarks.rb''
*''spec/factories/institution.rb''
*''spec/factories/join_team_requests.rb''
*''spec/factories/role.rb'': Added a line to make any new roles created, a Student, in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6]. Removed previously created lines, implemented finding or creating a Student's id (of 5), and sets the name to 'Student' in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7]
*''spec/factories/student_tasks.rb''
*''spec/factories/topics.rb''
*''spec/factories/users.rb'': Changed the password and added functionality to use existing Student roles or Institutions, or to create new ones if none exists, in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7].
*''spec/support/authentication_helper.rb:'' Added checks to see if User, or Token are nil values in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6].

==== Database files ====

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|}

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|All usages of ta mappings throughout the reimplementation-back-end repository use ta_id, whereas the schema refers to it as user_id. This created problems when trying to use ta mappings, especially in tests. To fix this, instead of refactoring all instances of ta mappings to have their field referred to as user_id, we changed the schema to reflect the existing usage of ta_id, while still having it understand that the foreign key refers to users. We created a migration (seen below) that accomplishes this task.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

db/migrate/20241030195109_update_ta_mappings_table_structure.rb
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing ta_mappings references from user_id to ta_id
|Facilitates the changes made to the schema
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11] [https://github.com/Grochocinski/expertiza-E2480/pull/12 Pull Request 12]
|-
|}

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

|}

==== Miscellaneous Files ====

bookmark_tester.sh
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Trying to automate Bookmark testing
|Swagger tests must be manually run, so this file attempts to automate testing for the Bookmark.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

==== Deleted Files ====

The following are files that were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4] or other PR's if specified

* spec/factories/bookmark.rb
* spec/factories.rb
* spec/factories/factories.rb
* spec/factories/topics.rb: [https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request #8]

=== Rubocop ===

==== Why Rubocop ====

Rubocop was one of the tools listed in the project instructions, and so we used it "to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods)". We chose it in part because it was easy to apply to our system (simple CLI), and easy to make changes based on the feedback (provided line numbers and violation types).

==== How it was applied ====

We applied rubocop using simple one line commands based on what we wanted it to do. The general template we followed was:

* ~~rubocop [-A] ./spec/requests/api/v1/bookmarks_controller_spec.rb [--out ./spec/requests/<temporary file>.txt]~~

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-10-31T22:10:29Z

Sdaniel8:

This page contains the details of OSS project E2480:Implement testing for new Bookmarks Controller done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

authorization_helper.rb

This file is used to aid tests in logging in to different users. The action_allowed? method in bookmarks_controller.rb uses this file.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Removed details from login_admin
|If there are no users in the database, there is something wrong, because there should always be an admin in the database
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Spec files ====

bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added. Lastly, functionality to look for a TA by role, and checking to see if a user is a TA is also added (and if a TA, then add the TA to the course).
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/roles.rb

This file was renamed from spec/factories/role.rb to spec/factories/roles.rb (originally taken from Authentication functionality from Expertiza).

===== Authentication functionality from Expertiza=====

The following were taken from the Expertiza repo and added into our repo for functionality purposes in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

*''spec/factories/bookmarks.rb''
*''spec/factories/institution.rb''
*''spec/factories/join_team_requests.rb''
*''spec/factories/role.rb'': Added a line to make any new roles created, a Student, in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6]. Removed previously created lines, implemented finding or creating a Student's id (of 5), and sets the name to 'Student' in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7]
*''spec/factories/student_tasks.rb''
*''spec/factories/topics.rb''
*''spec/factories/users.rb'': Changed the password and added functionality to use existing Student roles or Institutions, or to create new ones if none exists, in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7].
*''spec/support/authentication_helper.rb:'' Added checks to see if User, or Token are nil values in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6].

==== Database files ====

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

==== Miscellaneous Files ====

bookmark_tester.sh
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Trying to automate Bookmark testing
|Swagger tests must be manually run, so this file attempts to automate testing for the Bookmark.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

==== Deleted Files ====

The following are files that were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4] or other PR's if specified

* spec/factories/bookmark.rb
* spec/factories.rb
* spec/factories/factories.rb
* spec/factories/topics.rb: [https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request #8]

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-10-31T22:05:48Z

Sdaniel8:

This page contains the details of OSS project E2480:Implement testing for new Bookmarks Controller done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|6
|Changing a couple of method calls, and adding conditions, as well as checking to see if a TaMapping exists
|Instead of a create method to create a bookmark rating, we call the new method with the same parameters, and call the save method. Also added a few conditionals to the 'when' blocks. Lastly, when the User is a TA, we can check to see if a TaMapping exists for the TA and a Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

authorization_helper.rb

This file is used to aid tests in logging in to different users. The action_allowed? method in bookmarks_controller.rb uses this file.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Removed details from login_admin
|If there are no users in the database, there is something wrong, because there should always be an admin in the database
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]
|}

==== Spec files ====

bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|-
|7
|Adding tests for adding, updating, and deleting a bookmark rating, as well as updating/deleting bookmarks for other Users.
|This PR adds happy and sad tests for functionality of adding, updating, and deleting bookmark ratings as a Student. Tests for adding, updating, and deleting Bookmarks by TA, Instructor, Admin, and SuperAdmin are also added.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/roles.rb

This file was renamed from spec/factories/role.rb to spec/factories/roles.rb (originally taken from Authentication functionality from Expertiza).

===== Authentication functionality from Expertiza=====

The following were taken from the Expertiza repo and added into our repo for functionality purposes in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

*''spec/factories/bookmarks.rb''
*''spec/factories/institution.rb''
*''spec/factories/join_team_requests.rb''
*''spec/factories/role.rb'': Added a line to make any new roles created, a Student, in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6]. Removed previously created lines, implemented finding or creating a Student's id (of 5), and sets the name to 'Student' in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7]
*''spec/factories/student_tasks.rb''
*''spec/factories/topics.rb''
*''spec/factories/users.rb'': Changed the password and added functionality to use existing Student roles or Institutions, or to create new ones if none exists, in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7].
*''spec/support/authentication_helper.rb:'' Added checks to see if User, or Token are nil values in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6].

==== Database files ====

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-

db/schema.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Changing users to TAs
|Instead of "user_id", changed the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/11 Pull Request 11]

==== Miscellaneous Files ====

bookmark_tester.sh
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Trying to automate Bookmark testing
|Swagger tests must be manually run, so this file attempts to automate testing for the Bookmark.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

==== Deleted Files ====

The following are files that were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4] or other PR's if specified

* spec/factories/bookmark.rb
* spec/factories.rb
* spec/factories/factories.rb
* spec/factories/topics.rb: [https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request #8]

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.

CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller

2024-10-31T21:38:58Z

Sdaniel8:

This page contains the details of OSS project E2480:Implement testing for new Bookmarks Controller done for Expertiza in Fall 2024.

__TOC__

== Expertiza and Project Overview ==

=== Background ===

Expertiza is an open-source application used in CSC 517 (Object Oriented Design and Development) as a platform for creating teams, assigning programs, and reviewing submissions. Expertiza is also used to help students develop a working application in order to mimic how students will work on programs in the workplace.

A bookmark is a resource that a student can post for a certain topic, that other students may find useful if working on that topic. Students working on the topic have the ability to rate the bookmark based on usefulness, and using a rubric. The average of all ratings will be computed to get an average score.

As discussed in E2480's background, the BookmarksController is implemented with CRUD operations, as well as the ability to rate bookmarks, calculate average ratings, and check for proper authorization when accessing a Bookmark.

=== Prior Work and Related Projects ===

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2024_-_E2424._Reimplement_the_Bookmarks_Controller#Problem_Statement E2424 Reimplement the Bookmarks Controller]

* [https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2017/E1786_OSS_Project_Juniper:_Bookmark_enhancements E1786 Bookmark enhancements]

* [https://wiki.expertiza.ncsu.edu/index.php?title=E1915_Authorization_Utilities E1915 Authorization Utilities]

=== Requirements ===

There are four main requirements assigned for the project, seen as follows:

1. Add Tests and Generate API Documentation

* Develop comprehensive test cases for the BookmarksController to ensure the correctness of CRUD operations, bookmark ratings, and score calculations.
* The test coverage should aim to cover at least 90% of the code, ensuring thorough validation of edge cases and error handling.
* Utilize Swagger UI to generate API documentation for the controller and verify that routes are correctly defined.

2. Code Quality and Refactoring 

* Use tools like Rubocop and Code Climate to assess code quality, remove code smells, and refactor any inefficient sections (e.g., redundant loops or methods).
* Any unused or unclear functionality should be removed to improve maintainability.
* Rename methods with ambiguous names to improve clarity and ensure code follows the clean code principles.
* Review and optimize performance, particularly in areas where loops or methods can be refactored for better efficiency, such as handling the deletion of associated data (e.g., deleting all questions related to a questionnaire).

3. Authorization and Error Handling 

* Ensure proper enforcement of authorization rules and handle edge cases with informative error messages

4. Final Deliverables 

* Ensure that all tests pass and API documentation is fully generated using Swagger UI.
* Confirm that code coverage exceeds 90%, and there are no significant code smells or performance issues flagged by Rubocop and Code Climate.

==== Tasks ====

We can simplify the above requirements down to the following tasks:

* Create tests with at least 90% test coverage for BookmarksController
* Use Swagger UI to generate API documentation for the BookmarksController
* Fix and remove code smells, and refactor where necessary to optimize performance
* Implement authorization rules and handle edge cases using error messages that indicate what the error is

There are two more tasks (see details in ''Process --> Findings'' below) that were given, which can be summed up as the following:
* Create an action_allowed? method to see if a User has access to a Course, and if a Student has access to a Bookmark

== Process ==

=== Findings ===

Please note, the project repository was forked from reimplementation-back-end repository for Expertiza.

After searching through the repository, we noticed a few issues with the reimplementation-back-end that were not discussed in the OSS project details. After discussing with Dr. Gehringer and the TA, we decided on specific tasks to focus on during this project. One such issue was checking for if a user such as a TA, Instructor, or Admin has access to the bookmark, which is determined by if the user has access to the course that contains the assignment, which contains a topic, which then contains the bookmark.

Thus, the following additional tasks are expected in an action_allowed? method:

* Check to see if a User has access to a Course: The access a User (TA, Instructor, Admin) has to a Course is determined by the connection of Course to a Bookmark. A Course contains an Assignment, which contains a Topic, which then contains the Bookmark. Thus, if the user has access to a Course, they can access the Bookmark.
* Check to see if a Student has access to a Bookmark: This can be determined by checking to see if the Student had created the Bookmark. If so, then the Student can choose to edit or destroy the Bookmark.

=== New Changes ===

bookmark.rb

In the bookmark.rb file, the only change is to uncomment the line related to :topic. This way, we can identify which Users can identify Bookmarks.

bookmarks_controller.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added action_allowed? functionality
|Code was added to include the AuthorizationHelper, as well as run the action_allowed? method before the other methods run, so the User type can be identified.
|[https://github.com/Grochocinski/expertiza-E2480/pull/3 Pull Request 3]
|-
|2
|Added 'index' to a When case for Students
|Identified Students by additionally looking for an'index' parameter, and using just this and the 'list' parameters to determine if the User is a student.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|3
|Added 'show' and 'get_bookmark_rating_score' to a When case for Students
|Identified Students by looking for list, index, show, and get_bookmark_rating_score parameters. If the User is a Student, then the Student can view the bookmarks list.
|[https://github.com/Grochocinski/expertiza-E2480/pull/5 Pull Request 5]
|-
|4
|Added check_action_allowed method
|Making the before action as the new method check_action_allowed, which will pull up an Unauthorized access json file if action_allowed is false.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|5
|Changing the methods, and added a check_action_allowed
|Adding searching for a bookmark by id functionality, and if the bookmark is not found, a json page will be returned for the show, destroy, update, get_bookmark_rating_score, save_bookmark_rating_score. The check_allowed_action method will show a page if the user is allowed to perform the action. Code cleanup was also done.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|}

authorization_helper.rb

This file is used to aid tests in logging in to different users. The action_allowed? method in bookmarks_controller.rb uses this file.

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Removed details from login_admin
|If there are no users in the database, there is something wrong, because there should always be an admin in the database
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

models/bookmark_rating.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a validates statement
|The new validates statement will check to see if a bookmark's rating is present for a BookmarkRating.
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|2
|Improving validates statement
|The validates statement now allows for only integers that are between 0 and 5.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|}

==== Spec files ====

bookmarks_controller_spec.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Added and cleaned up happy and sad tests for getting bookmarks
|Cleaned up tests, and added expected functionality for the tests. Some tests were incorrectly named, so that was fixed as well. Added happy and sad tests such as not letting someone without a token access list of bookmarks, and letting the user access the list of bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding User type by role_id and creating headers for signing in, and adding tests.
|Added new tests to check for specific users such as TA, Instructor, and Student. Also, added headers that will be used as tokens in order to authorize User login.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Creating test cases for each User type, and removes the header authentication in this file.
|Added better tests to check for all types of Users, and removed the original getting a student and also moved the headers functionality from the file. Header authentication is now done within each test.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Add specific test cases for Students, TAs, Admin, and Super Admin
|Added better tests to check for all types of Users, such as letting a specific User type access lists of bookmarks, allowing a specific User type to query existing and non-existing bookmarks and seeing the results. Also, Users who are not signed in cannot access the list of bookmarks, or query bookmarks. Lastly, adds a create_bookmark method to make a bookmark while finding or creating a corresponding User, Course, Assignment, and SignUpTopic. The method will also decide that if the bookmark does not exist based on these values, then create it.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|-
|5
|Adding specific test cases for BookmarkRatings
|Making test cases even more specific by adding test cases for all kinds of Users that may access BookmarkRatings. Also tests to see if BookmarkRatings can be accessed when a User is not logged in. Lastly, makes a make_bookmark_rating method that will create a bookmark if it doesn't exist, along with assigning a user to it (and creating a Student type if the user is nil).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|-
|6
|Adding tests for adding, updating, deleting a bookmark, as well as other tests.
|Tests were created for seeing the path of how a Bookmark is created, updated, and deleted. Tests were also created in querying Bookmark Ratings for all kinds of Users (as well as signed out users), to make sure non-existent bookmarks will not be shown. There was also additions of sad tests to make sure that Users that are not Students cannot make, or update bookmarks.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|}

spec/spec_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding SimpleCov functionality to specific files
|Making certain files (such as bookmarks_controller.rb and bookmark.rb) run to check coverage of the files.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/rails_helper.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Seed the database
|If there are no Users in the database, then add functionality to seed the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

spec/factories/sign_up_topics.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Topic
|Create Topics using factories, and assign an assignment id to each Topic.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/course.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Factory Course
|Create Course data using factories. Then get an Instructor from the database or create a new one. Then, use an existing institution or make a new Institution for each Course.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/assignment.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create a Assignment from factories
|Create Assignment data using factories. Then get an existing Instructor from the database or create a new one to add to each Assignment.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmarks.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmarks
|Create Bookmark data using factories. We can add random strings to the title and description, as well as search the database for an existing Student to assign the Bookmark to.
|[https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request 8]
|}

spec/factories/bookmark_ratings.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Create Factory Bookmark Ratings
|Making data for bookmark ratings using Factory. Will make the bookmark and user null values (nil), and set a rating of 0).
|[https://github.com/Grochocinski/expertiza-E2480/pull/9 Pull Request 9]
|}

spec/factories/roles.rb

This file was renamed from spec/factories/role.rb to spec/factories/roles.rb (originally taken from Authentication functionality from Expertiza).

===== Authentication functionality from Expertiza=====

The following were taken from the Expertiza repo and added into our repo for functionality purposes in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4]:

*''spec/factories/bookmarks.rb''
*''spec/factories/institution.rb''
*''spec/factories/join_team_requests.rb''
*''spec/factories/role.rb'': Added a line to make any new roles created, a Student, in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6]. Removed previously created lines, implemented finding or creating a Student's id (of 5), and sets the name to 'Student' in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7]
*''spec/factories/student_tasks.rb''
*''spec/factories/topics.rb''
*''spec/factories/users.rb'': Changed the password and added functionality to use existing Student roles or Institutions, or to create new ones if none exists, in [https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request #7].
*''spec/support/authentication_helper.rb:'' Added checks to see if User, or Token are nil values in [https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request #6].

==== Database files ====

db/seeds.rb

{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Adding a role for the database
|Cleaned up the code, added useful comments, and added a role column for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|-
|2
|Finding and Creating a User by email
|Based on the email type, Users will be created as seeds for the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/6 Pull Request 6]
|-
|3
|Remove all Code except Admin find and creation
|Only admin values will be found or created (for testing purposes). All other users will not be created using seed. From now on, User's will be created by factories instead of seeded into the database.
|[https://github.com/Grochocinski/expertiza-E2480/pull/7 Pull Request 7]
|-
|4
|Changing users to TAs
|Instead of "user_id", change the value to ta_id. Also removing a foreign_key of mapping TA's to Users.
|[https://github.com/Grochocinski/expertiza-E2480/pull/10 Pull Request 10]
|}

==== Miscellaneous Files ====

bookmark_tester.sh
{| class="wikitable" style="width: 100%;
!  #  !! Change !! Rationale !! PR Link
|-
|1
|Trying to automate Bookmark testing
|Swagger tests must be manually run, so this file attempts to automate testing for the Bookmark.
|[https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request 4]
|}

==== Deleted Files ====

The following are files that were deleted in [https://github.com/Grochocinski/expertiza-E2480/pull/4 Pull Request #4] or other PR's if specified

* spec/factories/bookmark.rb
* spec/factories.rb
* spec/factories/factories.rb
* spec/factories/topics.rb: [https://github.com/Grochocinski/expertiza-E2480/pull/8 Pull Request #8]

== Tests ==

Originally, tests were run using RSpec, but these created 13/13 Bookmarks controller tests failed. These tests were removed, and remade.

Swagger UI is a set of tools that is used by the Expertiza developers to run RSpec tests through an interface instead of the command line. Swagger was originally needed in order to view and run tests, because the RSpec tests for this project need to be fixed. However, since RSpec tests have been recreated, the use of Swagger will not be required to run tests, but only check for code smells and coverage.

=== Running RSpec tests via Swagger ===

Steps
* Build the containers
* Go to http://localhost:3002/api-docs
* Login via the Authentication block at the bottom (U: "admin", P: "password123")
* Copy the token in the response
* Scroll up to the top and click "Authorize"
* In the popup, paste in the token and hit "Authorize"
* Navigate to the test you want to run, edit the parameters, and run it

=== Test Coverage ===

=== Failing Tests ===

== Recommended next steps ==

* There are methods in the authorization_helper.rb that check to see if a TA or Instructor can access an assignment, that have some code smells such as bad method names.
* The questionnaire (which is used to rate the bookmark) needs the functionality of identifying a user type. Possibly create a mixin to implement this.