Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T02:46:28Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged. See a screenshot from the previous implementation of a snippet of the information from the conflict report page:<br /> [[File:OldConflictGraph.PNG|center]]<br /><br />
==Issues with Previous Work ==
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units.<br />
2. The bar graph contained too much white space and took up too much area on the screen.<br />
3. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way.<br />
4. The bars in the chart were also not sorted in any meaningful way (i.e., ascending or descending) <br />
5. The list of students who reviewed an submission extended vertically down the page which contributed to each row of the table taking up considerable screen space, especially for submissions with many reviews.<br />
6. The metrics listed in the screenshot above don't specify the threshold for conflict. Therefore, you can't tell what review scores would actually cause a conflict.<br />
7. The code for the conflict report view included a lot of logic. <br />
8. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. In addition, the code to create the formatted bar chart will also be placed in a helper method in this file. Apart from these, any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round of reviews:
***List the students who reviewed the submission and the score they gave.
***Plot the review scores on a horizontal bar chart, highlighting those that caused a conflict.
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Modify the bar chart code to show the point values as raw scores instead of percentages.<br \>
2. Modify the bar chart code to eliminate unnecessary white space.<br \>
3. Modify the bar chart code to highlight bars representing scores that cause conflict. <br \>
4. Modify the bar chart code so that the bars are sorted by ascending score. <br \>
5. List the students who reviewed the submission in multiple columns rather than a single long column. <br \>
6. Add additional metrics specifying threshold for conflict to the conflict report so you can see what scores would cause a conflict. <br \>
7. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
8. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
== Addressing Problems with Conflict Emails and Mailer ==
In order to address the problem of hard coded URLs being sent in the email to instructors, we modified the <code>notify_instructor_on_difference</code> method in the file <code>app/models/response.rb</code>to accept a base_url parameter which gets set based on the request.base_url. This allows the links to be correct, even when the server is running on localhost. <br />
[[File:Base_url_changes.PNG]]

== Addressing Problems with Conflict Report and Previous Implementation ==

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.
<br />
[[File:TravisCI.PNG]]

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

== Demo ==
The video [https://drive.google.com/drive/u/0/folders/13_apTDwdFTm78dC_HAS9QVnynUMFc-Mr] demonstrates the functionalities implemented in this project.

= Files Changed =
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-0df434e3b51a1cb1604423db5db1ff66 Conflict Report] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-7786ab741930f6bace28b7e5da61b8d6 Response Controller] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-58ccb973f92cd2e70e2f56718ab05d59 Report Format Helper]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-8f6215af8180fd708a629b4094f01c3c Review Mapping Helper]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-7e2a53155464089f13c8462bd81b8116 Summary Helper]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-5354a099931d64748967998ea42d1fc1 Mailer]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-a983bfc492e6d1982c673bc208afe459 Answer]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-f2caf2e29f94ec94cb27bdb86ec85d40 Response]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-a847769ab4f73b3b644c52acd4aa6430 Notify Grade Conflict Message]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-7adec512db194a439744ab4d2d8d10ea Review Conflict Metric] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-6d120ad6f7e1d1e4d971f7fd9f0ea434 Review Conflict Report] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-127fb0956fcd466778195f8814733934 Searchbox] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-b3496c4d4cf7597d2ecfc22b28d78ab6 Response Report] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-2f71e4cd74da63b4f65224bbebe44a1c Reports Controller Spec]<br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-d431293fd1daca78b5d63309fe56e031 Review Mapping Helper Spec] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-9252f13f8d7039091ec9afd7d65ee10f Review Mapping Helper Spec] <br>
* [https://github.com/expertiza/expertiza/pull/1714/files#diff-a68a7982922073b9602c0fd6097e0bd2 Answer Spec] <br>

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T01:16:07Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged. See a screenshot from the previous implementation of a snippet of the information from the conflict report page:<br /> [[File:OldConflictGraph.PNG|center]]<br /><br />
==Issues with Previous Work ==
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units.<br />
2. The bar graph contained too much white space and took up too much area on the screen.<br />
3. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way.<br />
4. The bars in the chart were also not sorted in any meaningful way (i.e., ascending or descending) <br />
5. The list of students who reviewed an submission extended vertically down the page which contributed to each row of the table taking up considerable screen space, especially for submissions with many reviews.<br />
6. The metrics listed in the screenshot above don't specify the threshold for conflict. Therefore, you can't tell what review scores would actually cause a conflict.<br />
7. The code for the conflict report view included a lot of logic. <br />
8. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. In addition, the code to create the formatted bar chart will also be placed in a helper method in this file. Apart from these, any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round of reviews:
***List the students who reviewed the submission and the score they gave.
***Plot the review scores on a horizontal bar chart, highlighting those that caused a conflict.
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Modify the bar chart code to show the point values as raw scores instead of percentages.<br \>
2. Modify the bar chart code to eliminate unnecessary white space.<br \>
3. Modify the bar chart code to highlight bars representing scores that cause conflict. <br \>
4. Modify the bar chart code so that the bars are sorted by ascending score. <br \>
5. List the students who reviewed the submission in multiple columns rather than a single long column. <br \>
6. Add additional metrics specifying threshold for conflict to the conflict report so you can see what scores would cause a conflict. <br \>
7. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
8. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
== Addressing Problems with Conflict Emails and Mailer ==
== Addressing Problems with Conflict Report and Previous Implementation ==

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

= Files Changed =
1. <code>app/assets/javascripts/conflict_report.js</code>[https://github.com/expertiza/expertiza/pull/1714/files#diff-0df434e3b51a1cb1604423db5db1ff66] <br>
2. <code>app/controllers/response_controller.rb</code>[https://github.com/expertiza/expertiza/pull/1714/files#diff-7786ab741930f6bace28b7e5da61b8d6] <br>
3. <code>app/helpers/report_formatter_helper.rb</code>[https://github.com/expertiza/expertiza/pull/1714/files#diff-58ccb973f92cd2e70e2f56718ab05d59]<br>
4. <code>app/helpers/review_mapping_helper.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-8f6215af8180fd708a629b4094f01c3c]<br>
5. <code>app/helpers/summary_helper.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-7e2a53155464089f13c8462bd81b8116]<br>
6. <code>app/mailers/mailer.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-5354a099931d64748967998ea42d1fc1]<br>
7. <code>app/models/answer.rb</code>[https://github.com/expertiza/expertiza/pull/1714/files#diff-a983bfc492e6d1982c673bc208afe459]<br>
8. <code>app/models/response.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-f2caf2e29f94ec94cb27bdb86ec85d40]<br>
9. <code>app/views/mailer/notify_grade_conflict_message.html.erb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-a847769ab4f73b3b644c52acd4aa6430]<br>
10. <code>app/views/reports/_review_conflict_metric.html.erb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-7adec512db194a439744ab4d2d8d10ea] <br>
11. <code>app/views/reports/_review_conflict_report.html.erb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-6d120ad6f7e1d1e4d971f7fd9f0ea434] <br>
12. <code>app/views/reports/_searchbox.html.erb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-127fb0956fcd466778195f8814733934] <br>
13. <code>app/views/reports/response_report.html.haml</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-b3496c4d4cf7597d2ecfc22b28d78ab6] <br>
14. <code>spec/controllers/reports_controller_spec.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-2f71e4cd74da63b4f65224bbebe44a1c]<br>
15. <code>spec/features/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-d431293fd1daca78b5d63309fe56e031] <br>
16. <code>spec/helpers/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-9252f13f8d7039091ec9afd7d65ee10f] <br>
17. <code>spec/models/answer_spec.rb </code> [https://github.com/expertiza/expertiza/pull/1714/files#diff-a68a7982922073b9602c0fd6097e0bd2] <br>

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T01:10:13Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged. See a screenshot from the previous implementation of a snippet of the information from the conflict report page:<br /> [[File:OldConflictGraph.PNG|center]]<br /><br />
==Issues with Previous Work ==
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units.<br />
2. The bar graph contained too much white space and took up too much area on the screen.<br />
3. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way.<br />
4. The bars in the chart were also not sorted in any meaningful way (i.e., ascending or descending) <br />
5. The list of students who reviewed an submission extended vertically down the page which contributed to each row of the table taking up considerable screen space, especially for submissions with many reviews.<br />
6. The metrics listed in the screenshot above don't specify the threshold for conflict. Therefore, you can't tell what review scores would actually cause a conflict.<br />
7. The code for the conflict report view included a lot of logic. <br />
8. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. In addition, the code to create the formatted bar chart will also be placed in a helper method in this file. Apart from these, any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round of reviews:
***List the students who reviewed the submission and the score they gave.
***Plot the review scores on a horizontal bar chart, highlighting those that caused a conflict.
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Modify the bar chart code to show the point values as raw scores instead of percentages.<br \>
2. Modify the bar chart code to eliminate unnecessary white space.<br \>
3. Modify the bar chart code to highlight bars representing scores that cause conflict. <br \>
4. Modify the bar chart code so that the bars are sorted by ascending score. <br \>
5. List the students who reviewed the submission in multiple columns rather than a single long column. <br \>
6. Add additional metrics specifying threshold for conflict to the conflict report so you can see what scores would cause a conflict. <br \>
7. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
8. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
As stated in our Proposed Design, we made the following changes to address the Problem Statement:<br />
As stated in our Proposed Design, we made the following changes to address the Issues with the Previous Implementation.<br />

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

= Files Changed =
1. <code>app/assets/javascripts/conflict_report.js</code>[https://github.com/expertiza/expertiza/pull/1714/files#diff-0df434e3b51a1cb1604423db5db1ff66] <br>
2. <code>app/controllers/response_controller.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/controllers/response_controller.rb] <br>
3. <code>app/helpers/report_formatter_helper.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/report_formatter_helper.rb]<br>
4. <code>app/helpers/review_mapping_helper.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/review_mapping_helper.rb]<br>
5. <code>app/helpers/summary_helper.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/summary_helper.rb]<br>
6. <code>app/mailers/mailer.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/mailers/mailer.rb]<br>
7. <code>app/models/answer.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/models/answer.rb]<br>
8. <code>app/models/response.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/models/response.rb]<br>
9. <code>app/views/mailer/notify_grade_conflict_message.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/mailer/notify_grade_conflict_message.html.erb]<br>
10. <code>app/views/reports/_review_conflict_metric.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_review_conflict_metric.html.erb] <br>
11. <code>app/views/reports/_review_conflict_report.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_review_conflict_report.html.erb] <br>
12. <code>app/views/reports/_searchbox.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_searchbox.html.erb] <br>
13. <code>app/views/reports/response_report.html.haml</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/response_report.html.haml] <br>
14. <code>spec/controllers/reports_controller_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/controllers/reports_controller_spec.rb]<br>
15. <code>spec/features/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/features/review_mapping_helper_spec.rb] <br>
16. <code>spec/helpers/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/helpers/review_mapping_helper_spec.rb] <br>
17. <code>spec/models/answer_spec.rb </code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/models/answer_spec.rb] <br>

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T00:59:42Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged. See a screenshot from the previous implementation of a snippet of the information from the conflict report page:<br /> [[File:OldConflictGraph.PNG|center]]<br /><br />
==Issues with Previous Work ==
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units.<br />
2. The bar graph contained too much white space and took up too much area on the screen.<br />
3. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way.<br />
4. The bars in the chart were also not sorted in any meaningful way (i.e., ascending or descending) <br />
5. The list of students who reviewed an submission extended vertically down the page which contributed to each row of the table taking up considerable screen space, especially for submissions with many reviews.<br />
6. The metrics listed in the screenshot above don't specify the threshold for conflict. Therefore, you can't tell what review scores would actually cause a conflict.<br />
7. The code for the conflict report view included a lot of logic. <br />
8. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. Apart from these any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round where there was a review in conflict:
***List and highlight the students who's review caused a conflict and the score they gave
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Not include any unnecessary bar charts with redundant information in our conflict report view.<br \>
2. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
3. Replace the logic used in the review_mapping_helper.rb file to fetch the team member of a specific team. <br \>
4. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
As per the plan stated in the above section, following changes were implemented:

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

= Files Changed =
1. <code>app/assets/javascripts/conflict_report.js</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/assets/javascripts/conflict_report.js] <br>
2. <code>app/controllers/response_controller.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/controllers/response_controller.rb] <br>
3. <code>app/helpers/report_formatter_helper.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/report_formatter_helper.rb]<br>
4. <code>app/helpers/review_mapping_helper.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/review_mapping_helper.rb]<br>
5. <code>app/helpers/summary_helper.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/helpers/summary_helper.rb]<br>
6. <code>app/mailers/mailer.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/mailers/mailer.rb]<br>
7. <code>app/models/answer.rb</code>[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/models/answer.rb]<br>
8. <code>app/models/response.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/models/response.rb]<br>
9. <code>app/views/mailer/notify_grade_conflict_message.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/mailer/notify_grade_conflict_message.html.erb]<br>
10. <code>app/views/reports/_review_conflict_metric.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_review_conflict_metric.html.erb] <br>
11. <code>app/views/reports/_review_conflict_report.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_review_conflict_report.html.erb] <br>
12. <code>app/views/reports/_searchbox.html.erb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/_searchbox.html.erb] <br>
13. <code>app/views/reports/response_report.html.haml</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/views/reports/response_report.html.haml] <br>
14. <code>spec/controllers/reports_controller_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/controllers/reports_controller_spec.rb]<br>
15. <code>spec/features/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/features/review_mapping_helper_spec.rb] <br>
16. <code>spec/helpers/review_mapping_helper_spec.rb</code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/helpers/review_mapping_helper_spec.rb] <br>
17. <code>spec/models/answer_spec.rb </code> [https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/spec/models/answer_spec.rb] <br>

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T00:45:27Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged:<br /><br />
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units. Also the bar graph contained too much white space and took up too much area on the screen. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way. See a screenshot of the previous implementation below: [[File:OldConflictGraph.PNG|center]] <br />
2. The code for the conflict report view included a lot of logic. <br />
3. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. Apart from these any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round where there was a review in conflict:
***List and highlight the students who's review caused a conflict and the score they gave
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Not include any unnecessary bar charts with redundant information in our conflict report view.<br \>
2. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
3. Replace the logic used in the review_mapping_helper.rb file to fetch the team member of a specific team. <br \>
4. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
As per the plan stated in the above section, following changes were implemented:

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

= Files Changed =
1. app/assets/javascripts/conflict_report.js[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/assets/javascripts/conflict_report.js] <br>
2. app/controllers/response_controller.rb[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/controllers/response_controller.rb] <br>

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-25T00:44:23Z

Shmehta: /* Files Changed */

= Introduction =

== Expertiza Background ==

Expertiza is an open-source project written using the Ruby on Rails framework. The Expertiza platform aims to improve student learning by using active and cooperative learning, allowing distance education students to participate in active learning, and by discouraging plagiarism through the use of multiple deadlines. Expertiza helps to improve teaching and resource utilization by having students contribute to the creation of course materials and through peer-graded assignments. Expertiza manages objects related to a course such as instructors, TAs, students, and other users. It also manages assignments, teams, signup topics, rubrics, peer grading, and peer evaluations.

== Reports Background ==
Students can form teams in Expertiza to work on an assignment in a group. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams in the form of statistical reports. The students have access to view their feedback in the form of a Summary Report. The student Summary Report shows the scores submitted by the reviewers for each question of the rubric for that assignment. If there are more than one round, it shows the scores separated by round number. The instructors have more options to view student submitted feedback in the form of Reviewees Summary Reports which shows the average review scores for an assignment for each round with each reviewer's comments, and Review Reports which shows what teams' assignment were reviewed by each student.

== Review Conflict Background ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment.

== Problem Statement ==
When a review is submitted that triggers a conflict, an email gets sent to instructors that contains three links: a link to the conflicting review, a link to the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflicts. The information sent in the email should be updated to contain a link to a report page which should contain more details about the newest conflict as well as information on previous review conflicts for the assignment. The conflict report page should have an easy to understand visualization showing the assignment's reviews with scores that cause a conflict.

= Existing Implementation =
The existing system has the following implementation for conflict notification. On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment, an email is sent out to the instructor. The email sent to the instructor contains the names of the reviewer and reviewee and a link to the conflicting review, a link to the summary of review responses, and a link to edit the assignment. The flowchart below delineates the entire process:
<br />
<br />
[[File:Conflictflowchart.jpg|center]]

== Problems with Existing Implementation ==
The existing implementation has the following flaws:

=== No dedicated report for analyzing the conflict ===
The current implementation does not provide any dedicated view or report containing a detailed analysis of the conflicts that occur during the entire review period. The link to the conflicting review is helpful, but doesn't provide any context for for why that review was in conflict. The link to the summary of review responses provides more insight into how the assignment is being scored overall, but it doesn't provide a clear picture of which reviews are in conflict and have scores that are outliers.

=== Incorrect email URLs ===
When an review is in conflict and an email gets set to the instructor, the body of the email contains links to the review, review summary page, and a link to edit the assignment. See a copy of the email message below as it is being sent by the current implementation.
[[File:Conflictemail.png]]
<br>
The current implementation uses hardcoded URLs mentioned in the <code>notify_instructor_on_difference</code> method of the <code>models/response.rb</code> file. See the code:

conflicting_response_url: 'https://expertiza.ncsu.edu/response/view?id=' + response_id.to_s,
summary_url: 'https://expertiza.ncsu.edu/grades/view_team?id=' + reviewee_participant.id.to_s,
assignment_edit_url: 'https://expertiza.ncsu.edu/assignments/' + assignment.id.to_s + '/edit'

Being hardcoded, these link would not work in on other servers where Expertiza is running. For example, the links won't be valid if the setup was done on the localhost.

= Previous Work =

The [https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2019_-_Project_E1931._Conflict_notification Previous Work] resolved the email related issues and also created a simple UI report page for analyzing the conflict. However, the implementation had quite a few issues that prevented it from being merged:<br /><br />
1. The UI for the conflict report included a bar graph that showed the grades of reviews that were in conflict. The information in the bar graph was showing percentages while the scores shown beside it were in raw points. This is an inconsistency; they should display the data in the same units. Also the bar graph contained too much white space and took up too much area on the screen. It was also not obvious from the bar graph which review scores caused a conflict as they were not highlighted or distinguished in any way. See a screenshot of the previous implementation below: [[File:OldConflictGraph.PNG|center]] <br />
2. The code for the conflict report view included a lot of logic. <br />
3. There was minimal effort spent on refactoring test cases and the test cases added in the pull request were shallow tests, meaning that they simply asserted that the expected value was not empty rather than the correct way of asserting the expected value matched a specific value. <br />

= Design of New Implementation =
== Proposed Solution ==
As the mailing functionality was implemented perfectly in the previous implementation, we plan to use their implementation as our starting point. Building on top of it, we plan to address the highlighted issues with the previous implementation and refactor the conflict report so that it is able to provide better insights into the review conflicts.
Once the existing code determines that a conflict has occurred, the code will create a report view for the conflict. The report view will contain all the relevant statistics(mean, max, standard deviation, etc.) of all the reviews received for the submission. Next, a URL for the report page will be added to the email body. All URLs in the email will be relative URLs and therefore the URLs will be valid on all servers. Following files need to be updated in order to implement these changes:

*Related to Email:
**<code>app/mailers/mailer.rb</code>
*** Modify method <code>notify_grade_conflict_message</code> to add the link to the conflict report in the email body.
**<code>app/models/response.rb</code>
*** Modify method <code>notify_instructor_on_difference</code> to add a relative links to various pages that get included in the email body.
**<code>app/views/mailer/notify_grade_conflict_message.html.erb</code>
*** Modify the html to add a link to the email message using which the conflict report viewed.
**<code>app/controllers/response_contrller.rb</code>
*** Modify <code>update</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method
*** Modify <code>create</code> method to add <code>request.base_url</code> as a param sent to <code>notify_instructor_on_difference</code> method

*Related to the conflict report view:
**<code>app/views/reports/_searchbox.html.erb</code>
*** Modify the html to add the option to view a conflict report.
**<code>app/views/reports/response_report.html.haml</code>
*** Modify the html to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**<code>App/views/reports/_conflict_report.html.erb</code>
*** Need to create this entire file to make the view for a conflict report
**<code>app/helpers/report_formatter_helper.rb</code>
*** Need to add a method <code>conflict_reponse_map</code> to make specific instance variables available to our conflict report view. This method will get called from the ReportsController.
**<code>app/helpers/summary_helper.rb</code>
*** Need to add a method to get the maximum possible score of an assignment in each round
**<code>app/models/answer.rb</code>
*** Need to add a method to get the answers from a review for an assignment submitted by a specific reviewer.
**<code>app/helpers/review_mapping_helper.rb</code>
*** Need to add methods to get the review scores for a team in each round, calculate the average review score for a round, calculate the std. deviation of review scores for each round, and get the team members of each team. Apart from these any other method to find and calculate data to be viewed in the report can be added to this helper file.

The conflict report view should:
*For each team that had a submission with a review that was in a conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round where there was a review in conflict:
***List and highlight the students who's review caused a conflict and the score they gave
***List the threshold for conflict review score, max review score, average review score, and standard deviation. <br \><br \>

In order to resolve the issues with the previous team's pull request, we will:<br \>
1. Not include any unnecessary bar charts with redundant information in our conflict report view.<br \>
2. Put the necessary logic into helper methods in helper files to ensure logic stays out of our views.<br \>
3. Replace the logic used in the review_mapping_helper.rb file to fetch the team member of a specific team. <br \>
4. Refactor the necessary affected test cases and write new RSpec tests in accordance with our proposed test plan to ensure adequate coverage.<br \>

== UML Diagrams ==
=== Use Case Diagram ===
The usecase diagram highlights the simple workflow of the proposed system when trigger by the user.<br />
[[File:usecase-1.png]]
<br /> <br />
Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

=== Sequence Diagram ===
The sequence diagram models the interactions that will take place between various components of the system for the above use-case.
<br /> <br />
[[File:conflictSeq.jpg]]

= New Implementation =
As per the plan stated in the above section, following changes were implemented:

= Testing =
Both manual testing and RSpec testing were used to test the new functionalities.
=== RSpec Testing ===
In order to test the changes implemented in the model, controller and helper file new RSpec tests have been added.
<br />
'''1. Add a new test to <code>spec/controllers/reports_controller_spec.rb</code>''' <br />
The test checks if the new review conflict report is routed and rendered properly by the response controller.
<pre style="color: green">
describe '#review_conflict_response_map' do
context 'when type is ReviewConflictResponseMap' do
it 'renders response_report page with corresponding data' do
allow(TeammateReviewResponseMap).to receive(:teammate_response_report)
.with('1')
.and_return([participant, participant2])
params = {
id: 1,
report: {type: 'ReviewConflictResponseMap'},
user: 'no one'
}
get :response_report, params
expect(response).to render_template(:response_report)
end
end
end
</pre>

'''2. Add a new test to <code>spec/models/answer_spec.rb</code>''' <br />
This tests if the query in the newly added method 'answers_by_round_for_reviewee' works properly and fetches a valid result.
<pre style="color: green">
it "returns answers by reviewer for reviewee in round from db which is not empty" do
expect(Answer.answers_by_round_for_reviewee(@assignment_id, @reviewee_id,@round)).not_to be_empty
end
</pre>

'''3. Create and add a new tests to <code>spec/helpers/review_mapping_helper_spec.rb</code>''' <br />
This file tests various calculations performed by the newly added helper methods. The results of these calculations are not only used to display on the report but also used to generate the graph.
<pre style="color: green">
describe "ReviewMappingHelper" do
describe "#review_score_helper_for_team" do
before(:each) do
@review_answers = [{'reviewer_id': '2', 'answer': 54}, {'reviewer_id': '1', 'answer': 24}, {'reviewer_id': '2', 'answer': 25}]
end

it "The review_score_helper_for_team method calculates the total review scores for each review" do
question_answers = helper.review_score_helper_for_team(@review_answers)
expect(question_answers).to include('1' => 24,'2' => 79)
end
end

describe "#get_score_metrics" do
before(:each) do
@review_scores = {'1' => 50, '2' => 40, '3' => 60}
end

it "The get_score_metrics method calculates various metrics used in the conflict report" do
metric = helper.get_score_metrics(@review_scores, 50)
expect(metric[:average]).to eq 50.0
expect(metric[:std]).to eq 8.16
expect(metric[:upper_tolerance_limit]).to eq 50
expect(metric[:lower_tolerance_limit]).to eq 33.68
end
end
end
</pre>

'''4. Add new test in <code>spec/features/review_mapping_helper_spec.rb</code>''' <br />
This UI test navigates to the conflict report page and tests if the report is rendered properly.
<pre style="color: green">
describe "Test Review conflict report" do
before(:each) do
create(:instructor)
create(:assignment, course: nil, name: 'Test Assignment')
assignment_id = Assignment.where(name: 'Test Assignment')[0].id
login_as 'instructor6'
visit "/reports/response_report?id=#{assignment_id}"
page.select("Review conflict report", :from => "report[type]")
click_button "View"
end

# Check if the page renders

it "has a conflicting review table" do
expect(page).to have_css('table')
end

it "can display teams" do
expect(page).to have_content('Team')
end

it "can display author reviewers and metrics" do
expect(page).to have_selector(:link_or_button, 'Convert Points to Percents')
end
end
</pre>
<br />
All RSpec tests are passing for the current implementation as is evident by the passing Travis CI build.

== Manual Testing ==
In order to test the functionality manually, we follow the following steps: <br \>
1. Create a new assignment with 15% as a conflict threshold limit. <br />
2. Add 4 students to the assignment and divide them into 2 teams of 2 students in each. <br />
3. Add submission for each team. <br />
4. In the review phase, login to the first team student accounts and give scores of 100% and 50% to the second team's assignment. <br />
5. Since the scores differ by more than 15% the email will be sent to the instructor. <br />
6. Access the email and check if the email contains the following details: <br />
* Names of the reviewer and reviewee
* Link to the conflicting review
* Link to the newly created report view corresponding to the conflict
* Link to edit the assignment notification limit
7. View the conflict report and ensure that Team 2 shows up as having a conflicting review submitted by student 2 of Team 1.

= Files Changed =
1. app/assets/javascripts/conflict_report.js[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/assets/javascripts/conflict_report.js]
2. app/controllers/response_controller.rb[https://github.com/expertiza/expertiza/blob/d90830993d97df152d932cee7fd5df6be8df26f9/app/controllers/response_controller.rb]

= Useful Links =

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

= Team Information =

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

CSC/ECE 517 Spring 2020 - E2015. Conflict notification

2020-04-08T20:13:49Z

Shmehta: /* Use Case Diagram */

== '''Abstract''' ==
Expertiza is an open-source web application implemented using Ruby on Rails. It is used for management of courses and the assignments for respective courses, by the faculties and the students. Students can form teams in Expertiza to work on an assignment in a group. A student team can submit their work through multiple means such as file uploads and embedded links. Submissions made on Expertiza can be peer-reviewed by students within the course based on a rubric. Results of the peer review can be accessed by instructors and respective student teams. During the peer evaluation, Expertiza sends appropriate email notifications to the instructor and also provides a complete statistical report of the evaluation.

== '''Problem Statement''' ==
In the current implementation, during the peer review phase of an assignment, email notifications are sent out to the instructor whenever a submitted review score differs “significantly” from the average score of other submitted reviews for that submission. The threshold to trigger a notification is specified in the “Notification limit” on the Rubrics tab of assignment creation. The email that gets sent to instructors contains three links: a link the the conflicting review, a link the the summary page of grades for the reviewee, and a link to edit the assignment. Although the links sent in the email are helpful for reference, the process of examining a conflicting review could be made more efficient if we provide an easy to understand report of the conflict. The information sent in the email should be updated to contain a link to a report page which should contain more details about the conflict. In addition to creating a new conflict report page, we also want to change the algorithm to detect and trigger a notification if there is a significant difference between a submitted review and any previous submitted review for that assignment. Rather than compare the review score to the average of all review scores, an email should be sent whenever two review scores differ significantly for the same submission.

== '''Existing Implementation''' ==
On submitting a new review for a particular artifact, the submitted score is compared to the average score of the previously submitted reviews. If the difference between the average score and the newly submitted score is more than the notification threshold specified for the assignment an email is sent out to the instructor. The flowchart below delineates the entire process:

[[File:Flowwork.PNG|center]]

== '''Design''' ==
=== Proposed Solution ===

What we need to modify:
*Related to Email:
**App/mailers/mailer.rb in method notify_grade_conflict_message to make the link to the conflict report available in the email.
**App/models/response.rb in method notify_instructor_on_difference to add a link to the conflict report that gets sent to the mailer.
**App/views/mailer/notify_grade_conflict_message.html.erb to add to the message a link to view the conflict report.

*Related to the conflict report view:
**App/views/reports/_searchbox.html.erb to add the option to view a conflict report.
**App/views/reports/response_report.html.haml to render the conflict report partial if the option to view a conflict report was selected.

What we need to create:
*Related to the conflict report view:
**App/views/reports/_conflict_report.html.erb to make the view for a conflict report

The conflict report view should:
*For each team that had a submission with a review that was in conflict:
**List the team name and team members
**Provide a graphical view of the statistics displayed in the report
**For each round where there was a review in conflict:
***List the students who's review caused a conflict and the score they gave
***List the threshold for conflict review score, max review score, average review score, and standard deviation.

=== Why we chose this approach? ===

=== Use Case Diagram ===
[[File:usecase-1.png]]

Actors participating:

* Student: In this use case, fills out the reviews for different projects submitted by the peers.
* TA/Instructor: In this use case, goes over the conflicting review submitted by the students for the same project.

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

== '''Useful Links''' ==

'''Github:''' https://github.com/sid189/expertiza/tree/beta <br />
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1714

== '''Team Information''' ==

'''Project Mentor:'''
<br>Pratik Abhyankar

'''Project Members:'''<br>
Sahil Papalkar<br>
Sahil Mehta<br>
Siddharth Deshpande<br>
Carl Klier<br>

File:Usecase-1.png

2020-04-08T20:01:16Z

Shmehta:

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-31T08:58:31Z

Shmehta: /* Manual Testing */

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

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

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==
The ''criterion.rb'' model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.

For this project, we have to deal with the following two methods:
* The '''''complete''''' method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* The '''''view_completed_question''''' method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular.
We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions.
We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.
* High branch condition size problem: extracting three methods from method "'''complete'''"
** Method "'''dropdown_criterion_question'''" return html when choosing dropdown options
** Method "'''scale_criterion_question'''" return html when choosing scale options
** Method "'''advices_criterion_question'''" return html about showing advice for each criterion question
* Too long code
** Combining the short HTML strings into longer ones but not too long
** Using one readable line of code instead of three or more lines of if-else statements
* Fix incomplete condition problem
* Change the language to more Ruby friendly

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

=== The ''complete'' method ===
* We extracted three methods from "complete" method.
* We combined the short HTML strings into longer ones.
* We used the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code.

====''Examples of changes''====
'''Extracting three methods to reducing the condition branch'''
* Method "dropdown_criterion_question" return html when choosing dropdown options
* Method "scale_criterion_question" return html when choosing scale options
* Method "advices_criterion_question" return html about showing advice for each criterion question

'''Combining the short HTML strings into longer ones'''

We go from:
<pre>
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
To:
<pre>
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
'''Using one readable line of code instead of three or more lines of if-else statements'''

We go from:
<pre>
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end
html += j.to_s
</pre>
To:
<pre>
html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>' + j.to_s
</pre>

'''Use unless for negative conditions'''

We go from:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
</pre>
To:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?
</pre>

Same change for self.min_label and self.max_label. We go from:
<pre>
html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
</pre>
To:
<pre>
html += '<td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'
</pre>

'''Unsafe condition'''

We added condition to make sure when answer.comments is nil. We go from:
<pre>
html += answer.comments unless answer.nil?
</pre>
To:
<pre>
html += answer.comments if !answer.nil? && !answer.comments.nil?
</pre>

'''Incomplete condition'''

When create a new criterion, which size is "" that isn't nil. We add condition to make sure "cols" and "rows" will be assign when "self.size" is "". We go from:
<pre>
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
</pre>
To:
<pre>
if self.size.nil?||self.size.blank?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
</pre>

'''Ruby grammer'''

We change "and" to &&. We changed from:
<pre>
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
</pre>
To:
<pre>
html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)
</pre>

====''The modified ''complete'' method''====
* '''Method "complete"'''
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0
question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end
#show advice given for different questions
html += advices_criterion_question(count, question_advices) if !question_advices.empty? and advice_total_length > 0

#dropdown options to rate a project based on the quetion
html += dropdown_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'dropdown'

#scale optioins
html += scale_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'scale'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* '''Method "advices_criterion_question"'''
<pre>
# show advice for each criterion question
def advices_criterion_question(count, question_advices)
html = '<a id="showAdvice_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a><script>'
html += 'function showAdvice(i){var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){element.innerHTML="Show advice";} else{element.innerHTML="Hide advice";}toggleAdvice(i);}'
html += 'function toggleAdvice(i) {var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {elem.style.display = "";} else {elem.style.display = "none";}}</script>'
html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/><script>'
html += 'function changeScore(i, j) {var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}</script>'
end
html += '</div>'
end
</pre>
* '''Method "dropdown_criterion_question"'''
<pre>
def dropdown_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?
html = '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>'
html += j.to_s
html += "-" + self.min_label if self.min_label.present? && j == questionnaire_min
html += "-" + self.max_label if self.max_label.present? && j == questionnaire_max
html += "</option>"
end

html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
html += ' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments if !answer.nil? && !answer.comments.nil?
html += '</textarea></td>'
end
</pre>
* '''Method "scale_criterion_question"'''
<pre>
def scale_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
if self.size.nil? || !self.size.present?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html = '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '><table><tr><td width="10%"></td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end

html += '<td width="10%"></td></tr><tr><td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script><td width="10%">'
html += self.max_label unless self.max_label.nil?
html += '</td><td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments if !answer.nil? && !answer.comments.nil?
html += '</textarea>'
end
</pre>

=== The ''view_completed_question" method===

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function.
* We reduced the nested if-else branching.
* We concatenated small HTML string
* We used shorthand for if-else condition statements.

This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rb file would make more sense.

====''Example of changes''====
'''Combining the short HTML strings into longer ones'''

We concatenated small HTML which was from:
<pre>
html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
</pre>
To:
<pre>
html += '<table cellpadding="5"><tr><td>'
</pre>

'''Using one readable line of code instead of three or more lines of if-else statements'''

We changed from:
<pre>
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end
</pre>
To:
<pre>
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
</pre>

'''Delete useless variable - 'resp''''

We deleted this code
<pre>
resp = Response.find(answer.response_id)
</pre>

'''Change of language to make it more Ruby friendly'''

We changed from:
<pre>
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
To:
<pre>
if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
'''Reduce the branch condition'''

We combined condition
<pre>
unless tag_prompt_deployments.nil?
</pre>
and
<pre>
if tag_prompt_deployments.count > 0
</pre>
To:
<pre>
if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
</pre>
[[File:Multiple_2.png]]

==== The modified ''view_completed_question''====

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"
score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'

if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
# show check boxes for answer tagging
question = Question.find(answer.question_id)
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

== Test Plan ==

We are running Rspec tests to make sure the coverage is the same and manually checking the pages to make sure there are not any issues and the code is not breaking
=== RSpec Testing ===
The origin test about method "complete" didn't include the situation when the parameter "answer" isn't nil and the parameter "dropdown_or_scale" is "dropdown" or "scale". We added the test examples from 4 to 16 including the multiple for method "complete", "dropdown_criterion_question", "scale_criterion_question".
* '''Test for method "complete" with or without answer and dorpdown_or_scale'''
'''Origin test'''
<pre>
describe "#complete" do
it "returns the html " do
html = criterion.complete(0, nil, 0, 5).to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div>")
end
end
</pre>

'''New test'''
<pre>
describe "#complete" do
it "returns the html without answer and no dropdown or scale" do
html = criterion.complete(0, nil, 0, 5).to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div>")
end

it "returns the html without answer and dropdown" do
html = criterion.complete(0, nil, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" ><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max and dropdown" do
html = criterion.complete(0, answer_no_comments, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =8><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max and dropdown" do
html = criterion.complete(0, answer_comments, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =3><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3 selected=\"selected\">3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea></td>")
end

it "returns the html without answer and scale" do
html = criterion.complete(0, nil, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max and scale" do
html = criterion.complete(0, answer_no_comments, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"8\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max and scale" do
html = criterion.complete(0, answer_comments, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"3\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea>")
end

end
</pre>

* '''Test for method "dropdown_criterion_question" with or without answer'''
<pre>
describe "#dropdown_criterion_question" do
it "returns the html without answer" do
html = criterion.dropdown_criterion_question(0, nil, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" ><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max" do
html = criterion.dropdown_criterion_question(0, answer_no_comments, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =8><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with comments in answer and answer.answer between questionnaire min and max" do
html = criterion.dropdown_criterion_question(0, answer_comments, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =3><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3 selected=\"selected\">3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea></td>")
end
end
</pre>
* '''Test for method "scale_criterion_question" with or without answer'''
<pre>
describe "#scale_criterion_question" do
it "returns the html without answer" do
html = criterion.scale_criterion_question(0, nil, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max" do
html = criterion.scale_criterion_question(0, answer_no_comments, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"8\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max" do
html = criterion.scale_criterion_question(0, answer_comments, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"3\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea>")
end
end
</pre>

* '''All existing tests passed'''
[[File:Test1.png]]

=== Manual Testing ===
'''Test Log in'''
Website url: http://152.7.99.41:8080/ <br>
Log in: instructor6<br>
Password: password<br>
Log in: student2064<br>
Password: password<br>

'''Test criterion works well in questionnaire'''

1. After logging in as a student, click the "Assignments"<br>
2. Choose an assignment, like "Backchannel application review"<br>
3. You can choose "Your scores" to see the review results<br>
4. Click Criterion 1, you can see all the reviews for this criterion.<br>
5. Click "toggle question list", you can see all criterion questions.<br>
6. After logging in as an instructor, choose "Questionnaires" tag<br>
7. Click on the name Review: This should show a drop-down showing different reviews made.<br>
8. Choose a review, select the edit icon to the right.<br>
8. Change dropbox of question type to Criterion, select Add "2" more. Press the add button.<br>
9. Edit question content: "Test question Textarea", "Test question Criterion1", and "Test question Criterion2" in that order.<br>
10. You can also change the question content for exited criterion question<br>

These manual tests show criterion works well. Feel free to try your own test cases.

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-31T08:56:51Z

Shmehta:

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

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

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==
The ''criterion.rb'' model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.

For this project, we have to deal with the following two methods:
* The '''''complete''''' method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* The '''''view_completed_question''''' method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular.
We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions.
We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.
* High branch condition size problem: extracting three methods from method "'''complete'''"
** Method "'''dropdown_criterion_question'''" return html when choosing dropdown options
** Method "'''scale_criterion_question'''" return html when choosing scale options
** Method "'''advices_criterion_question'''" return html about showing advice for each criterion question
* Too long code
** Combining the short HTML strings into longer ones but not too long
** Using one readable line of code instead of three or more lines of if-else statements
* Fix incomplete condition problem
* Change the language to more Ruby friendly

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

=== The ''complete'' method ===
* We extracted three methods from "complete" method.
* We combined the short HTML strings into longer ones.
* We used the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code.

====''Examples of changes''====
'''Extracting three methods to reducing the condition branch'''
* Method "dropdown_criterion_question" return html when choosing dropdown options
* Method "scale_criterion_question" return html when choosing scale options
* Method "advices_criterion_question" return html about showing advice for each criterion question

'''Combining the short HTML strings into longer ones'''

We go from:
<pre>
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
To:
<pre>
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
</pre>
'''Using one readable line of code instead of three or more lines of if-else statements'''

We go from:
<pre>
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end
html += j.to_s
</pre>
To:
<pre>
html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>' + j.to_s
</pre>

'''Use unless for negative conditions'''

We go from:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
</pre>
To:
<pre>
current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?
</pre>

Same change for self.min_label and self.max_label. We go from:
<pre>
html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
</pre>
To:
<pre>
html += '<td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'
</pre>

'''Unsafe condition'''

We added condition to make sure when answer.comments is nil. We go from:
<pre>
html += answer.comments unless answer.nil?
</pre>
To:
<pre>
html += answer.comments if !answer.nil? && !answer.comments.nil?
</pre>

'''Incomplete condition'''

When create a new criterion, which size is "" that isn't nil. We add condition to make sure "cols" and "rows" will be assign when "self.size" is "". We go from:
<pre>
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
</pre>
To:
<pre>
if self.size.nil?||self.size.blank?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
</pre>

'''Ruby grammer'''

We change "and" to &&. We changed from:
<pre>
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
</pre>
To:
<pre>
html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)
</pre>

====''The modified ''complete'' method''====
* '''Method "complete"'''
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0
question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end
#show advice given for different questions
html += advices_criterion_question(count, question_advices) if !question_advices.empty? and advice_total_length > 0

#dropdown options to rate a project based on the quetion
html += dropdown_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'dropdown'

#scale optioins
html += scale_criterion_question(count, answer, questionnaire_min, questionnaire_max) if dropdown_or_scale == 'scale'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* '''Method "advices_criterion_question"'''
<pre>
# show advice for each criterion question
def advices_criterion_question(count, question_advices)
html = '<a id="showAdvice_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a><script>'
html += 'function showAdvice(i){var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){element.innerHTML="Show advice";} else{element.innerHTML="Hide advice";}toggleAdvice(i);}'
html += 'function toggleAdvice(i) {var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {elem.style.display = "";} else {elem.style.display = "none";}}</script>'
html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/><script>'
html += 'function changeScore(i, j) {var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}</script>'
end
html += '</div>'
end
</pre>
* '''Method "dropdown_criterion_question"'''
<pre>
def dropdown_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s unless answer.nil?
html = '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += '<option value=' + j.to_s
html += ' selected="selected"' if !answer.nil? && j == answer.answer
html += '>'
html += j.to_s
html += "-" + self.min_label if self.min_label.present? && j == questionnaire_min
html += "-" + self.max_label if self.max_label.present? && j == questionnaire_max
html += "</option>"
end

html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"'
html += ' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments if !answer.nil? && !answer.comments.nil?
html += '</textarea></td>'
end
</pre>
* '''Method "scale_criterion_question"'''
<pre>
def scale_criterion_question(count, answer = nil, questionnaire_min, questionnaire_max)
if self.size.nil? || !self.size.present?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end
html = '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '><table><tr><td width="10%"></td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end

html += '<td width="10%"></td></tr><tr><td width="10%">'
html += self.min_label unless self.min_label.nil?
html += '</td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? && answer.answer == j) or (answer.nil? && questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script><td width="10%">'
html += self.max_label unless self.max_label.nil?
html += '</td><td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments if !answer.nil? && !answer.comments.nil?
html += '</textarea>'
end
</pre>

=== The ''view_completed_question" method===

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function.
* We reduced the nested if-else branching.
* We concatenated small HTML string
* We used shorthand for if-else condition statements.

This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rb file would make more sense.

====''Example of changes''====
'''Combining the short HTML strings into longer ones'''

We concatenated small HTML which was from:
<pre>
html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
</pre>
To:
<pre>
html += '<table cellpadding="5"><tr><td>'
</pre>

'''Using one readable line of code instead of three or more lines of if-else statements'''

We changed from:
<pre>
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end
</pre>
To:
<pre>
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
</pre>

'''Delete useless variable - 'resp''''

We deleted this code
<pre>
resp = Response.find(answer.response_id)
</pre>

'''Change of language to make it more Ruby friendly'''

We changed from:
<pre>
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
To:
<pre>
if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
</pre>
'''Reduce the branch condition'''

We combined condition
<pre>
unless tag_prompt_deployments.nil?
</pre>
and
<pre>
if tag_prompt_deployments.count > 0
</pre>
To:
<pre>
if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
</pre>
[[File:Multiple_2.png]]

==== The modified ''view_completed_question''====

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"
score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'

if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
if !tag_prompt_deployments.nil? && tag_prompt_deployments.count > 0
# show check boxes for answer tagging
question = Question.find(answer.question_id)
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type && answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

== Test Plan ==

We are running Rspec tests to make sure the coverage is the same and manually checking the pages to make sure there are not any issues and the code is not breaking
=== RSpec Testing ===
The origin test about method "complete" didn't include the situation when the parameter "answer" isn't nil and the parameter "dropdown_or_scale" is "dropdown" or "scale". We added the test examples from 4 to 16 including the multiple for method "complete", "dropdown_criterion_question", "scale_criterion_question".
* '''Test for method "complete" with or without answer and dorpdown_or_scale'''
'''Origin test'''
<pre>
describe "#complete" do
it "returns the html " do
html = criterion.complete(0, nil, 0, 5).to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div>")
end
end
</pre>

'''New test'''
<pre>
describe "#complete" do
it "returns the html without answer and no dropdown or scale" do
html = criterion.complete(0, nil, 0, 5).to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div>")
end

it "returns the html without answer and dropdown" do
html = criterion.complete(0, nil, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" ><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max and dropdown" do
html = criterion.complete(0, answer_no_comments, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =8><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max and dropdown" do
html = criterion.complete(0, answer_comments, 0, 5, dropdown_or_scale = 'dropdown').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =3><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3 selected=\"selected\">3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea></td>")
end

it "returns the html without answer and scale" do
html = criterion.complete(0, nil, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max and scale" do
html = criterion.complete(0, answer_no_comments, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"8\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max and scale" do
html = criterion.complete(0, answer_comments, 0, 5, dropdown_or_scale = 'scale').to_s
expect(html).to eq("<div><label for=\"responses_0\">test txt</label></div><input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"3\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea>")
end

end
</pre>

* '''Test for method "dropdown_criterion_question" with or without answer'''
<pre>
describe "#dropdown_criterion_question" do
it "returns the html without answer" do
html = criterion.dropdown_criterion_question(0, nil, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" ><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max" do
html = criterion.dropdown_criterion_question(0, answer_no_comments, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =8><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3>3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea></td>")
end

it "returns the html with comments in answer and answer.answer between questionnaire min and max" do
html = criterion.dropdown_criterion_question(0, answer_comments, 0, 5).to_s
expect(html).to eq("<div><select id=\"responses_0_score\" name=\"responses[0][score]\" class=\"review-rating\" data-current-rating =3><option value = ''>--</option><option value=0>0</option><option value=1>1</option><option value=2>2</option><option value=3 selected=\"selected\">3</option><option value=4>4</option><option value=5>5</option></select></div><br><br><textarea id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea></td>")
end
end
</pre>
* '''Test for method "scale_criterion_question" with or without answer'''
<pre>
describe "#scale_criterion_question" do
it "returns the html without answer" do
html = criterion.scale_criterion_question(0, nil, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with no comments answer and answer.answer outside questionnaire min and max" do
html = criterion.scale_criterion_question(0, answer_no_comments, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"8\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\"></textarea>")
end

it "returns the html with comments answer and answer.answer between questionnaire min and max" do
html = criterion.scale_criterion_question(0, answer_comments, 0, 5).to_s
expect(html).to eq("<input id=\"responses_0_score\" name=\"responses[0][score]\" type=\"hidden\"value=\"3\"><table><tr><td width=\"10%\"></td><td width=\"10%\"><label>0</label></td><td width=\"10%\"><label>1</label></td><td width=\"10%\"><label>2</label></td><td width=\"10%\"><label>3</label></td><td width=\"10%\"><label>4</label></td><td width=\"10%\"><label>5</label></td><td width=\"10%\"></td></tr><tr><td width=\"10%\"></td><td width=\"10%\"><input type=\"radio\" id=\"0\" value=\"0\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"1\" value=\"1\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"2\" value=\"2\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"3\" value=\"3\" name=\"Radio_1\"checked=\"checked\"></td><td width=\"10%\"><input type=\"radio\" id=\"4\" value=\"4\" name=\"Radio_1\"></td><td width=\"10%\"><input type=\"radio\" id=\"5\" value=\"5\" name=\"Radio_1\"></td><script>jQuery(\"input[name=Radio_1]:radio\").change(function() {var response_score = jQuery(\"#responses_0_score\");var checked_value = jQuery(\"input[name=Radio_1]:checked\").val();response_score.val(checked_value);});</script><td width=\"10%\"></td><td width=\"10%\"></td></tr></table><textarea cols=70 rows=1 id=\"responses_0_comments\" name=\"responses[0][comment]\" class=\"tinymce\">text comments</textarea>")
end
end
</pre>

* '''All existing tests passed'''
[[File:Test1.png]]

=== Manual Testing ===
'''Test Log in'''
Website url: http://152.7.99.41:8080/
Log in: instructor6
Password: password
Log in: student2064
Password: password

'''Test criterion works well in questionnaire'''

1. After logging in as a student, click the "Assignments"
2. Choose an assignment, like "Backchannel application review"
3. You can choose "Your scores" to see the review results.
4. Click Criterion 1, you can see all the reviews for this criterion.
5. Click "toggle question list", you can see all criterion questions.

6. After logging in as an instructor, choose "Questionnaires" tag
7. Click on the name Review: This should show a drop-down showing different reviews made.
8. Choose a review, select the edit icon to the right.
8. Change dropbox of question type to Criterion, select Add "2" more. Press the add button.
9. Edit question content: "Test question Textarea", "Test question Criterion1", and "Test question Criterion2" in that order.
10. You can also change the question content for exited criterion question

These manual tests show criterion works well. Feel free to try your own test cases.

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-23T19:44:06Z

Shmehta:

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

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

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==
The ''criterion.rb'' model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.

For this project, we have to deal with the following two methods:
* The '''''complete''''' method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* The '''''view_completed_question''''' method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular. We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions. We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

=== The ''complete'' method ===

All the code in the model is just HTML. We thought of removing the code from here and putting it in a .erb file but it didn’t make much sense as the HTML code was heavily branched by the if-else conditions and it would just have been taking it out from this function and putting it in another file. We instead decided to refactor it in the file. We mainly combined the short HTML strings into longer ones but not too long so that the Code Climate cannot throw a “line too long” error. Another thing we did is reduce the number of lines used by condition statements by using the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code. (We also created another method for the repeated HTML code, so we just have to call the method rather than repeating the code. It increases code re-usability and reduces the number of lines of code.)

====''List of changes''====

* Combining the short HTML strings into longer ones
[[File:Combine1.png]]
* Using one readable line of code instead of three or more lines of if-else statements
[[File:Oneline1.png]]

====''The modified ''complete'' method''====
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdvice_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a><script>'
html += 'function showAdvice(i){var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){element.innerHTML="Show advice";} else{element.innerHTML="Hide advice";}toggleAdvice(i);}'

html += 'function toggleAdvice(i) {var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {elem.style.display = "";} else {elem.style.display = "none";}}</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/><script>'
html += 'function changeScore(i, j) {var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?

html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>' + "<option value = ''>--</option>" + '<option value='
questionnaire_min.upto(questionnaire_max).each do |j|
html += j.to_s + 'selected="selected"' if !answer.nil? and j == answer.answer + '>'
html += j.to_s + "-"
html += self.min_label if self.min_label.present? and j == questionnaire_min + "</option>"
html += self.max_label if self.max_label.present? and j == questionnaire_max + "</option>"
end
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil? + '><table><tr><td width="10%"></td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr><td width="10%">' + self.min_label if !self.min_label.nil? + '</td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) + '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script><td width="10%">'

html += self.max_label if !self.max_label.nil? + '</td><td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea>'
end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

====The ''view_completed_question" method====

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function. This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rb file would make more sense. We still made some changes in the file like concatenating small HTML string and using shorthand for if-else condition statements.

The modified ''view_completed_question'' method is as below:

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'

if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

== Testing ==

We are running Rspec tests to make sure the coverage is the same and manually checking the pages to make sure there are not any issues and the code is not breaking

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-23T15:59:26Z

Shmehta:

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

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

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==
The ''criterion.rb'' model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.

For this project, we have to deal with the following two methods:
* The '''''complete''''' method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* The '''''view_completed_question''''' method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular. We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions. We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

====The ''complete'' method====

All the code in the model is just HTML. We thought of removing the code from here and putting it in a .erb file but it didn’t make much sense as the HTML code was heavily branched by the if-else conditions and it would just have been taking it out from this function and putting it in another file. We instead decided to refactor it in the file. We mainly combined the short HTML strings into longer ones but not too long so that the Code Climate cannot throw a “line too long” error. Another thing we did is reduce the number of lines used by condition statements by using the shorthand which beautifully consolidates three or more lines of code (read: if-else statements) into one readable line of code. (We also created another method for the repeated HTML code, so we just have to call the method rather than repeating the code. It increases code re-usability and reduces the number of lines of code.)

The modified ''complete'' method is as below:

<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdvice_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a><script>'
html += 'function showAdvice(i){var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){element.innerHTML="Show advice";} else{element.innerHTML="Hide advice";}toggleAdvice(i);}'

html += 'function toggleAdvice(i) {var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {elem.style.display = "";} else {elem.style.display = "none";}}</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/><script>'
html += 'function changeScore(i, j) {var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?

html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>' + "<option value = ''>--</option>" + '<option value='
questionnaire_min.upto(questionnaire_max).each do |j|
html += j.to_s + 'selected="selected"' if !answer.nil? and j == answer.answer + '>'
html += j.to_s + "-"
html += self.min_label if self.min_label.present? and j == questionnaire_min + "</option>"
html += self.max_label if self.max_label.present? and j == questionnaire_max + "</option>"
end
html += '</select></div><br><br><textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil? + '><table><tr><td width="10%"></td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr><td width="10%">' + self.min_label if !self.min_label.nil? + '</td>'

(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j) + '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script><td width="10%">'

html += self.max_label if !self.max_label.nil? + '</td><td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil? + '</textarea>'
end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

====The ''view_completed_question" method====

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function. This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rb file would make more sense. We still made some changes in the file like concatenating small HTML string and using shorthand for if-else condition statements.

The modified ''view_completed_question'' method is as below:

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0
score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'

if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

== Testing ==

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-22T15:40:27Z

Shmehta:

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==
The ''criterion.rb'' model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So this file consists of a lot of HTML code that is rendered to the final questionnaire view.

For this project, we have to deal with the following two methods:
* The '''''complete''''' method, which is 104 lines long. this method returns the display for the students when they are filling the questionnaire. It includes the advice given for different questions, dropdown options to rate a project based on the question, a textarea to enter comments and so on.
<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>
* The '''''view_completed_question''''' method, which is 47 lies long. Thismethod is responsible to return the display if a student is viewingan already filled-out questionnaire.
<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular. We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions. We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

====The ''complete'' method====

All the code in the model is just HTML. We thought of removing the code from here and putting it in a .erb file but it didn’t make much sense as the HTML code was heavily branched by the if-else conditions and it would just have been taking it out from this function and putting it in another file. We instead decided to refactor it in the file. We mainly combined the short HTML strings into longer ones but not too long so that the Code Climate cannot throw a “line too long” error. Another thing we did is reduced the number of lines used by condition statements by using the shorthand which beautifully consolidates three or more lines of code into one readable line of code. (We also created another method for the repeated HTML code, so we just have to call the method rather than repeating the code. It increases code reusability and reduces the number of lines of code.)

The modified ''complete'' method is as below:

====The ''view_completed_question" method====

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function. This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rn file would make more sense. We still made some changes in the file like concatenating small HTML string and using shorthand for if-else condition statements.

The modified ''view_completed_question'' method is as below:

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '#</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

== Testing ==

CSC517 (Spring 2020) - E2010. Refactor criterion.rb

2020-03-22T15:25:12Z

Shmehta: Created page with " = E2010 Refactor criterion.rb = This page provides a description of the Expertiza based OSS project. == About Expertiza == == Problem Statement == The criterion.rb model..."

= E2010 Refactor criterion.rb =

This page provides a description of the Expertiza based OSS project.

== About Expertiza ==

== Problem Statement ==
The criterion.rb model consists of functions that decide what to display while creating, editing and viewing the questionnaires depending on whether the user is an instructor, a teaching assistant or a student. So, this file consists of a lot of HTML code that is rendered to the final questionnaire view.

The current version of this controller has four methods, but two methods are very long and would require refactoring. The primary problem with these functions is that it consists of many statements of string concatenation. The HTML code which is rendered to the final view is made up of concatenations.

Another issue with the code is that the branch conditions size for the complete and the view_completed_question method is very high. Also, this controller has very few comments. They are specifically needed to differentiate between the purpose of numerous nested branches from each other.

== About criterion.rb ==

<pre>
def complete(count, answer = nil, questionnaire_min, questionnaire_max, dropdown_or_scale)
if self.size.nil?
cols = '70'
rows = '1'
else
cols = self.size.split(',')[0]
rows = self.size.split(',')[1]
end

html = '<div><label for="responses_' + count.to_s + '">' + self.txt + '</label></div>'
# show advice for each criterion question
question_advices = QuestionAdvice.where(question_id: self.id).sort_by(&:id)
advice_total_length = 0

question_advices.each do |question_advice|
advice_total_length += question_advice.advice.length if question_advice.advice && question_advice.advice != ""
end

if !question_advices.empty? and advice_total_length > 0
html += '<a id="showAdivce_' + self.id.to_s + '" onclick="showAdvice(' + self.id.to_s + ')">Show advice</a>'
html += '<script>'
html += 'function showAdvice(i){'
html += 'var element = document.getElementById("showAdivce_" + i.toString());'
html += 'var show = element.innerHTML == "Hide advice";'
html += 'if (show){'
html += 'element.innerHTML="Show advice";'
html += '}else{'
html += 'element.innerHTML="Hide advice";}'
html += 'toggleAdvice(i);}'

html += 'function toggleAdvice(i) {'
html += 'var elem = document.getElementById(i.toString() + "_myDiv");'
html += 'if (elem.style.display == "none") {'
html += 'elem.style.display = "";'
html += '} else {'
html += 'elem.style.display = "none";}}'
html += '</script>'

html += '<div id="' + self.id.to_s + '_myDiv" style="display: none;">'
# [2015-10-26] Zhewei:
# best to order advices high to low, e.g., 5 to 1
# each level used to be a link;
# clicking on the link caused the dropbox to be filled in with the corresponding number
question_advices.reverse.each_with_index do |question_advice, index|
html += '<a id="changeScore_>' + self.id.to_s + '" onclick="changeScore(' + count.to_s + ',' + index.to_s + ')">'
html += (self.questionnaire.max_question_score - index).to_s + ' - ' + question_advice.advice + '</a><br/>'
html += '<script>'
html += 'function changeScore(i, j) {'
html += 'var elem = jQuery("#responses_" + i.toString() + "_score");'
html += 'var opts = elem.children("option").length;'
html += 'elem.val((' + self.questionnaire.max_question_score.to_s + ' - j).toString());}'
html += '</script>'
end
html += '</div>'
end

if dropdown_or_scale == 'dropdown'
current_value = ""
current_value += 'data-current-rating =' + answer.answer.to_s if !answer.nil?
html += '<div><select id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" class="review-rating" ' + current_value + '>'
html += "<option value = ''>--</option>"
questionnaire_min.upto(questionnaire_max).each do |j|
html += if !answer.nil? and j == answer.answer
'<option value=' + j.to_s + ' selected="selected">'
else
'<option value=' + j.to_s + '>'
end

html += j.to_s
if j == questionnaire_min
html += "-" + self.min_label if self.min_label.present?
elsif j == questionnaire_max
html += "-" + self.max_label if self.max_label.present?
end
html += "</option>"
end
html += "</select></div><br><br>"
html += '<textarea' + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea></td>'
elsif dropdown_or_scale == 'scale'
html += '<input id="responses_' + count.to_s + '_score" name="responses[' + count.to_s + '][score]" type="hidden"'
html += 'value="' + answer.answer.to_s + '"' unless answer.nil?
html += '>'

html += '<table>'
html += '<tr><td width="10%"></td>'
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><label>' + j.to_s + '</label></td>'
end
html += '<td width="10%"></td></tr><tr>'

html += if !self.min_label.nil?
'<td width="10%">' + self.min_label + '</td>'
else
'<td width="10%"></td>'
end
(questionnaire_min..questionnaire_max).each do |j|
html += '<td width="10%"><input type="radio" id="' + j.to_s + '" value="' + j.to_s + '" name="Radio_' + self.id.to_s + '"'
html += 'checked="checked"' if (!answer.nil? and answer.answer == j) or (answer.nil? and questionnaire_min == j)
html += '></td>'
end
html += '<script>jQuery("input[name=Radio_' + self.id.to_s + ']:radio").change(function() {'
html += 'var response_score = jQuery("#responses_' + count.to_s + '_score");'
html += 'var checked_value = jQuery("input[name=Radio_' + self.id.to_s + ']:checked").val();'
html += 'response_score.val(checked_value);});</script>'

html += if !self.max_label.nil?
'<td width="10%">' + self.max_label + '</td>'
else
'<td width="10%"></td>'
end

html += '<td width="10%"></td></tr></table>'
html += '<textarea cols=' + cols + ' rows=' + rows + ' id="responses_' + count.to_s + '_comments"' \
' name="responses[' + count.to_s + '][comment]" class="tinymce">'
html += answer.comments unless answer.nil?
html += '</textarea>'

end
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = if score != "-"
answer.answer * 1.0 / questionnaire_max
else
0
end

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5">'
html += '<tr>'
html += '<td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score
html += '</div>'
html += '</td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px">'
html += '<br>' + answer.comments.html_safe
html += '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
end
</pre>

== Proposed Solution ==
Our goal is to refactor the complete and view_completed_question method mainly by reducing the number of lines code for each function and also by introducing new methods to make the code more modular. We will also try to reduce the branch condition size wherever possible and hence reduce the cyclomatic complexity for these two functions. We also plan to introduce comments wherever needed to make the code more understandable. The main aim is to reduce the number of lines and make the code more compact without affecting the readability of the code.

== Implementation ==

We tried to tackle the issues mentioned in the problem statement as described below:

====The ''complete'' method====

All the code in the model is just HTML. We thought of removing the code from here and putting it in a .erb file but it didn’t make much sense as the HTML code was heavily branched by the if-else conditions and it would just have been taking it out from this function and putting it in another file. We instead decided to refactor it in the file. We mainly combined the short HTML strings into longer ones but not too long so that the Code Climate cannot throw a “line too long” error. Another thing we did is reduced the number of lines used by condition statements by using the shorthand which beautifully consolidates three or more lines of code into one readable line of code. (We also created another method for the repeated HTML code, so we just have to call the method rather than repeating the code. It increases code reusability and reduces the number of lines of code.)

The modified ''complete'' method is as below:

====The ''view_completed_question" method====

The main task here was to reduce the nested if-else branching and reduce the number of arguments for this function. This was a very small method with its major dependent functions present in response.rb file. To refactor this method, we would have had to refactor response.rb and in turn, the other question subclasses which would make it very complex and break the code. According to us, refactoring this method along with the response.rn file would make more sense. We still made some changes in the file like concatenating small HTML string and using shorthand for if-else condition statements.

The modified ''view_completed_question'' method is as below:

<pre>
def view_completed_question(count, answer, questionnaire_max, tag_prompt_deployments = nil, current_user = nil)
html = '<b>' + count.to_s + ". " + self.txt + ' [Max points: ' + questionnaire_max.to_s + "]</b>"

score = answer && !answer.answer.nil? ? answer.answer.to_s : "-"
score_percent = score != "-" ? answer.answer * 1.0 / questionnaire_max : 0

score_color = if score_percent > 0.8
"c5"
elsif score_percent > 0.6
"c4"
elsif score_percent > 0.4
"c3"
elsif score_percent > 0.2
"c2"
else
"c1"
end

html += '<table cellpadding="5"><tr><td>'
html += '<div class="' + score_color + '" style="width:30px; height:30px;' \
' border-radius:50%; font-size:15px; color:black; line-height:30px; text-align:center;">'
html += score + '</div></td>'
if answer && !answer.comments.nil?
html += '<td style="padding-left:10px"><br>' + answer.comments.html_safe + '</td>'
#### start code to show tag prompts ####
unless tag_prompt_deployments.nil?
# show check boxes for answer tagging
resp = Response.find(answer.response_id)
question = Question.find(answer.question_id)
if tag_prompt_deployments.count > 0
html += '<tr><td colspan="2">'
tag_prompt_deployments.each do |tag_dep|
tag_prompt = TagPrompt.find(tag_dep.tag_prompt_id)
if tag_dep.question_type == question.type and answer.comments.length > tag_dep.answer_length_threshold.to_i
html += tag_prompt.html_control(tag_dep, answer, current_user)
end
end
html += '</td></tr>'
end
end
#### end code to show tag prompts ####
end
html += '#</tr></table>'
safe_join(["".html_safe, "".html_safe], html.html_safe)
end
</pre>