CSC/ECE 517 Fall 2021 - E2128. Refactor student quizzes controller.rb & late policies controller.rb

From Expertiza_Wiki
Jump to navigation Jump to search

This wiki page is for the description of changes made under E2128 OSS assignment for Fall 2021, CSC/ECE 517.

Introduction

Background

Expertiza is a web application developed using Ruby on Rails that serves as a peer-review system. The application allows students to submit and peer-review learning objects (articles, code, websites, etc). It is an open-source project and its codebase is maintained on GitHub. We are contributing to Expertiza as a part of our Object-Oriented Design and Development's Open-Source Software (OSS) Project. Our goal in this project is to refactor the student_quizzes_controller and late_policies_controller. In this Wiki Page, we would be explaining the changes that we have made for the same.

Motivation

This project in particular intends that the students collaborate with each other and work on making enhancements to the code base by applying the concepts of Rails, RSpec, DRY code, Test-driven development, etc. This provides an opportunity for students to contribute to an open-source project and learn further about software deployment etc.

student_quizzes_controller needs several comments to explain the methods and the code in general while late_policies_controller has code duplication which could be refactored to make it DRY and follow the Ruby style guide and Rails 4 syntax.

Issues in student_quizzes_controller

  • Issue 1: finished_quiz method needs a comment describing its purpose (Line 23)
  • Issue 2: Update .first to .last (Line 24)
  • Issue 3: Check @participant variable name (Line 28)
  • Issue 4: Fix method comment for self.take_quiz (Line 34)
  • Issue 5: Refactor calculate_score method (Line 53-96 and Line 108)
  • Issue 6: record_response needs a method comment (Line 98)
  • Issue 7: Fix the English in the message (Line 110)
  • Issue 8: Refactor review_questions method (Line 119)

Issues in late_policies_controller

  • Issue 9: Create public & private late policies
  • Issue 10: Fix duplicate code in create and update method.
  • Issue 11: create and update method need comments and better identifier naming

Modifications made to student_quizzes_controller

Issue 1

Added a method comment to better describe the purpose of finished_quiz method.

Method : finished_quiz

After Refactoring 

 # Gets the last submission for a quiz by a student

Issue 2

Updated the query from .first to .last as we want to get the latest score and not the first score, in the case a team/student submitted the quiz multiple times.

Method : finished_quiz

Before Refactoring 

 @response = Response.where(map_id: params[:map_id]).first

After Refactoring 

 @response = Response.where(map_id: params[:map_id]).last # Get the last score if a student took a quiz multiple times

Issue 3

Method: finished_quiz

The code under consideration is the following 

@participant = AssignmentTeam.find(@map.reviewee_id).participants.first

We get the first participant from the list of participants from AssignmentTeam, which is stored in the variable @participant. Therefore no refactor is required for this issue.

Issue 4

Updated the method comment to better explain its purpose.

Method : self.take_quiz
Before Refactoring 

 # Create an array of candidate quizzes for current reviewer

After Refactoring 

 # Returns an array of candidate quizzes for current reviewer

Issue 5

As the method calculate_score has not yet been completed, skipping this issue, as directed by Professor Gehringer.

Issue 6

Added method comment to record response to describe its purpose.

Method : record_response

After Refactoring 

 # Saves the response by the student only if the student has not taken the quiz before.

Issue 7

Fixed the english of the flash message as the word records was ambiguous.

Method : record_response
Before Refactoring 

 flash[:error] = "You have already taken this quiz, below are the records for your responses."

After Refactoring 

 flash[:error] = "You have already taken this quiz. Please find your previous submission below."

Issue 8

Update the method comment to better describe review_questions.

Method : review_questions

Before Refactoring 

 # This method is only for quiz questionnaires, it is called when instructors click "view quiz questions" on the pop-up panel.

After Refactoring 

 # Gets all the questionnaire responses by all the teams for a particular assignment

Modifications made to late_policies_controller

Issue 9

It seems like some other team already fixed this issue, as all late policies are already shown in index. 

  # GET /late_policies
  # GET /late_policies.xml
  def index
    @penalty_policies = LatePolicy.all
    respond_to do |format|
      format.html # index.html.erb
      format.xml  { render xml: @penalty_policies }
    end
  end

Issue 10

We started by exploring the create and update methods in the late_policies_controller. We found an existing bug in the code. In an update, if the new policy name is the name of an existing policy, an error wasn't returned and the call went through, resulting in multiple policies with the same name. We fixed that error.


After the fix, the error message is shown properly when the new policy name is the name of an existing policy.

We also moved the duplicate code from update and create to a validator function which validates the policy name and penality points per unit for both create and update. The fix was done in the controller itself because the view already handled duplicate code using the partial form.

Before Refactoring 

  # POST /late_policies
  # POST /late_policies.xml
  def create
    invalid_penalty_per_unit = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i
    flash[:error] = "The maximum penalty cannot be less than penalty per unit." if invalid_penalty_per_unit
    same_policy_name = false
    if same_policy_name != LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
      flash[:error] = "A policy with the same name already exists."
      same_policy_name = true
    end
    if !invalid_penalty_per_unit && !same_policy_name
      @late_policy = LatePolicy.new(late_policy_params)
      @late_policy.instructor_id = instructor_id
      begin
        @late_policy.save!
        flash[:notice] = "The penalty policy was successfully created."
        redirect_to action: 'index'
      rescue StandardError
        flash[:error] = "The following error occurred while saving the penalty policy: "
        redirect_to action: 'new'
      end
    else
      redirect_to action: 'new'
    end
  end

  # PUT /late_policies/1
  # PUT /late_policies/1.xml
  def update
    @penalty_policy = LatePolicy.find(params[:id])
    invalid_penalty_per_unit = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i
    flash[:error] = "The maximum penalty cannot be less than penalty per unit." if invalid_penalty_per_unit
    same_policy_name = false
    # if name has changed then only check for this
    if params[:late_policy][:policy_name] != @penalty_policy.policy_name
      if same_policy_name == LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
        flash[:error] = "The policy could not be updated because a policy with the same name already exists."
      end
    end
    if !same_policy_name && !invalid_penalty_per_unit
      begin
        @penalty_policy.update_attributes(late_policy_params)
        @penalty_policy.save!
        LatePolicy.update_calculated_penalty_objects(@penalty_policy)
        flash[:notice] = "The late policy was successfully updated."
        redirect_to action: 'index'
      rescue StandardError
        flash[:error] = "The following error occurred while updating the penalty policy: "
        redirect_to action: 'edit', id: params[:id]
      end
    elsif invalid_penalty_per_unit
      flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit."
      redirect_to action: 'edit', id: params[:id]
    elsif same_policy_name
      flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists."
      redirect_to action: 'edit', id: params[:id]
    end
  end

After Refactoring 

  # validate checks if the entered penalty per unit and policy name is valid
  def validate(params, is_update, current_policy_name)
    valid = {}
    # penalty per unit cannot be greater than max penalty
    valid[:invalid_penalty_per_unit] = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i
    flash[:error] = "The maximum penalty cannot be less than penalty per unit." if valid[:invalid_penalty_per_unit]
    valid[:policy_name_already_exists] = false
    # entered policy name cannot be same as an existing one
    if (is_update && current_policy_name != params[:late_policy][:policy_name]) || !is_update
      if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
        flash[:error] = "A policy with the same name already exists."
        valid[:policy_name_already_exists] = true
      end
    end
    valid
  end
  # POST /late_policies
  # POST /late_policies.xml
  def create
    valid = validate(params, false, "")
    if !valid[:invalid_penalty_per_unit] && !valid[:policy_name_already_exists]
      @late_policy = LatePolicy.new(late_policy_params)
      @late_policy.instructor_id = instructor_id
      begin
        @late_policy.save!
        flash[:notice] = "The penalty policy was successfully created."
        redirect_to action: 'index'
      rescue StandardError
        flash[:error] = "The following error occurred while saving the penalty policy: "
        redirect_to action: 'new'
      end
    else
      redirect_to action: 'new'
    end
  end

  # PUT /late_policies/1
  # PUT /late_policies/1.xml
  def update
    @penalty_policy = LatePolicy.find(params[:id])
    valid = validate(params, true, @penalty_policy.policy_name)
    if !valid[:policy_name_already_exists] && !valid[:invalid_penalty_per_unit]
      begin
        @penalty_policy.update_attributes(late_policy_params)
        @penalty_policy.save!
        LatePolicy.update_calculated_penalty_objects(@penalty_policy)
        flash[:notice] = "The late policy was successfully updated."
        redirect_to action: 'index'
      rescue StandardError
        flash[:error] = "The following error occurred while updating the penalty policy: "
        redirect_to action: 'edit', id: params[:id]
      end
    elsif valid[:invalid_penalty_per_unit]
      flash[:error] = "Cannot edit the policy. The maximum penalty cannot be less than penalty per unit."
      redirect_to action: 'edit', id: params[:id]
    elsif valid[:policy_name_already_exists]
      flash[:error] = "Cannot edit the policy. A policy with the same name " + params[:late_policy][:policy_name] + " already exists."
      redirect_to action: 'edit', id: params[:id]
    end
  end

Issue 11

We removed 'same_policy_name' and 'invalid_penalty_per_unit' and used a hash for that functionality. Then we changed the 'same_policy_name' to 'policy_name_already_exists'. We also added comments to the validator function which is called in create and update. 

  # validate checks if the entered penalty per unit and policy name is valid
  def validate(params, is_update, current_policy_name)
    valid = {}
    # penalty per unit cannot be greater than max penalty
    valid[:invalid_penalty_per_unit] = params[:late_policy][:max_penalty].to_i < params[:late_policy][:penalty_per_unit].to_i
    flash[:error] = "The maximum penalty cannot be less than penalty per unit." if valid[:invalid_penalty_per_unit]
    valid[:policy_name_already_exists] = false
    # entered policy name cannot be same as an existing one
    if (is_update && current_policy_name != params[:late_policy][:policy_name]) || !is_update
      if LatePolicy.check_policy_with_same_name(params[:late_policy][:policy_name], instructor_id)
        flash[:error] = "A policy with the same name already exists."
        valid[:policy_name_already_exists] = true
      end
    end
    valid
  end

Affected Classes

Changed existent classes

  • controllers/student_quizzes_controller.rb
  • controllers/late_policies_controller.rb

Test Plan

As this is a refactoring project with mostly adding comments and updating variable names we can check if the code is working by running the existing tests itself.

Test the student quizzes controller with the following command: 

rspec spec/controllers/student_quizzes_controller_spec.rb

Test the late policies controller with the following command: 

rspec spec/controllers/late_policies_controller_spec.rb
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2021_-_E2128._Refactor_student_quizzes_controller.rb_%26_late_policies_controller.rb&oldid=139936"