Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-12-02T03:53:16Z

Gyu9: /* Test Plan */

== '''Introduction''' ==

=== Expertiza ===
[https://github.com/expertiza/expertiza Expertiza] is an open-source project on GitHub used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

=== Travis CI ===
[https://travis-ci.org Travis CI] is a hosted, distributed continuous integration service used to build and test software projects hosted at GitHub.

'''Configuration:'''
Travis CI is configured by adding a file named .travis.yml, which is a YAML format text file, to the root directory of the repository. This file specifies the programming language used, the desired building and testing environment (including dependencies which must be installed before the software can be built and tested), and various other parameters.

'''Operation:'''
When Travis CI has been activated for a given repository, GitHub will notify it whenever new commits are pushed to that repository or a pull request is submitted. It can also be configured to only run for specific branches, or branches whose names match a specific pattern. Travis CI will then check out the relevant branch and run the commands specified in .travis.yml, which usually build the software and run any automated tests. When that process has completed, Travis notifies the developer(s) in the way it has been configured to do so—for example, by sending an email containing the test results (showing success or failure), or by posting a message on an IRC channel. In the case of pull requests, the pull request will be annotated with the outcome and a link to the build log, using a GitHub integration.

=== Capybara ===
[https://github.com/jnicklas/capybara Capybara] supports selenium-webdriver, which is mostly used in web-based automation frameworks. It supports JavaScript, can access HTTP resources outside the application, and can also be setup for testing in headless mode which is especially useful for CI scenarios.

'''Using Capybara with [http://rspec.info/ RSpec]:'''
Load RSpec 2.x support by adding the following line(typically to the spec_helper.rb file):
''require 'capybara/rspec'
''

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the assignment is created by the instructor the only call to action for the student is assignment submission. The purpose of this project is to test whether the submission function could work properly under different scenarios. We will design multiple test cases to simulate different scenarios which the system could encounter. For example, how system respond when a user uploads an invalid URL. The following part will provide more and detailed information on test cases.

== '''Test Plan''' ==

When a user is attempting to submit an assignment, there are some possible cases need to be considered.

1. Upload single valid link: upload a valid link, the system should properly render the page and add this link into the database. We will use the following statements to simulate the submit action.

''
fill_in 'submission', with: "https://www.google.com"
click_on 'Upload link'
''

2. Upload invalid link: upload an invalid link (e.g. a non-existing link), the system should detect the invalid link, reject the submission and cast corresponding error message.

3. Upload multiple links: upload multiple links, the system should properly render the page and add the new link into the database. We will use the following statement to check if the current page has the uploaded URL.

''
expect(page).to have_content "https://www.google.com"
''

4. Upload duplicated links: upload an link which has already been uploaded before, the system should detect this duplicated link, reject the submission and cast error message.

5. Upload empty link: upload an empty link, the system should reject the submission and cast corresponding error message.

6. Upload a valid file: upload a valid file, the system should properly render the page and add this file into the directory. We will use the following statements to simulation the submit action.

''
attach_file('uploaded_file', file_path)
click_on 'Upload file'
''

7. Upload multiple files: upload multiple files, the system should properly render the page and add the new file into the directory.

8. Upload duplicated files: upload a file which has already been uploaded before, the system should update the existed file with new one. The test code will check whether the content of this specific file was successfully updated.

== '''Approach''' ==
1. First, using [https://github.com/expertiza/expertiza/blob/master/spec/factories/factories.rb factories] to create test data: create assignment, create topic, create participant.
''before(:each) do
create(:assignment, name: "Assignment1684", directory_path: "Assignment1684")
create_list(:participant, 3)
create(:topic, topic_name: "Topic")
create(:deadline_type, name: "submission")
create(:deadline_type, name: "review")
create(:deadline_type, name: "metareview")
create(:deadline_type, name: "drop_topic")
create(:deadline_type, name: "signup")
create(:deadline_type, name: "team_formation")
create(:deadline_right)
create(:deadline_right, name: 'Late')
create(:deadline_right, name: 'OK')
create(:assignment_due_date, deadline_type: DeadlineType.where(name: "submission").first, due_at: DateTime.now + 1)
end''
2. Mock the workflow of Assignment submission using Capybara.

[[File:Work flow.png]]

3. Check the result of the submission from both web page and database.

In database level, the submission results are stored in the table: TEAMS.

[[File:Teams.jpg]]

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-12-02T03:52:24Z

Gyu9: /* Test Plan */

== '''Introduction''' ==

=== Expertiza ===
[https://github.com/expertiza/expertiza Expertiza] is an open-source project on GitHub used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

=== Travis CI ===
[https://travis-ci.org Travis CI] is a hosted, distributed continuous integration service used to build and test software projects hosted at GitHub.

'''Configuration:'''
Travis CI is configured by adding a file named .travis.yml, which is a YAML format text file, to the root directory of the repository. This file specifies the programming language used, the desired building and testing environment (including dependencies which must be installed before the software can be built and tested), and various other parameters.

'''Operation:'''
When Travis CI has been activated for a given repository, GitHub will notify it whenever new commits are pushed to that repository or a pull request is submitted. It can also be configured to only run for specific branches, or branches whose names match a specific pattern. Travis CI will then check out the relevant branch and run the commands specified in .travis.yml, which usually build the software and run any automated tests. When that process has completed, Travis notifies the developer(s) in the way it has been configured to do so—for example, by sending an email containing the test results (showing success or failure), or by posting a message on an IRC channel. In the case of pull requests, the pull request will be annotated with the outcome and a link to the build log, using a GitHub integration.

=== Capybara ===
[https://github.com/jnicklas/capybara Capybara] supports selenium-webdriver, which is mostly used in web-based automation frameworks. It supports JavaScript, can access HTTP resources outside the application, and can also be setup for testing in headless mode which is especially useful for CI scenarios.

'''Using Capybara with [http://rspec.info/ RSpec]:'''
Load RSpec 2.x support by adding the following line(typically to the spec_helper.rb file):
''require 'capybara/rspec'
''

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the assignment is created by the instructor the only call to action for the student is assignment submission. The purpose of this project is to test whether the submission function could work properly under different scenarios. We will design multiple test cases to simulate different scenarios which the system could encounter. For example, how system respond when a user uploads an invalid URL. The following part will provide more and detailed information on test cases.

== '''Test Plan''' ==

When a user is attempting to submit an assignment, there are some possible cases need to be considered.

1. Upload single valid link: upload a valid link, the system should properly render the page and add this link into the database. We will use the following statements to simulate the submit action.

''
fill_in 'submission', with: "https://www.google.com"
click_on 'Upload link'
''

2. Upload invalid link: upload an invalid link (e.g. a non-existing link), the system should detect the invalid link, reject the submission and cast corresponding error message.
3. Upload multiple links: upload multiple links, the system should properly render the page and add the new link into the database. We will use the following statement to check if the current page has the uploaded URL.

''
expect(page).to have_content "https://www.google.com"
''

4. Upload duplicated links: upload an link which has already been uploaded before, the system should detect this duplicated link, reject the submission and cast error message.
5. Upload empty link: upload an empty link, the system should reject the submission and cast corresponding error message.
6. Upload a valid file: upload a valid file, the system should properly render the page and add this file into the directory. We will use the following statements to simulation the submit action.

''
attach_file('uploaded_file', file_path)
click_on 'Upload file'
''

7. Upload multiple files: upload multiple files, the system should properly render the page and add the new file into the directory.
8. Upload duplicated files: upload a file which has already been uploaded before, the system should update the existed file with new one. The test code will check whether the content of this specific file was successfully updated.

== '''Approach''' ==
1. First, using [https://github.com/expertiza/expertiza/blob/master/spec/factories/factories.rb factories] to create test data: create assignment, create topic, create participant.
''before(:each) do
create(:assignment, name: "Assignment1684", directory_path: "Assignment1684")
create_list(:participant, 3)
create(:topic, topic_name: "Topic")
create(:deadline_type, name: "submission")
create(:deadline_type, name: "review")
create(:deadline_type, name: "metareview")
create(:deadline_type, name: "drop_topic")
create(:deadline_type, name: "signup")
create(:deadline_type, name: "team_formation")
create(:deadline_right)
create(:deadline_right, name: 'Late')
create(:deadline_right, name: 'OK')
create(:assignment_due_date, deadline_type: DeadlineType.where(name: "submission").first, due_at: DateTime.now + 1)
end''
2. Mock the workflow of Assignment submission using Capybara.

[[File:Work flow.png]]

3. Check the result of the submission from both web page and database.

In database level, the submission results are stored in the table: TEAMS.

[[File:Teams.jpg]]

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-12-02T03:51:46Z

Gyu9: /* Test Plan */

== '''Introduction''' ==

=== Expertiza ===
[https://github.com/expertiza/expertiza Expertiza] is an open-source project on GitHub used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

=== Travis CI ===
[https://travis-ci.org Travis CI] is a hosted, distributed continuous integration service used to build and test software projects hosted at GitHub.

'''Configuration:'''
Travis CI is configured by adding a file named .travis.yml, which is a YAML format text file, to the root directory of the repository. This file specifies the programming language used, the desired building and testing environment (including dependencies which must be installed before the software can be built and tested), and various other parameters.

'''Operation:'''
When Travis CI has been activated for a given repository, GitHub will notify it whenever new commits are pushed to that repository or a pull request is submitted. It can also be configured to only run for specific branches, or branches whose names match a specific pattern. Travis CI will then check out the relevant branch and run the commands specified in .travis.yml, which usually build the software and run any automated tests. When that process has completed, Travis notifies the developer(s) in the way it has been configured to do so—for example, by sending an email containing the test results (showing success or failure), or by posting a message on an IRC channel. In the case of pull requests, the pull request will be annotated with the outcome and a link to the build log, using a GitHub integration.

=== Capybara ===
[https://github.com/jnicklas/capybara Capybara] supports selenium-webdriver, which is mostly used in web-based automation frameworks. It supports JavaScript, can access HTTP resources outside the application, and can also be setup for testing in headless mode which is especially useful for CI scenarios.

'''Using Capybara with [http://rspec.info/ RSpec]:'''
Load RSpec 2.x support by adding the following line(typically to the spec_helper.rb file):
''require 'capybara/rspec'
''

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the assignment is created by the instructor the only call to action for the student is assignment submission. The purpose of this project is to test whether the submission function could work properly under different scenarios. We will design multiple test cases to simulate different scenarios which the system could encounter. For example, how system respond when a user uploads an invalid URL. The following part will provide more and detailed information on test cases.

== '''Test Plan''' ==

When a user is attempting to submit an assignment, there are some possible cases need to be considered.

1. Upload single valid link: upload a valid link, the system should properly render the page and add this link into the database. We will use the following statements to simulate the submit action.

''
fill_in 'submission', with: "https://www.google.com"
click_on 'Upload link'
''

# Upload invalid link: upload an invalid link (e.g. a non-existing link), the system should detect the invalid link, reject the submission and cast corresponding error message.
# Upload multiple links: upload multiple links, the system should properly render the page and add the new link into the database. We will use the following statement to check if the current page has the uploaded URL.

''
expect(page).to have_content "https://www.google.com"
''

# Upload duplicated links: upload an link which has already been uploaded before, the system should detect this duplicated link, reject the submission and cast error message.
# Upload empty link: upload an empty link, the system should reject the submission and cast corresponding error message.
# Upload a valid file: upload a valid file, the system should properly render the page and add this file into the directory. We will use the following statements to simulation the submit action.

''
attach_file('uploaded_file', file_path)
click_on 'Upload file'
''

# Upload multiple files: upload multiple files, the system should properly render the page and add the new file into the directory.
# Upload duplicated files: upload a file which has already been uploaded before, the system should update the existed file with new one. The test code will check whether the content of this specific file was successfully updated.

== '''Approach''' ==
1. First, using [https://github.com/expertiza/expertiza/blob/master/spec/factories/factories.rb factories] to create test data: create assignment, create topic, create participant.
''before(:each) do
create(:assignment, name: "Assignment1684", directory_path: "Assignment1684")
create_list(:participant, 3)
create(:topic, topic_name: "Topic")
create(:deadline_type, name: "submission")
create(:deadline_type, name: "review")
create(:deadline_type, name: "metareview")
create(:deadline_type, name: "drop_topic")
create(:deadline_type, name: "signup")
create(:deadline_type, name: "team_formation")
create(:deadline_right)
create(:deadline_right, name: 'Late')
create(:deadline_right, name: 'OK')
create(:assignment_due_date, deadline_type: DeadlineType.where(name: "submission").first, due_at: DateTime.now + 1)
end''
2. Mock the workflow of Assignment submission using Capybara.

[[File:Work flow.png]]

3. Check the result of the submission from both web page and database.

In database level, the submission results are stored in the table: TEAMS.

[[File:Teams.jpg]]

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:47:21Z

Gyu9: /* Approach */

== '''Introduction to Expertiza''' ==
Expertiza is an online system that is used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

Expertiza is an open-source project with the source code available as a public repository on GitHub. It is developed using Ruby on Rails and is increasingly becoming robust thanks to the innumerable bugs being fixed by the community. The project has a micro-blog on SourceForge where the developer community report bugs and document updates.

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the Assignment is created by the instructor the only Call to action for the student is assignment submission.

== '''Approach''' ==

Mock the flow of Assignment submission using Capybara and check the result of the submission.

===Workflow===

Student login -> click on the assignment -> add details in text field -> upload zip file -> click submit button.

=== Scenarios ===

# Submit hyperlink → click hyperlink → have content.
# Submit file → download file → have content.

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:41:12Z

Gyu9: /* Approach */

== '''Introduction to Expertiza''' ==
Expertiza is an online system that is used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

Expertiza is an open-source project with the source code available as a public repository on GitHub. It is developed using Ruby on Rails and is increasingly becoming robust thanks to the innumerable bugs being fixed by the community. The project has a micro-blog on SourceForge where the developer community report bugs and document updates.

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the Assignment is created by the instructor the only Call to action for the student is assignment submission.

== '''Approach''' ==

Mock the flow of Assignment submission using Capybara.

===Workflow===

Student login -> click on the assignment -> add details in text field -> upload zip file -> click submit button.

=== Scenarios ===

# Submit hyperlink → click hyperlink → have content.
# Submit file → download file → have content.

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:38:37Z

Gyu9: /* Workflow */

== '''Introduction to Expertiza''' ==
Expertiza is an online system that is used by students to view/submit assignments and review others' work. Expertiza also provides tools to visualize the scores and gauge the improvements made during the course semester. It also facilitates and monitors team projects. It is targeted at educational and non-profit organizations.

Expertiza is an open-source project with the source code available as a public repository on GitHub. It is developed using Ruby on Rails and is increasingly becoming robust thanks to the innumerable bugs being fixed by the community. The project has a micro-blog on SourceForge where the developer community report bugs and document updates.

== '''Project Requirement''' ==

There should be a feature test for submission of the assignment by the student. Once the Assignment is created by the instructor the only Call to action for the student is assignment submission.

== '''Approach''' ==

Mock the flow of Assignment submission using Capybara.

===Workflow===

Student login -> click on the assignment -> add details in text field -> upload zip file -> click submit button.

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:36:29Z

Gyu9: /* Project Requirement */

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:29:12Z

Gyu9: /* Project Requirement */

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:22:24Z

Gyu9: /* Requirement */

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T23:22:00Z

Gyu9: /* Introduction to Expertiza */

CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission

2016-11-09T21:12:17Z

Gyu9: Created page with "== '''Introduction to Expertiza''' == Expertiza is an online system that is used by students to view/submit assignments and review others' work. Expertiza also provides tools to ..."

CSC/ECE 517 Fall 2016

2016-11-09T21:07:48Z

Gyu9: /* Final Project Design Document */

[http://www.example.com link title]==Calibration Assignment Submissions==
*[[Calibration Assignment Submission (Firebrick JS)]]
*[[Calibration Assignment Submission (Active Job)]]
==Writing Assignments 2==
*[[CSC/ECE 517 Fall 2016/E1666. Test team functionality]]
*[[CSC/ECE 517 Fall 2016/E1643. Refactor Suggestion controller]]
*[[CSC/ECE 517 Fall 2016/E1631. Refactoring Bidding Interface]]
*[[CSC/ECE 517 Fall 2016/E1674.Refactor leaderboard.rb and write unit tests]]
*[[CSC/ECE 517 Fall 2016/E1671. Unit Tests for participants.rb Hierarchy]]
*[[CSC/ECE 517 Fall 2016/E1668.Test e-mailing functionality]]
*[[CSC/ECE 517 Fall 2016/E1658. Refractor lottery_controller.rb and write integration tests]]
*[[CSC/ECE 517 Fall 2016/E1660. Review requirements and thresholds]]
*[[CSC/ECE 517 Fall 2016/E1650. Sort instructor views alphabetically by default]]
*[[CSC/ECE 517 Fall 2016/E1644. Refactor and test Teams Controller]]
*[[CSC/ECE 517 Fall 2016/E1645. Refactoring Tree Display Controller]]
*[[CSC/ECE 517 Fall 2016/E1659. Refactor on_the_fly_calc.rb]]
*[[CSC/ECE 517 Fall 2016/E1657. Introduce a Student View for instructors]]
*[[CSC/ECE 517 Fall 2016/E1653. Fix and improve rubric criteria]]
*[[CSC/ECE 517 Fall 2016/E1642. Refactor review_response_map.rb]]
*[[CSC/ECE 517 Fall 2016/E1633. Refactor different question types from quiz feature]]
*[[CSC/ECE_517_Fall_2016/E1664:_Feature_Test_Assignment_Creation]]
*[[CSC/ECE 517 Fall 2016/E1666. Test team functionality]]
*[[CSC/ECE_517_Fall_2016/E1654. Improve_date-picker_and_deadlines]]
*[[CSC/ECE_517_Fall_2016/E1652 Fix teammate advertisements and requests to join a team ]]
*[[CSC/ECE_517_Fall_2016/E1662. UI issues/fixes]]
*[[CSC/ECE_517_Fall_2016/E1673. Refactor question_type.rb]]
*[[CSC/ECE_517_Fall_2016/E1675. Timestamp for student file & hyperlink submissions]]
*[[CSC/ECE_517_Fall_2016/E1640. Refactor response.rb and response_helper.rb]]
*[[CSC/ECE 517 Fall 2016/E1634. Refactor and write unit test of due_date.rb and deadline_helper.rb]]
*[[CSC/ECE_517_Fall_2016/M1654._Improve_network_security_features]]
*[[CSC/ECE_517_Fall_2016/E1670._Unit_tests_for_answers.rb]]
*[[CSC/ECE_517_Fall_2016/E1641. Refactor review_mapping_controller.rb]]
*[[CSC/ECE_517_Fall_2016/M1652_Implement_ImageMap_Support_Servo]]
*[[CSC/ECE_517_Fall_2016/E1648/Add_past_due_assignment]]
*[[CSC/ECE 517 Fall 2016/E1656. Improve imports]]
*[[CSC/ECE_517_Fall_2016/M1653_Implement_HTML_form_validation]]
==Final Project Design Document==
*[[CSC/ECE 517 Fall 2016 E1696 Improve Self-Review]]
*[[CSC/ECE 517 Fall 2016 E1676 Role-based reviewing]]
*[[CSC/ECE 517 Fall 2016/E1680. Improve survey functionality]]
*[[CSC/ECE 517 Fall 2016/E1693. Drag-and-drop interface for creating rubrics]]
*[[CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing]]
*[[CSC/ECE 517 Fall 2016/E1705. Tracking the time students look at the others' submissions]]
*[[CSC/ECE 517 Fall 2016/E1688. Send feedback to support + tree display improvement]]
*[[CSC/ECE 517 Fall 2016 E1707: Top trading cycles to exclude previous teammates]]
*[[CSC/ECE 517 Fall 2016 E1678: Review configuration options]]
*[[CSC/ECE 517 Fall 2016 E1682: Improve score calculation]]
*[[CSC/ECE 517 Fall 2016 E1684: Feature Test for Assignment Submission]]

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:20:45Z

Gyu9: /* Refactor delete_all_reviewer */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor add_reviewer ==
def add_reviewer
msg = ''
case params[:object]
when 'reviewer'
topic_id = params[:topic_id]
user_id = User.where(name: params[:user][:name]).first.id
# If instructor want to assign one student to review his/her own artifact,
# it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
flash[:error] = "You cannot assign this student to review his/her own artifact."
else
# Team lazy initialization
assignment = Assignment.find(params[:id])
assignment_id = assignment.id
SignUpSheet.signup_team(assignment.id, user_id, topic_id)
create_reviewer(assignment)
end
when 'metareviewer'
mapping = ResponseMap.find(params[:id])
assignment_id = mapping.assignment.id
create_reviewer(mapping)
end
redirect_to action: 'list_mappings', id: assignment_id, msg: msg
end

def create_reviewer(object)
begin
user = User.from_params(params)
# contributor_id is team_id
case params[:object]
when 'reviewer'
assignment = object # assignment
reviewee_id = params[:contributor_id]
reviewed_object_id = assignment.id
response_map = ReviewResponseMap
id_symbol = :reviewee_id
id = reviewee_id
assignment = assignment
regurl = url_for action: 'add_user_to_assignment',
id: assignment.id,
user_id: user.id,
contributor_id: params[:contributor_id]
when 'metareviewer'
mapping = object # mapping
reviewee_id = mapping.reviewer.id
reviewed_object_id = mapping.map_id
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = reviewed_object_id
assignment = mapping.assignment
regurl = url_for action: 'add_user_to_assignment',
id: mapping.map_id,
user_id: user.id
end
# Get the assignment's participant corresponding to the user
reviewer = get_reviewer(user, assignment, regurl)
# ACS Removed the if condition(and corressponding else) which differentiate
# assignments as team and individual assignments
# to treat all assignments as team assignments
if response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
response_map.create(reviewee_id: reviewee_id,
reviewer_id: reviewer.id,
reviewed_object_id: reviewed_object_id)
else
case params[:object]
when 'reviewer'
raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
when 'metareviewer'
raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
end
end
rescue
msg = $ERROR_INFO
end
end

== Refactor delete_reviewer ==

def delete_reviewer
case params[:object]
when 'reviewer'
response_map = ReviewResponseMap
when 'metareviewer'
response_map = MetareviewResponseMap
end
# duplicate code
mapping = response_map.find(params[:id])
assignment_id = mapping.assignment.id
case params[:object]
when 'reviewer'
if !Response.exists?(map_id: mapping.id)
mapping.destroy
flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
else
flash[:error] = "This review has already been done. It cannot been deleted."
end
when 'metareviewer'
flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
begin
mapping.delete
rescue
flash[:error] = "A delete action failed: "
+ $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
+ mapping.map_id.to_s + "'>Delete this mapping anyway>?"
end
end
redirect_to action: 'list_mappings', id: assignment_id
end

== Refactor delete_all_reviewer ==

def delete_all_reviewers
mapping = ResponseMap.find(params[:id])
case params[:object]
when 'all_metareviewers'
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = mapping.map_id
end
mmappings = response_map.where(id_symbol => id)
failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
if failedCount > 0
url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
flash[:error] = "A delete action failed: #{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a> "
else
flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
end
redirect_to action: 'list_mappings', id: mapping.assignment.id
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:20:23Z

Gyu9: /* Refactor add_reviewer */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor add_reviewer ==
def add_reviewer
msg = ''
case params[:object]
when 'reviewer'
topic_id = params[:topic_id]
user_id = User.where(name: params[:user][:name]).first.id
# If instructor want to assign one student to review his/her own artifact,
# it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
flash[:error] = "You cannot assign this student to review his/her own artifact."
else
# Team lazy initialization
assignment = Assignment.find(params[:id])
assignment_id = assignment.id
SignUpSheet.signup_team(assignment.id, user_id, topic_id)
create_reviewer(assignment)
end
when 'metareviewer'
mapping = ResponseMap.find(params[:id])
assignment_id = mapping.assignment.id
create_reviewer(mapping)
end
redirect_to action: 'list_mappings', id: assignment_id, msg: msg
end

def create_reviewer(object)
begin
user = User.from_params(params)
# contributor_id is team_id
case params[:object]
when 'reviewer'
assignment = object # assignment
reviewee_id = params[:contributor_id]
reviewed_object_id = assignment.id
response_map = ReviewResponseMap
id_symbol = :reviewee_id
id = reviewee_id
assignment = assignment
regurl = url_for action: 'add_user_to_assignment',
id: assignment.id,
user_id: user.id,
contributor_id: params[:contributor_id]
when 'metareviewer'
mapping = object # mapping
reviewee_id = mapping.reviewer.id
reviewed_object_id = mapping.map_id
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = reviewed_object_id
assignment = mapping.assignment
regurl = url_for action: 'add_user_to_assignment',
id: mapping.map_id,
user_id: user.id
end
# Get the assignment's participant corresponding to the user
reviewer = get_reviewer(user, assignment, regurl)
# ACS Removed the if condition(and corressponding else) which differentiate
# assignments as team and individual assignments
# to treat all assignments as team assignments
if response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
response_map.create(reviewee_id: reviewee_id,
reviewer_id: reviewer.id,
reviewed_object_id: reviewed_object_id)
else
case params[:object]
when 'reviewer'
raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
when 'metareviewer'
raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
end
end
rescue
msg = $ERROR_INFO
end
end

== Refactor delete_reviewer ==

def delete_reviewer
case params[:object]
when 'reviewer'
response_map = ReviewResponseMap
when 'metareviewer'
response_map = MetareviewResponseMap
end
# duplicate code
mapping = response_map.find(params[:id])
assignment_id = mapping.assignment.id
case params[:object]
when 'reviewer'
if !Response.exists?(map_id: mapping.id)
mapping.destroy
flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
else
flash[:error] = "This review has already been done. It cannot been deleted."
end
when 'metareviewer'
flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
begin
mapping.delete
rescue
flash[:error] = "A delete action failed: "
+ $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
+ mapping.map_id.to_s + "'>Delete this mapping anyway>?"
end
end
redirect_to action: 'list_mappings', id: assignment_id
end

== Refactor delete_all_reviewer ==

def delete_all_reviewers
mapping = ResponseMap.find(params[:id])
case params[:object]
when 'all_metareviewers'
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = mapping.map_id
end
mmappings = response_map.where(id_symbol => id)

failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
if failedCount > 0
url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
flash[:error] = "A delete action failed: #{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a> "
else
flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
end
redirect_to action: 'list_mappings', id: mapping.assignment.id
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:20:05Z

Gyu9: /* Refactor add_reviewer */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor add_reviewer ==
def add_reviewer
msg = ''
case params[:object]
when 'reviewer'
topic_id = params[:topic_id]
user_id = User.where(name: params[:user][:name]).first.id
# If instructor want to assign one student to review his/her own artifact,
# it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
flash[:error] = "You cannot assign this student to review his/her own artifact."
else
# Team lazy initialization
assignment = Assignment.find(params[:id])
assignment_id = assignment.id
SignUpSheet.signup_team(assignment.id, user_id, topic_id)
create_reviewer(assignment)
end
when 'metareviewer'
mapping = ResponseMap.find(params[:id])
assignment_id = mapping.assignment.id
create_reviewer(mapping)
end
redirect_to action: 'list_mappings', id: assignment_id, msg: msg
end

def create_reviewer(object)
begin
user = User.from_params(params)
# contributor_id is team_id
case params[:object]
when 'reviewer'
assignment = object # assignment
reviewee_id = params[:contributor_id]
reviewed_object_id = assignment.id
response_map = ReviewResponseMap
id_symbol = :reviewee_id
id = reviewee_id
assignment = assignment
regurl = url_for action: 'add_user_to_assignment',
id: assignment.id,
user_id: user.id,
contributor_id: params[:contributor_id]
when 'metareviewer'
mapping = object # mapping
reviewee_id = mapping.reviewer.id
reviewed_object_id = mapping.map_id
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = reviewed_object_id
assignment = mapping.assignment
regurl = url_for action: 'add_user_to_assignment',
id: mapping.map_id,
user_id: user.id
end
# Get the assignment's participant corresponding to the user
reviewer = get_reviewer(user, assignment, regurl)
# ACS Removed the if condition(and corressponding else) which differentiate
# assignments as team and individual assignments
# to treat all assignments as team assignments
if response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
response_map.create(reviewee_id: reviewee_id,
reviewer_id: reviewer.id,
reviewed_object_id: reviewed_object_id)
else
case params[:object]
when 'reviewer'
raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
when 'metareviewer'
raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
end
end
rescue
msg = $ERROR_INFO
end
end

== Refactor delete_reviewer ==

def delete_reviewer
case params[:object]
when 'reviewer'
response_map = ReviewResponseMap
when 'metareviewer'
response_map = MetareviewResponseMap
end
# duplicate code
mapping = response_map.find(params[:id])
assignment_id = mapping.assignment.id
case params[:object]
when 'reviewer'
if !Response.exists?(map_id: mapping.id)
mapping.destroy
flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
else
flash[:error] = "This review has already been done. It cannot been deleted."
end
when 'metareviewer'
flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
begin
mapping.delete
rescue
flash[:error] = "A delete action failed: "
+ $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
+ mapping.map_id.to_s + "'>Delete this mapping anyway>?"
end
end
redirect_to action: 'list_mappings', id: assignment_id
end

== Refactor delete_all_reviewer ==

def delete_all_reviewers
mapping = ResponseMap.find(params[:id])
case params[:object]
when 'all_metareviewers'
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = mapping.map_id
end
mmappings = response_map.where(id_symbol => id)

failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
if failedCount > 0
url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
flash[:error] = "A delete action failed: #{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a> "
else
flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
end
redirect_to action: 'list_mappings', id: mapping.assignment.id
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:19:28Z

Gyu9: /* Refactor add_reviewer */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor add_reviewer ==
def add_reviewer
msg = ''
case params[:object]
when 'reviewer'
topic_id = params[:topic_id]
user_id = User.where(name: params[:user][:name]).first.id
# If instructor want to assign one student to review his/her own artifact,
# it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
flash[:error] = "You cannot assign this student to review his/her own artifact."
else
# Team lazy initialization
assignment = Assignment.find(params[:id])
assignment_id = assignment.id
SignUpSheet.signup_team(assignment.id, user_id, topic_id)
create_reviewer(assignment)
end
when 'metareviewer'
mapping = ResponseMap.find(params[:id])
assignment_id = mapping.assignment.id
create_reviewer(mapping)
end
redirect_to action: 'list_mappings', id: assignment_id, msg: msg
end
def create_reviewer(object)
begin
user = User.from_params(params)
# contributor_id is team_id
case params[:object]
when 'reviewer'
assignment = object # assignment
reviewee_id = params[:contributor_id]
reviewed_object_id = assignment.id
response_map = ReviewResponseMap
id_symbol = :reviewee_id
id = reviewee_id
assignment = assignment
regurl = url_for action: 'add_user_to_assignment',
id: assignment.id,
user_id: user.id,
contributor_id: params[:contributor_id]
when 'metareviewer'
mapping = object # mapping
reviewee_id = mapping.reviewer.id
reviewed_object_id = mapping.map_id
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = reviewed_object_id
assignment = mapping.assignment
regurl = url_for action: 'add_user_to_assignment',
id: mapping.map_id,
user_id: user.id
end
# Get the assignment's participant corresponding to the user
reviewer = get_reviewer(user, assignment, regurl)
# ACS Removed the if condition(and corressponding else) which differentiate
# assignments as team and individual assignments
# to treat all assignments as team assignments
if response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
response_map.create(reviewee_id: reviewee_id,
reviewer_id: reviewer.id,
reviewed_object_id: reviewed_object_id)
else
case params[:object]
when 'reviewer'
raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
when 'metareviewer'
raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
end
end
rescue
msg = $ERROR_INFO
end
end

== Refactor delete_reviewer ==

def delete_reviewer
case params[:object]
when 'reviewer'
response_map = ReviewResponseMap
when 'metareviewer'
response_map = MetareviewResponseMap
end
# duplicate code
mapping = response_map.find(params[:id])
assignment_id = mapping.assignment.id
case params[:object]
when 'reviewer'
if !Response.exists?(map_id: mapping.id)
mapping.destroy
flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
else
flash[:error] = "This review has already been done. It cannot been deleted."
end
when 'metareviewer'
flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
begin
mapping.delete
rescue
flash[:error] = "A delete action failed: "
+ $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
+ mapping.map_id.to_s + "'>Delete this mapping anyway>?"
end
end
redirect_to action: 'list_mappings', id: assignment_id
end

== Refactor delete_all_reviewer ==

def delete_all_reviewers
mapping = ResponseMap.find(params[:id])
case params[:object]
when 'all_metareviewers'
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = mapping.map_id
end
mmappings = response_map.where(id_symbol => id)

failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
if failedCount > 0
url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
flash[:error] = "A delete action failed: #{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a> "
else
flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
end
redirect_to action: 'list_mappings', id: mapping.assignment.id
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:18:21Z

Gyu9: /* Refactor */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor add_reviewer ==

def add_reviewer
msg = ''
case params[:object]
when 'reviewer'
topic_id = params[:topic_id]
user_id = User.where(name: params[:user][:name]).first.id
# If instructor want to assign one student to review his/her own artifact,
# it should be counted as “self-review” and we need to make /app/views/submitted_content/_selfreview.html.erb work.
if TeamsUser.exists?(team_id: params[:contributor_id], user_id: user_id)
flash[:error] = "You cannot assign this student to review his/her own artifact."
else
# Team lazy initialization
assignment = Assignment.find(params[:id])
assignment_id = assignment.id
SignUpSheet.signup_team(assignment.id, user_id, topic_id)

create_reviewer(assignment)
end
when 'metareviewer'
mapping = ResponseMap.find(params[:id])
assignment_id = mapping.assignment.id
create_reviewer(mapping)
end
redirect_to action: 'list_mappings', id: assignment_id, msg: msg
end

def create_reviewer(object)
begin
user = User.from_params(params)
# contributor_id is team_id
case params[:object]
when 'reviewer'
assignment = object # assignment
reviewee_id = params[:contributor_id]
reviewed_object_id = assignment.id
response_map = ReviewResponseMap
id_symbol = :reviewee_id
id = reviewee_id
assignment = assignment
regurl = url_for action: 'add_user_to_assignment',
id: assignment.id,
user_id: user.id,
contributor_id: params[:contributor_id]
when 'metareviewer'
mapping = object # mapping
reviewee_id = mapping.reviewer.id
reviewed_object_id = mapping.map_id
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = reviewed_object_id
assignment = mapping.assignment
regurl = url_for action: 'add_user_to_assignment',
id: mapping.map_id,
user_id: user.id
end
# Get the assignment's participant corresponding to the user
reviewer = get_reviewer(user, assignment, regurl)
# ACS Removed the if condition(and corressponding else) which differentiate
# assignments as team and individual assignments
# to treat all assignments as team assignments
if response_map.where(id_symbol => id,reviewer_id: reviewer.id).first.nil?
response_map.create(reviewee_id: reviewee_id,
reviewer_id: reviewer.id,
reviewed_object_id: reviewed_object_id)
else
case params[:object]
when 'reviewer'
raise "The reviewer, \"" + reviewer.name + "\", is already assigned to this contributor."
when 'metareviewer'
raise "The metareviewer \"" + reviewer.user.name + "\" is already assigned to this reviewer."
end
end
rescue
msg = $ERROR_INFO
end
end

== Refactor delete_reviewer ==

def delete_reviewer
case params[:object]
when 'reviewer'
response_map = ReviewResponseMap
when 'metareviewer'
response_map = MetareviewResponseMap
end
# duplicate code
mapping = response_map.find(params[:id])
assignment_id = mapping.assignment.id
case params[:object]
when 'reviewer'
if !Response.exists?(map_id: mapping.id)
mapping.destroy
flash[:success] = "The review mapping for \"" + mapping.reviewee.name + "\" and \"" + mapping.reviewer.name + "\" has been deleted."
else
flash[:error] = "This review has already been done. It cannot been deleted."
end
when 'metareviewer'
flash[:note] = "The metareview mapping for " + mapping.reviewee.name + " and " + mapping.reviewer.name + " has been deleted."
begin
mapping.delete
rescue
flash[:error] = "A delete action failed: "
+ $ERROR_INFO + "<a href='/review_mapping/delete_metareview/"
+ mapping.map_id.to_s + "'>Delete this mapping anyway>?"
end
end
redirect_to action: 'list_mappings', id: assignment_id
end

== Refactor delete_all_reviewer ==

def delete_all_reviewers
mapping = ResponseMap.find(params[:id])
case params[:object]
when 'all_metareviewers'
response_map = MetareviewResponseMap
id_symbol = :reviewed_object_id
id = mapping.map_id
end
mmappings = response_map.where(id_symbol => id)

failedCount = ResponseMap.delete_mappings(mmappings, params[:force])
if failedCount > 0
url_yes = url_for action: 'delete_all_reviewers', id: mapping.map_id, force: 1
url_no = url_for action: 'delete_all_reviewers', id: mapping.map_id
flash[:error] = "A delete action failed: #{failedCount} metareviews exist for these mappings. Delete these mappings anyway? <a href='#{url_yes}'>Yes</a> | <a href='#{url_no}'>No</a> "
else
flash[:note] = "All metareview mappings for contributor \"" + mapping.reviewee.name + "\" and reviewer \"" + mapping.reviewer.name + "\" have been deleted."
end
redirect_to action: 'list_mappings', id: mapping.assignment.id
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:12:59Z

Gyu9:

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

== Refactor==

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:11:05Z

Gyu9: /* Changes Implemented */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer. Based on the TA's requirement, we implemented this part on the other GitHub pull request. https://github.com/expertiza/expertiza/pull/836
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:08:19Z

Gyu9: /* Test */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer.
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

=Test=
# We test Refactor of add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer and delete_all_reviewer both from UI and Rspec. Here is a link of the UI test.https://www.youtube.com/watch?v=c9UdTnwlyvM. Also, you can check the code of this pull request. https://github.com/expertiza/expertiza/pull/836
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T07:01:39Z

Gyu9: /* Changes Implemented */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=
# Merged add_reviewer and add_metareviewer to add_reviewer. Merged delete_reviewer and delete_metareviewer to delete_reviewer. Modified delete_all_metareviewer to delete_all_reviewer.
# Change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

=Test=
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-11-05T06:39:40Z

Gyu9: /* Current Implementation and Problems */

=Background=

This controller sets up mappings between reviewer and reviewee. It handles all different types of response maps (review response map, author feedback response map, teammate review response map, meta review response map and quiz response map). The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes.

=Current Implementation and Problems=
The controller has many methods and involve with many other controllers and views, the code is long and complicated. Some of the methods in this controller have unreasonable name associated with their functions, some methods are too long, some methods haven't been used by any other methods or views. Our work is to refactor these problems and make the code more beautiful.

The problems are:
# The class has functionality for five different kinds of Responses: reviews, metareviews, teammate reviews, author feedback (“rejoinders”), and quizzes. Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method ''response_report'' has some SQL - like code. Rewrite with Active Record.
# Test whether method ''add_user_to_assignment'' is used. There is no way that this method should be in ReviewMappingController. Please remove this method and caller.
# There was a ''self-review'' feature, the method ''add_self_reviewer'', ''get_team_from_submission'' are related to this. Two views calls ''add_self_reviewer'' are "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". The names of views are not related to ''self_review'' feature. Plus those two views are not called anywhere. Please verify this and if so, you should delete those two views, two method and also related records (e.g. in routes.rb).
# Method ''delete_all_reviewers'' actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named ''delete_outstanding_reviewers''. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method ''release_reservation'' should be renamed as'' release_mapping''. In addition, delete it if you find this method is not called anywhere.
# Method ''delete_mappings'' is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method'' automatic_review_mapping_strategy'' is too long. Please refactor and test it.

=Changes Implemented=

# change the rails query from sql like code, like ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id]) to more rails quey like ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)
# We search the function name(''add_user_to_assignment'') in the whole files, and find that this method is invoked in "participants_helper.rb", and also being invoked in the function of ''add_reviewer'' and ''add_metareviewer'' in the controller of ''review_mapping_controller''
# By searching the whole project and routes, we verify that methods ''add_self_reviewer'' and ''get_team_from_submission'' in this controller are not called by any other methods except for views "show_available_submissions_for_quizzes.html.erb" and "show_available_submissions.html.erb". And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method ''delete_all_reviewers'' to ''delete_outstanding_reviewers'' and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods'' release_reservation'' in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods ''delete_mappings'' in this controller are not used in anywhere.
# Method ''automatic_review_mapping_strategy'' have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

=Code=

==Refactor SQL query code==

The rails query we change in this controller:

1. From

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ', params[:contributor_id], reviewer.id])

To

ReviewResponseMap.where(reviewee_id: params[:contributor_id],reviewer_id: reviewer.id)

2. From

MetareviewResponseMap.where(['reviewed_object_id = ? and reviewer_id = ?', mapping.map_id, reviewer.id])

To

MetareviewResponseMap.where(reviewed_object_id: mapping.map_id, reviewer_id: reviewer.id)

3. From

@assignments = Assignment
.where(["instructor_id = ? and substring(name,1,1) = ?", session[:user].id, letter])
.order('name')
.page(params[:page])
.per_page(10)

To

@assignments = Assignment
.where(instructor_id:session[:user].id)
.where("substring(name,1,1) = :letter",{letter:letter})
.order('name')
.page(params[:page])
.per_page(10)

4. From

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", assignment_id, 1])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id, calibrate_to: 1)

5. From

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", assignment_id, @@time_create_last_review_mapping_record, 0])

To

ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0).where("created_at > :time",{time:@@time_create_last_review_mapping_record})

6. From

AssignmentQuestionnaire.where(["assignment_id = ? and questionnaire_id IN (?)", params[:id], @review_questionnaire_ids])

To

AssignmentQuestionnaire.where(assignment_id: params[:id],:questionnaire_id => @review_questionnaire_ids)

7. From

SelfReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ?', team_id[0].t_id, params[:reviewer_id]])

To

SelfReviewResponseMap.where(reviewee_id:team_id[0].t_id,reviewer_id:params[:reviewer_id])

==Refactor long line method==

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(reviewed_object_id:assignment_id,calibrate_to:0)
.where("created_at > :time",
{time:@@time_create_last_review_mapping_record}).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(reviewed_object_id:assignment_id,
calibrate_to:0).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

=Test=
# We test delete_outstanding_reviewers refactor from the UI. Here is a video for this test. https://youtu.be/MZxOaSsm58E
# We use some data to test the result of old query and new query in rails console

ReviewResponseMap.where(['reviewee_id = ? and reviewer_id = ? ‘,6050, 9])
ReviewResponseMap.where(reviewee_id: 6050,reviewer_id: 9)

Assignment.where(["instructor_id = ? and substring(name,1,1) = ?", 2, ‘TestAssign2’]).order('name')
Assignment .where(instructor_id:2).where("substring(name,1,1) = :letter",{letter:’TestAssign2’}).order('name')

ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", 13, nil, 0])
ReviewResponseMap.where(reviewed_object_id:13,calibrate_to:0).where("created_at > :time",{time:nil})

# We write RSpec feature tests to prove that all changes are correct and the project performs well as before. All tests have passed.
Automatic_review_mapping method

The function of method automatic_review_mapping is to automatically assign reviews on students in teams when instructor set either student review number or submissions review number. There is constraint that only one of the two numbers must be set value other than 0 (that is, one is 0 and other is not 0). To test the correctness of our refactoring, we design a scenario and 4 cases of assigning reviews to students:

Case 1. Instructor has not set both students review number and submissions review number (They are both 0). The page will show a notice.

Case 2. Instructor set both numbers (They are both not 0). The page will show a notice.

Case 3. Instructors set students review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign student review number to 2, and there will be 20 peer reviews in total and need to allocate to 3 teams. So each team get 7 reviews on average and 1 team’s artifact will be reviewed 6 times.

Case 4. Instructors set submissions review number. The controller will change database: create new relationship in ReviewResponseMap. For example, if assign submission review number to 3, and there will be 21 peer reviews in total to allocate to 10 participants. So every participants get 2 reviews. 9 among 10 participants will review 2 teams’ artifacts and 1 participant will review 3 teams’ artifacts.

require 'rails_helper'
describe "review mapping", js: true do
before(:each) do
.#create test data
end

it "show error when assign both 0" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 0
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 0
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student)')
end

it "show error when assign both numbers" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 1
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 1
click_button 'second_submit_tag'
#click_button 'Save'
expect(page).to have_content('Please choose either the number of reviews per student or the number of reviewers per team (student), not both')
end

it "calculate reviewmapping from given review number per student" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
fill_in 'num_reviews_per_student', with: 2
click_button 'first_submit_tag'
num = ReviewResponseMap.where(reviewee_id: @team1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(7)
num2 = ReviewResponseMap.where(reviewee_id: @team3.id, reviewed_object_id: @assignment.id).count
expect(num2).to eq(6)
end

it "calculate reviewmapping from given review number per submission" do
login_as("instructor6")
visit '/assignments/1/edit'
find_link('ReviewStrategy').click
select "Instructor-Selected", from: 'assignment_form_assignment_review_assignment_strategy'
choose 'num_reviews_submission'
fill_in 'num_reviews_per_submission', with: 7
click_button 'second_submit_tag'
#click_button 'Save'
num = ReviewResponseMap.where(reviewer_id: @teamuser1.id, reviewed_object_id: @assignment.id).count
expect(num).to eq(2)
end
end

Refactoring add_reviewer, add_metareviewer, delete_reviewer, delete_metareviewer, delete_outstanding_reviewers and delete_all_metareviewers
Since we merge methods have similar functions, we need to test their functionality. Here is a feature test work flow include those refactored functions.
it "can add reviewer then delete it" do

@student_reviewer = create :student,name:'student_reviewer'
@participant_reviewer = create :participant, assignment: @assignment, user: @student_reviewer
@student_reviewer2 = create :student,name:'student_reviewer2'
@participant_reviewer2 = create :participant, assignment: @assignment, user: @student_reviewer2
login_and_assign_reviewer("instructor6",@assignment.id,0,0)

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
expect(page).to have_content @student_reviewer.name
click_link('delete')
expect(page).to have_content ("The review mapping for \"#{@team1.name}\" and \"#{@student_reviewer.name}\" has been deleted")

first(:link,'add reviewer').click
add_reviewer(@student_reviewer.name)
click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
expect(page).to have_content @student_reviewer2.name
find(:xpath, "//a[@href='/review_mapping/delete_metareviewer?id=3']").click
expect(page).to have_content ("The metareview mapping for #{@student_reviewer.name} and #{@student_reviewer2.name} has been deleted")

click_link('add metareviewer')
add_matareviewer(@student_reviewer2.name)
click_link('delete all metareviewers')
expect(page).to have_content ("All metareview mappings for contributor \"#{@team1.name}\" and reviewer \"#{@student_reviewer.name}\" have been deleted")

first(:link,'delete outstanding reviewers').click
expect(page).to have_content ("All review mappings for \"#{@team1.name}\" have been deleted")

end

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-30T02:23:41Z

Gyu9: /* Test */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T04:14:41Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?",
assignment_id,
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?",
assignment_id, 0]).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T04:11:40Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?",
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?",
assignment_id, 0]).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id,
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id,
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.
where(reviewed_object_id: assignment_id).
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done
by each student to be greater than or equal to total number of teams
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T03:19:25Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def automatic_review_mapping_strategy(assignment_id,
participants, teams, student_review_num = 0,
submission_review_num = 0)
participants_hash = {}
participants.each {|participant| participants_hash[participant.id] = 0 }
# calculate reviewers for each team
num_participants = participants.size
if student_review_num != 0 and submission_review_num == 0
num_reviews_per_team = (participants.size * student_review_num * 1.0 / teams.size).round
student_review_num = student_review_num
exact_num_of_review_needed = participants.size * student_review_num
elsif student_review_num == 0 and submission_review_num != 0
num_reviews_per_team = submission_review_num
student_review_num = (teams.size * submission_review_num * 1.0 / participants.size).round
exact_num_of_review_needed = teams.size * submission_review_num
end
execute_peer_review_strategy(assignment_id, teams, num_participants,
student_review_num, num_reviews_per_team,
participants, participants_hash)
# after assigning peer reviews for each team,
# if there are still some peer reviewers not obtain enough peer review,
# just assign them to valid teams
assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,
exact_num_of_review_needed)
end

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
assignment_id, 0]).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id, \
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.\
where(reviewed_object_id: assignment_id).\
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done \
by each student to be greater than or equal to total number of teams \
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T03:15:28Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
assignment_id, 0]).each do |response_map|
if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id, \
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.\
where(reviewed_object_id: assignment_id).\
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done \
by each student to be greater than or equal to total number of teams \
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T03:15:02Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed

participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}

ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
assignment_id, 0]).each do |response_map|

if unsorted_teams_hash.key? response_map.reviewee_id
unsorted_teams_hash[response_map.reviewee_id] += 1
else
unsorted_teams_hash[response_map.reviewee_id] = 1
end
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h

participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id, \
user_id: Participant.find(participant_id).user_id)

ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
reviewed_object_id: assignment_id).first_or_create

teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.\
where(reviewed_object_id: assignment_id).\
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done \
by each student to be greater than or equal to total number of teams \
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:59:08Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
assignment_id, 0]).each do |response_map|
unsorted_teams_hash[response_map.reviewee_id] = \
calculate_unsorted_teams_hash_reviewee_id(unsorted_teams_hash, response_map)
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id, \
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.\
where(reviewed_object_id: assignment_id).\
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done \
by each student to be greater than or equal to total number of teams \
[or "participants" if it is an individual assignment].'
end
peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:58:51Z

Gyu9: /* Changes Implemented */

==Background==

==Current Implementation and Problems==
# Method names for dealing with the 5 different kinds of objects should be as similar as possible, and they should share helper functions when possible.
# Method response_report has some sql - like code. Rewrite with Active Record.
# Test whether method add_user_to_assignment is used. There is no way that this method should be in ReviewMappingController.--plz remove this method and caller.
# There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==
#
#
#
# By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
# We already renamed method delete_all_reviewers to delete_outstanding_reviewers and changed the corresponding button name at the corresponding view (_list_review_mappings.html.erb).
# By searching the whole project and routes, we verify that methods release_reservation in this controller are not used in anywhere.
# By searching the whole project and routes, we verify that methods delete_mappings in this controller are not used in anywhere.
# This function have many long lines. We already shorten each long line to multiple lines. And also, we split this long function into three different functions.

def assign_reviewers_for_team(assignment_id,student_review_num,participants_hash,exact_num_of_review_needed)
if ReviewResponseMap.where(["reviewed_object_id = ? and created_at > ? and calibrate_to = ?", \
assignment_id, \
@@time_create_last_review_mapping_record, 0]).size < exact_num_of_review_needed
participants_with_insufficient_review_num = []
participants_hash.each do |participant_id, review_num|
participants_with_insufficient_review_num << participant_id if review_num < student_review_num
end
unsorted_teams_hash = {}
ReviewResponseMap.where(["reviewed_object_id = ? and calibrate_to = ?", \
assignment_id, 0]).each do |response_map|
unsorted_teams_hash[response_map.reviewee_id] = \
calculate_unsorted_teams_hash_reviewee_id(unsorted_teams_hash, response_map)
end
teams_hash = unsorted_teams_hash.sort_by {|_, v| v }.to_h
participants_with_insufficient_review_num.each do |participant_id|
teams_hash.each do |team_id, _num_review_received|
next if TeamsUser.exists?(team_id: team_id, \
user_id: Participant.find(participant_id).user_id)
ReviewResponseMap.where(reviewee_id: team_id, reviewer_id: participant_id, \
reviewed_object_id: assignment_id).first_or_create
teams_hash[team_id] += 1
teams_hash = teams_hash.sort_by {|_, v| v }.to_h
break
end
end
end
@@time_create_last_review_mapping_record = ReviewResponseMap.\
where(reviewed_object_id: assignment_id).\
last.created_at
end

def execute_peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
if student_review_num >= teams.size
flash[:error] = 'You cannot set the number of reviews done \
by each student to be greater than or equal to total number of teams \
[or "participants" if it is an individual assignment].'
end

peer_review_strategy(assignment_id, teams, num_participants, \
student_review_num, num_reviews_per_team, \
participants, participants_hash)
end

==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:44:33Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:42:04Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:40:24Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:39:05Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:38:41Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:37:16Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:33:24Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:31:18Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:24:47Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:04:11Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T02:03:33Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T01:55:40Z

Gyu9: /* Changes Implemented */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T01:48:46Z

Gyu9: /* Current Implementation and Problems */

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T01:47:46Z

Gyu9: /* Current Implementation and Problems */

==Background==

==Current Implementation and Problems==
4. There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
# Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
# Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
# Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
# Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==

4. By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
==Test==

CSC/ECE 517 Fall 2016/E1641. Refactor review mapping controller.rb

2016-10-29T01:44:04Z

Gyu9: /* Current Implementation and Problems */

==Background==

==Current Implementation and Problems==
4. There was a self-review feature, the method add_self_reviewr, get_team_from_submission are related to this. Two views calls add_self_reviewer are ç. The names of views are not related to self_review feature. Plus those two views are not called anywhere.
5. Method delete_all_reviewers actually only deletes the outstanding review response maps (the ones which has been initiated, but there is no response yet). So it should better be named delete_outstanding_reviewers. You can try to test this method by clicking “Assign reviewers” icon on an assignment.
6. Method release_reservation should be renamed as release_mapping. In addition, delete it if you find this method is not called anywhere.
7. Method delete_mappings is problematic. It does not looks like a controller method. Please refactor it or delete it if you can validate that this method is not called anywhere.
8. Method automatic_review_mapping_strategy is too long. Please refactor and test it.

==Changes Implemented==

4. By searching the whole project and routes, we verify that methods add_self_reviewr and get_team_from_submission in this controller are not called by any other methods except for views add_self_reviewr, get_team_from_submission. And those two views are not linked to any other views. So they are deleted from the project.
==Test==

CSC/ECE 517 Fall 2016

2016-10-29T01:24:46Z

Gyu9: /* Writing Assignments 2 */

CSC/ECE 517 Fall 2016

2016-10-29T01:18:03Z

Gyu9: /* Writing Assignments 2 */