CSC/ECE 517 Fall 2024 - E2480. Implement testing for new Bookmarks Controller
This page contains the details of OSS project E2480: Implement testing for new Bookmarks Controller done for Expertiza in Fall 2024.
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, or, if the instructor has set up the assignment that way, by using a rubric. In either case, 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
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
| # | 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. | 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. | 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. | 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. | 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. | 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. | Pull Request 11 |
| 7 | Added commented-out non-working previous implementation | Added the previous (non-working) code that relied on the non-working app/helpers/authorization_helper.rb file (see below for notes). This was added so that in the future (when session[:user] hopefully works), action_allowed? can be easily switched over. | Pull Request 14 |
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 currently not implemented and not possible to implement in Expertiza/reimplementation-back-end as there is no login path (see this stack overflow post for more details). Due to this, this file was not used in this implementation, but the file and added functionality was kept for future use.
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Copied over and fixed dependencies | This was copied over and dependencies that did not exist in expertiza/reimplementation-back-end were adjusted so they lined up with files that did exist. | Pull Request 2 |
app/models/bookmark_rating.rb
| # | 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. | Pull Request 9 |
| 2 | Improving validates statement | The validates statement now allows for only integers that are between 0 and 5. | Pull Request 11 |
Tests & Helpers
Spec Files
spec/requests/api/v1/bookmarks_controller_spec.rb
| # | 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. | 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. | 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. | 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. | 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). | 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. | 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). | Pull Request 11 |
Spec Helpers
spec/rails_helper.rb
| # | 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. | 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. | Pull Request 4 |
spec/spec_helper.rb
| # | 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. | Pull Request 4 |
spec/support/authentication_helper.rb
| # | 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. | 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. | Pull Request 6 |
Spec Factories
spec/factories/assignment.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Initial creation | Bookmarks require sign-up topics and each topic requires an assignment. In order for the functionality to be properly tested, an assignment factory needed to be created. | Pull Request 8 |
spec/factories/bookmark_ratings.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Initial Creation | The bookmark rating factory was created to assist with the testing of bookmark ratings. | Pull Request 9 |
spec/factories/bookmarks.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Split bookmarks out of singular factory file | The standard RSpec file structure dictates that factories should be in their own file in /spec/factories/, but the bookmark factory was originally in /spec/factories.rb. | Pull Request 4 |
| 2 | Switch from Hardcoded IDs to Associations | Instead of using hardcoded user_id and topic_ids, bookmarks now use user and topic associations. This is the preferred approach as it better handles situations where the specified ID does not exist in the associated table. | Pull Request 4 |
| 3 | Switched to Faker names | In order to better reflect real-world bookmarks, Faker was used to generate fake URLs, titles, and descriptions rather than have them be static. | Pull Request 8 |
spec/factories/course.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Initial creation | Each bookmark belongs to a topic, which in turn belongs to an assignment, which in turn must belong to a course. In order for bookmarks to be created, a course factory needed to be made. | Pull Request 8 |
spec/factories/institution.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Created initial factory | Created an initial factory as users needed to be associated with an institution for them to be created. | Pull Request 4 |
spec/factories/join_team_requests.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Split join team requests out of singular factory file | The standard RSpec file structure dictates that factories should be in their own file in /spec/factories/, but the join team request factory was originally in /spec/factories.rb. | Pull Request 4 |
spec/factories/roles.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Created initial factory | Created an initial factory as each user needed to have a role assigned in order for the user to be created. | Pull Request 4 |
| 2 | Added id field | Since roles (and their subsequent access controls) are handled by id, an id field was added to the role to facilitate having multiple roles with different access levels. | Pull Request 6 |
| 3 | Added ID Lookup and Name | In addition to adding the role name to the factory, functionality was also added to look up the id of the student role (if it is in the database) rather than assume it will always be 5. This makes the factory more resilient to future changes. | Pull Request 7 |
| 4 | Renamed from role.rb to roles.rb | Factory names should be plural. | Pull Request 8 |
spec/factories/sign_up_topics.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Created initial factory | Created an initial factory as each student task needed to be associated with a topic in order for the task to be created. | Pull Request 4 |
| 2 | Renamed from topics.rb to sign_up_topics.rb | This factory was renamed to better match the model it represents. | Pull Request 8 |
spec/factories/student_tasks.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Split student tasks out of singular factory file | The standard RSpec file structure dictates that factories should be in their own file in /spec/factories/, but the student task factory was originally in /spec/factories.rb. | Pull Request 4 |
spec/factories/users.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Split users out of singular factory file | The standard RSpec file structure dictates that factories should be in their own file in /spec/factories/, but the user factory was originally in /spec/factories.rb. | Pull Request 4 |
| 2 | Changed default password, added role/institution lookup | The default password was changed from 'password' to 'password123' to better match the default passwords already in use. Additionally, both the role and institution fields attempt to look up the default value for the 'Student' role and 'NCSU' institution respectively. This lack of hardcoding makes the factory more resilient to future database changes. | Pull Request 7 |
Database files
db/schema.rb
| # | 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. | Pull Request 11, Pull Request 12 |
| # | 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. | Pull Request 11 |
db/seeds.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Add checks for missing data | The prior behavior of seeds.rb was to error out whenever it was called after seeding had already been done. This did not allow for tweaks to aspects of the seeds without dropping and completely rebuilding the entire database. The new behavior checks if each entry is already in the database. Any entry already in the database is skipped, while any entry that is missing is added. This ensures that all data in seeds.rb is in the database after it completes running and avoids raising errors for actions that shouldn't raise them. | Pull Request 4 |
| 2 | Now seeds all 5 user types | Instead of seeding just an admin, all 5 user types are now seeded. This enabled testing of different user types as the user factory could not set the user role at this time. | Pull Request 6 |
| 3 | Removed seeding for non-admins | After user factories received the ability to set roles for users, there was no need to have them pre-seeded in the database since no test ever required more than two users. The seeded users in PR6 were removed and any tests requiring them now used the factories to create them. | Pull Request 7 |
db/migrate/20241030195109_update_ta_mappings_table_structure.rb
| # | Change | Rationale | PR Link |
|---|---|---|---|
| 1 | Changing ta_mappings references from user_id to ta_id | Facilitates the changes made to the schema | Pull Request 11, Pull Request 12 |
Deleted Files
Two files regarding factories were deleted in 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:
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
BookmarksController.rb
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.
Team Information
Project Members:
Sarah Daniel (sdaniel8@ncsu.edu)
William Grochocinski (wagrocho@ncsu.edu)
Tipton Middleton (tgmiddl2@ncsu.edu)