Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-12-15T01:31:05Z

Jdeng8: /* Performance improvement */

== '''Expertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.

== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs nearly 9 min to run all tests.
[[File:Expertiza test time.png]]
One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to support all RSpec test without creating new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== Refactor files==
test database

Rspec tests

== '''Design''' ==
=== Design idea explanation ===
[[File: Design explanation.png]]

In this design, we create a new database for testing, and we save some related information in this database. Every test just need to fetch data from this test database, and after the testing, we need to add some operation to rollback the data. For example, if the test is about creating a new quiz, in this situation, we need to delete this quiz after this test. Because of it, the state of database can be saved, and also it will not influence other test or test this test case again.

=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.

We will try two method to achieve the goal:
# Write a script to create all test data automatically then don't clean the database.
# We transfer the data in girls_factory into the data base, and there are some steps:
1. we delete some related information in Rails_hepler.rb

config.before(:suite) do
DatabaseCleaner.clean_with(:truncation)
end
config.before(:each) do |_example|
DatabaseCleaner.strategy = :truncation
DatabaseCleaner.start
end

config.after(:each) do
DatabaseCleaner.clean
end
Those statements is used for adding the technology to clean the database before everytime using Rspec test, and then also clean the database(using turncate to increase the time of cleaning database).
2. We generate a new create rb file, and in this file we just put the create statements in Rspec style function, like create(:deadline_type, name: "submission") to transfer the data into database when running rspec create.rb

=== Feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests must create their own data before each test. For an example, now a test first create a user before it can do the login operation. The creation latency is considerable. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database. This change will not only accelerate the testing time, but also dry out the code.

The way tests reference data from database is the same as it does in the development mode. To login, or do other operations, the test can invoke existing user data in test database. Using "User.find()" or "User.where()", test data can easily be invoked by tests.

For existing tests, they can directly use the test database. For new tests added later, they can use the records in test database or create specialized data use FactoryGirl. So to keep the test database up to date ,it needs to be maintained regularly.

== ''' Modify feature test''' ==

With test data in the database, we modified the RSpec feature test files:
1. Disable the data clean statement in rails_helper.rb.
config.before(:suite) do
DatabaseCleaner.clean_with(:truncation)
end
config.before(:each) do |_example|
DatabaseCleaner.strategy = :truncation
DatabaseCleaner.start
end

config.after(:each) do
DatabaseCleaner.clean
end

2. Eliminate data creation statement in each test case.
Delete” create(:assignment)” like code

3. Modify url visit statement.
Modify “visit '/student_teams/view?student_id=1'”
to
@student=User.find_by(name:”student2064”)
“visit '/student_teams/view?student_id=@student.id’”

4. Recover test database before or after each change data operation
For tests involved with create or delete actions, the data modified should be rollback.

== ''' The file we need to edit''' ==

airbrake_expection_errors_feature_tests_spec.rb

assignment_creation_spec.rb

calibration_spec.rb

inherit_teams_display_spec.rb

instructor_interface_spec.rb

list_teams_spec.rb

questionnaire_spec.rb

review_mapping_spec.rb

staggered_deadline_spec.rb

topic_suggestion_spec.rb

==Performance improvement==
We run a single rspec test "airbrake_expection_errors_feature_tests_spec.rb" to test the performance improvement when using test database.

Without a test database, the feature test costs 1:37 minutes:
[[File:After refactor.png]]
Using the test database, the same test costs 1:10 minutes:
[[File:Before refactor.png]]
The time is decreased by about 20%. Consider the browser's response time in both trial, the improvement proportion should be higher.

Here is a video to show the improvement:
https://www.youtube.com/watch?v=9y0i-zSRHe8&feature=youtu.be

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-12-15T01:29:55Z

Jdeng8:

== '''Expertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.

== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs nearly 9 min to run all tests.
[[File:Expertiza test time.png]]
One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to support all RSpec test without creating new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== Refactor files==
test database

Rspec tests

== '''Design''' ==
=== Design idea explanation ===
[[File: Design explanation.png]]

In this design, we create a new database for testing, and we save some related information in this database. Every test just need to fetch data from this test database, and after the testing, we need to add some operation to rollback the data. For example, if the test is about creating a new quiz, in this situation, we need to delete this quiz after this test. Because of it, the state of database can be saved, and also it will not influence other test or test this test case again.

=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.

We will try two method to achieve the goal:
# Write a script to create all test data automatically then don't clean the database.
# We transfer the data in girls_factory into the data base, and there are some steps:
1. we delete some related information in Rails_hepler.rb

config.before(:suite) do
DatabaseCleaner.clean_with(:truncation)
end
config.before(:each) do |_example|
DatabaseCleaner.strategy = :truncation
DatabaseCleaner.start
end

config.after(:each) do
DatabaseCleaner.clean
end
Those statements is used for adding the technology to clean the database before everytime using Rspec test, and then also clean the database(using turncate to increase the time of cleaning database).
2. We generate a new create rb file, and in this file we just put the create statements in Rspec style function, like create(:deadline_type, name: "submission") to transfer the data into database when running rspec create.rb

=== Feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests must create their own data before each test. For an example, now a test first create a user before it can do the login operation. The creation latency is considerable. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database. This change will not only accelerate the testing time, but also dry out the code.

The way tests reference data from database is the same as it does in the development mode. To login, or do other operations, the test can invoke existing user data in test database. Using "User.find()" or "User.where()", test data can easily be invoked by tests.

For existing tests, they can directly use the test database. For new tests added later, they can use the records in test database or create specialized data use FactoryGirl. So to keep the test database up to date ,it needs to be maintained regularly.

== ''' Modify feature test''' ==

With test data in the database, we modified the RSpec feature test files:
1. Disable the data clean statement in rails_helper.rb.
config.before(:suite) do
DatabaseCleaner.clean_with(:truncation)
end
config.before(:each) do |_example|
DatabaseCleaner.strategy = :truncation
DatabaseCleaner.start
end

config.after(:each) do
DatabaseCleaner.clean
end

2. Eliminate data creation statement in each test case.
Delete” create(:assignment)” like code

3. Modify url visit statement.
Modify “visit '/student_teams/view?student_id=1'”
to
@student=User.find_by(name:”student2064”)
“visit '/student_teams/view?student_id=@student.id’”

4. Recover test database before or after each change data operation
For tests involved with create or delete actions, the data modified should be rollback.

== ''' The file we need to edit''' ==

airbrake_expection_errors_feature_tests_spec.rb

assignment_creation_spec.rb

calibration_spec.rb

inherit_teams_display_spec.rb

instructor_interface_spec.rb

list_teams_spec.rb

questionnaire_spec.rb

review_mapping_spec.rb

staggered_deadline_spec.rb

topic_suggestion_spec.rb

==Performance improvement==
We run a single rspec test "airbrake_expection_errors_feature_tests_spec.rb" to test the performance improvement when using test database.

Without a test database, the feature test costs 1:37 minutes:
[[File:After refactor.png]]
Using the test database, the same test costs 1:10 minutes:
[[File:Before refactor.png]]
The time is decreased by about 20%. Consider the browser's response time in both trial, the improvement

Here is a video to show the improvement:
https://www.youtube.com/watch?v=9y0i-zSRHe8&feature=youtu.be

File:After refactor.png

2016-12-15T01:20:44Z

Jdeng8:

File:Before refactor.png

2016-12-15T01:20:24Z

Jdeng8:

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:56:44Z

Jdeng8: /* Problem Statement */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:54:38Z

Jdeng8: /* Database design */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:54:19Z

Jdeng8: /* Database design */

File:Expertiza test time.png

2016-11-14T18:46:27Z

Jdeng8: uploaded a new version of "File:Expertiza test time.png"

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:44:04Z

Jdeng8: /* Problem Statement */

File:Expertiza test time.png

2016-11-14T18:43:19Z

Jdeng8: uploaded a new version of "File:Expertiza test time.png"

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:39:31Z

Jdeng8: /* Problem Statement */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:36:12Z

Jdeng8: /* Feature test design */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:35:13Z

Jdeng8: /* Feature test design */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:32:43Z

Jdeng8: /* ""Refactor files"" */

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:32:26Z

Jdeng8:

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-14T18:28:10Z

Jdeng8: /* Problem Statement */

File:Expertiza test time.png

2016-11-11T18:52:43Z

Jdeng8:

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-09T01:12:27Z

Jdeng8: /* Feature test design */

== '''Eexpertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.

One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== '''Design''' ==
=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.
INSERT INTO User VALUES (xxxxx, xxxxxx, xxxxx, xxxxxxxx, xxxxxxx, xxxxxxx, xxxx );

=== Feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests must create their own data before each test. For an example, now a test first create a user before it can do the login operation. The creation latency is considerable. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database. This change will not only accelerate the testing time, but also dry out the code.

The way tests reference data from database is the same as it does in the development mode. To login, or do other operations, the test can invoke existing user data in test database. Using "User.find()" or "User.where()", test data can easily be invoked by tests.

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-09T01:10:42Z

Jdeng8: /* Feature test design */

== '''Eexpertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.

One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== '''Design''' ==
=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.
INSERT INTO User VALUES (xxxxx, xxxxxx, xxxxx, xxxxxxxx, xxxxxxx, xxxxxxx, xxxx );

=== Feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests must create their own data before each test. For an example, now a test first create a user before it can do the login operation. The creation latency is considerable. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database.

The way tests reference data from database is the same as it does in the development mode. To login, or do other operations, the test can invoke existing user data in test database. Using "User.find()" or "User.where()", test data can easily be invoked by tests.

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-09T01:09:58Z

Jdeng8: /* feature test design */

== '''Eexpertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.

One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== '''Design''' ==
=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.
INSERT INTO User VALUES (xxxxx, xxxxxx, xxxxx, xxxxxxxx, xxxxxxx, xxxxxxx, xxxx );

=== Feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests must create their own data before each test. For an example, now a test first create a user before it can do the login operation. The creation latency is considerable. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database.

The way tests reference data from database is the same as it does in the development mode. To login, or do other operations, the test can just invoke existing user data in test database. Using "User.find()" or "User.where()", test data can easily be invoked by tests.

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-08T23:38:22Z

Jdeng8: /* feature test design */

== '''Eexpertiza''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
== '''Rspec''' ==
Rspec is a meta-gem, which depends on the rspec-core, rspec-expectations and rspec-mocks gems. Each of these can be installed separately and loaded in isolation using require. Among other benefits, this allows you to use rspec-expectations, for example, in Test::Unit::TestCase if you happen to prefer that style.
Conversely, if you like RSpec's approach to declaring example groups and examples (describe and it) but prefer Test::Unit assertions and mocha, rr or flexmock for mocking, you'll be able to do that without having to install or load the components of RSpec that you're not using.
== '''Problem Statement''' ==
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.

One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.

== '''Task''' ==
Formally, we need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.
To finish these tasks, we need to modify all RSpec test files in Expertiza and using a new Database:Expertiza_test.

== '''Design''' ==
=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
like the Rspec statements in creating some User objects.:
factory :admin, class: User do
sequence(:name) {|n| "admin#{n}" }
role { Role.where(name: 'Administrator').first || association(:role_of_administrator) }
password "password"
password_confirmation "password"
sequence(:fullname) {|n| "#{n}, administrator" }
email "expertiza@mailinator.com"
parent_id 1
private_by_default false
mru_directory_path nil
email_on_review true
email_on_submission true
email_on_review_of_review true
is_new_user false
master_permission_granted 0
handle "handle"
leaderboard_privacy false
digital_certificate nil
timezonepref nil
public_key nil
copy_of_emails false
end

then transfer this into SQL query and then add the information the same as designed for Rspec test into expertiza_test database.
INSERT INTO User VALUES (xxxxx, xxxxxx, xxxxx, xxxxxxxx, xxxxxxx, xxxxxxx, xxxx );

=== feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests have to create their own data before each test. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database.

The way tests reference data from database is the same as it does in project. For an example, if a test want to use "instructor6" to login, now the test first create a user named "instruc"

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-08T22:08:23Z

Jdeng8: /* Task */

== '''Eexpertiza''' ==
== '''Rspec''' ==
== '''Problem Statement''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.
One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.
== '''Related Work''' ==
* Classes involved: All RSpec test files in Expertiza.
* Database involved: Expertiza_test

== '''Task''' ==
Formally, you need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type)).
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to Expertiza.

== '''Design''' ==
=== Database design ===
We design a expertiza_test database to save the test date used for Rspec. The test database owns the same structure as the real expertiza database, including the relations between tables and some restriction of attribute like it cannot be null or some other requirements for different attributes. Besides, the data in test database is the same as the data in factories part in spec, which includes FeedbackResponseMap.rb, Respone.rb, factories.rb, quiz_factory.rb. In this situation we do not need to create some data before testing, and we can use the data in test DB directly. Because of it, the overhead of testing cases will experience obvious decreasing.
=== feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests have to create their own data before each test. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database.

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-08T22:06:27Z

Jdeng8: /* factories design */

== '''Eexpertiza''' ==
== '''Rspec''' ==
== '''Problem Statement''' ==
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). It is used in select courses at NC State and by professors at several other colleges and universities.
Unfortunately, Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.
One reason is that we use fixture to create records in test DB each time running tests.

For example, the codes in quiz_spec.rb contain too much create so that prolong testing time.
<pre>
# Create an assignment due date
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, due_at: (DateTime.now + 1)
</pre>

One solution is building an complete database to one time to support all RSpec test without create new records.
== '''Related Work''' ==
* Classes involved: All RSpec test files in Expertiza.
* Database involved: Expertiza_test

== '''Task''' ==
Formally, you need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type))
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to expertiza.

== '''Design''' ==
=== Database design ===
=== feature test design ===
To accelerate the feature test, we have to eliminate pseudo data creation. Now all the feature tests have to create their own data before each test. After building a test database, all the pseudo data creation statement can be removed from the test files. Instead, the tests reference data stored in the test database.

CSC/ECE 517 Fall 2016/E1701. Accelerate RSpec testing

2016-11-08T21:44:12Z

Jdeng8: /* E1701. Accelerate RSpec testing */

= Problem Statement =
Expertiza tests are really slow. If you check the TravisCI, it needs more than 9 min to run all tests.
One reason is that we use fixture to create records in test DB each time running tests.

= Related Work =
* Classes involved: All RSpec test files in Expertiza.
* Database involved: Expertiza_test

= Task =
Formally, you need to:
* Create the records in test DB according to the content in fixtures.
* Check each test file and delete certain DB records creation code that insert default records (eg. create(:deadline_type))
* And keep all the test cases passing when using test DB and make sure the time running test cases is shorter than before.
* You should submit the sql file of test DB to expertiza.

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

2016-11-04T18:20:59Z

Jdeng8: /* 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:

# 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-11-04T18:19:41Z

Jdeng8: /* 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:

# 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-11-04T18:19:04Z

Jdeng8: /* 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:

# 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-11-04T18:17:58Z

Jdeng8: /* 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:

# 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)

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

#add_meta_reviewer
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
#delete_meta_reviewer
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)
#delete_all_meta_reviewer
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

File:WechatIMG57.jpeg

2016-10-31T00:32:38Z

Jdeng8:

File:Testpass.png

2016-10-31T00:26:40Z

Jdeng8: uploaded a new version of "File:Testpass.png"

File:Testpass.png

2016-10-31T00:25:59Z

Jdeng8:

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

2016-10-31T00:25:17Z

Jdeng8: /* 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:

# 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.

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

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

2016-10-30T23:50:44Z

Jdeng8: /* 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:

# 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.

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

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

2016-10-30T23:50:23Z

Jdeng8: /* 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:

# 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.

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

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

2016-10-30T23:49:53Z

Jdeng8: /* 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:

# 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.

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

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

2016-10-30T23:49:21Z

Jdeng8: /* 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 other controller and views, so 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:

# 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.

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

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

2016-10-30T23:38:55Z

Jdeng8: /* 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=

# 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.

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

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

2016-10-29T05:46:04Z

Jdeng8: /* 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=

# 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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:44:14Z

Jdeng8: /* 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=

# 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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:41:14Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[#wikipedia:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:41:02Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[#w:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:40:40Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[#SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:39:47Z

Jdeng8: /* 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=

# Method ''response_report'' has some SQL[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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:38:51Z

Jdeng8: /* 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=

# Method ''response_report'' has some [w:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:38:33Z

Jdeng8: /* 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=

# 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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:37:33Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[w:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:37:21Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[w:SQL/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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:34:42Z

Jdeng8: /* 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=

# 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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:32:14Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[w:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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

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

2016-10-29T05:30:53Z

Jdeng8: /* 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=

# Method ''response_report'' has some [[wikipedia:SQL|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 = ? 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=
We write RSpec feature tests to prove that all changes are correct and the project performs well as before.

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