CSC/ECE 517 Fall 2019 - E1956. There is no shortcut to get free review points: Review Assignment Bug

E1956. There is no shortcut to get free review points: Review Assignment Bug

About Expertiza

Expertiza is an open-source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

Introduction

Problem Statement

E1956. There is no shortcut to get free review points: Review Assignment Bug

Background

Each assignment contains an assignment policy. We can generally submit n reviews according to assignment policy. For an assignment with topics, a student has an option to choose a submission to review or can say “I don’t care” and the system chooses any available topic for review.

Issue 1

• The number of reviews done by any student is not checked in the back-end with the maximum number of submissions allowed as per the assignment policy.

Issue 2

• There is no check to see if the submission is already assigned to the student.

Issue 3

• There is no check on the number of outstanding reviews a user can have.

Motivation

This project, in particular intends that the students collaborate and work on making enhancements to the code base by applying the concepts of Rails,RSpec, DRY code,Test driven development etc. This provides an opportunity for students to contribute to an open-source project and learn further about software deployment etc.

Currently, there is no check in the backend that limits the number of reviews a student can be assigned. Students can get more peer reviews than assignment policy. Also, there is no check to see if the user has submitted enough reviews for that assignment before getting a new review.

Current Implementation

Problem 1: No limitation on the maximum number of peer reviews.

• There is no check in the backend that limits the number of reviews assigned to a student. There is a check in the UI, but one could evade the limit by typing in a URL to make the same post request. One can also evade it by clicking multiple times on "Request a review for submission" button in UI.

Problem 2: No check on duplicate submissions

• There is no check to see if the submission is already assigned to a student (on consulting the TA, it was made known that the feature was working correctly without editing any of the code and thus no refactoring was performed for this task).
• If the same request is re-sent, the system adds the same submission for review a second time.

Problem 3: There is no check on the number of outstanding reviews

• A user can request for submissions even if the current outstanding ones are pending.

Proposed Implementation

Files modified

review_mapping_controller.rb

• The code here checks the number of reviews assigned to a student and then compares it with the maximum number of reviews allowed as per assignment policy. If the student is asking for more reviews than the assignment policy then a flash notice is sent to the student.

def is_review_allowed?(assignment, reviewer)
  @review_mappings = ReviewResponseMap.where(reviewer_id: reviewer.id, reviewed_object_id:  assignment.id)
  assignment.num_reviews_allowed > @review_mappings.size
end

• Also there is a check on the number of outstanding assignment reviews a user can have at a time. The check_outstanding_review keeps a count of the number of reviews in progress as well as the number of reviews currently completed by the user and returns a Boolean value depending upon whether the former is less than the maximum outstanding reviews allowed as per the assignment policy or not. The user can only request for a submission if the check_outstanding_review returns True.

def check_outstanding_reviews?(assignment, reviewer)
    @review_mappings = ReviewResponseMap.where(reviewer_id: reviewer.id, reviewed_object_id: assignment.id)
    @num_reviews_total = @review_mappings.size
    if @num_reviews_total == 0
      true
    else
      @num_reviews_completed = 0
      @review_mappings.each do |map|
        @num_reviews_completed += 1 if !map.response.empty? && map.response.last.is_submitted
      end
      @num_reviews_in_progress = @num_reviews_total - @num_reviews_completed
      @num_reviews_in_progress < Assignment.max_outstanding_reviews
    end
  end

review_mapping_controller_spec.rb

• Change has been done in the implementation of review_mapping_controller.rb where a new check was added. This check fetches the number of reviews done by a student currently from ReviewResponseMap table. To adapt to those changes, two new mocks were added to the review_mapping_controller_spec.rb.
• ReviewResponseMap is mocked to return 0. This is the number of reviews that a student has done so far.
• Assignment is mocked to return 1 as number of reviews allowed for the assignment.

context 'when assignment has topics and no topic is selected by reviewer' do
      it 'shows an error message and redirects to student_review#list page' do
        allow(assignment).to receive(:topics?).and_return(true)
        allow(assignment).to receive(:can_choose_topic_to_review?).and_return(true)
        params = {
          assignment_id: 1,
          reviewer_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(flash[:error]).to eq('No topic is selected.  Please go back and select a topic.')
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end

    context 'when assignment has topics and a topic is selected by reviewer' do
      it 'assigns reviewer dynamically and redirects to student_review#list page' do
        allow(assignment).to receive(:topics?).and_return(true)
        topic = double('SignUpTopic')
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
        allow(ReviewResponseMap).to receive(:reviewer_id).with(1).and_return(0)
	allow(assignment).to receive(:num_reviews_allowed).and_return(1)
	params = {
          assignment_id: 1,
          reviewer_id: 1,
          topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end

    context 'when assignment does not have topics' do
      it 'runs another algorithms and redirects to student_review#list page' do
        allow(assignment).to receive(:topics?).and_return(false)
        team1 = double('AssignmentTeam')
        team2 = double('AssignmentTeam')
        teams = [team1, team2]
        allow(assignment).to receive(:candidate_assignment_teams_to_review).with(participant).and_return(teams)
        allow(teams).to receive_message_chain(:to_a, :sample).and_return(team2)
        allow(assignment).to receive(:assign_reviewer_dynamically_no_topic).with(participant, team2).and_return(true)
        allow(ReviewResponseMap).to receive(:reviewer_id).with(1).and_return(0)
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
	params = {
          assignment_id: 1,
          reviewer_id: 1,
          topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end

    context 'when number of reviews are less than the assignment policy' do
      it 'redirects to student review page' do
        allow(assignment).to receive(:topics?).and_return(true)
        topic = double('SignUpTopic')
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
        allow(ReviewResponseMap).to receive(:where).with(reviewer_id: 1, reviewed_object_id: 1 )
                                  .and_return([])
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
        params = {
            assignment_id: 1,
            reviewer_id: 1,
            topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end

    context 'when number of reviews are greater than the assignment policy' do
      it 'shows a flash error and redirects to student review page' do
        allow(assignment).to receive(:topics?).and_return(true)
        topic = double('SignUpTopic')
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
        allow(ReviewResponseMap).to receive(:where).with(reviewer_id: 1, reviewed_object_id: 1 )
                                        .and_return([1,2,3])
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
        params = {
            assignment_id: 1,
            reviewer_id: 1,
            topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
        expect(flash[:error]).to be_present
      end
    end

    context 'when user has outstanding reviews less than assignment policy'  do
      it 'redirects to student review page' do
        allow(assignment).to receive(:topics?).and_return(true)
        topic = double('SignUpTopic')
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
        allow(ReviewResponseMap).to receive(:where).with(reviewer_id: 1, reviewed_object_id: 1 )
                                        .and_return(:review_response_map)
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
        allow(assignment).to receive(:max_outstanding_reviews).and_return(0)

        params = {
            assignment_id: 1,
            reviewer_id: 1,
            topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end

    context 'when user has outstanding reviews greater than assignment policy'  do
      it 'redirects to student review page and shows flash error' do
        allow(assignment).to receive(:topics?).and_return(true)
        topic = double('SignUpTopic')
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
        allow(ReviewResponseMap).to receive(:where).with(reviewer_id: 1, reviewed_object_id: 1 )
                                        .and_return(:review_response_map)
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
        allow(assignment).to receive(:max_outstanding_reviews).and_return(3)

        params = {
            assignment_id: 1,
            reviewer_id: 1,
            topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(flash[:error]).to be_present
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end
  end

Test Plan

For users intending to view the deployed Expertiza associated with this assignment, the credentials are below: Instructor Login: username -> instructor6, password -> password

Testing from UI

Use the given link: http://152.46.19.135:8080/

Follow the instructions below to test the implemented changes:
1. Login as instructor.
2. Go to Manage->Assignments and navigate to assignment name 'test1'.
3. Click on add participants. This will give a list of all participants.
4. Now click on any student username in order to impersonate.
5. Go to test1->other's_work and click on request submission button continuously and wait for the response.
6. It would be noted that the request would never exceed the number of submissions mentioned in the assignment policy.

Automated Test Cases For Review Limit enforcement

All the test cases have been automated in the review_mapping_controller_spec File

1. Student has done reviews less than assignment policy:

context 'when number of reviews are less than the assignment policy'

1. Assign Review Dynamically.

2. Redirect to Student review page.

2. Student has done reviews more than the assignment policy:

context 'when number of reviews are greater than the assignment policy'

1. Redirect to Student review page.

2. Show Flash Error. [flash[:error] = "You cannot do more than " + assignment.num_reviews_allowed.to_s + " reviews based on assignment policy"]

Automated Test Cases For Review pending check

All the test cases have been automated in the review_mapping_controller_spec File

1. Student has pending reviews less than assignment policy [default 2 pending reviews at most]:

context 'when user has outstanding reviews less than assignment policy'

1. Assign Review Dynamically

2. Redirect to Student review page

2. Student has done reviews more than the assignment policy [default 2 pending reviews at most]:

context 'when user has outstanding reviews greater than assignment policy'

1. Redirect to Student review page.

2. Show Flash Error. [flash[:error] = "You cannot do more reviews when you have "+ assignment.max_outstanding_reviews + "reviews to do"]

Code Coverage

22.581%

Team Information

Dhruvil Shah

Neel Parikh

Steve Menezes

Mentor: Suraj Siddharudh

References

1.Expertiza on GitHub

2.GitHub Project Repository Fork

3.Live Expertiza website

4.Demo Link

5.Project documentation wiki

6.Rspec Documentation