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

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(92 intermediate revisions by 3 users not shown)
Line 1: Line 1:
==<b>E1956. There is no shortcut to get free review points: Review Assignment Bug</b>==
==<b>E1956. There is no shortcut to get free review points: Review Assignment Bug</b>==
 
This page provides a description of the Expertiza based OSS project.


__TOC__
__TOC__
Line 7: Line 6:
===About Expertiza===
===About Expertiza===


[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ 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.
[http://expertiza.ncsu.edu/ Expertiza] is an open-source project based on [http://rubyonrails.org/ 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==
==Project Description==
 
===Problem Statement===
E1956. There is no shortcut to get free review points: Review Assignment Bug
===Background===
===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.  
Each assignment contains an review strategy. We can generally submit 'n' reviews according to the review strategy. 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 review strategy.
====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===
===Motivation===


This project in particular intends that the students collaborate with each other 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.
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.


==List of Changes==
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 review strategy. Also, there is no check to see if the user has submitted enough reviews for that assignment before getting a new review.
===Issues Presented===
*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.
* There is no check to see if the submission is already assigned to the student.


===Current Implementation===
==Current Implementation==
<b>Problem 1: No limitation on the maximum number of peer reviews.</b>
*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.


* No limitation on maximum number of peer reviews.
<b>Problem 2: No check on duplicate submissions</b>
::There is no check in the backend that limits the number of reviews a student can be assigned.  There is a check in the UI, but you could evade the limit by typing in a URL to make the same post request. You can also evade it by clicking multiple times on "Request a review for submission" button in UI.
* 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).
* There is no check to see if the submission is already assigned to the 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.
* If the same request is re-sent, the system adds the same submission for review a second time.


[[File:Merged.png]]
<b>Problem 3: There is no check on the number of outstanding reviews</b>
*A user can request for submissions even if the current outstanding ones are pending.


===New Implementation===
[[File:Pro.png]]
[[File:Issue.png]]
 
==Proposed Implementation==


===Files modified===
===Files modified===
'''review_mapping_controller_spec.rb'''
'''review_mapping_controller.rb'''
* Change has been done in the implementation of review_mapping_controller.rb where new check was added. This check fetches number of reviews done by the student currently from ReviewResponseMap table. To adapt to those changes two new mocks were added to review_mapping_controller_spec.rb.  
* The is_reviews_allowed method checks the number of reviews assigned to a student and then compares it with the maximum number of reviews allowed as per review strategy. If the student is asking for more reviews than the review strategy then it returns False.
* ReviewResponseMap is mocked to return 0. This is the number of reviews that a student has done so far.
<pre>
* Assignment is mocked to return 1 as number of reviews allowed for the assignment.  
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
</pre>
* The check_outstanding_reviews checks the number of outstanding assignment reviews a user can have at a time. The check_outstanding_reviews 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 review strategy or not. The user can only request for a submission if the check_outstanding_reviews returns True.
<pre>
<pre>
context 'when assignment has topics and a topic is selected by reviewer' do
def check_outstanding_reviews?(assignment, reviewer)
      it 'assigns reviewer dynamically and redirects to student_review#list page' do
    @review_mappings = ReviewResponseMap.where(reviewer_id: reviewer.id, reviewed_object_id: assignment.id)
        allow(assignment).to receive(:topics?).and_return(true)
    @num_reviews_total = @review_mappings.size
        topic = double('SignUpTopic')
    if @num_reviews_total == 0
        allow(SignUpTopic).to receive(:find).with('1').and_return(topic)
      true
        allow(assignment).to receive(:assign_reviewer_dynamically).with(participant, topic).and_return(true)
    else
        allow(ReviewResponseMap).to receive(:reviewer_id).with(1).and_return(0)
      @num_reviews_completed = 0
allow(assignment).to receive(:num_reviews_allowed).and_return(1)
      @review_mappings.each do |map|
params = {
         @num_reviews_completed += 1 if !map.response.empty? && map.response.last.is_submitted
          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
      @num_reviews_in_progress = @num_reviews_total - @num_reviews_completed
      @num_reviews_in_progress < assignment.max_outstanding_reviews
     end
     end
  end
</pre>
'''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.
* To test number of reviews, we added new tests. The code works on size of object returned by querying reviewer_id and reviewed_object_id. To avoid database call, ReviewResponseMap is mocked to return empty list and a list of values. Empty list indicates that student has not done any review and therefore must be allowed. List of values is returned to check if the size of list is greater than allowed number of reviews according to review strategy.
* To test outstanding number of reviews, we added new tests. The code works on number of reviews completed. If the number of reviews completed are less than assignment's max outstanding reviews, then student must not be allowed new review. To test the functionality we mocked ReviewResponseMap to return an object.
*For one of the test cases we mocked the assignment's max outstanding reviews to be 0, which means without doing even a single review he can request as many reviews as he wants. For the other test case we mocked assignment's max outstanding reviews to be 2 and hence without doing two reviews he can't ask for more. We mocked ReviewResposneMap to have one object.
==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:<br/>
1. Login as instructor.<br/>
2. Go to Manage->Assignments and navigate to assignment name 'test1'. <br/>
3. Click on add participants. This will give a list of all participants. <br/>
4. Now click on any student username in order to impersonate. <br/>
5. Go to test1->other's_work and click on request submission button continuously and wait for the response.<br/>
6. It would be noted that the request would never exceed the number of submissions mentioned in the review strategy.
To test outstanding number of reviews:<br/>
Follow the steps above, you will get 2 reviews at first.<br/>
1. Fill up one of the review, You will get the button to request again.<br/>
2. Click on button continuously.<br/>
3. It should be noted that despite continuous request, user will not get any more reviews than difference of max outstanding review and completed reviews.<br/>


    context 'when assignment does not have topics' do
===Automated Test Cases For Review Limit enforcement===
      it 'runs another algorithms and redirects to student_review#list page' do
 
        allow(assignment).to receive(:topics?).and_return(false)
All the test cases have been automated in the review_mapping_controller_spec File
        team1 = double('AssignmentTeam')
 
        team2 = double('AssignmentTeam')
::1. Student has done reviews less than review strategy: <pre>context 'when number of reviews are less than the review strategy'</pre>
        teams = [team1, team2]
::::1. Assign Review Dynamically.
        allow(assignment).to receive(:candidate_assignment_teams_to_review).with(participant).and_return(teams)
::::2. Redirect to Student review page.
        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)
::2. Student has done reviews more than the review strategy: <pre>context 'when number of reviews are greater than the review strategy'</pre>
        allow(ReviewResponseMap).to receive(:reviewer_id).with(1).and_return(0)
::::1. Redirect to Student review page.
        allow(assignment).to receive(:num_reviews_allowed).and_return(1)
::::2. Show Flash Error. [flash[:error] = "You cannot do more than " + assignment.num_reviews_allowed.to_s + " reviews based on review strategy"]
params = {
 
          assignment_id: 1,
===Automated Test Cases For Review pending check===
          reviewer_id: 1,
          topic_id: 1
        }
        post :assign_reviewer_dynamically, params
        expect(response).to redirect_to '/student_review/list?id=1'
      end
    end
  end


All the test cases have been automated in the review_mapping_controller_spec File


::1. Student has pending reviews less than review strategy [default 2 pending reviews at most]: <pre>context 'when user has outstanding reviews less than review strategy'</pre>
::::1. Assign Review Dynamically
::::2. Redirect to Student review page


</pre>
::2. Student has done reviews more than the review strategy [default 2 pending reviews at most]: <pre>context 'when user has outstanding reviews greater than review strategy'</pre>
'''review_mapping_controller.rb'''
::::1. Redirect to Student review page.
* A new check was added to find the number of reviews done by the student. Whenever a student ask for review, a new entry is done in the ReviewResponseMap table in the database.  
::::2. Show Flash Error. [flash[:error] = "You cannot do more reviews when you have "+ assignment.max_outstanding_reviews + "reviews to do"]
* The code here checks the number of reviews assigned to the 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 than a flash notice is sent to the student.
<pre>


def assign_reviewer_dynamically
==Code Coverage==
    assignment = Assignment.find(params[:assignment_id])
24.317%
    reviewer = AssignmentParticipant.where(user_id: params[:reviewer_id], parent_id: assignment.id).first
    @review_mappings = ReviewResponseMap.where(reviewer_id: reviewer.id)
    if params[:i_dont_care].nil? && params[:topic_id].nil? && assignment.topics? && assignment.can_choose_topic_to_review?
      flash[:error] = "No topic is selected.  Please go back and select a topic."
    else
      if @review_mappings.size >= assignment.num_reviews_allowed
        flash[:notice] = "You cannot do more than " + assignment.num_reviews_allowed.to_s + " reviews based on assignment policy"
       
      else


      # begin
==Team Information==
      if assignment.topics? # assignment with topics
Dhruvil Shah
        topic = if params[:topic_id]
                  SignUpTopic.find(params[:topic_id])
                else
                  assignment.candidate_topics_to_review(reviewer).to_a.sample rescue nil
                end
        if topic.nil?
          flash[:error] = "No topics are available to review at this time. Please try later."
        else
          assignment.assign_reviewer_dynamically(reviewer, topic)
        end


      else # assignment without topic -Yang
Neel Parikh
        assignment_teams = assignment.candidate_assignment_teams_to_review(reviewer)
        assignment_team = assignment_teams.to_a.sample rescue nil
        if assignment_team.nil?
          flash[:error] = "No artifacts are available to review at this time. Please try later."
        else
          assignment.assign_reviewer_dynamically_no_topic(reviewer, assignment_team)
        end


      end
Steve Menezes
    end
    # rescue Exception => e
    #  flash[:error] = (e.nil?) ? $! : e
    # end
    end
    redirect_to controller: 'student_review', action: 'list', id: reviewer.id
  end


</pre>
<b>Mentor: Suraj Siddharudh</b>


====Changes Implemented====
==References==
* The assign_review_dynamically method was previously not keeping a track of the number of assignments reviewed by a student.
1.[https://github.com/expertiza/expertiza/ Expertiza on GitHub]
The code in order to assign reviews in a dynamic fashion has been implemented in the review_mappings_controller.rb. The method assign_review_dynamically, checks initially if student has selected a topic for which he wants a review or if the "I don't care" is selected. Based on the selection the method would then select a submission which is
::1. Not assigned previously to given student
::2. Is not his own submission


However, since the code did not have a check for how many reviews already are allocated to given student, one could easily bypass the review limit by sending the same post request again and again.
2.[https://github.com/dhruvil009/expertiza GitHub Project Repository Fork]


<b>ReviewResponseMap:</b>
3.[http://expertiza.ncsu.edu/ Live Expertiza website]
::Creates a mapping from the student to assigned submission for reviewing as soon as assign_review_dynamically assigns a submission to student for review.


We decided to get all the reviews assigned to given student from ReviewResponseMap (where(reviewer_id: reviewer.id)) and then check if the size of the reviews assigned to the given reviewer is more than maximum allocated for the assignment and creates a flash error if the maximum review limit is exceded.
4.[http://152.46.19.135:8080/ Demo Link]


==Testing from UI:==
5.[http://wiki.expertiza.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1956._There_is_no_shortcut_to_get_free_review_points:_Review_Assignment_Bug Project documentation wiki]
Use the given link to test the implementation: http://152.46.19.135:8080/


==Scope for Future Improvement==
6.[https://relishapp.com/rspec Rspec Documentation]
1. Currently there is no check on whether a user has completely submitted the previous two reviews before requesting for additional reviews. So one of the future tasks would be to allow a user to request for a new submission if and only if the previously requested submissions have been completed.

Latest revision as of 22:10, 17 November 2019

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.

Project Description

Problem Statement

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

Background

Each assignment contains an review strategy. We can generally submit 'n' reviews according to the review strategy. 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 review strategy.

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 review strategy. 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 is_reviews_allowed method checks the number of reviews assigned to a student and then compares it with the maximum number of reviews allowed as per review strategy. If the student is asking for more reviews than the review strategy then it returns False.
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
  • The check_outstanding_reviews checks the number of outstanding assignment reviews a user can have at a time. The check_outstanding_reviews 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 review strategy or not. The user can only request for a submission if the check_outstanding_reviews 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.
  • To test number of reviews, we added new tests. The code works on size of object returned by querying reviewer_id and reviewed_object_id. To avoid database call, ReviewResponseMap is mocked to return empty list and a list of values. Empty list indicates that student has not done any review and therefore must be allowed. List of values is returned to check if the size of list is greater than allowed number of reviews according to review strategy.
  • To test outstanding number of reviews, we added new tests. The code works on number of reviews completed. If the number of reviews completed are less than assignment's max outstanding reviews, then student must not be allowed new review. To test the functionality we mocked ReviewResponseMap to return an object.
  • For one of the test cases we mocked the assignment's max outstanding reviews to be 0, which means without doing even a single review he can request as many reviews as he wants. For the other test case we mocked assignment's max outstanding reviews to be 2 and hence without doing two reviews he can't ask for more. We mocked ReviewResposneMap to have one object.

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 review strategy.

To test outstanding number of reviews:
Follow the steps above, you will get 2 reviews at first.
1. Fill up one of the review, You will get the button to request again.
2. Click on button continuously.
3. It should be noted that despite continuous request, user will not get any more reviews than difference of max outstanding review and completed reviews.

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 review strategy: 
context 'when number of reviews are less than the review strategy'
1. Assign Review Dynamically.
2. Redirect to Student review page.
2. Student has done reviews more than the review strategy: 
context 'when number of reviews are greater than the review strategy'
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 review strategy"]

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 review strategy [default 2 pending reviews at most]: 
context 'when user has outstanding reviews less than review strategy'
1. Assign Review Dynamically
2. Redirect to Student review page
2. Student has done reviews more than the review strategy [default 2 pending reviews at most]: 
context 'when user has outstanding reviews greater than review strategy'
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

24.317%

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

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2019_-_E1956._There_is_no_shortcut_to_get_free_review_points:_Review_Assignment_Bug&oldid=129561"