CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb

Expertiza Background

Expertiza is an assignment portal developed by faculties and students at NCSU. It provides a platform for the faculties to create assignments for students. Faculties can have assignments with staged deadlines. Expertiza has support to create teams for students, track the team members and provide review for the work done by team members. Students can also provide reviews for peer team's assignment submission. Expertiza has been developed on Ruby on Rails and is available on github.

Problem Description

Following is an OSS project which deals with refactoring of stage deadlines in assignment.rb file. An assignment can have incremental deadlines for different topics in a single assignment. This project involves refactoring the functions in assignment.rb file related to stage deadlines. There were five functions to check what kind of stage an assignment is in. However, the names of these functions were ambiguous and functionalities implemented in some of them overlapped with each other. By the end of this project we have refactored these deadline functions.

Important Links

1. VCL Link for expertiza deployed with changes
2. OSS documentation

Current Scenario

The assignment.rb file has the following functions implemented :
1. current_stage_name(topic_id = nil)
2. find_current_stage(topic_id = nil)
3. get_current_stage(topic_id = nil)
4. link_for_current_stage(topic_id = nil)
5. stage_deadline(topic_id = nil)

Solution Provided

We have found the following issues with the code with respect to Ruby conventions :

Multiple calls to DueDate.get_next_due_date()

Problem with Existing code:
In the existing code, DueDate.get_next_due_date(self.id, topic_id) has been called at numerous places. This does not follow the ruby coding standards.

Solution:
New private method next_due_date(topic_id) has been added. This function returns the next_due_date by calling the get_next_due_date method on DueDate.

File: app/models/assignment.rb
Code:

def next_due_date(topic_id = nil)
 DueDate.get_next_due_date(self.id, topic_id)
end

Following places have been refactored.

Screenshots.

Checking finished status with static string “Finished”

Problem with Existing code:
The above methods have been used at various places to check finish status of assignment by comparing with “Finished”. This does not follow ruby coding standards, as we should never compare with static things. Otherwise it becomes difficult to refactor the code later.

Solution:
Assignment is said to be finished if the next due is nil. A new private method finished? has been added. It calls the above next_due_date method and returns true if the next_due_date is nil.

File: app/models/assignment.rb Code:

def finished?( topic_id = nil )
 next_due_date(topic_id).nil?
end

The check with “Finished” has been replaces with a call to finished? . The code has been refactored at following places.

Screenshots.

Return “Unknown” when topic_id is missing

Problem with existing code:
When it is staggered deadline and the topic_id is nil, “Unknown” is returned. This does not follow the DRY Principle. Never should we compare with static strings.

Solution:
To make the code more readable and understandable, and to DRY out the code we have added a new private method topic_missing? to check if the topic is missing in case of staggered deadline.

File:app/models/assignment.rb
Code:

def topic_missing?( topic_id = nil)
 topic_id.nil? and self.staggered_deadline?
end

The code has been refactored at following places:

1. TODO

find_current_stage method removed

Problem with the existing code:
The method find_current_stage has been used locally in assignment.rb and once in students controller. However internally, this method calls the next_due_date method for the assignment which has already been separated out.

Solution:
The method find_current_stage has been removed. The places where it was called has been replaced by a call to next_due_date method.

File: app/models/assignment.rb , app/controllers/student_controller.rb

Code:

Before:
 due_date = find_current_stage(topic_id)

After:
 due_date = DueDate.get_next_due_date(self.id.topic_id)

The code has been refactored in assignments.rb and the students controller at following places:

1. TODO

link_for_current_stage method removed

Problem with existing code:
The method link_for_current_stage was called once in list.html.erb. This method checks if the current assignment has any URL specified with it and would return it if present. However, the database has no such value. As a result, the if condition where the function was called always evaluated to false.

Solution:
The function link_for_current_stage has been removed. The call to this function has also been removed and the code has been refactored accordingly.

File: app/models/assignment.rb , app/views/list.html.erb

The code in list.html.erb has been refactored as follows:

1. TODO

Automated Testing using RSPEC

Tests for get_current_stage()

The current stage of expertiza did not have any tests for get_current_stage. We have added the test for the same as follows:

File: spec/models/assignment_spec.rb
Code:

describe '#get_current_stage ' do
   context 'when topic id is nil and current assignment has staggered deadline' do
     it 'returns Unknown' do
       allow(assignment).to receive(:topic_missing?).and_return(true)
       expect(assignment.get_current_stage).to eq('Unknown')
     end
   end
   context 'when current assignment does not have staggered deadline' do
     context 'when due date is nil' do
       it 'returns Finished' do
         allow(assignment).to receive(:finished?).with(123).and_return(true)
         expect(assignment.get_current_stage (123)).to eq('Finished')
       end
     end
     context 'when due date is not nil and due date is not equal to Finished' do
       it 'returns current stage name' do
         allow(assignment).to receive(:finished?).with(123).and_return(false)
         allow(assignment).to receive(:topic_missing?).with(123).and_return(false)
         allow(assignment).to receive(:next_due_date).with(123).and_return(assignment_due_date)
         deadline = create(:deadline_type, id: 1, name:"Review")
         allow(DeadlineType).to receive(:find).with(1).and_return(deadline)
         expect(assignment.get_current_stage(123)).to eq('Review')
       end
     end
   end
end

Tests for finished?

Tests for new finished? function have been added.
File: spec/models/assignment_spec.rb

describe '#finished?' do
   context 'when assignment next due date is nil' do
     it 'returns True' do
       allow(DueDate).to receive(:get_next_due_date).with(1, 123).and_return(nil)
       expect(assignment.finished?(123)).to eq(true)
     end
   end
 end
 describe '#finished?' do
   context 'when there is a next due date' do
     it 'returns False' do
       allow(DueDate).to receive(:get_next_due_date).with(1,123).and_return('2021-11-11 11:11:11')
       expect(assignment.finished?(123)).to eq(false)
     end
   end
 end

topic_missing method is a private method and hence no tests have been added for the same.

Team

Omkar Kashid
Deeksha Rao
Swathi Dinakar