CSC/ECE 517 Fall 2023 - E2366. Reimplement assignment model and assignment controller (Phase 2)

Expertiza

Expertiza is a Ruby on Rails based open source project. Instructors have the ability to add new projects, assignments, etc., as well as edit existing ones. Later on, they can view student submissions and grade them. Students can also use Expertiza to organize into teams to work on different projects and assignments and submit their work. They can also review other students' submissions.

Introduction

This project is a reimplementation for the assignment model and assignment controller in the Expertiza backend.

Problem Statement

The following tasks were required for the reimplementation :

1. Enhancing Code Clarity: This involves renaming methods with ambiguous names, optimizing loops and adding comments for unclear lines of code.
2. Removing unused methods Removing unused methods will reduce complexity.
3. Reimplement Assignment model and controller: Reimplement the methods from Assignment model that are complex and can be implemented in a better way to reduce complexity and by not violating the DRY principles.
4. Writing tests for assignment models: Tests have to be written for Assignment Model.
5. Writing tests for the assignment controller: RSwag tests should be written for Assignment Controller

Design Goal

While reimplementation of Assignment model and controller, the following design rules have to be ensured:

• Validate proper functioning of all existing and anticipated methods, making any required enhancements or adjustments.
• Establish loose coupling and tight cohesion for the model and controller to enhance code organization and maintainability.
• Refactor redundant code in the previous implementation's controller and model methods using DRY principle, eliminating functionality duplications already present in the expertiza system.
• Confirm the continued effectiveness of existing test cases following the aforementioned modifications and generate additional test cases as per the need.

Implementation Plan

Documentation for methods that will be re-implemented and implemented as it is:

• valid_num_review

This method raises an error if more reviews are required than are allowed. But it is a single method for both reviews and meta reviews. In the reimplemented method, type of review will be a parameter.

• varying_rubrics_by_round?

This method checks whether different rubrics are being used in different rounds of review. It does it, however, by checking whether more than one rubric is listed as being used in Round 2 (used_in_round = 2). The reimplemented method will check if we have more than one rubric of a given type for an assignment.

• badge?

This method returns the value of the has_badge instance variable and this should be renamed to has_badge?.

• calibrated?

This method returns the value of the is_calibrated instance variable and should be renamed is_calibrated?.

• current_stage

his method returns the current stage (Submission, Review, Finished, etc.) potentially based on which topic the logged-in user holds. It returns a string, which is useful for printing, though a bit informal. However, since this method is quite similar to two other methods - current_stage_name and stage_deadline, these three can be combined with some modifications in the reimplementation for clarity. For these, we will create a duedate model and implement the get_next_due_date method 2 subclasses- TopicDueDate and AssignmentDueDate.

• current_stage_name

This is a convoluted method that returns the same thing as current_stage, except in a few specialized cases, such as when the assignment has topics and the user hasn’t chosen a topic. However, since this method is quite similar to two other methods - current_stage and stage_deadline, these three can be combined with some modifications in the reimplementation for clarity. For these, we will create a duedate model and implement the get_next_due_date method 2 subclasses- TopicDueDate and AssignmentDueDate.

• stage_deadline

This method is similar to current_stage, but returns the time of the next deadline unless the assignment is finished. However, since this method is quite similar to two other methods - current_stage_name and current_stage, these three can be combined with some modifications in the reimplementation for clarity. For these, we will create a duedate model and implement the get_next_due_date method 2 subclasses- TopicDueDate and AssignmentDueDate.

• num_reviews_greater?

This method checks whether the number of reviews required is greater than the number of reviews allowed. Basically, it just compares two instance variables of Assignment,but handles the special cases of reviews_allowed being null or negative. In the reimplemented method, we will check if reviews_allowed is valid or not in a separate function.

• quiz_allowed

This method returns the quiz_allowed_id field of the next due date, i.e., whether a participant is currently allowed to take a quiz. This method will be reimplemented without making a direct call to the check_condition.

• response_map_to_metareview

This method finds the “most appropriate” ResponseMap to use as the reviewed_object in a new MetareviewResponseMap. This can be made as a class method of MetareviewResponseMap and combined with the reviewer_metareviews_map method that is basically a step of this method.

• review_questionnaire_id

This method finds the questionnaire in use for reviews of work submitted for the assignment. This is a complicated method because it calculates to find the round number, instead of using number_of_current_round(...). In the reimplementation, instead of creating a new form, we can use an already existing form that is associated with the assignment. The logic behind determining a round number based on the topic can also be improved.

• reviewer_metareviews_map

This is a step in finding the response_map_to_metareview. There is no need for a separate method for finding reviewer_metareviews_map. This method can be combined with response_map_to_metareview during reimplementation.

Note : The following methods do not require specific changes and will be implemented accordingly.

• create_node

This method creates an AssignmentNode for this assignment.

• pair_programming_enabled?

This method returns the value of the enable_pair_programming instance variable.

• staggered_and_no_topic?

This method checks if the assignment is a staggered-deadline assignment and the logged-in user has not chosen a topic.

• team_assignment?

This method checks whether max_team_size > 0 (should be > 1).

• teams?

This method is used to check whether any teams exist for this assignment.

• topics?

This method is used to check whether any topics exist for this assignment.

• create_node

This method creates an AssignmentNode for this assignment.

Note : The following methods will be removed as they are no longer required in the new implementation -

Methods to be moved to another models :

• assign_metareviewer_dynamically
• can_review
• check_condition
• find_current_stage
• find_due_dates
• find_reveiw_period
• link_for_current_stage
• metareview_allowed
• num_review_rounds
• num_of_current_round
• submission_allowed
• user_on_team?

Methods to be removed :

• microtask?
• min_metareview
• questionnaire_ids
• remove_empty_teams

Test Plan

We plan to use the given test skeletons for assignment model and implement the respective Rspec test cases. The test cases for other methods will be included later as reimplementation might have methods which are combined versions of multiple other methods. The test scenarios for some for the methods are mentioned in the following table.

test_valid_num_review

The testcases are designed to validate the relationship between the required and allowed numbers of reviews or metareviews, depending on the specified review_type. The tests cover scenarios where review_type is either "review" or "metareview" and evaluate the method's behavior in two cases: when the required number is less than or equal to the allowed number (expected to return success), and when the required number is greater than the allowed number (expected to return an error message).

 describe '#valid_num_review' do
   let(:assignment) { Assignment.new }
   context 'when review_type is "review"' do
     it 'returns success: true if num_reviews_required is less than or equal to num_reviews_allowed' do
       assignment.num_reviews_required = 2
       assignment.num_reviews_allowed = 5
       result = assignment.valid_num_review('review')
       expect(result[:success]).to be true
       expect(result[:message]).to be_nil
     end
     it 'returns an error message if num_reviews_required is greater than num_reviews_allowed' do
       assignment.num_reviews_required = 5
       assignment.num_reviews_allowed = 2
       result = assignment.valid_num_review('review')
       expect(result[:success]).to be false
       expect(result[:message]).to eq('Number of reviews required cannot be greater than number of reviews allowed')
     end
   end

   context 'when review_type is "metareview"' do
     it 'returns success: true if num_metareviews_required is less than or equal to num_metareviews_allowed' do
       assignment.num_metareviews_required = 2
       assignment.num_metareviews_allowed = 5
       result = assignment.valid_num_review('metareview')
       expect(result[:success]).to be true
       expect(result[:message]).to be_nil
     end
     it 'returns an error message if num_metareviews_required is greater than num_metareviews_allowed' do
       assignment.num_metareviews_required = 5
       assignment.num_metareviews_allowed = 2
       result = assignment.valid_num_review('metareview')
       expect(result[:success]).to be false
       expect(result[:message]).to eq('Number of metareviews required cannot be greater than number of reviews allowed')
     end
   end
 end

test_is_calibrated?

The test covers two scenarios: 1. Context: 'when is_calibrated is true': The test creates an Assignment object using the create(:assignment) factory. It sets is_calibrated to true. The expectation is that calling is_calibrated? on the assignment returns true.

2. Context: 'when is_calibrated is false': The test creates an Assignment object using the create(:assignment) factory. It sets is_calibrated to false. The expectation is that calling is_calibrated? on the assignment returns false.

 describe '#is_calibrated?' do
   let(:assignment) { create(:assignment) }
   context 'when is_calibrated is true' do
     it 'returns true' do
       assignment.is_calibrated = true
       expect(assignment.is_calibrated?).to be true
     end
   end

   context 'when is_calibrated is false' do
     it 'returns false' do
       assignment.is_calibrated = false
       expect(assignment.is_calibrated?).to be false
     end
   end
 end

test_has_badge?

The test covers two scenarios: 1. Context: 'when has_badge is true': The test creates an Assignment object. It sets has_badge to true. The expectation is that calling has_badge? on the assignment returns true. 2. Context: 'when has_badge is false': The test creates an Assignment object. It sets has_badge to false. The expectation is that calling has_badge? on the assignment returns false.

 describe '#has_badge?' do
   let(:assignment) { Assignment.new }
   context 'when has_badge is true' do
     it 'returns true' do
       assignment.has_badge = true
       expect(assignment.has_badge?).to be true
     end
   end

   context 'when has_badge is false' do
     it 'returns false' do
       assignment.has_badge = false
       expect(assignment.has_badge?).to be false
     end
   end
 end

test_pair_programming_enabled?

The test is organized into two contexts: 1. Context: "when pair programming is enabled": The test sets up an Assignment object using the create(:assignment) factory. It uses a before block to enable pair programming (assignment.enable_pair_programming = true) before each test in this context. The test asserts that when pair_programming_enabled? is called on the assignment, it should return true. 2. Context: "when pair programming is disabled": The test sets up an Assignment object using the create(:assignment) factory. It uses a before block to disable pair programming (assignment.enable_pair_programming = false) before each test in this context. The test asserts that when pair_programming_enabled? is called on the assignment, it should return false.

 describe "pair_programming_enabled?" do
   let(:assignment) {create(:assignment)}
   context "when pair programming is enabled" do
     before do
       # Enable pair programming before each test in this context
       assignment.enable_pair_programming = true
     end
     it "returns true" do
       expect(assignment.pair_programming_enabled?).to eq(true)
     end
   end

   context "when pair programming is disabled" do
     before do
       # Disable pair programming before each test in this context
       assignment.enable_pair_programming=false
       # You may need a method to disable pair programming if it's not the inverse of enable_pair_programming
     end
     it "returns false" do
       expect(assignment.pair_programming_enabled?).to eq(false)
     end
   end
 end

1. test_staggered_and_no_topic?(topic_id)

The test covers various scenarios based on the combination of staggered deadlines and the presence of a topic_id. Here's a summary: 1. Context: "when staggered deadline is enabled and topic_id is not provided": The test creates an Assignment object using the create(:assignment) factory. It sets up a scenario where staggered deadlines are enabled and no topic_id is provided. The test expects the staggered_and_no_topic? method to return true. 2. Context: "when staggered deadline is enabled and topic_id is provided": Similar to the previous context, but with a scenario where a topic_id is provided. The test expects the staggered_and_no_topic? method to return false. 3. Context: "when staggered deadline is disabled and topic_id is not provided": The test sets up a scenario where staggered deadlines are disabled and no topic_id is provided. The test expects the staggered_and_no_topic? method to return false. 4. Context: "when staggered deadline is disabled and topic_id is provided": Similar to the previous context, but with staggered deadlines disabled and a topic_id provided. The test expects the staggered_and_no_topic? method to return false.

 describe "staggered_and_no_topic?" do
   let(:assignment) {create(:assignment)}
   context "when staggered deadline is enabled and topic_id is not provided" do
     it "returns true" do
       allow(assignment).to receive(:staggered_deadline?).and_return(true)
       expect(assignment.staggered_and_no_topic?(nil)).to eq(true)
     end
   end

   context "when staggered deadline is enabled and topic_id is provided" do
     it "returns false" do
       allow(assignment).to receive(:staggered_deadline?).and_return(true)
       expect(assignment.staggered_and_no_topic?("some_topic_id")).to eq(false)
     end
   end

   context "when staggered deadline is disabled and topic_id is not provided" do
     it "returns false" do
       allow(subject).to receive(:staggered_deadline?).and_return(false)
       expect(assignment.staggered_and_no_topic?(nil)).to eq(false)
     end
   end

   context "when staggered deadline is disabled and topic_id is provided" do
     it "returns false" do
       allow(assignment).to receive(:staggered_deadline?).and_return(false)
       expect(assignment.staggered_and_no_topic?("some_topic_id")).to eq(false)
     end
   end
 end

test_teams?

The test covers two scenarios: 1. Context: 'when teams are associated with the assignment': The test creates an Assignment object using the create(:assignment) factory. It creates a team associated with the assignment using FactoryBot (create(:team, assignment: assignment)). The expectation is that calling teams? on the assignment returns true. 2. Context: 'when no teams are associated with the assignment': The test creates an Assignment object using the create(:assignment) factory. The expectation is that calling teams? on the assignment returns false.

describe '#teams?' do

   let(:assignment) {create(:assignment)}
   context 'when teams are associated with the assignment' do
     it 'returns true' do
       # Create a team associated with the assignment using FactoryBot
       team = create(:team, assignment: assignment)
       expect(assignment.teams?).to be true
     end
   end

   context 'when no teams are associated with the assignment' do
     it 'returns false' do
       expect(assignment.teams?).to be false
     end
   end
 end

test_topics?

The test covers two scenarios: 1. Context: 'when no topic is associated with the assignment': The test creates an Assignment object using the create(:assignment) factory. It sets up a scenario where no topic is associated with the given assignment. The expectation is that calling topics? on the assignment returns false. 2. Context: 'when any topic is associated with the assignment': The test creates an Assignment object using the create(:assignment) factory. It creates a sign_up_topic associated with the assignment using FactoryBot (create(:sign_up_topic, assignment: assignment)).. The expectation is that calling topics? on the assignment returns true.

 describe '#topics?' do
   let(:assignment) { create(:assignment) }
   context 'when sign_up_topics is empty' do
     it 'returns false' do
       # Assuming sign_up_topics is an empty collection
       expect(assignment.topics?).to be false
     end
   end

   context 'when sign_up_topics is not empty' do
     it 'returns true' do
       # Assuming sign_up_topics is a non-empty collection
       sign_up_topic = create(:sign_up_topic, assignment: assignment)
       expect(assignment.topics?).to be true
     end
   end
 end

test_create_node

The test covers two scenarios: 1. Context: "when the parent node exists": The test creates an Assignment object using the create(:assignment) factory.It stubs CourseNode.find_by to return a mock parent node. It sets up expectations for the creation of an AssignmentNode, including setting the parent_id and saving the new node. The test calls the create_node method. 2. Context: "when the parent node does not exist": Similar to the previous context, but it stubs CourseNode.find_by to return nil (indicating no parent node). It sets up expectations for the creation of an AssignmentNode, excluding the setting of parent_id (since there is no parent), and saving the new node. The test calls the create_node method.

 describe "#create_node" do
   let(:assignment) { create(:assignment) }
   context "when the parent node exists" do
     it "creates a new assignment node with the given id, sets parent_id, and saves the new node" do
       # Stub CourseNode.find_by to return a mock parent node
       allow(CourseNode).to receive(:find_by).and_return(double("CourseNode", id: 10))
       # Expectations for AssignmentNode creation
       expect(AssignmentNode).to receive(:create).with(node_object_id: assignment.id).and_call_original
       # Expectations for any_instance of AssignmentNode
       assignment_node_instance = instance_double(AssignmentNode)
       allow(assignment_node_instance).to receive(:parent_id=)
       allow(assignment_node_instance).to receive(:save)
       expect(AssignmentNode).to receive(:new).and_return(assignment_node_instance)
       # Call the method under test
       assignment.create_node
     end
   end

   context "when the parent node does not exist" do
     it "creates a new assignment node with the given id, does not set parent_id, and saves the new node" do
       # Stub CourseNode.find_by to return nil (no parent node)
       allow(CourseNode).to receive(:find_by).and_return(nil)
       # Expectations for AssignmentNode creation
       expect(AssignmentNode).to receive(:create).with(node_object_id: assignment.id).and_call_original
       # Expectations for any_instance of AssignmentNode
       assignment_node_instance = instance_double(AssignmentNode)
       expect(assignment_node_instance).not_to receive(:parent_id=)
       allow(assignment_node_instance).to receive(:save)
       expect(AssignmentNode).to receive(:new).and_return(assignment_node_instance)
       # Call the method under test
       assignment.create_node
     end
   end
 end

test_varying_rubrics_by_round?

The test is divided into two contexts: 1. Context 'when rubrics with specified rounds are present': The test creates an Assignment and a Questionnaire. It then creates an AssignmentQuestionnaire associating the assignment with the questionnaire and specifying that it's used in round 1. The test expects the varying_rubrics_by_round? method to return true, indicating that rubrics with specified rounds are present for the assignment. 2. Context 'when no rubrics with specified rounds are present': The test creates an Assignment. It expects the varying_rubrics_by_round? method to return false when no rubrics with specified rounds are associated with the assignment.

 describe '#varying_rubrics_by_round?' do
   let(:assignment) { create(:assignment) }
   let(:questionnaire) {create(:questionnaire)}
   context 'when rubrics with specified rounds are present' do
     it 'returns true' do
       # Assuming rubrics with specified rounds exist for the assignment
       create(:assignment_questionnaire, assignment: assignment, questionnaire: questionnaire, used_in_round: 1)
       expect(assignment.varying_rubrics_by_round?).to be true
     end
   end

   context 'when no rubrics with specified rounds are present' do
     it 'returns false' do
       # Assuming no rubrics with specified rounds exist for the assignment
       expect(assignment.varying_rubrics_by_round?).to be false
     end
   end
 end

test_ team_assignment?

The test covers three scenarios: 1. Context: 'when max_team_size is greater than 0': The test creates an Assignment object using the create(:assignment) factory. It sets the max_team_size attribute to a value greater than 0. The test expects that calling team_assignment? on the assignment returns true. 2. Context: 'when max_team_size is equal to 0': Similar to the previous context, but it sets max_team_size to 0. The test expects that calling team_assignment? on the assignment returns false. 3. Context: 'when max_team_size is less than 0': Similar to the previous contexts, but it sets max_team_size to a negative value. The test expects that calling team_assignment? on the assignment returns false.

 describe 'team_assignment?' do
   let(:assignment) { create(:assignment) }
   context 'when max_team_size is greater than 0' do
     it 'returns true' do
       assignment.max_team_size = 5
       expect(assignment.team_assignment?).to be true
     end
   end

   context 'when max_team_size is equal to 0' do
     it 'returns false' do
       assignment.max_team_size = 0
       expect(assignment.team_assignment?).to be false
     end
   end

   context 'when max_team_size is less than 0' do
     it 'returns false' do
       assignment.max_team_size = -3
       expect(assignment.team_assignment?).to be false
     end
   end
 end

Team

Mentor

• Ameya Vaichalkar

Members

• Bhavya Harchandani <bharcha@ncsu.edu>
• Akshat Saxena<asaxen24@ncsu.edu>
• Mitali Sethi <msethi@ncsu.edu>

Pull Request

• [1]

References

• Project Instructions