CSC/ECE 517 Fall 2019 - Project E1973. Team Based Reviewing

Introduction - Purpose & Problem

Currently, reviews of other student’s work can only be performed by individual students in Expertiza, not by groups of students. Since doing reviews together could help students learn more than by doing them alone, it has been requested that reviews must now have the option to be done by teams instead of individual participants. Therefore, there should be an option when creating assignments that allows the creator to select whether the assignment will use team reviewers or individual reviewers. For simplification, we were allowed to assume that the teams that worked on an assignment together would review together. Each participant should be able to make individual changes to the review (while logged in from their account), but these changes should apply to the team’s collective review. This creates an issue where teammates could accidentally overwrite each other’s work if they edit the review at once. Therefore, it has been decided that the review should be locked while one edits it so that only one participant can edit it at once. Locking the review presents its own challenges.

Proposed Solution

• Modification to Response Map Classes
  • A field should be added to ResponseMap which indicates whether responses are done by teams or by individuals.
• Modification to Assignment Class
  • A field indicating if the assignment is to be done with team or individuals. This is necessary because part of the suggested requirements is to add a drop down on the review strategy section of the assignment.
• Locking Solution
  • Research needs to be done as to whether a rails mechanism already exists to facilitate a lock on page edits
  • A solution can be implemented from scratch by storing a flag in the database on the review’s table entry. This would require an additional migration.
  • The ability to have a lock on a review requires the implementation of some kind of auto-unlock feature. If a user never unlocks a review, his/her teammates still need to be able to modify the review.
    • We should be able to use the “updated at” field to check if the lock has been held for too long and needs to be released.

More on Locks

The locking solution we added works as a general locking solution. It adds a new table in the database called locks which create a mapping between a user and a resource. This database change does not force a lock on the resource. Just because there exists a lock between some resource and some user, does not mean that other users cannot edit that resource. The actual prevention of edits, be it redirecting or preventing access to controller methods, is the responsibility of you, the programmer. This class just provides an easy interface to facilitate that behavior.

Database Changes

Locks created the following changes:

• locks
  • timeout_period
  • created_at
  • updated_at
  • user_id
  • lockable_id
  • lockable_type

Code Changes

How to implement

Whichever model needs to be able to be locked must include this line:

include Lockable

See app/models/response.rb

To check to see if a resource is locked, use

resource = Lock.get_lock(resource, user, timeout_period)

If the resource is nil, it has been locked. If it's not nil, the given user owns the lock over the current resource. See app/controllers/response_controller.rb#edit

To unlock a resource after it is done being used, use

Lock.release_lock(resource)

See app/controllers/lock_controller.rb#release_lock

To check to see if a lock exists between a user and a resource, use

Lock.lock_between?(resource, user)

See app/controllers/response_controller.rb#update Because locks can time out, if user1 makes changes and stalls on a page, user2 could get the lock, make edits, and release it. If that's the case, we may not want to keep user1's changes. The way to check for that scenario is by seeing if the user still has a lock on this object.

About lock timeouts

Lock.get_lock takes a timeout_period. Timeout_period minutes after a user gets a lock, any user who calls get_lock on a resource will acquire a lock.

As the code stands, you MUST include a timeout period if you wish to get a lock. The rationale being that permanently preventing access to a resource is a heavy price for forgetting to unlock a resource. If you wish to do away with this feature, you will need to add the code yourself. (Perhaps using -1 for no time limit). Though I would heavily recommend using the timeout feature.

Changes to Code

• review_response_map.rb (migration required)
  • Field - reviewer_is_team: boolean

 def after_initialize
   # If an assignment supports team reviews, it is marked in each mapping
   reviewer_is_team = assignment.reviewer_is_team
 end

• assignment.rb (migration required)
  • Field - has_team_reviews: boolean
• assignments/edit/_review_strategy.html.erb - added check box for has_team_reviews

     <input name="assignment_form[assignment][reviewer_is_team]" type="hidden" value="false"/>
     <%= check_box_tag('assignment_form[assignment][reviewer_is_team]', 'true', @assignment_form.assignment.reviewer_is_team) %>
     <%= label_tag('assignment_form[assignment][reviewer_is_team]', 'Is Review done by Teams?') %>
     <img src="/assets/info.png" title='You can select whether the reviews should be done by individual students or teams'>

• assignment_participant.rb
  • Method - get_reviewer
    • Returns the participant's team if the reviews for the assignment are done by teams.
    • Several lines of code treated reviewers explicitly as participants. In order to avoid changing too much functionality, and to go with Dr. Gehringer's request that the changes be polymorphic, we just inserted this method whenever the code treated a participant as a reviewer.
    • Example (review_mapping_controller.rb):
    • Reviewer used to be retrieved by the call to AssignmentParticipant.where. Now, reviewer and participant are treated seperately.

def assign_reviewer_dynamically assignment = Assignment.find(params[:assignment_id]) participant = AssignmentParticipant.where(user_id: params[:reviewer_id], parent_id: assignment.id).first reviewer = participant.get_reviewer ...

• Added lock.rb, lock_controller.rb, and lockable.rb
• app/views/responses/response.html.erb
  • Javascript was added to get locks to be released when the page is exited:

function release_lock() { $.post("../../../lock/release_lock?id=<%= @response.id %>,&type=Response"); } window.onbeforeunload = release_lock;

Changes Represented in UML

UI Changes

Assignment Review Strategy

Before

After

Assign Reviewers

Before

After

Test Plan

Rspec

• Tests were added in response_controller_spec.rb to test that the controller methods properly handle locks and redirect when responses are locked:

     it 'Does not allow a user to update a response if a lock exists on the response' do
       allow(ResponseMap).to receive(:find).with(2).and_return(team_response_map)
       allow(Lock).to receive(:get_lock).and_return(nil)
       params = {
         id: 2,
         review: {
           comments: 'some comments'
         },
         responses: {
           '0' => {score: 98, comment: 'LGTM'}
         },
         isSubmit: 'No'
       }
       session = {user: instructor}
       post :update, params, session
       expect(response).not_to redirect_to('/response/save?id=1&msg=&review%5Bcomments%5D=some+comments')
     end

• Tests were created for lock.rb to ensure that all database functionality is handled properly, locks time out, and locks correctly handle requests from multiple users:

it 'Should create new locks when a user requests them' do
  # smyoder should have a lock on the response for 10 minutes
  expect(Lock.get_lock(@response, @smyoder, 10)).to eq(@response)
  firstLock = Lock.find_by(user: @smyoder, lockable: @response)
  expect(firstLock).not_to be_nil
  # A user with a lock on a resource should be able to renew the lock
  expect(Lock.get_lock(@response, @smyoder, 8)).to eq(@response)
  secondLock = Lock.find_by(user: @smyoder, lockable: @response)
  expect(secondLock).not_to eq(firstLock)
end

• Since many of expertiza's tests use mocks, several lines in other tests needed to be added since previously un-called methods were now being called:
  • Example from review_response_map_spec.rb

allow(participant1).to receive(:get_reviewer).and_return(participant1)

UI Testing

Our UI tests aim to capture the following core pieces of functionality:

Students on the same team can view/edit the same response:

Prerequisites: we use our fixture to ensure that an assignment has been created, and that students 9 and 10 are on a team together.

1. Login as student10.
2. Navigate to assignments, and click on an assignment.
3. Request a new submission to review.
4. In the review, leave the comment "Excellent work done!" and click save.
5. Logout and login as student 9. Repeat step 2.
6. Click View. Notice that the review already says "Excellent work done!".
7. Go back to the review and click Edit. Change the message to "Decent work here".
8. Logout and login as student 10. Repeat step 2.
9. Click View. Notice that the review now says "Decent work here".

This test was implemented in the file 'teams_as_reviewers_spec.rb':

Manual Testing

Students cannot edit the response at the same time:
Prerequisites: Same as the above test.

1. Repeat steps 1-4 (inclusive) from the above UI test. However, remain on the edit page.
   
2. Open another browser and navigate to Expertiza. Then, login as student9 in another browser.
   
3. Navigate to the assignment and click "Edit". Verify that you are redirected and unable to edit the review response since student10 is already editing it.
   

We leave a manual testing section here to cover a scenario we previously planned to add to our feature tests, but realized to be unfeasible. We have functionality that checks that 2 users on the same team cannot edit a response at the same time. However, Expertiza only uses one browser while testing currently. Setting up 2 of them to run at once was deemed beyond the scope of this project.

Relevant Links

GitHub - https://github.com/TheRazzler/expertiza
Pull Request - https://github.com/expertiza/expertiza/pull/1643/files
Demo Video - https://youtu.be/U7MK7HrrmQI Contributors:
Spencer Yoder - smyoder@ncsu.edu
Ben Fisher - bjfisher@ncsu.edu