CSC/ECE 517 Fall 2021 - E2151. Allow reviewers to bid on what to review

Topic Overview

Statement of Problem

Assigning reviews to users is a complicated process. Currently, reviews are still assigned using a first-come-first-served basis. However, reviews could be bid on like how topics are bid on. This would involve matching multiple students to review a submission up to the maximum reviewers for a submission. A similar feature is present in bidding for topics.

The goal of this topic is to allow both reviews and topics to be bid on by students following a bidding algorithm, and for both of these features to come from the same root functionality. This will involve refactoring the current bidding functionality of expertiza, as well as incorporating new code to allow this functionality to also be utilized for reviews.

Prior Work

Before starting, it is vital to note the substantial amount of prior work done on this task. The prior work done in Fall of 2020 (linked here [[1]]) achieved much of the desired functionality for this task, and contains a comprehensive explanation of the underlying design principles behind the algorithm implemented. However, the implementation was rejected due to DRY violations.

Thus, the primary goal of this project topic was to rebuild and refactor this prior work to make it consistent with DRY principles and able to be merged into the expertiza environment. A brief review will be given of the desired functionality and the underlying design for the bidding algorithm. For a more exhaustive explanation, please see the aforementioned link to the prior year's work.

Explanation of Feature

Existing Bidding Functionality for Topics

Currently in expertiza, students may bid on which topics they would like to work on. An image showcasing this functionality is given below:

Students may choose a number of topics to bid on, in order of priority, and once the deadline is reached, all students are given a topic.

Behind the scenes, this works by utilizing a modified version of the Gale-Shapely Algorithm. This algorithm seeks to find a 'stable' matching of each student to each topic, such that in the end, all students have a topic they wanted (at least to some degree), and that the preferences of students is respected as well as possible. To help break situations where there are ties in preferences, each topic assigns a Priority to each student. This Priority is determined by A. whether the student is part of a group bidding for this topic (groups getting higher priority), and B. the time at which the bid for the topic was made (earlier bidders are given Priority). Once this has been determined and the deadline for bidding has ended, the following steps occur:

1. Each student is matched with each topic they most prefer.
2. If the number of students assigned to a topic is above the maximum allowed for that topic, the students with the highest Priority are assigned the topic
3. All maximally filled topics are removed from the pool of available topics
4. All unmatched students begin this cycle again, now choosing the most preferred topic remaining
5. Repeat until all students are assigned a topic
6. If any matches are 'unstable' (two students would have their preferences better met if they were swapped), perform any swaps until matches are as stable as possible.

This algorithm has a time complexity of O(n^2). For more details on this algorithm and its implementation, refer to the previous year's work, where this algorithm was implemented.

Desired Functionality for Reviews

The desired feature for reviews should have an almost identical structure to this, with reviews instead of topics. Students should be shown a list of the topics that were worked on, and may bid on which of those topics they would like to review the most. The Priority system could be handled in the same way, with the only caveat being that a student should not be able to review their own work.

In terms of what this feature looks like, it can be enabled as an option when editing/creating a new assignment, under the "Topics" section, using interface drop down show below.

Once this option is enabled, during the review phase of an assignment, students will be able to access a new view listing all the topics of all the submitted assignments (except for their own). They will then be able to bid on as many topics as they like, assigning them a preference, in a manner similar to the interface for topic bidding. The table that allows for this drag-and-drop priorities is shown below.

Whenever the instructor decides they would like to close the window for review bidding, they may return to the options for the given assignment and navigate to "Topics", where they will see a button marked "Run Review Algorithm". This will make a call to the external webservice to determine which students are assigned what reviews, and will then assign those reviews to all students. Once this button is pressed, it locks out the ability for students to perform any more bidding, and cannot be pressed again. The button allowing this option is shown below.

After this point, students may navigate to the "Others Work" option under their assignment and see all of their assigned reviews. They will see up to the maximum number of allowed reviews, but will be informed of how many they are required to do (for example, a student may see 4 reviews, but will be told they are only required to complete 2).

Use Case Diagram

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

Edge Case Consideration

There are several edge cases worth consideration with the feature, some of the most prominent being:

• What if there are not enough allowed/possible reviews for all students to get the minimum number of required or possible reviews?
• What if students do not select a topic?
• What if students select a topic but do not submit any work?
• What if students give no preferences whatsoever?

From our testing, we found the implementation which we are adapting and refactoring from last year only considers the last of these edge cases, running into errors with any of the others. Because the goal of this project was primarily to adapt and refactor the currently existing functionality, we did not address these edge cases, but they should be strongly considered before being merged into expertiza (please see the Future Work section at the end for more about this).

Plan and List of Work Done

General Design Goals

• Move all prior work for this topic into a new version of the expertiza beta branch, without loss in functionality.
• Ensure all prior/expected functionality is working correctly, and if not, make any repairs or changes necessary.
• DRY out prior implementation's controller and model methods which have functionality that already exists within the expertiza system.
• Remove repetitive views and partials that exist by merging views for review bids into the set of views for sign up sheets, from which it uses partials.
• Ensure all present test cases still work properly after making the above changes.

Specific Tasks Completed

• Transferred over all code files relevant to the review bidding feature
• Re-deployed the external website which handles the review bidding algorithm
• Added that website's url to the webservices url's within expertiza
• Removed an instance variable within the review_bids_controller which would have made any practical deployment fail, removing the unnecessary functionality it was created for
• Updated method calls to assignment.find_current_stage to their new form assignment.current_stage
• Removed the review_bid model method self.reviewer_self_topic and replaced its usage with the already-existing SignedUpTeam.topic_id method
• Removed DRY-violating code used to calculate the current assignment phase which was never utilized by the controller or views
• Moved several view files from the review_bids view directory into the sign_up_sheet view directory. Partials which were duplicates in the two directories (_all_actions.html.erb and _topic_names.html.erb were no longer necessary and deleted.
• Checked all tests to ensure the prior changes did not cause any to fail

Initial Implementation

A considerable amount of time and effort was necessary to review, understand, and re-implement the previous implementation of the Review Bidding feature. While the feature was only created a year ago, several underlying functions and methods have since been refactored, and so needed to be changed in the implementation. Additionally, certain parts of how the implementation was designed did not seem to be properly implemented, and so had to be revised.

Code Transfer

First, all files that were added in the original pull request were copied and moved to the new version of the expertiza beta branch. This includes the following files:

• controllers/review_bids_controller.rb: the primary controller that handles calls to review bids functionality from views, managing them itself or directing them to the appropriate model
• helpers/review_bids_helper.rb: contains helper methods for rendering the rows of topics available to choose a review for
• models/review_bid.rb: contains the methods that each individual review bid knows and manages, including assigning a review to a student, getting info on the topic it applies to, etc.
• views/review_bids/_all_actions.html.erb: a view partial used to render the actions available to a student on the review bidding view. Nearly identical to the _all_actions file for topic bidding.
• views/review_bids/_table_header.html.erb: a view partial used to render the head of the table for review bids. Though structurally similar to that of topics reviews, contains significantly less information, as less needs to be known by a student for particular topics to review.
• views/review_bids/_table_line.html.erb: a view partial used to render individual rows of the review bids table. Like the table header, is structurally similar to that of topic bidding, but functions differently.
• views/review_bids/show.html.erb: the view which displays all the available topics to bid on reviewing. Uses the aforementioned partials to render this bidding table.
• views/review_bids/others_work.html.erb: displays the available review work which has the user has bid for.
• db/migrate/20201107203204_create_review_bids.rb: the migration which creates the review bid objects within the expertiza database.
• spec/controllers/review_bids_controller_spec.rb: the unit tests for the review bids controller
• spec/models/review_bid_spec.rb: the unit tests for the review bids model
• test/controllers/review_bids_controller_test.rb: a more elaborate feature test for the review bids functionality
• test/fixtures/review_bids.yml: fixtures that are utilized for the review_bids_controller_test
• test/models/review_bid_test.rb: A more elaborate feature test of the review bid model.

In addition to adding the previous files, the following files were also added to existing pages as modifications:

• controllers/student_review_controller.rb: the student review controller's "list" method now redirects to the review_bid_controller's "index" method if the assignment in question has a review_choosing_algorithm set to "Bidding" (the option an instructor sets when they wish to allow students to bid on reviews).
• student_task/view.html.erb: Instead of simply linking directly to the student_review controller's "list" method, the view instead checks if the assignment this view pertains to has their review_choosing_algorithm set to Bidding. If it does, and the assignment allows students to choose the topic to review (meaning the algorithm to assign bids has not yet been run), they are redirected to the 'show' method of the review_bids_controller. Otherwise, they are redirected to the 'index' method (showing them the info on their currently available topics to review).
• config/webservices.yml: added a new url for the website that is called to run the review bidding algorithm
• config/routes.rb: routes added for review_bids.
• views/assignments/edit/_topics.html.erb: Added a new option box which allows an instructor to choose to allow bidding to occur using the review bidding algorithm, rather than the default 'Simple Choose.' Once this option is selected, it also offers a button that, when clicked, runs the review bidding algorithm on all currently bid upon reviews.

Website Reimplementation

In the previous implementation, the algorithm which handles the assigning of reviews was hosted on an external website. This was done for the sake of improved performance. Thus, to get the feature in working condition, we pulled the code necessary to run the algorithm and hosted it on our own website (linked at the end of this documentation). We also made a note in the pull request that the place where this website is referenced (in the "assign_reviews_algorithm" method of the review_bids_controller) should be replaced by a proper external web tool, as is done for similar web tools in other parts of expertiza.

Modifications to Methods

Certain methods utilized in the original version of the webpages were changed or renamed in later versions, and so, needed to be modified. The primary example of this is the method that assignments use to determine what stage of assignment completion they are in. The previous year's implementation called this method as "assignment.find_current_stage(topic_id)," but this failed when trying to execute. After comparing how this page worked to how normal review pages work, we determined the new call to this method is "assignment.current_stage(topic_id)." All calls to this method were replaced in all files.

Errors Corrected

A notable period of time was also spent on determining the purpose of a particular local area that caused issues when trying to view the bids, that being a variable named "@number_of_reviews_to_show". This variable apparently kept track of how many reviews a student needed to be shown, so that they could request another. The issue was that this was created as a local variable within the review_bids_controller, and therefore would never work outside of a manual testing environment, as the variable would need to be different from every student and every assignment. After some investigation, we realized that all this variable did was limit the number of already-assigned reviews the student was able to see (as the review bidding algorithm already assigned all possible reviews). Thus, rather than try to recreate this functionality, we simply removed it.

This means that, when the review bidding algorithm runs, a student is able to see all of the reviews assigned to them, up to the maximum number of reviews for the assignment, and therefore have no reason to ask for any more. It may be the case that this feature could be improved by adding a possibility for students to request any reviews not assigned to any other student, or that still have available slots, using the "normal" review assignment scheme. For the time being, we did not consider this part of the baseline necessary functionality, and so did not implement it.

Refinement Implementation

DRY Principle and Code Duplication Refactoring

Per comments and feedbacks on the previous implementation E2085, there were 3 sections of the code that potentially were DRY violations. For each of those violations, we have had made appropriate and provided the justification on the results.

review_bids_controller.rb

For the refactoring of review bids controller.eb, the original issue is the DRY principle violation on the internal logic of finding the phase of assignments. The original implementation to be refactored and optimized are shown as follow.

    # Finding the current phase that we are in
    due_dates = AssignmentDueDate.where(parent_id: @assignment.id)
    @very_last_due_date = AssignmentDueDate.where(parent_id: @assignment.id).order("due_at DESC").limit(1)
    next_due_date = @very_last_due_date[0]
    for due_date in due_dates
      if due_date.due_at > Time.now
        next_due_date = due_date if due_date.due_at < next_due_date.due_at
      end
    end
    @review_phase = next_due_date.deadline_type_id

Through some discovery process, we have found some related logics that is already implemented from the system which enable the refactoring and reuse of the method. Some code snippets are demonstrated as follow.

  def self.current_due_date(due_dates)
    #Get the current due date from list of due dates
    due_dates.each do |due_date|
      if due_date.due_at > Time.now
        current_due_date = due_date
        return current_due_date
      end
    end
    #in case current due date not found
    return nil 
  end

  def self.teammate_review_allowed(student)
    # time when teammate review is allowed
    due_date = current_due_date(student.assignment.due_dates)
    student.assignment.find_current_stage == 'Finished' ||
    due_date &&
    (due_date.teammate_review_allowed_id == 3 ||
    due_date.teammate_review_allowed_id == 2) # late(2) or yes(3)
  end

As we can observed above, both methods of current_due_date and teammate_review_allowed provides the capability of getting the review_phase without the repetition. Additionally, we can see that those existing implementation still have some difference in implementation and purpose to be fully reusable by the logic that find current phase of a topic because the methods provided are helper methods for broader abstraction.

Furthermore, the section that we attempted to refactoring out have a similar logic that is used to get the current stage as shown below.

  def find_current_stage(topic_id = nil)
    next_due_date = DueDate.get_next_due_date(self.id, topic_id)
    return 'Finished' if next_due_date.nil?
    next_due_date
  end

However, it is worth pointing out that the method is not fully substitutable, particularly the internal functionality that finds the @very_last_due_date. A similar logic does exist that is meant to use to acquire current due date and assist with finding current stage.

  def current_stage_name(topic_id = nil)
    if self.staggered_deadline?
      return (topic_id.nil? ? 'Unknown' : current_stage(topic_id))
    end

    due_date = find_current_stage(topic_id)
    unless due_date == 'Finished' || due_date.nil? || due_date.deadline_name.nil?
      return due_date.deadline_name
    end
    current_stage(topic_id)
  end

With all the findings, we have come to conclusion that the portion of the code to be dry is in similar, but required full restructure, form of existing methods. The internal logic flow itself is not fully matching the reusable methods consists across the controllers. Because of the clarity of limited reusability of existing methods, we have come to conclusion that the portions to be refactored are not duplication of any existing methods. Additionally, after a more detailed discovery, it has come to realization that the code portion of getting the current phase of the next due date was, in fact, meant to find the last due date. After further analysis of the purpose of the code, the portion has been deleted from review_bids_controller.rb because there was no usage observed with the result of the execution.

Refactoring Views (_all_actions.html.erb and _topic_names.html.erb)

The last element of the prior implementation that caused DRY violations had to do with its implementation of views. Two primary views are necessary for the review bids functionality: one which shows a table where students can bid on which topic they wish to review (rendered by a file called 'show.html.erb'), and a list of assigned reviews which a student must select from (rendered by a file called 'others_work.html.erb').

In order to render the elements in 'show.html.erb' in particular, a number of partials must be utilized:

• a partial for the table's header (_table_header.html.erb)
• a partial for rendering each of the table's rows, describing a topic to bid on (_table_line.html.erb)
• a partial to contain information related to what actions are available for a student to do for a review, like edit or view (_all_actions.html.erb)
• a partial for finding and rendering the name of each topic one must review (_topic_names.html.erb)

Both of these views and all four of these partials were contained within the "review_bids" directory within the views directory. The DRY violation incurred by these views has to do with these partials: in particular, '_topic_names.html.erb' and '_all_actions.html.erb' are near identical code copies to those within the sign_up_sheet views, used for rendering and displaying information for topic bidding and normal reviews.

In order to DRY out this portion of the code, we decided that having a separate set of views for the results related to review bids were unnecessary. Within expertiza's structure, most of the views related to students choosing which topics or reviews to work on are all contained within the sign_up_sheet views. The review_vids views were copying the partials within the sign_up_sheet views because they were using the same fundamental components. Thus, we thought the best solution would be to move the two primary views for review_bids ('show.html.erb' and 'others_work.html.erb') into the sign_up_sheet views, and leave behind all redundant partials. In doing so, the redundancy of '_all_actions.html.erb' and '_topic_names.html.erb' were eliminated.

It could be argued that this implementation could be further DRY-ed by leaving behind the other two partials, '_table_header.html.erb' and '_table_line.html.erb', as they are structurally similar to those used for topic bidding. However, this is not actually permissible, as the header and lines for topics when bidding on them for reviews are significantly different than when bidding on them for assignments. More information needs to be available when bidding on topics for an assignment, such as the number of slots and number on the waitlist, but such an idea is not applicable for bids on reviews. While it would be possible to have a single file which produces one view or another depending upon if it's bidding for topics or reviews, we believed this violated the Expert Pattern, by attempting to create a single view for two distinct purposes. Instead, we thought it better to simply have two separate partials, each which precisely gave the desired functionality. Thus, the other two partials were brought over, being renamed '_review_bids_table_header.html.erb' and '_review_bids_table_line.html.erb'.

Tests Case Update

Throughout the refactoring and merging of E2085 to E2151, there were some updates that has been made to the test suits to maintain corrosponding testing automation.

Test Plan

In the design planning phase of the project, most of the test plan will be focusing on manual testing that is highly concise for lightweight validation.

Once the design feature is further into develop, this page will be updated with specific code regarding the RSpec test cases desired for this feature. Until then, the following are outlined below:

Bidding Controller/View Tests

All previously created tests for the bidding functionality of topics must still pass.

Review Controller/View Tests

Unit Test Cases:

• A review bidding instance can be created by an instructor
• A review bidding instance cannot be created by a student
• A student can assign their preferences to a review bidding instance
• When the deadline for bidding passes, students are given their reviews
• (Edge Case) A Student cannot be choose to, nor be assigned to, review their own project.
• (Edge Case) When a student does not bid on a review when the deadline passes, they are assigned one randomly.
• (Edge Case) In a sufficiently large class, if no students bid on a review, that project can still be reviewed.

Feature Test Cases:

• A professor can see the option to make an assignment's reviews have the "Bidding" style and select it
• A student can see the project names of available reviews
• A student can drag multiple reviews into their review preferences
• When multiple students wish to be given the same review, the number of students assigned to a review does not exceed the maximum allowed
• Under a specific combination of choices of preference, the algorithm assigns students to the reviews they should theoretically receive

Conclusions and Future Work

While the current state of the review bidding code is operational, and is certainly more DRY and stable than the previous incarnation of this work, there are still several issues with the current state of this feature that make it difficult to recommend an immediate merge into expertiza.

Enhancement of Algorithm Service Stability

Across the process of refining the refactored front end page view along with more robust edge cases testing towards the algorithm service, we have discovered an underlying issue that could impact the availability of the review bidding algorithm service. Through cause isolation, we have observed a high correlation to flask application crash with the edge case scenario input where the algorithm service is trying to match a student who was not signed up with any topic with other students.

Because the scope of our project was focused on refactoring, we did not have the time or prior warning to need to address these edge cases. However, we will provide the error log for the reference of any future teams who may work on the stability improvement of the algorithm service.

"POST /match_topics HTTP/1.1" 500 -
{'tid': [5133, 5134, 5135, None], 'users': {'45514': {'otid': 5135, 'tid': [5133, 5134], 'time': ['2021-11-30T17:57:20.000-05:00', '2021-11-30T17:57:24.000-05:00'], 'priority': [2, 1]}, '45513': {'otid': 5134, 'tid': [5133, 5135], 'time': ['2021-11-30T17:57:41.000-05:00', '2021-11-30T17:57:43.000-05:00'], 'priority': [2, 1]}, '45512': {'otid': 5133, 'tid': [5134, 5135], 'time': ['2021-11-30T17:57:58.000-05:00', '2021-11-30T17:58:00.000-05:00'], 'priority': [2, 1]}, '45515': {'otid': None, 'tid': [], 'time': [0], 'priority': [3, 1, 4, 2]}}, 'max_accepted_proposals': 3}
[2021-11-28 18:10:40,576] ERROR in app: Exception on /match_topics [POST]
Traceback (most recent call last):
  File "/home/usr/.local/lib/python3.5/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/usr/.local/lib/python3.5/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/usr/.local/lib/python3.5/site-packages/flask/app.py", line 1821, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/usr/.local/lib/python3.5/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/home/usr/.local/lib/python3.5/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/usr/.local/lib/python3.5/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/usr/IntelligentAssignment/app/app.py", line 19, in topic_matching
    return jsonify(assigned_topics_model.get_student_topic_matches())
  File "/home/usr/IntelligentAssignment/app/topics_matcher.py", line 57, in get_student_topic_matches
    student.accept_proposal()
  File "/home/usr/IntelligentAssignment/app/student.py", line 24, in accept_proposal
    self.proposals.sort(key=lambda proposal: self.choices.index(proposal))
  File "/home/usr/IntelligentAssignment/app/student.py", line 24, in <lambda>
    self.proposals.sort(key=lambda proposal: self.choices.index(proposal))
ValueError: None is not in list

As we can see from the additional information provided above, the error is due to an unhandled edge case of attempting to access missing value. Our further observation also concluded that such missing value is due to the fact that one or more to-be-matched students did not have any topic signed up at the time of algorithm execution. To address the issue, a general enhancement can be implemented on the algorithm service side with Flask to increase the crash recovery stability. Further refinement of interaction flow with user in the front end can also reduce the possibility of algorithm service error.

Github

The Github corresponding to this task is publicly available here: https://github.com/WeiRui-Wang/expertiza

Contributors

This feature was created as part of Dr. Edward Gehringer's "CSC/ECE 517: Object-Oriented Design and Development" class, Fall 2021. The contributors were: Geoff Garrido, Ayush Luthra, and WeiRui Wang. Our project mentor was Luis Delossantos (lgdeloss@ncsu.edu)