CSC/ECE 517 Fall 2024 - E2470. Reimplement grades controller

E2470. Reimplement grades controller

This page provides a description of the Expertiza based OSS project.

About Expertiza

Expertiza is an open source project utilizing the Ruby on Rails framework. It is meant to serve as an environment for classes to host assignments, teams, and evaluations of submissions. It is designed to provide instructors the ability to create and modify assignments. Instructors have access to viewing all completed assignments and grades given as well. Additionally, it allows the instructor to provide topic lists that students can access to determine which projects they sign up for. Students are able to form teams for projects and assignments, and during each they can review both peers and other submissions from teams. Expertiza is designed to support submission types ranging from URLs to varying documents. Additionally, we will be testing the controller with rswag.

Introduction

The purpose of this project is to refactor grade_controller.rb, which is responsible for all grading functionality within Expertiza. Its main purpose is to enable instructors to view all grades for an assignment and allow students to view their own grades. Our main focus is to streamline functionality, remove redundancies, and provide more clarity to this refactored class.

API::V1::GradesController utilizes action-based permissions through action_allowed to provide broader functionality to instructors versus students. With the frontend systems of the Grades Controller relying on heat map systems, view and view_team provide the detailed grading information to populate them (from the student perspective, reviewer identities are kept hidden). The edit method retrieves the relevant assignment to be modified by the frontend view. For accessing reviews, instructor_review is utilized, leading to access of previous reviews or instantiating new ones. Finally, grade changes are saved through the update function, which additionally covers comment feedback.

This controller structure aligns closely with the traditional CRUD operations providing an organized way to handle grade management. The view and view_team actions are for the Read functionality, allowing both instructors and students to access and review grade data based on their roles. The Create aspect is handled in instructor_review, which either retrieves existing reviews or initiates new ones as needed, ensuring that instructors can easily add comments or feedback for an assignment.

The Update action is fulfilled by update, which is responsible for saving new grading data and feedback from instructors. The Delete functionality is not something the grades_controller is responsible for handling.

This CRUD focused approach makes sure that the refactored grade_controller.rb is efficient and easily maintainable, while offering both instructors and students a seamless experience in managing and reviewing assignment grades. The refactor further enhances clarity and accessibility, aiming to make each action intuitive and easily accessible to the relevant users within Expertiza.

Problem Statement

The following tasks have been completed in this assignment:

• Controller Refactoring:
  • Reimplement the existing grades_controller.rb to make it more modular, reusable, and compliant with the DRY (Don’t Repeat Yourself) principle.
  • Improve the readability and maintainability of the codebase, ensuring it is easily understandable for future developers.
  • Refactor code to eliminate unnecessary repetition and reduce complexity by adhering to SOLID design principles.
• CRUD Operations:
  • Ensure that the required CRUD operations (Read, Update) are properly implemented and optimized.
  • Update methods to reflect best practices and consistent error handling mechanisms.

Previous Implementation

• One of the first major concerns we noticed was the reliance on an excessive amount of helper classes. Helper classes were being defined and provided to the grades_controller, but no other class actually used them. It could be more consistent to keep that functionality stored within helper methods local to the class.
• Functions such as view/view_my_scores/view_team would contain calculations and functionality beyond the purpose of the method, which should be delegated to private methods to preserve modularity.
• The class contained obsolete variables in multiple methods which could be cleaned up.
• Chart methodology is contained within the backend which can now be delegated to the front end, since the controller should only be responsible for grading functionality and returning appropriate data. Model data should be provided to the front end.
• Penalty calculations are currently no longer relevant for the grading controller.
• There are currently no render statements for appropriately providing data to the front end.

Our Implementation

Our Goals

• Simplify the code structure in order to enhance readability by taking complex operations and unneeded local variable assignments into private helper methods within the grades_controller.
• Enforce the intended functionality of the grades_controller by removing unneeded functionality and clutter, some of which is the responsibility of other controllers (i.e. calculating scores and penalties) or the frontend (i.e. rendering the bar chart information). The functionalities of the grades_controller are as follows:
  • Verify the user to ensure they are permitted to view the particular page they are looking for with the action_allowed controller method
  • Provide needed information for the grade heatmap from the instructor's point of view, with no hidden information such as reviewers id’s or names
  • Provide needed information for the grade heatmap from the student's point of view which obscures information such as the reviewer name/id
  • Update the student's grade and make comments on it from the instructor's point of view
• Cut down on repeated code to adhere to the DRY principle in order to improve maintainability and consistency within the code.
• Update the return values of the grades_controller methods in order to provide needed information and HTTP response codes that consistently reflect state and errors, as well as provide the required information for frontend tasks such as making the heatmap itself.

Routes and Methods

• GET /api/v1/grades/action_allowed – action_allowed
  • Checks if the current user is allowed to view scores or team information based on the queried action. Through helpers it focuses on checking if a student is viewing their own team’s scores, or if it’s an instructor trying to access grades. It renders a JSON response with status codes based on resulting output.
• GET /api/v1/grades/view/:id – view
  • Prepares and provides data required for rendering a heat map for an instructor’s point of view. The helper method gathers all the relevant data before providing it in a JSON response with the scores, assignment details, average scores, and review score count.
• GET /api/v1/grades/view_team/:id – view_team
  • Serves a similar function as the view statement, but focusing purely on the student’s perspective of their team. It provides data for rendering a heat map, but hides any sensitive information pertaining to grading, while also acquiring questions from the assignment. It renders a JSON response containing scores, assignment details, average scores, review score count, and questions.
• GET /api/v1/grades/edit/:id – edit
  • Represents functionality for editing grade information, setting the scores for every question after listing the questions out. Renders a JSON response with details about the participant, questions, scores, and assignment.
• GET /api/v1/grades/instructor_review/:id – instructor_review
  • Provides the instructor with a rendered JSON response on whether there’s a new record or preexisting instance of a review.
• PATCH /api/v1/grades/update/:participant_id – update
  • Updates the total score and comments for a team submission based on the received data. It finds the specified assignment, modifies the grade, and makes an attempt to save those changes. Upon failure it will provide an error message.

Rationale for Changes

Change	Rationale
Refactored `action_allowed?` into `action_allowed`, and added new helper methods	The `grades#action_allowed?` method was not being called anywhere in the backend, which means this functionality is only presently used by the frontend. As such, `action_allowed?` needed to become a Rails API method and endpoint, meaning that it no longer returned a boolean (which is why it was renamed). Now it is a GET request to the appropriate route.
Added `participant_scores` model	Added a participant scores model to belong to an assignment, `assignment_participant` and `question`. A participant score also holds a `score`, `total_score` and `round`.
Changed return types of all controller methods to render JSON Strings	As Rails API methods the easiest and expected way to communicate with the new frontend is by rendering appropriate JSON response strings with HTTP status codes appended, this can allow the frontend to parse and handle returned data.
Added a new suite of API endpoint tests that are compatible with Rswag and tested our API endpoints thoroughly, additionally added factories for several projects in the system.	As with any software change in a large system, testing is extremely important, so we wrote comprehensive tests for our endpoints. Additionally, since the grades_controller is dependent on many other models (mainly because there is no grades model). These changes will help future projects by providing factory methods for various models that did not currently have them in the system which should make testing for other groups easier in the future.
Refactored all the main API endpoint methods, added new helper methods, and removed redundant methods	Many of the methods were redundant and unused in the front end (all the chart methods). We removed all of those methods and reimplemented the logic for the view methods, as previously they did grade calculation within the grades_controller, something that does not follow the best programming practices. Instead, we have the calculated elsewhere and just have grades_controller displaying all the heat map information on the front end
Overhauled method comment documentation replaced with correct comments that describe what the method does, and did a quick style pass to ensure code readability	A lot of the comments within the grades_controller were not very good, some of which were outright incorrect or at the very least we're not correct to the current implementation of the controller at the time that we started it. So updating all of the documentation to be accurate and reflect the current state of the controller after our changes was a necessity, trying to aim for a high understanding of what each method does without even needing to read the code for future work on the codebase. Additionally, code readability was an issue in the old controller, primarily due to long or dense lines, we made an effort to limit these issues and promote clean, readable, and maintainable code.

Code Snippet

# Determines if the current user is able to perform :action as specified by the path parameter
  # If the user has the role of TA or higher they are granted access to all operations beyond view_team
  # Uses a switch statement for easy maintainability if added functionality is ever needed for students or
  # additional roles, to add more functionality simply add additional switch cases in the same syntax with
  # case 'action' and then some boolean check determining if that is allowed or forbidden.
  # GET /api/v1/grades/:action/action_allowed
  def action_allowed
    permitted = case params[:action]
                when 'view_team'
                  view_team_allowed?
                else
                  user_ta_privileges?
                end
    render json: { allowed: permitted }, status: permitted ? :ok : :forbidden
  end

  # Provides the needed functionality of querying needed values from the backend db and returning them to build the
  # heat map in the frontend from the TA/staff view.  These values are set in the get_data_for_heat_map method
  # which takes the assignment id as a parameter.
  # GET /api/v1/grades/:id/view
  def view
    get_data_for_heat_map(params[:id])
    render json: { scores: @scores, assignment: @assignment, averages: @averages, avg_of_avg: @avg_of_avg,
                   review_score_count: @review_score_count }, status: :ok
  end

  # Provides all relevant data for the student perspective for the heat map page as well as the
  # needed information to showcase the questionnaires from the student view.  Additionally, handles the removal of user
  # identification in the reviews within the hide_reviewers_rom_student method.
  # GET /api/v1/grades/:id/view_team
  def view_team
    get_data_for_heat_map(params[:id])
    @scores[:participants] = hide_reviewers_from_student
    questionnaires = @assignment.questionnaires
    questions = retrieve_questions(questionnaires, @assignment.id)
    render json: {scores: @scores, assignment: @assignment, averages: @averages, avg_of_avg: @avg_of_avg,
                  review_score_count: @review_score_count, questions: questions }, status: :ok
  end

  # Sets information required for editing the grade information, this includes the participant, questions, scores, and
  # assignment
  # GET /api/v1/grades/:id/edit
  def edit
    begin
      participant = AssignmentParticipant.find(params[:id])
    rescue ActiveRecord::RecordNotFound
      render json: {message: "Assignment participant #{params[:id]} not found"}, status: :not_found
      return
    end
    assignment = participant.assignment
    questions = list_questions(assignment)
    scores = review_grades(assignment, questions)
    render json: {participant: participant, questions: questions, scores: scores, assignment: assignment}, status: :ok
  end

  # Provides functionality that handles informing the frontend which controller and action to direct to for instructor
  # review given the current state of the system.  The intended controller to handle the creation or editing of a review
  # is the response controller, however this method just determines if a new review must be made based on figuring out
  # whether or not an associated review_mapping exists from the participant already.  If one does they should go to
  # Response#edit and if one does not they should go to Response#new.  Only ever returns a status of ok.
  # GET /api/v1/grades/:id/instructor_review
  def instructor_review
    begin
      participant = AssignmentParticipant.find(params[:id])
    rescue ActiveRecord::RecordNotFound
      render json: { message: "Assignment participant #{params[:id]} not found" }, status: :not_found
      return
    end
    
    review_mapping = find_participant_review_mapping(participant)
    if review_mapping.new_record?
      render json: { controller: 'response', action: 'new', id: review_mapping.map_id,
                     return: 'instructor'}, status: :ok
    else
      review = Response.find_by(map_id: review_mapping.map_id)
      render json: { controller: 'response', action: 'edit', id: review.id, return: 'instructor'}, status: :ok
    end
  end

  # Update method for the grade associated with a team, allows an instructor to upgrade a team's grade with a grade
  # and a comment on their assignment for submission.  The team is then saved with this change in order to be accessed
  # elsewhere in the code for needed scoring evaluations.  If a failure occurs while saving the team then this will
  # return a bad_request response complete with a message and the global ERROR_INFO which can be set elsewhere for
  # further error handling mechanisms
  # PATCH /api/v1/grades/:participant_id/update/:grade_for_submission
  def update
    participant = AssignmentParticipant.find_by(id: params[:participant_id])

    team = participant.team
    team.grade_for_submission = params[:grade_for_submission]
    team.comment_for_submission = params[:comment_for_submission]
    begin
      team.save
    rescue StandardError => e
      render json: {message: "Error occurred while updating grade for team #{team.id}",
                    error: e.message }, status: :bad_request
      return
    end
    render json: { controller: 'grades', action: 'view_team', id: participant.id}, status: :ok
  end

CRUD functionality

Before we did our refactor and reimplementation the grades_controller contained several unnecessary methods as well as functions vastly beyond the CRUD operations that belong within a controller like this. The two CRUD operations that are presently in the system for the grades_controller are read and updated. The read operation is implemented within the view and view_team method to provide the needed information to view the grade heat map. Then the update functionality is primarily implemented in the needed edit method which supplies information before the update and the update method has been vastly modified from its old implementation where it was not actually saving objects, we have replaced that with a proper update that modifies the grade and comment similarly to how the old save_grade_and_comment_for_submission method used to work.

SOLID Principles

Overall we strove to meet SOLID guidelines by limiting the scope of the controller back down to its intended core functionality. This helped satisfy the single responsibility principle by removing additional functionality beyond the intended scope of this class. We have also significantly cut down on the required helpers needed for these operations through our implementation of a new model participant_scores which helps serve the singular goal of keeping track of score information for questions on an assignment for a given participant. We have also improved the maintainability of the code by making our methods more atomic which should promote extension outside of our methods for new functionality satisfying the Open/Close principle. The remaining three principles did not really apply to this project as the grades_controller has minimal interactions with it's superclass and has no independent model that it controls or is responsible for governing.

DRY principle

To satisfy the don't repeat yourself principle we sought to make private helper methods to abstract repeated code that was adding complexity to our methods. One such method is the get_data_for_heat_map method which replaces a substantial amount of what used to be complex and repeated code, which has now been significantly simplified for readability. Please see the snippet below for the new method, it's primary goal is to set the instance variables of the controller as needed so we can prepare data for the heat map as needed in the front end. We also relocated complex code that did not aid in the readability into helper methods which have also been refactored, the main goal of this was to promote readability in the main endpoint methods remove unnecessary clutter from the code, and limit code repetition in future modifications by providing modular helper methods for functionality that we believe may need to be repeated in future enhancements such as listing the questions or querying review grades.

# Provides data for the heat maps in the view statements
  def get_data_for_heat_map(id)
    # Finds the assignment
    @assignment = Assignment.find(id)
    # Extracts the questionnaires
    @questions = filter_questionnaires(@assignment)
    @scores = review_grades(@assignment, @questions)
    @review_score_count = @scores[:teams].length # After rejecting nil scores need original length to iterate over hash
    @averages = vector(@scores)
    @avg_of_avg = mean(@averages)
  end

Testing Details

The test file can be located at:

spec/requests/api/v1/grades_controller_spec.rb

• action_allowed
  • Passes in the view action and an assignment we wish to view. Determines if the user has the appropriate permissions to view it.
• view
  • Passes in the id of the assignment and expects the scores, averages, avg_of_avg, review_score_count.
• view_team
  • Pasess in the assignment id and expects the scores, averages, avg_of_avg, review_score_count.
• edit
  • Passes in the id of the assignment and expects the participant's id, the same assignment's id, questions, and scores.
• instructor_review
  • Passes in the id of a participant and expects controller, action, the same participant id, and the instructor.
• update
  • Passes in a participant id and a body which contains grade_for_submission (integer) and comment_for_submission (string). Expects the same grade, comment, action, and participant's id.

Rswag UI

Final project additions

The Problem and Remaining Work

While working on our OSS project we ran into substantial difficulties with testing our controller. The primary reason for this is that the grades_controller is one of the first controller classes in the reimplementation backend that does not have a primary model associated with it. This is because it does not seem prudent to make a grades model, as scores can be held and tracked as fields in other objects as primitives so this abstraction would not be particularly helpful. However, this poses an interesting challenge when testing and reimplementing the controller. Specifically, the dependency on the successful initialization of numerous models throughout the controller including Assignment, Participant, and Team to name a few of the model classes required for operations within the grades_controller for the present create and edit CRUD operations implemented within. During our OSS project, we were able to slim down the controller and implement more elegant and atomic solutions to the tasks that the original controller was responsible for with some refactoring and reimplementation of various methods. However, even with these changes, the grades_controller is still dependent on several other classes and their interactions within the system being initialized and in a valid state in order to perform (or test) the required operations.

The primary difficulty in testing our controller as indicated above was this high dependency on other models for correct functionality. Because of this, we had to construct and instantiate several different models in our test code. This, in a vacuum, may not be an issue. However, as the reimplementation backend codebase continues to grow this issue has the potential to become a recurring one where the ultimate result will be several violations of the DRY principle in the testing suite instantiating needed objects inside of controller tests similarly to the way that we had to do this at the start. In the current codebase, the only models containing factory code are student_task, join_team_request, bookmark, and user, making this substantially shy of the thirty-six models currently residing within the reimplementation_backend repository. We will seek to change this in order to limit the likelihood of DRY violations in the test suite moving forward to improve code maintainability and consistency.

Our solution

Our solution to improving our OSS submission and the overall codebase is four-pronged. This includes expanding the factory suite to include many additional models. Our second plan is to improve our test cases and our overall test plan to be more comprehensive of all branches present in our code to conduct more thorough testing using our new factories. Our third change will be to modify the seeding files for the db in order to facilitate manual testing with RSwag. Finally our fourth goal for the project is to improve our RSwag UI by implementing the previous three steps and then ensuring that manual testing capability is achieved and correct.

Expanded Factory Suite

For our final project we are seeking to improve the code base by expanding the factory suite to include factories for most of the 37 models present in the reimplementation backend repository. The rationale behind this decision is to improve the testing capabilities of all controllers either currently in the system or ones that will be added. As the code base grows, we expect an increased interdependency of the controllers on the model classes. Without factory classes, this would result in an opportunity for several DRY violations in the test suite. This hinders the maintainability of the code base and as small changes in the models could break the entire test suite and would need to be solved individually in every test case; however, with factories, we would only need to fix the code within the factories to reflect the new code structure and the test cases will remain largely unchanged. We would also suggest that factories be continuously updated as new models are added so that it aids in future maintainability.

Improved Test Plan

• action_allowed
  • If the user is a TA or higher, then a successful status will be returned for all actions
  • If the user is a student and requests to view their team then, then a successful status will be returned
  • Otherwise, a status of 403 is rendered regardless of the action

Flow Chart

Team and Prior information

Team

Mentor

• Srusti, Chaitanya

Members

• Eastin, Charlie
• Hulse, Jeremy
• Middleton, Maverick

Previous Iterations

• Spring 2024
  • This iteration was not approved to be pulled into expertiza, but provided our team with additional insight on how to more appropriately refactor the grades controller.
• Spring 2021
  • Added additional methodology for providing model data, along with a greater focus on helper classes.
• Fall 2017
  • Refactored penalties, review systems, and utilized rspec testing.