CSC/ECE 517 Fall 2022 - E2256: Refactor late policies controller.rb

Background

The SCRUM from late policies is managed by the later policies controller. The file primarily determines whether newly added or updated late policies are valid policies by ensuring that they do not share the same name as an already existing policy. The file also ensures that the late penalty per unit of time is less than the maximum late penalty, and restricts late penalty to have a maximum value of 100.

Team

Mentor

Vinay Deshmukh (vdeshmu@ncsu.edu)

Team Members

Kalgee Anand Kotak (kkotak@ncsu.edu)
Ashrita Ramaswamy (aramasw@ncsu.edu)
Nitish Kumar Murali (nmurali2@ncsu.edu)

Description about project

This wiki describes the work done to refactor late policies controller.rb. The main goals of the effort were to eliminate redundant code and to make the create and update methods' code easier to read. This also includes adding code comments for understandability.

Files Modified

• app/controllers/late_policies_controller.rb
• app/views/late_policies/new.html.erb
• app/views/late_policies/_form.html.erb
• app/views/late_policies/show.html.erb
• app/views/late_policies/edit.html.erb
• app/views/late_policies/index.html.erb
• spec/controllers/late_policies_controller_spec.rb

Issues to be addressed

I1 : Rename all usages of @penalty_policy to @late_policy within the controller as well as the views
I2 : Refactor the method “duplicate_name_check” such that it doesn’t violate the Single Responsibility Principle. Use a better name than “duplicate_name_check”
I3 : Make the validate_input method as “stateless” as possible.
I4 : Update the code so that initially you assume that the penalty is NOT VALID, and then you need to verify that it is valid during validation.
I5 : Fix the Error Message output. It currently prints only one error message even if more than one validation is failing.
I6 : Use local variables instead of instance variables in the create and update method if possible.
NOTE: I6 is already addressed, as both create and update methods have minimal usage of instance variables. The instance variables in the create method are used by the other parts of the program and hence cannot be replaced.
I7 : Add RSpec tests (Task completed, as the controller's CRUD operations along with the methods are being tested by the existing test cases even after the refactoring)

Changes Implemented

• Renaming @penalty_policy to @late_policy within controller and views
  

Commit Link: https://github.com/NitishKumar2404/Expertiza/commit/61b01faf67bc903beb03e6d33d6abcc640e78d48

• Refactor the method duplicate_name_check such that it doesn’t violate the Single Responsibility Principle. Use a better name than “duplicate_name_check”.
  

Fix: The duplicate_name_check method takes a boolean parameter is_update as a “flag” to either run or not run a block of code (line 140-145). We have refactor this method and renamed it to check_if_name_exists. We move the if condition in line 140-145 to update method. The check_if_name_exists method is called from both create and update (if policy name is update) to check if a policy with same name does not already exists.

• Make the validate_input method as “stateless” as possible. The validate_input method calls duplicate_name_check , both are using instance variables like `params`. Update the code so that initially you assume that the penalty is NOT VALID, and then you need to verify that it is valid.
  

Fix: The validate_input is refactored by moving the logic of lines 162-163 to create and update method. Instance variables like `params` is substituted with late_policy_params method. The variable "valid_penalty" is initially set to false and is update to true if error message is not generated.

• While creating or updating a late policy, in cases where a user enters multiple invalid inputs, only the last error message was shown.
  

Fix: In the earlier code in case of not valid input the error message is generated based on the last if loop condition it satisfied. Ideally, all error messages should be printed so that the user can fix all the errors before the next time they submit. We modified the code in late_policies_controller.rb to incorporate this change by concatenating the error message so that all errors are taken into account.

Peer Review Information

For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:

• Instructor login: username -> instructor6, password -> password

Test Plan

RSpec

This is a refactoring project, so we tested using the existing test cases in late_policies_controller_spec.rb. Since all the test cases are passing it implies that the refactored code does not break any of the existing functionality. We have also added some new test cases in the rspec.
To test this controller, run the following commands to verify that the code is working properly.

rspec spec/controllers/late_policies_controller_spec.rb

Screenshot of RSpec Test Cases Running Successfully

Edge Cases

The test cases defined to cover the CRUD operations associated with the controller along with the edge cases as follows:

1 : Maximum penalty given is greater than 100 and the maximum penalty is less than the penalty per unit while creating a late policy.

 context 'when maximum penalty is less than penalty per unit' do
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 30,
            penalty_per_unit: 100,
            policy_name: 'Policy2'
          },
          id: 1
        }
        post :create, params: request_params
        expect(flash[:error]).to eq('Cannot edit the policy. The maximum penalty cannot be less than penalty per unit.')
        expect(response).to redirect_to('/late_policies/1/edit')
      end
  context 'when maximum penalty is greater than 100' do
      before(:each) do
        latePolicy = LatePolicy.new
        allow(latePolicy).to receive(:check_policy_with_same_name).with(any_args).and_return(false)
      end
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 101,
            penalty_per_unit: 30,
            policy_name: 'Policy1'
          }
        }
        post :create, params: request_params
        expect(flash[:error]).to eq('Maximum penalty cannot be greater than or equal to 100.')
        expect(response).to redirect_to('/late_policies/new')
      end
    end

2 : Maximum penalty is less than the penalty per unit while updating a late policy.

 context 'when maximum penalty is less than penalty per unit' do
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 30,
            penalty_per_unit: 100,
            policy_name: 'Policy2'
          },
          id: 1
        }
        post :update, params: request_params
        expect(flash[:error]).to eq('Cannot edit the policy. The maximum penalty cannot be less than penalty per unit.')
        expect(response).to redirect_to('/late_policies/1/edit')
      end
    end

3 : Penalty per unit is negative while creating or updating the late policy.

  context 'when penalty per unit is negative while creating late policy' do
      before(:each) do
        allow(LatePolicy).to receive(:check_policy_with_same_name).with(any_args).and_return(false)
      end
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 30,
            penalty_per_unit: -10,
            policy_name: 'Invalid_Policy'
          }
        }
        # post:update while updating the policy
        post :create, params: request_params
        expect(flash[:error]).to eq('Penalty per unit cannot be negative.')
      end
    end

4: Policy with the same name exists or not while creating or updating the late policy.

context 'when policy with same name exists' do
      before(:each) do
        latePolicy = LatePolicy.new
        allow(LatePolicy).to receive(:check_policy_with_same_name).with(any_args).and_return(true)
      end
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 30,
            penalty_per_unit: 10,
            policy_name: 'Policy1'
          }
        }
        # post:update while updating the policy
        post :create, params: request_params
        expect(flash[:error]).to eq('A policy with the same name Policy1 already exists.')
      end
    end

5: Ensure that the changes are saved while creating a late policy.

context 'when the late_policy is not saved' do
      before(:each) do
        latePolicy = LatePolicy.new
        latePolicy.policy_name = 'Policy1'
        latePolicy.max_penalty = 40
        latePolicy.penalty_per_unit = 30
        latePolicy.instructor_id = 6
        allow(latePolicy).to receive(:check_policy_with_same_name).with(any_args).and_return(false)
        allow(latePolicy).to receive(:new).with(any_args).and_return(latePolicy)
        allow(latePolicy).to receive(:save!).and_return(false)
      end
      it 'throws a flash error ' do
        request_params = {
          late_policy: {
            max_penalty: 40,
            penalty_per_unit: 30,
            policy_name: 'Policy1'
          }
        }
        post :create, params: request_params
        expect(flash[:error]).to eq('The following error occurred while saving the late policy: ')
      end

UI Testing

The functionality can be tested manually by following steps.
1. Navigate over to the late policies index page: http://152.7.177.201:8080/late_policies.
2. Create a new policy using same name as existing policy and see if you get proper error message.
3. Try updating an existing policy by passing in the name that is already in use and see if you get a proper error message.
4. Try different input combinations and test if error messages appears for invalid input.

Screenshot of Deployed UI Running

Important Links

Github Links

Link to Expertiza repository: https://github.com/expertiza/expertiza
Link to the forked repository: https://github.com/NitishKumar2404/Expertiza
Linked to Pull Request: https://github.com/expertiza/expertiza/pull/2466

Video Link

The following contains a link to a video capturing the test cases passing, Project Board, and UI running in the deployed VCL:

https://drive.google.com/file/d/1m-3q7iTECIQh8_KHoxNRU2OP4vBS5K2z/view?usp=share_link