CSC/ECE 517 Fall 2019 - E1956. There is no shortcut to get free review points: Review Assignment Bug: Difference between revisions
No edit summary |
No edit summary |
||
Line 183: | Line 183: | ||
==Code Coverage== | ==Code Coverage== | ||
29.77% | |||
==Team Information== | ==Team Information== |
Revision as of 00:54, 7 November 2019
E1956. There is no shortcut to get free review points: Review Assignment Bug
This page provides a description of the Expertiza based OSS project.
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
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.
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.
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.
Proposed Changes
- The assign_review_dynamically method was previously not keeping a track of the number of assignments reviewed by a student.
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" option 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 a given student, one could easily bypass the review limit by sending the same post request again and again.
ReviewResponseMap:
- 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 the maximum value allocated for the assignment and creates a flash error if the maximum review limit is exceded.
List of Changes
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
- No limitation on maximum number of peer reviews.
- 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 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.
Proposed Implementation
Files modified
review_mapping_controller_spec.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.
- 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 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 end
review_mapping_controller.rb
- 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.
- 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 then a flash notice is sent to the student.
def assign_reviewer_dynamically assignment = Assignment.find(params[:assignment_id]) reviewer = AssignmentParticipant.where(user_id: params[:reviewer_id], parent_id: assignment.id).first 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 is_review_allowed?(assignment, reviewer) if check_outstanding_reviews?(assignment, reviewer) # begin if assignment.topics? # assignment with topics 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 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 end else flash[:notice] = "You cannot do more than " + assignment.num_reviews_allowed.to_s + " reviews based on assignment policy" end end redirect_to controller: 'student_review', action: 'list', id: reviewer.id end 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 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
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
Automated Test Cases For Review pending check
Code Coverage
29.77%
Team Information
Dhruvil Shah
Neel Parikh
Steve Menezes
Mentor: Suraj Siddharudh