CSC/ECE 517 Spring 2023 - E2323. Refactor DueDate functionality from assignment.rb

E2323. Refactor DueDate functionality from assignment.rb

About Expertiza

Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Project Overview and Mission

The goal of this project was to: reduce the length of assignment.rb by removing some related methods into a mixin, refactor several methods using topic_id and check_condition to be more readable, and reduce the use of complex data structures in these methods. The methods removed from assignment.rb include:

• check_condition(column, topic_id)
• submission_allowed(topic_id)
• can_review(topic_id)
• quiz_allowed(topic_id)
• metareview_allowed(topic_id)
• next_due_date(topic_id)
• current_stage(topic_id)
• num_review_rounds
• number_of_current_round(topic_id)

Additionally, the check_conditon method was removed.

---

Refactoring Subjects

Creating MixIn of Methods

All of the methods in this project take topic_id as an argument and therefore could be moved into a mixin. This serves the dual purpose of reducing the length of assignment.rb and allowing other parts of the project to access the due_date functionality.

The methods in the scope of this project were moved from app/models/assignment.rb to lib/due_date_mix_in.rb and placed within the module "DueDateMixIn".

Creating a method to find topic_id from current user

One of the unifying themes of the current implementation of these methods is their use of the topic_id argument. The topic_id can be easily found using the assignment id and the participant's id, and we can increase reliability and usability by changing the argument for these classes into participant id. Below is the implementation of the find_topic_id class.

  def find_topic_id(participant_id)
    topic_id = if participant_id.nil?
                 nil
               else
                 SignedUpTeam.topic_id(id, participant_id)
               end
  end

This method invokes the SingedUpTeam class' topic_id method and gives it the assignment id and the participant id. Additionally, the function will return nil, if a nil participant id is given to the function. This is to retain some of the functionality of the student_task view page without changing the view page too much.

Refactoring *_allowed Methods

Current Functions

The following methods all used the check_condition method using a topic_id argument:

• submission_allowed(topic_id)
• can_review(topic_id)
• quiz_allowed(topic_id)
• metareview_allowed(topic_id)

This method can thus be refactored as a group into a more readable and understandable form.

  # Check whether review, metareview, etc.. is allowed
  # The permissions of TopicDueDate is the same as AssignmentDueDate.
  # Here, column is usually something like 'review_allowed_id'
  def check_condition(column, topic_id = nil)
    next_due_date = DueDate.get_next_due_date(id, topic_id)
    return false if next_due_date.nil?

    right_id = next_due_date.send column
    right = DeadlineRight.find(right_id)
    right && (right.name == 'OK' || right.name == 'Late')
  end

This was implemented in the other methods like in the example below:

  # Determine if the next due date from now allows for submissions
  def submission_allowed(topic_id = nil)
    check_condition('submission_allowed_id', topic_id)
  end

This implementation can be confusing as it relies on passing a string and topic_id, which are not immediately evident in their function.

Refactor

This method was refactored into the form below. Majorly the argument was changed from topic_id to participant_id in order to make the code more readable, and therefore the function of this method can be parsed more quickly.

  # Determine if the next due date from now allows for submissions
  def submission_allowed(participant_id = nil)
    # Find topic id for given participant for selected assignment
    # Return nil if no participant is given
    topic_id = find_topic_id(participant_id)
    # only need to pass @particpiant to search, can this be done locally
    next_due_date = DueDate.get_next_due_date(id, topic_id)
    return false if next_due_date.nil?

    # find the quiz allowed id, then check if that deadline is passed
    right_id = next_due_date.submission_allowed_id
    right = DeadlineRight.find(right_id)
    # check is assignment action deadline is ok or late (i.e. not no)
    right && (right.name != 'No')
  end

In order to search for a due date, we need to have the assignment id and the topic_id. The assignment id is given by the params. So if we refactor the method to accept the current participant as an argument, we need to add methods to find the topic_id. Below is a text diff run on the current implementation of check_condition and the updated refactoring of submission allowed to compare the functionality.

Major Changes Include:

• Argument is now the participant user_id and not topic_id
• Finding right_id no longer invokes the send method.
• The conditional statement at the end of the method was also simplified as Deadline is tristate and an additional "or" statement is not needed.

Updating Tests

A new rspec file was created in the spec/lib/ folder called due_date_mix_in_spec.rb

The tests for the *_allowed methods were written in the following manor as exemplified by the submission_allowed method.

  describe '#submission_allowed' do
    it 'returns true when the next topic due date is allowed to submit sth' do
      #allow(assignment).to receive(:check_condition).with('submission_allowed_id', 123).and_return(true)
      #expect(assignment.submission_allowed(123)).to be true
      assignment_due_date = double('AssignmentDueDate')
      assignment_topic_id = double('AssignmentTopicId')
      allow(SignedUpTeam).to receive(:topic_id).with(1, 1).and_return(assignment_topic_id) # 1,1
      allow(DueDate).to receive(:get_next_due_date).with(1, assignment_topic_id).and_return(assignment_due_date)
      allow(assignment_due_date).to receive(:submission_allowed_id).and_return(1)
      allow(DeadlineRight).to receive(:find).with(1).and_return(double('DeadlineRight', name: 'OK'))
      expect(assignment.submission_allowed(1)).to be true
    end
  end

These tests were modified to allow for the tests to access the methods and objects now being used within the methods.