CSC/ECE 517 Fall 2024 - E2485. Allow reviewers to bid on what to review

Project Overview

This project will fix the defects and improve the functionality in Expertiza of the Reviewer Bidding feature. It will make the functionality smoother, more reliable, and align with Expertiza's collaborative learning objectives.

The Review Bidding feature lets reviewers focus on topics they know best or care about most. If someone feels qualified or interested in a topic, they can bid to review submissions in that area. This helps ensure reviews are insightful and enjoyable for the reviewer while benefiting the other participants and the overall process.

Problem Statements

Assigning reviews to users on Expertiza involves a complex bidding algorithm that requires optimization before production deployment. Below are key areas we plan to address as per the project requirements:

Code Refactoring and Best Practices
- Utilize Authentication Utilities: Modify the action_allowed function in the ReviewBidsController to use existing authentication utilities, enhancing code reusability and adhering to the Single Responsibility Principle (SRP) and Separation of Concerns (SoC).
- Eliminate Code Duplication: Refactor review_bids_others_work in the SignUpSheetController to comply with the Don't Repeat Yourself (DRY) principle.
- Rename Functions for Clarity: Change run_bidding_algorithm in ReviewBidsController to assign_reviewers to better reflect its purpose.
- Refactor Methods in review_bid.rb:: Convert class methods to instance methods or relocate them to helper classes for improved code organization.
Functionality Enhancements
- Handle Missing Bids Gracefully: Adjust the bidding algorithm to accommodate students who do not bid, allowing them to select from the remaining options.
- Support Both Individual and Team Assignments: Ensure the code functions correctly for participant and team-based assignments when signing up for topics.
User Interface Improvements
- Increase Transparency in the Bidding Process: Display messages indicating how many students are eligible to bid, how many have submitted bids, and the deadline.
- Accurate Bid Status Indicators: Ensure topic colors change correctly based on the number of outstanding bids, providing clear visual feedback to users.
Testing and Validation
- Exhaustive Edge Case Testing: Conduct thorough testing for edge cases not previously covered, adding additional test cases as needed.

Design Goals

While addressing the existing defects and enhancing the Reviewer Bidding feature, we plan to adhere to the following design guidelines:

Code Quality and Maintainability
- Improve Code Structure: Implement refactoring strategies to enhance code readability and maintainability, ensuring adherence to best practices like the DRY principle.
- Standardize Authentication Logic: Utilize a centralized authentication utility to promote consistency and reusability across the application.
System Stability and Flexibility
- Enhance Algorithm Robustness: Modify the bidding algorithm to handle scenarios where students do not bid, ensuring the system remains stable under all conditions.
- Ensure Assignment Versatility: Design the system to seamlessly support both individual and team assignments without additional configuration.
User Experience Enhancements
- Increase Bidding Transparency: Develop user interface elements that clearly display bidding status, deadlines, and participation metrics.
- Implement Dynamic Visual Feedback: Use real-time visual cues, such as color changes, to inform users about the bidding status instantly.
Testing and Validation
- Develop Comprehensive Test Suites: Create extensive automated tests covering edge cases and critical functionalities to guarantee reliable system behavior.

UML Diagram

Below is a visual representation of the models associated with the review bids functionality, depicting the data structures and their relationships.

ReviewBid: The ReviewBid manages the bidding process, where participants indicate their preferences for reviewing different topics or assignments. This helps distribute review tasks fairly based on participants' interests.

Assignment: The Assignment model represents team or individual assignments in Expertiza. It stores all the essential information required to keep the assignment process organized, like managing participants, setting due dates, forming teams, and overseeing the review process.

SignUpTopic: The SignUpTopic model identifies specific topics or areas within an assignment for which participants can sign up. It manages the availability of these topics, tracks bids, coordinates team sign-ups, and assigns topics based on both bids and participant availability.

Participant: The Participant model is about the individual users who participate in an assignment. This includes students, instructors, teaching assistants, or other relevant roles. This model is responsible for managing key user information, monitoring interactions with the assignment, keeping track of team memberships, and recording things like badges and review grades.

Implementation

Decoupling Authorization Logic from `ReviewBidsController`

Problem	Design Goals
`action_allowed` needs to use the authentication utilities The function `action_allowed` should use the available authentication utilities to support the Single Responsibility Principle and Separation of Concerns and to support code reusability.	Authentication Consistency: Remove authentication logic in the controller and call logic in the authentication utility class instead to ensure consistent use of the same logic across the application, foster reusability, and eliminate DRY.
	Code Readability and Maintainability: Refactor and simplify code, especially where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.

Authorization Flow Before Refactoring

The sequence diagram depicts the authorization process within the ReviewBidsController. When a user invokes an action, the controller checks if the action is allowed by verifying the user's role through the current_role_name() method in the ApplicationController. Depending on the action and the user's role, the controller either authorizes the user to proceed or denies access, ultimately executing the action logic if authorized and responding to the user.

`ReviewBidsController` Updates

We simplified the action_allowed? method by decoupling authorization logic from the controller to use helper methods in the AuthorizationHelper module. The ReviewBidsController should not handle authentication, which is a violation of SRP.

Note: We also made similar updates to the action_allowed methods for SignUpController and StudentReviewController.

Original Code

Refactored Code

def action_allowed?
  case params[:action]
  when 'show', 'set_priority', 'index'
    ['Instructor',
     'Teaching Assistant',
     'Administrator',
     'Super-Administrator',
     'Student'].include? current_role_name and
        ((%w[list].include? action_name) ? are_needed_authorizations_present?(params[:id], "participant", "reader", "submitter", "reviewer") : true)
  else
    ['Instructor',
     'Teaching Assistant',
     'Administrator',
     'Super-Administrator'].include? current_role_name
  end
end

def action_allowed?
  case params[:action]
  when 'show', 'set_priority', 'index', 'list'
    current_user_has_student_privileges? && list_authorization_check
  else
    current_user_has_ta_privileges?
  end
end

Refactor `ReviewBidsController#index` action

Previously, the ReviewBidsController's index action was overloaded with responsibilities—it handled everything from locating and validating assignments to fetching review mappings and counting completed reviews. This all-in-one approach made the controller action messy.

We have moved the business logic from the controller action and into dedicated service objects. With this change, we introduced two key service objects: ParticipantService and ReviewService. The ParticipantService focuses on retrieving the participant along with their assignments, while the ReviewService takes on the task of managing review mappings and calculating review-related stats, like how many reviews have been completed.

This separation of responsibilities aligns with the Single Responsibility Principle. The logic for fetching assignments, retrieving review mappings, and counting reviews is now more flexible and reusable throughout the application. For example, we used methods in the ReviewService in both the ReviewBidsController and the StudentReviewController, helping to avoid code duplication.The article Rails Service Objects Tutorial served as a guide on when and how to use service objects in Rails.

Old Code

New Code

def index
  @participant = AssignmentParticipant.find(params[:id])
  return unless current_user_id?(@participant.user_id)
  @assignment = @participant.assignment
  @review_mappings = ReviewResponseMap.where(reviewer_id: @participant.id)

  # Finding how many reviews have been completed
  @num_reviews_completed = 0
  @review_mappings.each do |map|
    @num_reviews_completed += 1 if !map.response.empty? && map.response.last.is_submitted
  end

  # render view for completing reviews after review bidding has been completed
  render 'sign_up_sheet/review_bids_others_work'
end

def index
  @assignment = participant_service.assignment
  unless @assignment.is_a?(Assignment)
    flash[:error] = 'Assignment not found.'
    redirect_back fallback_location: root_path && return
  end

  @review_mappings = review_service.review_mappings
  @num_reviews_completed = review_service.review_counts[:completed]

  render 'sign_up_sheet/review_bids_others_work'
end

Optimize `ReviewBidsController#set_priority` Action functionality

In the earlier version of the set_priority action in the ReviewBidsController, the server only needed a couple of key pieces of information: an array of topic IDs (params[:topic]) and the participant ID (params[:id}). From there, it would figure out the assignment_id by using the first topic as a reference. This is risky because the assignment_id associated with the topic may not be a valid assignment_id.

With the new implementation, we structured the parameters to include the participant_id, an array of topic IDs, and assignment_id. This change ensures that the server always receives complete and reliable information, preventing issues caused by missing or inconsistent data. It also removes the need for the server to perform unnecessary lookups, as in the case of assignment_id, allowing the controller to focus on receiving requests and rendering appropriate responses.

While these updates have cleaned up the set_priority action, they still contain a lot of business logic, like validations and priority updates. To make the code even more maintainable, there’s room for further refactoring—perhaps moving some of this logic into a service class or model method to streamline the controller even more.

Old Code

New Code

  def set_priority
    if params[:topic].nil?
      ReviewBid.where(participant_id: params[:id]).destroy_all
    else
      participant = AssignmentParticipant.find_by(id: params[:id])
      assignment_id = SignUpTopic.find(params[:topic].first).assignment.id
      @bids = ReviewBid.where(participant_id: params[:id])
      signed_up_topics = ReviewBid.where(participant_id: params[:id]).map(&:signuptopic_id)
      signed_up_topics -= params[:topic].map(&:to_i)
      signed_up_topics.each do |topic|
        ReviewBid.where(signuptopic_id: topic, participant_id: params[:id]).destroy_all
      end
      params[:topic].each_with_index do |topic_id, index|
        bid_existence = ReviewBid.where(signuptopic_id: topic_id, participant_id: params[:id])
        if bid_existence.empty?
          ReviewBid.create(priority: index + 1, signuptopic_id: topic_id, participant_id: params[:id], assignment_id: assignment_id)
        else
          ReviewBid.where(signuptopic_id: topic_id, participant_id: params[:id]).update_all(priority: index + 1)
        end
      end
    end
    redirect_to action: 'show', assignment_id: params[:assignment_id], id: params[:id]
  end

  def set_priority
    participant_id = params[:participant_id].to_i
    selected_topic_ids = params[:topic]&.map(&:to_i) || []
    assignment_id = params[:assignment_id].to_i

    unless current_user_can_modify_participant?(participant_id)
      flash[:error] = 'You are not authorized to modify these bids.'
      respond_to do |format|
        format.html { redirect_back fallback_location: root_path }
        format.json { render json: { status: 'unauthorized' }, status: :unauthorized }
      end
      return
    end

    if selected_topic_ids.empty?
      ReviewBid.where(participant_id: participant_id).destroy_all
    else
      assignment = Assignment.find_by(id: assignment_id)
      if assignment.nil?
        flash[:error] = "Invalid assignment."
        respond_to do |format|
          format.html { redirect_back fallback_location: root_path }
          format.json { render json: { status: 'invalid_assignment' }, status: :unprocessable_entity }
        end
        return
      end

      unless SignUpTopic.where(id: selected_topic_ids, assignment_id: assignment_id).count == selected_topic_ids.size
        flash[:error] = "One or more selected topics are invalid."
        respond_to do |format|
          format.html { redirect_back fallback_location: root_path }
          format.json { render json: { status: 'invalid_topics' }, status: :unprocessable_entity }
        end
        return
      end

      ReviewBid.where(participant_id: participant_id).where.not(signuptopic_id: selected_topic_ids).destroy_all

      selected_topic_ids.each_with_index do |topic_id, index|
        bid = ReviewBid.find_or_initialize_by(signuptopic_id: topic_id, participant_id: participant_id)
        bid.priority = index + 1
        bid.assignment_id = assignment_id
        bid.save!
      end
    end

    respond_to do |format|
      format.html { redirect_to action: 'show', assignment_id: assignment_id, id: participant_id, notice: 'Review bids updated successfully.' }
      format.json { render json: { status: 'success' }, status: :ok }
    end
  rescue ActiveRecord::RecordInvalid => e
    flash[:error] = "Failed to update priorities: #{e.message}"
    respond_to do |format|
      format.html { redirect_back fallback_location: root_path }
      format.json { render json: { status: 'error', message: e.message }, status: :unprocessable_entity }
    end
  end

Client Side Changes to Support `ReviewBidsController#set_priority` Changes

We refactored the client-side code to ensure it passed the correct information from the view to the set_priority action. On the client side, we now grab the topicIDs and send them as selected_topic_ids, allowing the controller to figure out which topics were selected and in what order.

When working with jQuery, AJAX is awesome because it lets us update the server without reloading the page, making the user experience smooth and responsive. When we pull the participant_id from the DOM, we can grab it along with other data, like the list of topics, and include it as a single request. This way, we don’t have to rely on the URL to know which participant we’re working with, which makes the code cleaner.

We also added an update handler so that any time the order of the selected topic changes in the view, information is sent to the appropriate controller action. This ensures that the controller and the view remain in sync, with real-time updates, and includes a mechanism for providing feedback if something goes wrong.

Note: One interesting aspect of the application was the mix of JavaScript and CoffeeScript code. It wasn't clear why both types of code were used.

Old Code

New Code

jQuery ->
  $("#topics").sortable
    cursor: 'move',
    opacity: 0.65,
    tolerance: 'pointer'
    connectWith: ".connectedSortable"
    items: ">*:not(.sort-disabled)"

  $("#selections").sortable
    cursor: 'move',
    opacity: 0.65,
    tolerance: 'pointer'
    connectWith: ".connectedSortable"
    items: ">*:not(.sort-disabled)"
    update: ->
      $.post($(this).data('update-url'), $(this).sortable('serialize'))

jQuery(document).ready ($) ->
  sendUpdatedOrder = ->
    sortedIDs = $(this).sortable('toArray')

    topicIDs = sortedIDs.map (id) ->
      id.replace 'topic_', ''

    participant_id = $(this).data('participant-id')

    assignment_id = new URL($(this).data('update-url'), window.location.origin).searchParams.get('assignment_id')

    $.ajax
      type: 'POST'
      url: $(this).data('update-url')
      data:
        topic: topicIDs
        participant_id: participant_id
        assignment_id: assignment_id
      success: (response) ->
        console.log 'Selections updated successfully.'
      error: (xhr, status, error) ->
        alert 'An error occurred while saving your selections. Please try again.'
        console.error error

  $("#topics").sortable
    cursor: 'move'
    opacity: 0.65
    tolerance: 'pointer'
    connectWith: ".connectedSortable"
    items: ">*:not(.sort-disabled)"

  $("#selections").sortable
    cursor: 'move'
    opacity: 0.65
    tolerance: 'pointer'
    connectWith: ".connectedSortable"
    items: ">*:not(.sort-disabled)"
    update: sendUpdatedOrder

`SignUpSheetController` Updates

The ReviewBidsController#index method renders the review_bids_others_work action However, the view is associated with the SignUpSheetController.

class ReviewBidsController < ApplicationController
  require 'json'
  require 'uri'
  require 'net/http'
  require 'rest_client'

  # provides variables for reviewing page located at views/review_bids/others_work.html.erb
  def index
    @participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(@participant.user_id)

    @assignment = @participant.assignment
    @review_mappings = ReviewResponseMap.where(reviewer_id: @participant.id)

    # Finding how many reviews have been completed
    @num_reviews_completed = 0
    @review_mappings.each do |map|
      @num_reviews_completed += 1 if !map.response.empty? && map.response.last.is_submitted
    end

    # render view for completing reviews after review bidding has been completed
    render 'sign_up_sheet/review_bids_others_work'
  end  
end

`review_bids_others_work.html.erb` View Updates

The view is technically part of the SignUpSheetController, but it's being rendered by the ReviewBidsController, which initially threw us off. After chatting with the professor, we decided not to change this view. He indicated that the plan is to reuse these views for both assignment and review bidding.

We suggest building the assignment bidding and review bidding features separately first to ensure each one works independently. Once that’s done, we can clean things up by combining any shared functionality. This could include creating shared views or even introducing an abstract model to handle both types of bidding more efficiently. It could also mean replacing the ReviewBidsController with something more generic, like a BidsController, to keep things reusable and easier to maintain down the line.

Rename `run_bidding_algorithm` method

Problem	Design Goals
`run_bidding_algorithm` should be `assign_reviewers`	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
`run_bidding_algorithm` should be `assign_reviewers`	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

The primary activity here is to refactor the method name run_bidding_algorithm to assign_reviewers and fix the hardcoded URL. However, the match topic bidding algorithm no longer works since Heroku is no longer free, per the professor. The reference link below has detailed information about the web service that we can leverage to gain that understanding.

Reference Link for Webservice Details: match_topics webservice link details

Updated Implementation

The original requirement was to rename the run_bidding_algorithm method to assign_reviewers, but after digging into the code, we realized that renaming it didn’t make sense. The method runs the bidding algorithm to match topics, and there’s already a separate method to handle assigning review topics.

However, we improved the logic in the assign_bidding method (which calls the private run_bidding_algorithm) by introducing a separate BiddingAlgorithmService object. This keeps the business logic out of the controller action, which makes things cleaner and more maintainable.

review_bids_controller

  def run_bidding_algorithm
    review_bid = ReviewBid.new
    bidding_data = review_bid.bidding_data
    matched_topics = BiddingAlgorithmService.new(bidding_data).run
    raise ArgumentError, 'Failed to assign reviewers. Please try again later.' unless matched_topics

    matched_topics
  end

bidding_algorithm_service

# frozen_string_literal: true

# The `BiddingAlgorithmService` run the bid assignment algorithm
# Sends student IDs, topic IDs, student preferences, and timestamps to the web service
# The web service returns the matched assignments in the JSON response body
class BiddingAlgorithmService
  SERVICE_URL = 'http://app-csc517.herokuapp.com/match_topics'.freeze

  # MOCK_DATA is based on the documentation detailing how the webservice behaves.
  # Reference: https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2020_-_E2085._Allow_reviewers_to_bid_on_what_to_review#Webservice
  MOCK_DATA = {
    36239 => [3970, 3972, 3975],
    36240 => [3973, 3974, 3972],
    36241 => [3969, 3971, 3972],
    36242 => [3969, 3971, 3973],
    36243 => [3969, 3970, 3971]
  }.freeze

  def initialize(bidding_data)
    @bidding_data = bidding_data
  end

  def run
    return MOCK_DATA if Rails.application.config.use_mock_bidding_algorithm

    perform_request
  rescue RestClient::ExceptionWithResponse, JSON::ParserError
    false
  end

  private

  def perform_request
    response = RestClient.post(
      SERVICE_URL,
      @bidding_data.to_json,
      content_type: :json,
      accept: :json
    )

    validate_response(response)
    JSON.parse(response.body)
  end

  def validate_response(response)
    raise 'Invalid response format' unless response.headers[:content_type] && response.headers[:content_type].include?('application/json')
  end
end

Resolve Functionality Issues for Non Bidders

Problem	Design Goals
Currently it doesn't work if some student does not bid. In this case, algorithm needs to be fixed to ignore anyone who didn’t bid, and let them choose from what’s left over.	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

`ReviewBidsController` Updates

We plan to do several things within this controller. We will set a default state for non-bidders that we can point to to give them options to select topics to review from the leftovers. We will consolidate @sign_up_topics to resolve the DRY violation. Lastly, we will remove business logic from the show method and push it to a new helper to conform to the SRP.

Current Implementation

class ReviewBidsController < ApplicationController
  require 'json'
  require 'uri'
  require 'net/http'
  require 'rest_client'

  # Additional method

  # Provides variables for review bidding page
  def show
    @participant = AssignmentParticipant.find(params[:id].to_i)
    @assignment = @participant.assignment
    @sign_up_topics = SignUpTopic.where(assignment_id: @assignment.id, private_to: nil)
    my_topic = SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id)
    @sign_up_topics -= SignUpTopic.where(assignment_id: @assignment.id, id: my_topic)
    @num_participants = AssignmentParticipant.where(parent_id: @assignment.id).count
    @selected_topics = nil # This is used to list the topics assigned to review (i.e., select == assigned)
    @bids = ReviewBid.where(participant_id: @participant, assignment_id: @assignment.id)
    signed_up_topics = []
    
    @bids.each do |bid|
      sign_up_topic = SignUpTopic.find_by(id: bid.signuptopic_id)
      signed_up_topics << sign_up_topic if sign_up_topic
    end

    signed_up_topics &= @sign_up_topics
    @sign_up_topics -= signed_up_topics
    @bids = signed_up_topics
    @num_of_topics = @sign_up_topics.size
    @assigned_review_maps = []

    ReviewResponseMap.where(reviewed_object_id: @assignment.id, reviewer_id: @participant.id).each do |review_map|
      @assigned_review_maps << review_map
    end

    # Explicitly render view since it's in the sign up sheet views
    render 'sign_up_sheet/review_bids_show'
  end

  # Additional methods
end

Updated Implementation

 # Assigns bidding topics to reviewers
  def assign_bidding
      assignment = validate_assignment(params[:assignment_id])

      reviewers = validate_reviewers(assignment.id)
      reviewer_ids = reviewers.map(&:id)

      matched_topics = run_bidding_algorithm

      if matched_topics.blank?
        flash[:alert] = 'Topic or assignment is missing'
        redirect_back fallback_location: root_path && return
      end

      leftover_topics = find_leftover_topics(assignment.id, matched_topics)
      assign_leftover_topics(reviewer_ids, matched_topics, leftover_topics)

      ReviewBid.new.assign_review_topics(matched_topics)
      assignment.update!(can_choose_topic_to_review: false)

      flash[:notice] = 'Reviewers were successfully assigned to topics.'
      redirect_back fallback_location: root_path
    rescue ArgumentError => e
      Rails.logger.error "ArgumentError: #{e.message}"
      redirect_back fallback_location: root_path, alert: e.message
    rescue ActiveRecord::RecordInvalid => e
      Rails.logger.error "ActiveRecord::RecordInvalid: #{e.message}"
      redirect_back fallback_location: root_path, alert: 'Failed to assign reviewers due to database error. Please try again later.'
    rescue ActiveRecord::ActiveRecordError => e
      Rails.logger.error "ActiveRecord::ActiveRecordError: #{e.message}"
      redirect_back fallback_location: root_path, alert: 'Failed to assign reviewers due to database error. Please try again later.'
    rescue StandardError => e
      Rails.logger.error "StandardError: #{e.message}"
      redirect_back fallback_location: root_path, alert: 'Failed to assign reviewers. Please try again later.'
    end

  private

  #service methods here ...

  def find_leftover_topics(assignment_id, matched_topics)
    all_topic_ids = SignUpTopic.where(assignment_id: assignment_id).pluck(:id)
    assigned_topic_ids = matched_topics.map { |match| match[:topic_id] }

    # Calculate leftover topics
    all_topic_ids - assigned_topic_ids
  end

  def assign_leftover_topics(reviewer_ids, matched_topics, leftover_topics)
    return if leftover_topics.blank?

    # Find non-bidders by excluding already matched reviewers
    non_bidders = reviewer_ids - matched_topics.keys

    # Assign leftover topics to non-bidders in a round-robin fashion
    non_bidders.each_with_index do |reviewer_id, index|
      topic_id = leftover_topics[index % leftover_topics.length]
      ReviewBid.create(priority: 1, signuptopic_id: topic_id, participant_id: reviewer_id)
    end
  end

Confirm Functionality for Single-person Teams

Problem	Design Goals
Make sure your code works on individual assignments, i.e., assignments where participants sign up for topics instead of teams. In this case, a team is created for each participant when they sign up. So the code should work for assignments to which either individuals or teams submit.	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

`ReviewBidsController` Updates

We plan to make an update to focus on topic rather than team or have it where teams are updated based on topic id.We will probably utilize sign up topic or a similar object to accomplish this.

Bid Status Messaging

Problem	Design Goals
To make the functionality more intuitive, include a message to say how many students are eligible to submit bids, how many have submitted their bids, when the deadline for submitting bids is.	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

`ReviewBidsHelper` Updates

The logic for these messages will go into the helper file. It is effectively the same reasoning as our migration of other business/functionality code from the controller to the helper to conform to the SRP. It will go hand in hand with the SignUpTopic portion of the code.

Refactor `ReviewBid` Model

Problem	Design Goals
Why are the methods in review_bid.rb class methods? Can we change them to instance methods or move it to helpers?	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

`ReviewBid` Model Updates

Since the current class methods are tied to ReviewBid model instances, we will change them to instance methods. There is no need to move them to helpers.

Review Bid Model Flow Before Refactoring

This sequence diagram describes how the ReviewBidsController interacts with the ReviewBid model in assigning review topics to reviewers according to their bids. Once the assign_bidding action is initiated by the user, the controller gathers the IDs of reviewers and calls ReviewBid.bidding_data to gather bidding information. That is sent out to an external bidding algorithm, and the matched topics that come back are used by ReviewBid.assign_review_topics to create ReviewResponseMap entries, assigning topics to reviewers for the assignment.

Current Implementation

class ReviewBid < ApplicationRecord
  belongs_to :topic, class_name: 'SignUpTopic'
  belongs_to :participant, class_name: 'Participant'
  belongs_to :assignment, class_name: 'Assignment'

  # method to get bidding data
  # returns the bidding data needed for the assigning algorithm
  # student_ids, topic_ids, student_preferences, topic_preferences, max reviews allowed

  def self.bidding_data(assignment_id, reviewer_ids)
    # create basic hash and set basic hash data
    bidding_data = { 'tid' => [], 'users' => {}, 'max_accepted_proposals' => [] }
    bidding_data['tid'] = SignUpTopic.where(assignment_id: assignment_id).ids
    bidding_data['max_accepted_proposals'] = Assignment.where(id: assignment_id).pluck(:num_reviews_allowed).first

    # loop through reviewer_ids to get reviewer specific bidding data
    reviewer_ids.each do |reviewer_id|
      bidding_data['users'][reviewer_id] = reviewer_bidding_data(reviewer_id, assignment_id)
    end
    bidding_data
  end

  # assigns topics to reviews as matched by the webservice algorithm
  def self.assign_review_topics(assignment_id, reviewer_ids, matched_topics, _min_num_reviews = 2)
    # if review response map already created, delete it
    if ReviewResponseMap.where(reviewed_object_id: assignment_id)
      ReviewResponseMap.where(reviewed_object_id: assignment_id).destroy_all
    end
    # loop through reviewer_ids to assign reviews to each reviewer
    reviewer_ids.each do |reviewer_id|
      topics_to_assign = matched_topics[reviewer_id.to_s]
      topics_to_assign.each do |topic|
        assign_topic_to_reviewer(assignment_id, reviewer_id, topic)
      end
    end
  end

  # method to assign a single topic to a reviewer
  def self.assign_topic_to_reviewer(assignment_id, reviewer_id, topic)
    team_to_review = SignedUpTeam.where(topic_id: topic).pluck(:team_id).first
    team_to_review.nil? ? [] : ReviewResponseMap.create(reviewed_object_id: assignment_id, reviewer_id: reviewer_id, reviewee_id: team_to_review, type: 'ReviewResponseMap')
  end

  # method for getting individual reviewer_ids bidding data
  # returns user's bidding data hash
  def self.reviewer_bidding_data(reviewer_id, assignment_id)
    reviewer_user_id = AssignmentParticipant.find(reviewer_id).user_id
    self_topic = SignedUpTeam.topic_id(assignment_id, reviewer_user_id)
    bidding_data = { 'tid' => [], 'otid' => self_topic, 'priority' => [], 'time' => [] }
    bids = ReviewBid.where(participant_id: reviewer_id)

    # loop through each bid for a topic to get specific data
    bids.each do |bid|
      bidding_data['tid'] << bid.signuptopic_id
      bidding_data['priority'] << bid.priority
      bidding_data['time'] << bid.updated_at
    end
    bidding_data
  end
end

Updated Implementation

class ReviewBid < ApplicationRecord
  belongs_to :topic, class_name: 'SignUpTopic'
  belongs_to :participant, class_name: 'Participant'

  # method to get bidding data
  # returns the bidding data needed for the assigning algorithm
  # student_ids, topic_ids, student_preferences, topic_preferences, max reviews allowed

  # Instance method to get bidding data for this review bid
  def bidding_data
    {
      'tid' => topic_ids,
      'otid' => self_topic,
      'priority' => priorities,
      'time' => updated_times
    }
  end

  # bid object to limit db touches for similar data acquisitions
  def bids
    @bids ||= ReviewBid.where(participant_id: participant_id)
  end

  def topic_ids
    bids.pluck(:signuptopic_id)
  end

  def priorities
    bids.pluck(:priority)
  end

  def updated_times
    bids.pluck(:updated_at)
  end

  def self_topic
    return nil unless topic && topic.assignment_id

    SignedUpTeam.topic_id(topic.assignment_id, participant.user_id)
  end

  # Assign topics to reviews
  def assign_review_topics(matched_topics)
    validate_topics_and_assignment!(matched_topics)

    ReviewResponseMap.where(reviewed_object_id: topic.assignment_id).destroy_all

    matched_topics.each do |reviewer_id, topics|
      Array(topics).each { |topic_id| assign_topic_to_reviewer(reviewer_id, topic_id) }
    end
  end

  # Assign a single topic to a reviewer
  def assign_topic_to_reviewer(reviewer_id, topic_id)
    team_to_review = SignedUpTeam.where(topic_id: topic_id).pluck(:team_id).first
    return if team_to_review.nil?

    ReviewResponseMap.create(
      reviewed_object_id: topic.assignment_id,
      reviewer_id: reviewer_id,
      reviewee_id: team_to_review,
      type: 'ReviewResponseMap'
    )
    # error handling
  rescue StandardError => e
    Rails.logger.error("Failed to assign topic #{topic_id} to reviewer #{reviewer_id}: #{e.message}")
  end

  private

  def validate_topics_and_assignment!(matched_topics)
    raise ArgumentError, 'Topic or assignment is missing' if topic.nil? || topic.assignment_id.nil?
    raise ArgumentError, 'Matched topics must be a Hash' unless matched_topics.is_a?(Hash)
  end
end

`ReviewBidsController` Updates

Once we changed them to instance methods, we modified their use in the controller code to instantiate the object first.

`Schema.db` Updates

We also need to remove the foreign key reference to Assignment in the ReviewBid model since the Participant model already has access to Assignment, and that is the Assignment we should be referencing. This will also involve modifying the database to remove the foreign key reference to the Assignment.

Steps required:

Review the records in the ReviewBid table in the database to see if there are any records and if those records have values for assignment_id
Create a migration file using the command: rails generate migration RemoveAssignmentFromReviewBids
In the generated file, indicate the change that needs to be made using the change method and migration columns remove_foreign_key and remove_column. Migration should be straightforward since there is no cascading behavior (i.e., dependent:destroy) to worry about.
Once comfortable with the updates to the generated file, run the migration using the rails db:migrate command.
Run a query in the DB to ensure no orphaned records are in the ReviewBid table after the updates.

Implement Edge Case Testing

Problem	Design Goals
In the previous implementation wiki, there are edge cases which are not exhaustively tested. Should test those edge cases thoroughly and add more edge case testing - link	Code Readability and Maintainability: Refactor and simplify code, especially in places where the code violates the DRY principle, to reduce complexity and make the code base more straightforward to understand and maintain.
	System Stability: Debug the existing bidding algorithm and fine-tune the assignment logic. Also, the existing functionality related to the reviewer bidding feature should work and be enhanced if required so that it does not fail in circumstances such as when a student does not bid.

`ReviewBidsHelper` Updates

TODO:We just got initial test results back. We still need to figure out why tests are failing and what may be slipping through the cracks.

Design Patterns and Principles

Below are the design patterns and principles we plan to use to address the problem statements outlined in the project requirements:

Single Responsibility Principle (SRP)
- Controller Responsibility
  - To address the issue of authentication logic residing in ReviewBidsController, we will refactor the code to ensure the controller solely handles request and response flows.
  - Authentication logic will be moved to dedicated methods in AuthorizationHelper, making the codebase easier to maintain and test.
  - We will move this logic into helper methods or models, ensuring each component has a single responsibility and improving code readability.

Separation of Concerns (SoC)
- By relocating authorization logic out of the controller, we separate authentication from request handling.
- Adhering to SoC enhances readability and maintainability by allowing each part of the application to focus on its specific role.

Don't Repeat Yourself (DRY)
- We will inspect the existing implementation for code duplication, particularly in the controller actions.
- Any repetitive code will be refactored or removed to reduce redundancy and improve clarity.

Dependency Inversion Principle (DIP)
- We plan to update class methods to instance methods, allowing the controller to depend on abstractions rather than concrete implementations.

- Clear and intentional naming during refactoring will enhance code clarity and maintainability.

Test Plan

The Excel spreadsheet below details our test plan for testing the code we changed or created to ensure code coverage. The process involved reviewing the code and determining all the possible scenarios that needed to be covered based on what the method was doing, then creating test cases accordingly.

Team

Mentor

Ed Gehringer

Members

Gavin Teague
Janice Uwujaren

Links

Pull Request: Expertiza Pull Request #2901

Demo Video: E2485 Review Bidding Feature Demo Video

Previous Wiki: Link

Reference Link for Webservice Details: match_topics webservice link details

Guidance on Using Service Objects in Rails: Rails Service Objects Tutorial

References

Project Instructions

CSC/ECE 517 Fall 2024 - E2485. Allow reviewers to bid on what to review

Project Overview

Problem Statements

Design Goals

UML Diagram

Implementation

Decoupling Authorization Logic from ReviewBidsController

Authorization Flow Before Refactoring

ReviewBidsController Updates

Refactor ReviewBidsController#index action

Optimize ReviewBidsController#set_priority Action functionality

Client Side Changes to Support ReviewBidsController#set_priority Changes

SignUpSheetController Updates

review_bids_others_work.html.erb View Updates

Rename run_bidding_algorithm method

Updated Implementation

review_bids_controller

bidding_algorithm_service

Resolve Functionality Issues for Non Bidders

ReviewBidsController Updates

Current Implementation

Updated Implementation

Confirm Functionality for Single-person Teams

ReviewBidsController Updates

Bid Status Messaging

ReviewBidsHelper Updates

Refactor ReviewBid Model

ReviewBid Model Updates

Review Bid Model Flow Before Refactoring

Current Implementation

Updated Implementation

ReviewBidsController Updates

Schema.db Updates

Implement Edge Case Testing

ReviewBidsHelper Updates

Design Patterns and Principles

Test Plan

Team

Mentor

Members

Links

References

Navigation menu

Search

Decoupling Authorization Logic from `ReviewBidsController`

`ReviewBidsController` Updates

Refactor `ReviewBidsController#index` action

Optimize `ReviewBidsController#set_priority` Action functionality

Client Side Changes to Support `ReviewBidsController#set_priority` Changes

`SignUpSheetController` Updates

`review_bids_others_work.html.erb` View Updates

Rename `run_bidding_algorithm` method

`ReviewBidsController` Updates

`ReviewBidsController` Updates

`ReviewBidsHelper` Updates

Refactor `ReviewBid` Model

`ReviewBid` Model Updates

`ReviewBidsController` Updates

`Schema.db` Updates

`ReviewBidsHelper` Updates