CSC/ECE 517 Spring 2024/ OSS E2400 Allow Reviewers to Bid on What to Review

This wiki page provides information regarding the refactoring of E2400 review_mapping_controller.rb OSS project for Spring 2024, CSC/ECE 517

Introduction

The Expertiza project makes use of peer review to allow students to learn from one another. It's a Ruby on Rails-based open-source application. It is used by faculty and students for the management of courses and assignments for specific courses. Different screens in the manage content area of the application offer information about users, courses, assignments, questionnaires, and reviews.

A review bid is a system used in Expertiza, where students indicate their preferences for reviewing certain assignments or projects. Students bid on the tasks they prefer to review through ranking their interests. The allocation of review assignments is determined by a bidding algorithm, balancing student preferences with fair and efficient distribution.

Problem Statement

1. The process of reviewing plays a crucial role in the learning and improvement cycle, benefiting both the reviewer and the reviewee. Allowing reviewers to choose their topics freely can lead to more thorough reviews and increased engagement. However, the current system of assigning review topics on a first-come-first-served basis disadvantages students who are slower to sign up, preventing them from reviewing topics they are most interested in.
2. An improved approach would be to introduce a bidding system for review topics, similar to the one used for topic selection. This system would enable students to express their preferences for topics they are keen to review. An effective bidding algorithm could ensure that all interested students receive a topic they are eager to review.
3. While the prior work met the functional requirements, it could not be integrated due to numerous violations of the DRY (Don't Repeat Yourself) principle.

Background

When topics are opened up for bidding, students can see how “hot” each topic is by the color it has on their topic list. However, instructors have no way to view the bidding process except by impersonating students. Furthermore, when the bidding assignment algorithm is run, there is no way to verify that it did in fact assign teams to topics they had chosen.

Feature Overview

Preferred Features for Reviews

The following Use Case Diagram demonstrates the primary functionality explained in the previous section for different types of users.

The desired feature for reviews should have a nearly same layout as this, but with reviews rather than themes. Students should be presented a list of topics that have been worked on, and they should be able to bid on the ones they want to review the most. The Priority system might be implemented in the same way, with the exception that a student should not be able to review their own work.

Current Topic Bidding Feature

Students may bid on topics they would like to work on. The following image shows the functionality of this feature. The students can view the page before bidding for the topics to review. The below image shows how the page looks before selecting topics.

Students are can see how “hot” each topic is by the color it has on their topic list allowed to bid on several topics, ranking them by priority.

This allocation process utilizes a modified Gale-Shapely Algorithm to ensure a stable match between students and topics, aiming to satisfy student preferences as much as possible. To resolve preference ties, topics prioritize students based on whether they are bidding as part of a group (with group members receiving higher priority) and the timing of their bid (earlier bids receive higher priority). After the bidding deadline:

1. Students are initially paired with their top-choice topic.
2. If a topic exceeds its capacity, it retains only the students with the highest priority.
3. Topics that have reached capacity are then removed from the selection pool.
4. Unmatched students repeat the process, selecting from the remaining topics.
5. This cycle continues until every student has been assigned a topic.
6. The algorithm checks for and corrects any 'unstable' matches, where swapping students between topics would more accurately fulfill their preferences.

This feature appears as an option when editing/creating a new assignment under the "Topics" area, as shown in the below.

When this option is enabled, students will be able to access a new view that lists of the topics from submitted assignments (except their own). The user can drag the topics from topics table from the left into the selection table. The user can rearrange the selected topics in the selection table based on the priority. The list of prioritized topics are used to run the bidding algorithm. If the student has not selected any topic for review then the review_bidding algorithm will assign the required number of topics automatically after the given deadline to choose the topics to review.

This process, with a time complexity of O(n^2), ensures that all students are matched with a topic they prefer, respecting their choices as closely as possible. For further information on this algorithm and its application, one can refer to documentation from the previous year when this method was first implemented.

After the deadline to choose the topics to review, the instructor runs the run bidding algorithm. The topics selected by the student is allocated based on the priority as shown in the below image.

After the deadline, if a student fails to bid the topics to review, the run bid algorithm allocated topics to the student after the instructor runs the run bidding algorithm .

Proposed Solutions

Design Principle

The codebase adheres to the Single Responsibility Principle (SRP), ensuring that each component or function has only one reason to change by fulfilling a single purpose. This minimizes the need for modifications and enhances the design's simplicity.

review_bids_others_work is a DRY violation

The previously implemented code had DRY violation found here. The below code is implemented to overcome the DRY violation. The use of helper methods (display_topic_and_participant and display_assignment_reviews_info) modularizes the code. The code avoids repetition and enhances readability. These methods can be reused wherever similar functionality is required, reducing the need to duplicate logic. The code efficiently processes each review mapping by iterating over @review_mappings with a loop, eliminating the need for repeated markup or logic for each review's display. This approach is significantly more effective than individually coding the display for each review.

review_bid_controller.rb

run_bidding_algorithm

The previously implemented code used a webserver which is no longer active.

• We reimplemented the run_bidding_algorithm to use a simplified algorithm which runs as part of Expertiza rather than calling an external service.
• When the instructor runs the Run Review Algorithm,assign_bid_review method is called from the review_bid_controller.The bidding_data information is retrieved from bidding_data method which is present in the review_bid

set_priority

• There was code for bidding for topics and in a different part of the system, code for bidding for reviews. The two code blocks were eerily similar to each other.
• The previously implemented code found here has implemented the set_priority logic in both sign_up_sheet and in review_bid_controller making these functions dependent on each other.
• We have refactored the set_priority code but not reimplemented it as the sign_up_sheet controller has the same set_priority method[1].

action_allowed needs to use Authorization Utilities

The problems listed below are examples of the four main classes of problems that were encountered in previously implemented code and the fixes made while reimplementing the same:

Problem 1: Much of the authorization logic is repeated (un-DRY). For example, multiple controllers contain the following exact code.

 ['Super-Administrator',
 'Administrator',
 'Instructor',
 'Teaching Assistant'].include? current_role_name

Solution 1: Use one of the helper methods from the new authorization_helper.rb (link) to allow TAs *and above* (instructors, admins, super-admins) to perform this work.

 current_user_has_ta_privileges?

Problem 2: Some logic is slightly incorrect. For example, some places call for a specific user type, when users "above" this type should also be allowed to perform the work. In the following example (advertise_for_partner_controller.rb), only Students may advertise for partners. However per Dr. Gehringer, "There are no cases I am aware of where a particular type of user can do something that more-privileged users cannot do".

 current_user.role.name.eql?("Student")

Solution 2: Use one of the helper methods from the new authorization_helper.rb (link) to allow Students *and above* (TAs, instructors, admins, super-admins) to perform this work.

 current_user_has_student_privileges?

However, in case there IS a need to know if the current user has one specific role, this is still supported by the helper method current_user_is_a? Problem 3: Too much authorization logic is present in the controllers. This makes the controllers more difficult to read, and scatters authorization logic, when it would be easier to understand if it were all in one place.

 def action_allowed?
   if %w[edit update list_submissions].include? params[:action]
     assignment = Assignment.find(params[:id])
     (%w[Super-Administrator Administrator].include? current_role_name) ||
     (assignment.instructor_id == current_user.try(:id)) ||
     TaMapping.exists?(ta_id: current_user.try(:id), course_id: assignment.course_id) ||
     (assignment.course_id && Course.find(assignment.course_id).instructor_id == current_user.try(:id))
   else
     ['Super-Administrator',
      'Administrator',
      'Instructor',
      'Teaching Assistant'].include? current_role_name
   end
 end

Solution 3: Establish helper methods in the new authorization_helper.rb (link) to centralize as much authorization logic as possible. In this way, a developer with questions about authorization knows just where to look to find answers - authorization_helper.rb (link).

 def action_allowed?
   if %w[edit update list_submissions].include? params[:action]
     current_user_has_admin_privileges? || current_user_teaching_staff_of_assignment?(params[:id])
   else
     current_user_has_ta_privileges?
   end
 end

Problem 4: Some action_allowed? methods are difficult to follow, and/or knowledge about how the action parameter should affect authorization is buried in another method.

 def action_allowed?
   current_user_has_student_privileges?and
   ((%w[edit].include? action_name) ? are_needed_authorizations_present?(params[:id], "reader", "reviewer") : true) and
   one_team_can_submit_work?
 end

Solution 4: Clean up action_allowed? methods and make the influence of the action parameter visible at this level.

 def action_allowed?
   case params[:action]
   when 'edit'
     current_user_has_student_privileges? &&
     are_needed_authorizations_present?(params[:id], "reader", "reviewer")
   when 'submit_file', 'submit_hyperlink'
     current_user_has_student_privileges? &&
     one_team_can_submit_work?
   else
     current_user_has_student_privileges?
   end
 end

Tests

Test Login Credentials

UserId: instructor6

Password: password

bidding_data

This method is intended to fetch and return the bidding data relevant to an assignment and the reviewer. The tests validate the robustness and correctness of the data under various scenarios as follows:

Valid Inputs

1. Test Case 1 (Multiple Reviewers)
   • To check if the application correctly handles multiple reviewers for a valid assignment id.
   • Expected output is a hash containing bidding data for assignment and reviewers.
2. Test Case 2 (Single Reviewer)
   • To check if the application can handle and return correct data for a single reviewer.
   • Expected output is a hash containing all the submissions to review for the assignment and reviewer.
3. Test Case 3 (No Reviewers)
   • To verify if the application would respond when no reviewers are provided but the assignment ID is valid.
   • Expected output is an empty hash as there are no reviewers.

Invalid Inputs

1. Test Case 4 (Both Assignment ID and Reviewer ID is Nil)
   • Tests the application's error handling when the assignment ID is nil.
   • Expected Output is an empty array.
2. Test Case 5 (Nil Reviewer IDs)
   • To evaluate how the application works when reviewer IDs are not provided.
   • Expected Ouput is an empty array.
3. Test Case 6 (When assignment_id is valid but reviewer_ids is Nil)
   • To evaluate how the application works if assignment_id is valid but reviewer_id is not valid.
   • Expected output is an empty array.
4. Test Case 7 (When reviewer_ids is valid but assignment_id is Nil):
   • To evaluate how the application works if the reviewer_ids is valid but the assignment_id is not valid.
   • Expected output is an empty array.
5. Test Case 8 (When assignment_id has valid data but reviewer_ids is an empty array):
   • To evaluate how the application works if the reviewers_id is has an empty array.
   • Expected output is an empty array

assign_topic_to_reviewer

This method assigns topics to reviewers based on the availability of signed-up teams for a given topics. The test scenarios ensure that the system accurately creates mapping records in scenarios with valid data and handles cases without valid data.

1. Test Case 1
   • This test case is to test the creation of ReviewResponseMap when there is a team signed up for the topic.
   • Expected output is that the ReviewResponseMap with assignment IDs should be created.
2. Test Case 2
   • To check if assignment IDs are handled accurately when multiple teams are signed up.
   • Expected output is the ReviewResponseMap has been linked to first signed up team and so on.

reviewer_bidding_data

This method retrieves detailed bidding information for a specific reviewer and assignment. The tests generated check if the system is capable of retrieving data and can address errors.

1. Test Case 1 (Invalid Reviewer ID):
   • This test verifies that an appropriate error is raised when an invalid reviewer ID is used.
   • Expected Output is to raise an exception error ActiveRecord::RecordNotFound

Team

Mentor

• Edward F. Gehringer

Members

• Ruchika Malhotra
• Dinesh Kannan
• Vindhya Rani Mangapuram

Links

• Demo Video Link
• Pull Request

References

• Project Instructions