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

Topic Overview

Statement of Problem

Reviewing is an important stage of learning and improving, both as a reviewer and a reviewee, and so allowing reviewers more freedom and choice in what they review would result in better reviews and more engaged reviewers. However, currently the only way to choose a topic to review is a first-come-first-served basis, penalizing students who do not sign up quickly by not allowing them to review the topics they would be most interesting in engaging with.

A better alternative would be to implement a bidding system, similar to that used for topics, which would allow students to establish preferences for which topics they would most like to review. A good bidding algorithm implementing with this idea would ensure that all students (who care enough to bid) receive a topic to review that they actively want.

A prior project group created a prototype for this function, allowing students to bid on what topics they wanted to review. While the functionality was as desired, the feature was not able to be merged due to having too many DRY violations. Thus, the goal of this topic was to bring this prior implementation into the current version of the expertiza beta branch, and remove these DRY violations, so that it may be merged.

As development continued, it became clear this prototype also had several bugs and issues that made it unable to work under several conditions. Thus, a secondary goal was to fix any issues which existed such that the feature would generally work with the functionality the prior implementation demonstrated.

Prior Work

The prior work which is the basis for this topic is linked here is linked here. This work was done in Fall of 2020, and as previously mentioned, achieved much of the desired functionality for this task. This prior work will be referenced several times within this documentation, as it has a more comprehensive explanation of the underlying design principles behind the views, controller, model, algorithm, and RSpec tests implemented. These details will only be briefly summarized here.

As previously noted, this prior implementation was rejected due to DRY violations, and so correcting these violations--and explaining how those corrections were done--will be the major focus of this project documentation.

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.

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
• Enhanced the stability of the webservice for the bidding algorithm by testing with edge cases and optimize the endpoint by restoring the original access endpoint reference
• Analyzed the necessity of the phase finding related logic code section
• Optimized phase finding related logic by removing unused code segment
• 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
• Refactored the naming of the variable that represents the id of the reviewer object from 'reviewer' to 'reviewer_id' for clarity
• Removed the unnecessary 'create' function within the review_bids_controller
• Removed the sparse testing files under the 'test' directory in keeping with typical expertiza structure
• Removed a test case from the unit tests which relied upon the removed 'create' method of the review_bids_controller

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.

Another change to methods was the removal of a method from the review_bids_controller named 'create'. This method primarily served as a redirect for an aspect of the feature that was unnecessary and so removed (see the following section), that being the button to show another review. With this feature removed, this method was no longer used, and so it was removed from the controller.

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.

review_bid.rb

Per the previous iteration's feedback, the review_bid.rb model file presented some DRY violations. These violations are highlighted in the method below:

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

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.

Video Demonstration

Click this link to see a video demonstrating the functionality of the review bidding feature.

RSpec Testing

One of the primary design goals when doing this refactoring was to ensure that all test cases still passed after all changes were made.

There was a single change that needed to be made to the RSpec files for the review bidding algorithm, which was the removal of a test relying on the 'create' method of the review_bids_controller. As previously mentioned, this function was removed because it had become obsolete. Thus, the test case for this function (which simply checked if it redirected to the right page) was no longer necessary, and thus was deleted.

Otherwise, the test cases for both the review_bids_controller and the review_bids model were manually run and found to have no errors, as shown below.

For details on what these specific tests were, please refer to the previous work referenced before.

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.

Additional to the unhandled cases from the algorithm service, we also noticed an inconsistency of the system due to the performance limitation of the Heroku deployment.

As we can see from the above recorded screen capture animation, with the exact same input, the output and algorithm service result are unstable. Because of the performance limitation of the Heroku, sometime the Flask app that deployed were overloaded. Furthermore, we have also attempted to deploy on VCL, however, we notice the limitation of VCL deployment that will become offline once the VCL reservation expired. After all, because of the focus of our tasks is on the DRY principal optimization, the external algorithm service can be improved in the future. To access and deploy the algorithm on local environment or another environment, setup guide and resource are available on IntelligentAssignment repository.

Lack of Comprehensive Testing

A notable contradiction that was noticed when working with this project was that the previously-mentioned failed edge cases existed, yet all tests for this feature passed. This is illustrative of the fact that this feature requires better and more exhaustive testing.

With the scope limitation of our tasks that focusing on the DRY principal enhancement of the Expertiza internally, the external algorithm service module was not within our focus. However, with our experience of deployment with the algorithm service, we can offer some suggestions for the future team who will address those edge cases of the algorithm service.

When dealing with an issue with so many factors as how students bid on reviews, an extensive suite of edge cases should be considered. Any future project group working on this function should consider the following edge cases to cover:

• Does a student who does not bid on any reviews still get reviews?
• Does a student who did not submit anything for their assignment still have a review to bid on?
• What if there are less assignments to have reviews for than the minimum number required for all students to have the minimum number of reviews?

Conclusion

While we believe the current state of this feature is greatly improved, with many of its DRY violations removed and several noteworthy bugs corrected, we believe the feature of allowing students to bid on what to review still needs further development before it is deployed. While the basic functionality is working under ideal situations, the issues mentioned above should be addressed before this feature is put in the hands of actual instructors, as if this feature fails, it could ruin the entire reviewing process for an assignment.

Still, we think there is notable promise to this feature, and it would be a very engaging and interesting feature if deployed successfully, so we believe develop on it should continue until it is ready for deployment.

Github and Related Link

Github repository corresponding to this task is publicly available at E2151 GitHub.

Github pull request and file changes can be viewed at E2151 Pull Request.

Github of source code of externally hosted bidding algorithm service deployment manual and files can be access at IntelligentAssignment.

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)