Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-26T18:29:36Z

Pjloheid: /* Automated Test Plan */

== '''Overview of Project''' ==

In Expertiza, meta-review is a feature that enables a third party to evaluate the quality of feedback that a reviewer gives a reviewee. meta-review is an important feature that can help students become better reviewers for their peers by providing detailed feedback that is actionable and positively formulated. Unfortunately, this feature is broken and the following issues were identified. The task is to fix these issues so that this feature can be used again.

* An assignment is deemed to have meta-reviews if and only if there is a meta-review deadline for this assignment in the `assignments_questionnaires` table.
* If there is no meta-review deadline, then don’t show the question about whether there is a max meta-review limit.
* In the DB the default number of meta-reviews allowed and required should be null.
* As soon as someone checks the Has meta-review Limit box on the Review Strategy tab, the UI should fill in 3 and 3 as the required and allowed number.
* Student pages should show the list of reviews that have been performed.
* When a user requests a new meta-review, the field which says the number of meta-reviews left ( refer to above image “Numbers of Meta-reviews left:”) does not change and the button to request a new meta-review disappears.

===Previous Work===

This assignment is a repeat of E1997 which was assigned to students last semester, Fall 2019.

Details of the previous implementation can be found here:
[http://https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1997._Issues_related_to_meta-reviewing E1997]

Our goals are to update the logic, refactor non-DRY code, and fix bugs.

== '''[https://github.com/RPHolloway/expertiza/issues/32 #32 - Remove all debug comments in the UI]''' ==

===Issue Description===

It was noted in the previous team's implementation that multiple debugging comments were showing up during their video testing.

Debug messages shown when viewing a completed meta review:
[[File:Debug_comments.png]]

===Changes===

There was a mistake in <code>app/views/response/_review.html.erb</code> where "<<nowiki/>br/><<nowiki/>hr/><<nowiki/>br/>" was being displayed under the review being meta-reviewed. This was due to being included as a string constant rather than html code.

[[File:E2025_debug_comments_response_review.PNG]]

There was an instance in <code>app/views/response/view.html.erb</code> where ruby code responsible for getting a review to be displayed was being interpreted and displayed, even though the response was being displayed as html in the line before. This resulted in the response details being listed in the UI erroneously.

[[File:E2025_debug_response_view.PNG]]

== '''[https://github.com/RPHolloway/expertiza/issues/49 #49 - "Request a new meta-review" button crashes when there are no available reviews]''' ==

===Issue Description===

When there are no valid metareviews to perform (either there are no reivews performed on the assignment, the only valid reviews concern your own work or own reviews, you have completed all allowed metareviews, or you have reviewed all valid reviews already) then the "Request a New Review" button still is visible and displays the following error (correctly) when pressed.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and check "Use meta-review deadline" checkbox

3. Select "Yes" for all submission allowed, Review allowed and Meta-review allowed in all of the deadline types

4. Navigate back to Assignment Tree View and click on "Add participant"

5. Add couple of students(say student1 and student2) to the assignment

6. Impersonate/login as student1 and make a submission

7. Log in as instructor6 as previously described and change the submission due date to be passed so we are currently in the review stage

8. Impersonate/login as student2 and Request for a new review under others work, submit the review

9. Log in as instructor6 as previously described and change the review due date to be passed so we are currently in the meta-review stage

10. Impersonate/login back as student1 (the one with the submission) and Request for a new meta review

[[File:E2025_outofreviews.png]]

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

Here we conditionally hide the button based on whether or not the participant has reviews available to perform. These checks are located in assignment.rb which is the model for an Assignment. The following method was added to assignment.rb and returns true when the participant has a valid meta-review available.

# Returns true if there is a meta-review task available for the user
def metareview_available?(user_id)
metareviewer = AssignmentParticipant.where(user_id: user_id, parent_id: self.id).first
begin
self.response_map_to_metareview(metareviewer)
true
rescue
false
end
end

== '''[https://github.com/RPHolloway/expertiza/issues/27 #27 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database]''' ==

===Issue Description===

Currently, the number of required and allowed meta-reviews is always being set to 3 in the database. Per the specs of the feature, it should default the database values to NULL. When checkbox "Use meta-review deadline", in the "Due Dates" tab, is selected then the view should populate required and allowed meta-reviews to 3.

===Changes===

We created a migration that defaulted the metareviews_allowed and metareviews_required in the Assignment table to nil instead of what the previous team had as 3.

[[File:E2025_13.png]]

Since the number of meta-reviews allowed and required are recorded using the values in the text fields, changes were made to <code>app/views/assignment/edit/_review_strategy.html.erb</code> to set the values and display them as expected.

When the checkbox for setting meta-review limits is unchecked, the values of number of meta-reviews allowed and required should be null. When the checkbox is checked, the values should be set to 3 and 3 by default, or to the values already defined before (in case the assignment was saved with the values set already). On page load, if the checkbox is enabled, then the inputs should be shown with the default or pre-set values, but if the checkbox is disabled then they should be hidden and set to null.

[[File:E2025_metareview_limit_1.PNG]]

[[File:E2025_metareview_limit_2.PNG]]

Changes had to be made to <code>app/views/student_review/list.html.erb</code> to account for the values being <code>null</code>.

[[File:E2025_metareview_limit_3.PNG]]

===Testing===

Tests have been created that test if a metareview can have nil limits. In this case, no limit will be shown to the user. Before this change that was impossible

== '''Issue 5 - Meta Review Limit is Not Functioning Correctly in Assignment Creation''' ==

===Issue Description===

The "has meta review limit" should determine whether a limit or requirement is placed on the student to complete meta reviews. If disabled, then they should be able to complete as many as they want. This is independent of the Meta Review STAGE that is enabled/disabled in the due dates tab. They should not impact each other and currently do within the assignment creation.

===Reproducing Existing Issue===

1. If the "Use meta-review deadline" is disabled, then the Meta Review Limit panel disappears

2. "Has meta-review limit?" is hidden on edit

3. Possibility the "Has meta-review limit?" not being saved on assignment creation?

4. If the "Use meta-review deadline" is toggled (disabled, enabled), then the "Has meta-review limit?" resets to disabled

===Proposed Changes===

The code in _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder is lazily written and does not really adhere to the project specifications of how metareviews should work on assignment creation. These views need to be edited to work correctly and also respond to the changes in issue 5.

===Changes===

The changes were too long to screenshot, so here is the specific diff section:

https://github.com/expertiza/expertiza/pull/1715/files#diff-540fd2bd6c34b48213716b1dd713733a
https://github.com/expertiza/expertiza/pull/1715/files#diff-80f39fa5f62c73ff8741b02d3b11656c

== '''Discovered Bugs:''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/45 #45 - "Request a new meta-review" button bugs]''' ==

===Issue Description===

The "Request new a meta-review" button should be hidden when:
# The number of meta-reviews in-progress is greater than the maximum allowed outstanding reviews.
# The number of reviews allowed is limited AND the total number of meta-reviews completed or in-progress is greater than the number allowed.
# There are no valid reviews available for the participant.

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 0 reviewers. The user should NOT be able to see the 'Request a new meta-review to perform' button when there are no reviews available.
** with 1 reviewer.
*** A student should NOT be able to request a meta-review on their own review.
*** When the meta-review limit on the assignment is set to 1 then a student should NOT be able to request a second meta-review.
*** The user should not be able to see the 'Request a new meta-review to perform' button when they have performed all valid reviews.
** with 4 reviewers.
*** The user should be able to click the 'Begin' button and still see the 'Request a new meta-review to perform' button.
*** The user should NOT be able to see the 'Request a new meta-review to perform' button when they have reached the meta-review limit.
*** If the user requests two meta-reviews but has not submitted, then they should not be able to request a new meta-review.
*** The user should be able to request a meta-review if they are above their required but below the allowed number of meta-reviews.

== '''[https://github.com/RPHolloway/expertiza/issues/30 #30 - “Numbers of Meta-reviews left:” does not update after a user requests a review.]''' ==
===Issue Description===
Number of meta-reviews left should decrement when a user requests a meta-review. Current behavior only decremented when a review was submitted.

===Changes===

Previous behavior:

Number of Meta-Reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_completed %>

Fixed behavior:

Number of meta-reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_total %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user should see the number of meta-reviews left decrement after a new meta-review is requested.
*** The user should see the number of meta-reviews left decrement after a new meta-review is completed.

== '''[https://github.com/RPHolloway/expertiza/issues/43 #43 - "Review Allowed" flag is required to be set on a due_date for a user to perform a meta-review.]''' ==
===Issue Description===
When an assignment with a meta-review is created the round that meta-reviews will be performed in should have the "Meta-review allowed" option set to yes. This should be the only requirement for making meta-reviews available in that round. The problem with the current implementation is that meta-reviews can only be performed in rounds where the "Review allowed" option is set.

[[File:E2025 Issue43.png]]

===Changes===
Code changes were applied to app/views/student_review/_responses.html.erb. Since this partial is shared by both reviews and meta-reviews in list.html.erb the title variable was used to determine if the begin option should be active or not.

Previous behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" && @assignment.can_review(topic_id) %>
Current behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" &&
((@assignment.can_review(topic_id) && title == "Review") ||
(@assignment.metareview_allowed(topic_id) && title == "Meta-review")) %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. All meta-review tests make meta-reviews allowed only during the meta-review round. This means that if the simple meta-review test is passing then this feature is working as intended.

== '''[https://github.com/RPHolloway/expertiza/issues/50 #50 - The submit button is not available for meta-reviews.]''' ==
===Issue Description===
Users should see save and submit buttons as part of the meta-review form. These buttons should have the same behavior as the review form. The problem right now is there is not submit button for meta-reviews.
===Changes===
Code changes were made adding 'Metareview' to the conditional that determines if the submit button should be shown or not.

Previous behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review')) %>
New behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review') || (@title.eql? 'Metareview'))%>
<%= submit_tag "Submit "+@title, :name => "Submit", :id => "Submit" %>
<img src="/assets/info.png" title='- SAVE will only store the peer-review as draft and the peer-review will not be displayed to the reviewee. After clicking SUBMIT, the peer-review will be displayed to reviewee, and you will not be able to update it any more.'>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user is able to SUBMIT a simple meta-review.

== '''Testing''' ==

=== Manual Testing ===

1. Log in as super_administrator2. Password is "password". Click on mange...-> Assignments.

2. Create a new Assignment by clicking the "+" button. Give the assignment these fields: Name, Course and Submission Directory.

[[File:New_assignment_general.png]]

3. Go to "Due Dates" tab and check "Use meta-review deadline" and then set the due dates 1 into the FUTURE for each review round and meta-review. Leave everything else as default as shown below:

[[File:New_assignment_duedates.png]]

4. Go to the "Rubrics" tab and populate as shown below:

[[File:New_assignment_rubrics.png]]

5. OPTIONAL: Go to "Review strategy page and select "Has meta-review limit?" and set the number of allowed and required meta-reviews.

6. Once done, "Create" the assignment.

7. Go back to Assignments. manage...-> Assignments.

8. Find the newly created assignment and click on "Add participant". Add the students below:

[[File:add_participants.png]]

9. Go to manage...Impersonate User and impersonate "Student485"

10. Go to the newly created assignment and submit a link for the assignment.

11. Revert back to super_administrator2 and change the submission due date to a date in the PAST so we are currently in the review stage.

12. Impersonate student486 and request for a new review under others work, submit the review.

13. Revert back to super_administrator2 as previously described and change the review due date to be passed so we are currently in the meta-review stage.

14. Impersonate as student485 and Request for a new meta review.

15. Press "Begin" and complete the meta-review

16. When the review is complete, the "Begin" should disappear and be replaced with "View" and "Edit"

===Automated Test Plan===
1. User should NOT be able to see 'Request a new meta-review to perform' button when there are no reviews available. When there are reviews that don't belong to the meta-reviewer submitted, meta-reviews can be performed.

2. User is able to SAVE a simple meta-review.

3. User is able to go BACK during a simple meta-review since this functionality is available for reviews.

4. When the limit and required number of meta-reviews on the assignment are equal, then a student will see they need to submit exactly that number of meta-reviews.

5. When there are more meta-reviews allowed than required, then a student will see they need to submit between the two numbers of meta-reviews

6. When the meta-review limits for an assignment are unset then a student will see that the number of meta-reviews aren't limited

7. A student should not see a number of required or allowed meta-reviews when there are no limits on meta-reviews

8. A student should see the number of meta-reviews decrement after a review is requested

9. A student should see the number of meta-reviews decrement after they complete a review

10. A student should not be able to request a meta-review on their own reviews

11. If the meta-review limit on the assignment is set to 1 then a student should not be able to request a second meta review

12. User should not be able to see 'Request a new meta-review to perform' button when they have reviewed all valid reviews already

13. User should be able to request more than one meta-review before completing any

14. User should not be able to see 'Request a new meta-review to perform' button when they have reached the meta-review limit

15. User should not be able to request more than two meta-reviews without completing at least 1

16. A student should be able to request a meta-review if they are above their required but below their allowed meta-reviews

[[File:E2025_tests_passing.PNG]]

===Coverage===
Current test coverage is 41.348%. Code coverage will increase based on the test plan above and then if the failing test case described below would be fixed.

Travis CI is currently failing because of:
Failures:
1) assignment function general tab should edit quiz number available to students
Failure/Error:
child_nodes.each do |node|
initialize_fnode_update_children(params, node, tmp_res)
end
NoMethodError:
undefined method `each' for "":String
# ./app/controllers/tree_display_controller.rb:209:in `children_node_ng'
# ./config/diagnostic.rb:11:in `call'
# ------------------
# --- Caused by: ---
# Capybara::CapybaraError:
# Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
# /home/travis/build/expertiza/expertiza/vendor/bundle/ruby/2.2.0/gems/capybara-2.17.0/lib/capybara/session.rb:145:in `raise_server_error!'

File:E2025 tests passing.PNG

2020-04-26T18:29:11Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-26T18:25:55Z

Pjloheid: /* Automated Test Plan */

== '''Overview of Project''' ==

In Expertiza, meta-review is a feature that enables a third party to evaluate the quality of feedback that a reviewer gives a reviewee. meta-review is an important feature that can help students become better reviewers for their peers by providing detailed feedback that is actionable and positively formulated. Unfortunately, this feature is broken and the following issues were identified. The task is to fix these issues so that this feature can be used again.

* An assignment is deemed to have meta-reviews if and only if there is a meta-review deadline for this assignment in the `assignments_questionnaires` table.
* If there is no meta-review deadline, then don’t show the question about whether there is a max meta-review limit.
* In the DB the default number of meta-reviews allowed and required should be null.
* As soon as someone checks the Has meta-review Limit box on the Review Strategy tab, the UI should fill in 3 and 3 as the required and allowed number.
* Student pages should show the list of reviews that have been performed.
* When a user requests a new meta-review, the field which says the number of meta-reviews left ( refer to above image “Numbers of Meta-reviews left:”) does not change and the button to request a new meta-review disappears.

===Previous Work===

This assignment is a repeat of E1997 which was assigned to students last semester, Fall 2019.

Details of the previous implementation can be found here:
[http://https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1997._Issues_related_to_meta-reviewing E1997]

Our goals are to update the logic, refactor non-DRY code, and fix bugs.

== '''[https://github.com/RPHolloway/expertiza/issues/32 #32 - Remove all debug comments in the UI]''' ==

===Issue Description===

It was noted in the previous team's implementation that multiple debugging comments were showing up during their video testing.

Debug messages shown when viewing a completed meta review:
[[File:Debug_comments.png]]

===Changes===

There was a mistake in <code>app/views/response/_review.html.erb</code> where "<<nowiki/>br/><<nowiki/>hr/><<nowiki/>br/>" was being displayed under the review being meta-reviewed. This was due to being included as a string constant rather than html code.

[[File:E2025_debug_comments_response_review.PNG]]

There was an instance in <code>app/views/response/view.html.erb</code> where ruby code responsible for getting a review to be displayed was being interpreted and displayed, even though the response was being displayed as html in the line before. This resulted in the response details being listed in the UI erroneously.

[[File:E2025_debug_response_view.PNG]]

== '''[https://github.com/RPHolloway/expertiza/issues/49 #49 - "Request a new meta-review" button crashes when there are no available reviews]''' ==

===Issue Description===

When there are no valid metareviews to perform (either there are no reivews performed on the assignment, the only valid reviews concern your own work or own reviews, you have completed all allowed metareviews, or you have reviewed all valid reviews already) then the "Request a New Review" button still is visible and displays the following error (correctly) when pressed.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and check "Use meta-review deadline" checkbox

3. Select "Yes" for all submission allowed, Review allowed and Meta-review allowed in all of the deadline types

4. Navigate back to Assignment Tree View and click on "Add participant"

5. Add couple of students(say student1 and student2) to the assignment

6. Impersonate/login as student1 and make a submission

7. Log in as instructor6 as previously described and change the submission due date to be passed so we are currently in the review stage

8. Impersonate/login as student2 and Request for a new review under others work, submit the review

9. Log in as instructor6 as previously described and change the review due date to be passed so we are currently in the meta-review stage

10. Impersonate/login back as student1 (the one with the submission) and Request for a new meta review

[[File:E2025_outofreviews.png]]

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

Here we conditionally hide the button based on whether or not the participant has reviews available to perform. These checks are located in assignment.rb which is the model for an Assignment. The following method was added to assignment.rb and returns true when the participant has a valid meta-review available.

# Returns true if there is a meta-review task available for the user
def metareview_available?(user_id)
metareviewer = AssignmentParticipant.where(user_id: user_id, parent_id: self.id).first
begin
self.response_map_to_metareview(metareviewer)
true
rescue
false
end
end

== '''[https://github.com/RPHolloway/expertiza/issues/27 #27 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database]''' ==

===Issue Description===

Currently, the number of required and allowed meta-reviews is always being set to 3 in the database. Per the specs of the feature, it should default the database values to NULL. When checkbox "Use meta-review deadline", in the "Due Dates" tab, is selected then the view should populate required and allowed meta-reviews to 3.

===Changes===

We created a migration that defaulted the metareviews_allowed and metareviews_required in the Assignment table to nil instead of what the previous team had as 3.

[[File:E2025_13.png]]

Since the number of meta-reviews allowed and required are recorded using the values in the text fields, changes were made to <code>app/views/assignment/edit/_review_strategy.html.erb</code> to set the values and display them as expected.

When the checkbox for setting meta-review limits is unchecked, the values of number of meta-reviews allowed and required should be null. When the checkbox is checked, the values should be set to 3 and 3 by default, or to the values already defined before (in case the assignment was saved with the values set already). On page load, if the checkbox is enabled, then the inputs should be shown with the default or pre-set values, but if the checkbox is disabled then they should be hidden and set to null.

[[File:E2025_metareview_limit_1.PNG]]

[[File:E2025_metareview_limit_2.PNG]]

Changes had to be made to <code>app/views/student_review/list.html.erb</code> to account for the values being <code>null</code>.

[[File:E2025_metareview_limit_3.PNG]]

===Testing===

Tests have been created that test if a metareview can have nil limits. In this case, no limit will be shown to the user. Before this change that was impossible

== '''Issue 5 - Meta Review Limit is Not Functioning Correctly in Assignment Creation''' ==

===Issue Description===

The "has meta review limit" should determine whether a limit or requirement is placed on the student to complete meta reviews. If disabled, then they should be able to complete as many as they want. This is independent of the Meta Review STAGE that is enabled/disabled in the due dates tab. They should not impact each other and currently do within the assignment creation.

===Reproducing Existing Issue===

1. If the "Use meta-review deadline" is disabled, then the Meta Review Limit panel disappears

2. "Has meta-review limit?" is hidden on edit

3. Possibility the "Has meta-review limit?" not being saved on assignment creation?

4. If the "Use meta-review deadline" is toggled (disabled, enabled), then the "Has meta-review limit?" resets to disabled

===Proposed Changes===

The code in _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder is lazily written and does not really adhere to the project specifications of how metareviews should work on assignment creation. These views need to be edited to work correctly and also respond to the changes in issue 5.

===Changes===

The changes were too long to screenshot, so here is the specific diff section:

https://github.com/expertiza/expertiza/pull/1715/files#diff-540fd2bd6c34b48213716b1dd713733a
https://github.com/expertiza/expertiza/pull/1715/files#diff-80f39fa5f62c73ff8741b02d3b11656c

== '''Discovered Bugs:''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/45 #45 - "Request a new meta-review" button bugs]''' ==

===Issue Description===

The "Request new a meta-review" button should be hidden when:
# The number of meta-reviews in-progress is greater than the maximum allowed outstanding reviews.
# The number of reviews allowed is limited AND the total number of meta-reviews completed or in-progress is greater than the number allowed.
# There are no valid reviews available for the participant.

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 0 reviewers. The user should NOT be able to see the 'Request a new meta-review to perform' button when there are no reviews available.
** with 1 reviewer.
*** A student should NOT be able to request a meta-review on their own review.
*** When the meta-review limit on the assignment is set to 1 then a student should NOT be able to request a second meta-review.
*** The user should not be able to see the 'Request a new meta-review to perform' button when they have performed all valid reviews.
** with 4 reviewers.
*** The user should be able to click the 'Begin' button and still see the 'Request a new meta-review to perform' button.
*** The user should NOT be able to see the 'Request a new meta-review to perform' button when they have reached the meta-review limit.
*** If the user requests two meta-reviews but has not submitted, then they should not be able to request a new meta-review.
*** The user should be able to request a meta-review if they are above their required but below the allowed number of meta-reviews.

== '''[https://github.com/RPHolloway/expertiza/issues/30 #30 - “Numbers of Meta-reviews left:” does not update after a user requests a review.]''' ==
===Issue Description===
Number of meta-reviews left should decrement when a user requests a meta-review. Current behavior only decremented when a review was submitted.

===Changes===

Previous behavior:

Number of Meta-Reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_completed %>

Fixed behavior:

Number of meta-reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_total %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user should see the number of meta-reviews left decrement after a new meta-review is requested.
*** The user should see the number of meta-reviews left decrement after a new meta-review is completed.

== '''[https://github.com/RPHolloway/expertiza/issues/43 #43 - "Review Allowed" flag is required to be set on a due_date for a user to perform a meta-review.]''' ==
===Issue Description===
When an assignment with a meta-review is created the round that meta-reviews will be performed in should have the "Meta-review allowed" option set to yes. This should be the only requirement for making meta-reviews available in that round. The problem with the current implementation is that meta-reviews can only be performed in rounds where the "Review allowed" option is set.

[[File:E2025 Issue43.png]]

===Changes===
Code changes were applied to app/views/student_review/_responses.html.erb. Since this partial is shared by both reviews and meta-reviews in list.html.erb the title variable was used to determine if the begin option should be active or not.

Previous behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" && @assignment.can_review(topic_id) %>
Current behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" &&
((@assignment.can_review(topic_id) && title == "Review") ||
(@assignment.metareview_allowed(topic_id) && title == "Meta-review")) %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. All meta-review tests make meta-reviews allowed only during the meta-review round. This means that if the simple meta-review test is passing then this feature is working as intended.

== '''[https://github.com/RPHolloway/expertiza/issues/50 #50 - The submit button is not available for meta-reviews.]''' ==
===Issue Description===
Users should see save and submit buttons as part of the meta-review form. These buttons should have the same behavior as the review form. The problem right now is there is not submit button for meta-reviews.
===Changes===
Code changes were made adding 'Metareview' to the conditional that determines if the submit button should be shown or not.

Previous behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review')) %>
New behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review') || (@title.eql? 'Metareview'))%>
<%= submit_tag "Submit "+@title, :name => "Submit", :id => "Submit" %>
<img src="/assets/info.png" title='- SAVE will only store the peer-review as draft and the peer-review will not be displayed to the reviewee. After clicking SUBMIT, the peer-review will be displayed to reviewee, and you will not be able to update it any more.'>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user is able to SUBMIT a simple meta-review.

== '''Testing''' ==

=== Manual Testing ===

1. Log in as super_administrator2. Password is "password". Click on mange...-> Assignments.

2. Create a new Assignment by clicking the "+" button. Give the assignment these fields: Name, Course and Submission Directory.

[[File:New_assignment_general.png]]

3. Go to "Due Dates" tab and check "Use meta-review deadline" and then set the due dates 1 into the FUTURE for each review round and meta-review. Leave everything else as default as shown below:

[[File:New_assignment_duedates.png]]

4. Go to the "Rubrics" tab and populate as shown below:

[[File:New_assignment_rubrics.png]]

5. OPTIONAL: Go to "Review strategy page and select "Has meta-review limit?" and set the number of allowed and required meta-reviews.

6. Once done, "Create" the assignment.

7. Go back to Assignments. manage...-> Assignments.

8. Find the newly created assignment and click on "Add participant". Add the students below:

[[File:add_participants.png]]

9. Go to manage...Impersonate User and impersonate "Student485"

10. Go to the newly created assignment and submit a link for the assignment.

11. Revert back to super_administrator2 and change the submission due date to a date in the PAST so we are currently in the review stage.

12. Impersonate student486 and request for a new review under others work, submit the review.

13. Revert back to super_administrator2 as previously described and change the review due date to be passed so we are currently in the meta-review stage.

14. Impersonate as student485 and Request for a new meta review.

15. Press "Begin" and complete the meta-review

16. When the review is complete, the "Begin" should disappear and be replaced with "View" and "Edit"

===Automated Test Plan===
1. User should NOT be able to see 'Request a new meta-review to perform' button when there are no reviews available. When there are reviews that don't belong to the meta-reviewer submitted, meta-reviews can be performed.

2. User is able to SAVE a simple meta-review.

3. User is able to go BACK during a simple meta-review since this functionality is available for reviews.

4. When the limit and required number of meta-reviews on the assignment are equal, then a student will see they need to submit exactly that number of meta-reviews.

5. When there are more meta-reviews allowed than required, then a student will see they need to submit between the two numbers of meta-reviews

6. When the meta-review limits for an assignment are unset then a student will see that the number of meta-reviews aren't limited

7. A student should not see a number of required or allowed meta-reviews when there are no limits on meta-reviews

8. A student should see the number of meta-reviews decrement after a review is requested

9. A student should see the number of meta-reviews decrement after they complete a review

10. A student should not be able to request a meta-review on their own reviews

11. If the meta-review limit on the assignment is set to 1 then a student should not be able to request a second meta review

12. User should not be able to see 'Request a new meta-review to perform' button when they have reviewed all valid reviews already

13. User should be able to request more than one meta-review before completing any

14. User should not be able to see 'Request a new meta-review to perform' button when they have reached the meta-review limit

15. User should not be able to request more than two meta-reviews without completing at least 1

16. A student should be able to request a meta-review if they are above their required but below their allowed meta-reviews

===Coverage===
Current test coverage is 41.348%. Code coverage will increase based on the test plan above and then if the failing test case described below would be fixed.

Travis CI is currently failing because of:
Failures:
1) assignment function general tab should edit quiz number available to students
Failure/Error:
child_nodes.each do |node|
initialize_fnode_update_children(params, node, tmp_res)
end
NoMethodError:
undefined method `each' for "":String
# ./app/controllers/tree_display_controller.rb:209:in `children_node_ng'
# ./config/diagnostic.rb:11:in `call'
# ------------------
# --- Caused by: ---
# Capybara::CapybaraError:
# Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
# /home/travis/build/expertiza/expertiza/vendor/bundle/ruby/2.2.0/gems/capybara-2.17.0/lib/capybara/session.rb:145:in `raise_server_error!'

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-25T22:00:37Z

Pjloheid: /* Changes */

== '''Overview of Project''' ==

In Expertiza, meta-review is a feature that enables a third party to evaluate the quality of feedback that a reviewer gives a reviewee. meta-review is an important feature that can help students become better reviewers for their peers by providing detailed feedback that is actionable and positively formulated. Unfortunately, this feature is broken and the following issues were identified. The task is to fix these issues so that this feature can be used again.

* An assignment is deemed to have meta-reviews if and only if there is a meta-review deadline for this assignment in the `assignments_questionnaires` table.
* If there is no meta-review deadline, then don’t show the question about whether there is a max meta-review limit.
* In the DB the default number of meta-reviews allowed and required should be null.
* As soon as someone checks the Has meta-review Limit box on the Review Strategy tab, the UI should fill in 3 and 3 as the required and allowed number.
* Student pages should show the list of reviews that have been performed.
* When a user requests a new meta-review, the field which says the number of meta-reviews left ( refer to above image “Numbers of Meta-reviews left:”) does not change and the button to request a new meta-review disappears.

===Previous Work===

This assignment is a repeat of E1997 which was assigned to students last semester, Fall 2019.

Details of the previous implementation can be found here:
[http://https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1997._Issues_related_to_meta-reviewing E1997]

Our goals are to update the logic, refactor non-DRY code, and fix bugs.

== '''[https://github.com/RPHolloway/expertiza/issues/32 #32 - Remove all debug comments in the UI]''' ==

===Issue Description===

It was noted in the previous team's implementation that multiple debugging comments were showing up during their video testing.

Debug messages shown when viewing a completed meta review:
[[File:Debug_comments.png]]

===Changes===

There was a mistake in <code>app/views/response/_review.html.erb</code> where "<<nowiki/>br/><<nowiki/>hr/><<nowiki/>br/>" was being displayed under the review being meta-reviewed. This was due to being included as a string constant rather than html code.

[[File:E2025_debug_comments_response_review.PNG]]

There was an instance in <code>app/views/response/view.html.erb</code> where ruby code responsible for getting a review to be displayed was being interpreted and displayed, even though the response was being displayed as html in the line before. This resulted in the response details being listed in the UI erroneously.

[[File:E2025_debug_response_view.PNG]]

== '''[https://github.com/RPHolloway/expertiza/issues/49 #49 - "Request a new meta-review" button crashes when there are no available reviews]''' ==

===Issue Description===

When there are no valid metareviews to perform (either there are no reivews performed on the assignment, the only valid reviews concern your own work or own reviews, you have completed all allowed metareviews, or you have reviewed all valid reviews already) then the "Request a New Review" button still is visible and displays the following error (correctly) when pressed.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and check "Use meta-review deadline" checkbox

3. Select "Yes" for all submission allowed, Review allowed and Meta-review allowed in all of the deadline types

4. Navigate back to Assignment Tree View and click on "Add participant"

5. Add couple of students(say student1 and student2) to the assignment

6. Impersonate/login as student1 and make a submission

7. Log in as instructor6 as previously described and change the submission due date to be passed so we are currently in the review stage

8. Impersonate/login as student2 and Request for a new review under others work, submit the review

9. Log in as instructor6 as previously described and change the review due date to be passed so we are currently in the meta-review stage

10. Impersonate/login back as student1 (the one with the submission) and Request for a new meta review

[[File:E2025_outofreviews.png]]

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

Here we conditionally hide the button based on whether or not the participant has reviews available to perform. These checks are located in assignment.rb which is the model for an Assignment. The following method was added to assignment.rb and returns true when the participant has a valid meta-review available.

# Returns true if there is a meta-review task available for the user
def metareview_available?(user_id)
metareviewer = AssignmentParticipant.where(user_id: user_id, parent_id: self.id).first
begin
self.response_map_to_metareview(metareviewer)
true
rescue
false
end
end

== '''[https://github.com/RPHolloway/expertiza/issues/27 #27 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database]''' ==

===Issue Description===

Currently, the number of required and allowed meta-reviews is always being set to 3 in the database. Per the specs of the feature, it should default the database values to NULL. When checkbox "Use meta-review deadline", in the "Due Dates" tab, is selected then the view should populate required and allowed meta-reviews to 3.

===Changes===

We created a migration that defaulted the metareviews_allowed and metareviews_required in the Assignment table to nil instead of what the previous team had as 3.

[[File:E2025_13.png]]

Since the number of meta-reviews allowed and required are recorded using the values in the text fields, changes were made to <code>app/views/assignment/edit/_review_strategy.html.erb</code> to set the values and display them as expected.

When the checkbox for setting meta-review limits is unchecked, the values of number of meta-reviews allowed and required should be null. When the checkbox is checked, the values should be set to 3 and 3 by default, or to the values already defined before (in case the assignment was saved with the values set already). On page load, if the checkbox is enabled, then the inputs should be shown with the default or pre-set values, but if the checkbox is disabled then they should be hidden and set to null.

[[File:E2025_metareview_limit_1.PNG]]

[[File:E2025_metareview_limit_2.PNG]]

Changes had to be made to <code>app/views/student_review/list.html.erb</code> to account for the values being <code>null</code>.

[[File:E2025_metareview_limit_3.PNG]]

===Testing===

Tests have been created that test if a metareview can have nil limits. In this case, no limit will be shown to the user. Before this change that was impossible

== '''Issue 5 - Meta Review Limit is Not Functioning Correctly in Assignment Creation''' ==

===Issue Description===

The "has meta review limit" should determine whether a limit or requirement is placed on the student to complete meta reviews. If disabled, then they should be able to complete as many as they want. This is independent of the Meta Review STAGE that is enabled/disabled in the due dates tab. They should not impact each other and currently do within the assignment creation.

===Reproducing Existing Issue===

1. If the "Use meta-review deadline" is disabled, then the Meta Review Limit panel disappears

2. "Has meta-review limit?" is hidden on edit

3. Possibility the "Has meta-review limit?" not being saved on assignment creation?

4. If the "Use meta-review deadline" is toggled (disabled, enabled), then the "Has meta-review limit?" resets to disabled

===Proposed Changes===

The code in _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder is lazily written and does not really adhere to the project specifications of how metareviews should work on assignment creation. These views need to be edited to work correctly and also respond to the changes in issue 5.

===Changes===

The changes were too long to screenshot, so here is the specific diff section:

https://github.com/expertiza/expertiza/pull/1715/files#diff-540fd2bd6c34b48213716b1dd713733a
https://github.com/expertiza/expertiza/pull/1715/files#diff-80f39fa5f62c73ff8741b02d3b11656c

== '''Discovered Bugs:''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/48 #48 - Number of meta-reviews allowed for an assignment is always nil]''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/45 #45 - "Request a new meta-review" button bugs]''' ==

===Issue Description===

The "Request new a meta-review" button should be hidden when:
# The number of meta-reviews in-progress is greater than the maximum allowed outstanding reviews.
# The number of reviews allowed is limited AND the total number of meta-reviews completed or in-progress is greater than the number allowed.
# There are no valid reviews available for the participant.

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 0 reviewers. The user should NOT be able to see the 'Request a new meta-review to perform' button when there are no reviews available.
** with 1 reviewer.
*** A student should NOT be able to request a meta-review on their own review.
*** When the meta-review limit on the assignment is set to 1 then a student should NOT be able to request a second meta-review.
*** The user should not be able to see the 'Request a new meta-review to perform' button when they have performed all valid reviews.
** with 4 reviewers.
*** The user should be able to click the 'Begin' button and still see the 'Request a new meta-review to perform' button.
*** The user should NOT be able to see the 'Request a new meta-review to perform' button when they have reached the meta-review limit.
*** If the user requests two meta-reviews but has not submitted, then they should not be able to request a new meta-review.
*** The user should be able to request a meta-review if they are above their required but below the allowed number of meta-reviews.

== '''[https://github.com/RPHolloway/expertiza/issues/30 #30 - “Numbers of Meta-reviews left:” does not update after a user requests a review.]''' ==
===Issue Description===
Number of meta-reviews left should decrement when a user requests a meta-review. Current behavior only decremented when a review was submitted.

===Changes===

Previous behavior:

Number of Meta-Reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_completed %>

Fixed behavior:

Number of meta-reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_total %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user should see the number of meta-reviews left decrement after a new meta-review is requested.
*** The user should see the number of meta-reviews left decrement after a new meta-review is completed.

== '''[https://github.com/RPHolloway/expertiza/issues/43 #43 - "Review Allowed" flag is required to be set on a due_date for a user to perform a meta-review.]''' ==
===Issue Description===
When an assignment with a meta-review is created the round that meta-reviews will be performed in should have the "Meta-review allowed" option set to yes. This should be the only requirement for making meta-reviews available in that round. The problem with the current implementation is that meta-reviews can only be performed in rounds where the "Review allowed" option is set.

[[File:E2025 Issue43.png]]

===Changes===
Code changes were applied to app/views/student_review/_responses.html.erb. Since this partial is shared by both reviews and meta-reviews in list.html.erb the title variable was used to determine if the begin option should be active or not.

Previous behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" && @assignment.can_review(topic_id) %>
Current behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" &&
((@assignment.can_review(topic_id) && title == "Review") ||
(@assignment.metareview_allowed(topic_id) && title == "Meta-review")) %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. All meta-review tests make meta-reviews allowed only during the meta-review round. This means that if the simple meta-review test is passing then this feature is working as intended.

== '''[https://github.com/RPHolloway/expertiza/issues/50 #50 - The submit button is not available for meta-reviews.]''' ==
===Issue Description===
Users should see save and submit buttons as part of the meta-review form. These buttons should have the same behavior as the review form. The problem right now is there is not submit button for meta-reviews.
===Changes===
Code changes were made adding 'Metareview' to the conditional that determines if the submit button should be shown or not.

Previous behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review')) %>
New behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review') || (@title.eql? 'Metareview'))%>
<%= submit_tag "Submit "+@title, :name => "Submit", :id => "Submit" %>
<img src="/assets/info.png" title='- SAVE will only store the peer-review as draft and the peer-review will not be displayed to the reviewee. After clicking SUBMIT, the peer-review will be displayed to reviewee, and you will not be able to update it any more.'>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user is able to SUBMIT a simple meta-review.

== '''Testing''' ==

=== Manual Testing ===

1. Log in as super_administrator2. Password is "password". Click on mange...-> Assignments.

2. Create a new Assignment by clicking the "+" button. Give the assignment these fields: Name, Course and Submission Directory.

[[File:New_assignment_general.png]]

3. Go to "Due Dates" tab and check "Use meta-review deadline" and then set the due dates 1 into the FUTURE for each review round and meta-review. Leave everything else as default as shown below:

[[File:New_assignment_duedates.png]]

4. Go to the "Rubrics" tab and populate as shown below:

[[File:New_assignment_rubrics.png]]

5. OPTIONAL: Go to "Review strategy page and select "Has meta-review limit?" and set the number of allowed and required meta-reviews.

6. Once done, "Create" the assignment.

7. Go back to Assignments. manage...-> Assignments.

8. Find the newly created assignment and click on "Add participant". Add the students below:

[[File:add_participants.png]]

9. Go to manage...Impersonate User and impersonate "Student485"

10. Go to the newly created assignment and submit a link for the assignment.

11. Revert back to super_administrator2 and change the submission due date to a date in the PAST so we are currently in the review stage.

12. Impersonate student486 and request for a new review under others work, submit the review.

13. Revert back to super_administrator2 as previously described and change the review due date to be passed so we are currently in the meta-review stage.

14. Impersonate as student485 and Request for a new meta review.

15. Press "Begin" and complete the meta-review

16. When the review is complete, the "Begin" should disappear and be replaced with "View" and "Edit"

===Automated Test Plan===
1. If I create an assignment, then the number of meta-reviews is set to null.

2. If 'Use metareview deadline' is not enabled in the due dates tab, then the meta-review fields in the review strategy tab are not visible.

3. If the metareview limit on the assignment is set to 3, then a student will see they need to submit 3 meta reviews

4. If the metareview deadline is enabled, the assignment cannot be created without a metareview rubric

5. If the assignment created is in a stage that does not have metareview enabled, a metareview cannot be created

6. If the assignment does not have any legal meta reviews, then the "Request a new metareview to perform" button is not visible

7. A student should not be able to request a metareview if they have reached the limit of their allowed reviews

8. A student should be able to request a metareview if they are above their required but below their allowed reviews

9. A student should not be able to request a metareview about their own work.

10. A student should not be able to request a metareview about themselves.

11. If there is a valid metareview to perform related to two other students, then the metareview should be able to be performed.

12. If a student has requested two metareviewes but have not submitted it, then they should not be able to request a new metareview.

13. If the metareview limit on the assignment is set to 1 then a student should not be able to request a second meta review

===Coverage===
Current test coverage is 41.348%. Code coverage will increase based on the test plan above and then if the failing test case described below would be fixed.

Travis CI is currently failing because of:
Failures:
1) assignment function general tab should edit quiz number available to students
Failure/Error:
child_nodes.each do |node|
initialize_fnode_update_children(params, node, tmp_res)
end
NoMethodError:
undefined method `each' for "":String
# ./app/controllers/tree_display_controller.rb:209:in `children_node_ng'
# ./config/diagnostic.rb:11:in `call'
# ------------------
# --- Caused by: ---
# Capybara::CapybaraError:
# Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
# /home/travis/build/expertiza/expertiza/vendor/bundle/ruby/2.2.0/gems/capybara-2.17.0/lib/capybara/session.rb:145:in `raise_server_error!'

File:E2025 metareview limit 3.PNG

2020-04-25T21:56:52Z

Pjloheid:

File:E2025 metareview limit 2.PNG

2020-04-25T21:54:23Z

Pjloheid:

File:E2025 metareview limit 1.PNG

2020-04-25T21:54:12Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-25T21:41:24Z

Pjloheid: /* #27 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database */

== '''Overview of Project''' ==

In Expertiza, meta-review is a feature that enables a third party to evaluate the quality of feedback that a reviewer gives a reviewee. meta-review is an important feature that can help students become better reviewers for their peers by providing detailed feedback that is actionable and positively formulated. Unfortunately, this feature is broken and the following issues were identified. The task is to fix these issues so that this feature can be used again.

* An assignment is deemed to have meta-reviews if and only if there is a meta-review deadline for this assignment in the `assignments_questionnaires` table.
* If there is no meta-review deadline, then don’t show the question about whether there is a max meta-review limit.
* In the DB the default number of meta-reviews allowed and required should be null.
* As soon as someone checks the Has meta-review Limit box on the Review Strategy tab, the UI should fill in 3 and 3 as the required and allowed number.
* Student pages should show the list of reviews that have been performed.
* When a user requests a new meta-review, the field which says the number of meta-reviews left ( refer to above image “Numbers of Meta-reviews left:”) does not change and the button to request a new meta-review disappears.

===Previous Work===

This assignment is a repeat of E1997 which was assigned to students last semester, Fall 2019.

Details of the previous implementation can be found here:
[http://https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1997._Issues_related_to_meta-reviewing E1997]

Our goals are to update the logic, refactor non-DRY code, and fix bugs.

== '''[https://github.com/RPHolloway/expertiza/issues/32 #32 - Remove all debug comments in the UI]''' ==

===Issue Description===

It was noted in the previous team's implementation that multiple debugging comments were showing up during their video testing.

Debug messages shown when viewing a completed meta review:
[[File:Debug_comments.png]]

===Changes===

There was a mistake in <code>app/views/response/_review.html.erb</code> where "<<nowiki/>br/><<nowiki/>hr/><<nowiki/>br/>" was being displayed under the review being meta-reviewed. This was due to being included as a string constant rather than html code.

[[File:E2025_debug_comments_response_review.PNG]]

There was an instance in <code>app/views/response/view.html.erb</code> where ruby code responsible for getting a review to be displayed was being interpreted and displayed, even though the response was being displayed as html in the line before. This resulted in the response details being listed in the UI erroneously.

[[File:E2025_debug_response_view.PNG]]

== '''[https://github.com/RPHolloway/expertiza/issues/49 #49 - "Request a new meta-review" button crashes when there are no available reviews]''' ==

===Issue Description===

When there are no valid metareviews to perform (either there are no reivews performed on the assignment, the only valid reviews concern your own work or own reviews, you have completed all allowed metareviews, or you have reviewed all valid reviews already) then the "Request a New Review" button still is visible and displays the following error (correctly) when pressed.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and check "Use meta-review deadline" checkbox

3. Select "Yes" for all submission allowed, Review allowed and Meta-review allowed in all of the deadline types

4. Navigate back to Assignment Tree View and click on "Add participant"

5. Add couple of students(say student1 and student2) to the assignment

6. Impersonate/login as student1 and make a submission

7. Log in as instructor6 as previously described and change the submission due date to be passed so we are currently in the review stage

8. Impersonate/login as student2 and Request for a new review under others work, submit the review

9. Log in as instructor6 as previously described and change the review due date to be passed so we are currently in the meta-review stage

10. Impersonate/login back as student1 (the one with the submission) and Request for a new meta review

[[File:E2025_outofreviews.png]]

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

Here we conditionally hide the button based on whether or not the participant has reviews available to perform. These checks are located in assignment.rb which is the model for an Assignment. The following method was added to assignment.rb and returns true when the participant has a valid meta-review available.

# Returns true if there is a meta-review task available for the user
def metareview_available?(user_id)
metareviewer = AssignmentParticipant.where(user_id: user_id, parent_id: self.id).first
begin
self.response_map_to_metareview(metareviewer)
true
rescue
false
end
end

== '''[https://github.com/RPHolloway/expertiza/issues/27 #27 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database]''' ==

===Issue Description===

Currently, the number of required and allowed meta-reviews is always being set to 3 in the database. Per the specs of the feature, it should default the database values to NULL. When checkbox "Use meta-review deadline", in the "Due Dates" tab, is selected then the view should populate required and allowed meta-reviews to 3.

===Changes===

== '''Issue 5 - Meta Review Limit is Not Functioning Correctly in Assignment Creation''' ==

===Issue Description===

The "has meta review limit" should determine whether a limit or requirement is placed on the student to complete meta reviews. If disabled, then they should be able to complete as many as they want. This is independent of the Meta Review STAGE that is enabled/disabled in the due dates tab. They should not impact each other and currently do within the assignment creation.

===Reproducing Existing Issue===

1. If the "Use meta-review deadline" is disabled, then the Meta Review Limit panel disappears

2. "Has meta-review limit?" is hidden on edit

3. Possibility the "Has meta-review limit?" not being saved on assignment creation?

4. If the "Use meta-review deadline" is toggled (disabled, enabled), then the "Has meta-review limit?" resets to disabled

===Proposed Changes===

The code in _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder is lazily written and does not really adhere to the project specifications of how metareviews should work on assignment creation. These views need to be edited to work correctly and also respond to the changes in issue 5.

== '''Discovered Bugs:''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/48 #48 - Number of meta-reviews allowed for an assignment is always nil]''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/45 #45 - "Request a new meta-review" button bugs]''' ==

===Issue Description===

The "Request new a meta-review" button should be hidden when:
# The number of meta-reviews in-progress is greater than the maximum allowed outstanding reviews.
# The number of reviews allowed is limited AND the total number of meta-reviews completed or in-progress is greater than the number allowed.
# There are no valid reviews available for the participant.

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 0 reviewers. The user should NOT be able to see the 'Request a new meta-review to perform' button when there are no reviews available.
** with 1 reviewer.
*** A student should NOT be able to request a meta-review on their own review.
*** When the meta-review limit on the assignment is set to 1 then a student should NOT be able to request a second meta-review.
*** The user should not be able to see the 'Request a new meta-review to perform' button when they have performed all valid reviews.
** with 4 reviewers.
*** The user should be able to click the 'Begin' button and still see the 'Request a new meta-review to perform' button.
*** The user should NOT be able to see the 'Request a new meta-review to perform' button when they have reached the meta-review limit.
*** If the user requests two meta-reviews but has not submitted, then they should not be able to request a new meta-review.
*** The user should be able to request a meta-review if they are above their required but below the allowed number of meta-reviews.

== '''[https://github.com/RPHolloway/expertiza/issues/30 #30 - “Numbers of Meta-reviews left:” does not update after a user requests a review.]''' ==
===Issue Description===
Number of meta-reviews left should decrement when a user requests a meta-review. Current behavior only decremented when a review was submitted.

===Changes===

Previous behavior:

Number of Meta-Reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_completed %>

Fixed behavior:

Number of meta-reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_total %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user should see the number of meta-reviews left decrement after a new meta-review is requested.
*** The user should see the number of meta-reviews left decrement after a new meta-review is completed.

== '''[https://github.com/RPHolloway/expertiza/issues/43 #43 - "Review Allowed" flag is required to be set on a due_date for a user to perform a meta-review.]''' ==
===Issue Description===
When an assignment with a meta-review is created the round that meta-reviews will be performed in should have the "Meta-review allowed" option set to yes. This should be the only requirement for making meta-reviews available in that round. The problem with the current implementation is that meta-reviews can only be performed in rounds where the "Review allowed" option is set.

[[File:E2025 Issue43.png]]

===Changes===
Code changes were applied to app/views/student_review/_responses.html.erb. Since this partial is shared by both reviews and meta-reviews in list.html.erb the title variable was used to determine if the begin option should be active or not.

Previous behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" && @assignment.can_review(topic_id) %>
Current behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" &&
((@assignment.can_review(topic_id) && title == "Review") ||
(@assignment.metareview_allowed(topic_id) && title == "Meta-review")) %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. All meta-review tests make meta-reviews allowed only during the meta-review round. This means that if the simple meta-review test is passing then this feature is working as intended.

== '''[https://github.com/RPHolloway/expertiza/issues/50 #50 - The submit button is not available for meta-reviews.]''' ==
===Issue Description===
Users should see save and submit buttons as part of the meta-review form. These buttons should have the same behavior as the review form. The problem right now is there is not submit button for meta-reviews.
===Changes===
Code changes were made adding 'Metareview' to the conditional that determines if the submit button should be shown or not.

Previous behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review')) %>
New behavior:
<% if @title && ((@title.eql? 'Review') || (@title.eql? 'Self Review') || (@title.eql? 'Metareview'))%>
<%= submit_tag "Submit "+@title, :name => "Submit", :id => "Submit" %>
<img src="/assets/info.png" title='- SAVE will only store the peer-review as draft and the peer-review will not be displayed to the reviewee. After clicking SUBMIT, the peer-review will be displayed to reviewee, and you will not be able to update it any more.'>
<% end %>

===Testing===
Functional tests from a student perspective are in /spec/features/meta_review_spec.rb. The tests assume the allowed number of meta-reviews is 5 and the required number of meta-reviews is 3. Tests for this feature are explained in summary below:
* For assignments that have a meta-review deadline...
** with 1 reviewer.
*** The user is able to SUBMIT a simple meta-review.

== '''Testing''' ==

=== Manual Testing ===

1. Log in as super_administrator2. Password is "password". Click on mange...-> Assignments.

2. Create a new Assignment by clicking the "+" button. Give the assignment these fields: Name, Course and Submission Directory.

[[File:New_assignment_general.png]]

3. Go to "Due Dates" tab and check "Use meta-review deadline" and then set the due dates 1 into the FUTURE for each review round and meta-review. Leave everything else as default as shown below:

[[File:New_assignment_duedates.png]]

4. Go to the "Rubrics" tab and populate as shown below:

[[File:New_assignment_rubrics.png]]

5. OPTIONAL: Go to "Review strategy page and select "Has meta-review limit?" and set the number of allowed and required meta-reviews.

6. Once done, "Create" the assignment.

7. Go back to Assignments. manage...-> Assignments.

8. Find the newly created assignment and click on "Add participant". Add the students below:

[[File:add_participants.png]]

9. Go to manage...Impersonate User and impersonate "Student485"

10. Go to the newly created assignment and submit a link for the assignment.

11. Revert back to super_administrator2 and change the submission due date to a date in the PAST so we are currently in the review stage.

12. Impersonate student486 and request for a new review under others work, submit the review.

13. Revert back to super_administrator2 as previously described and change the review due date to be passed so we are currently in the meta-review stage.

14. Impersonate as student485 and Request for a new meta review.

15. Press "Begin" and complete the meta-review

16. When the review is complete, the "Begin" should disappear and be replaced with "View" and "Edit"

===Automated Test Plan===
1. If I create an assignment, then the number of meta-reviews is set to null.

2. If 'Use metareview deadline' is not enabled in the due dates tab, then the meta-review fields in the review strategy tab are not visible.

3. If the metareview limit on the assignment is set to 3, then a student will see they need to submit 3 meta reviews

4. If the metareview deadline is enabled, the assignment cannot be created without a metareview rubric

5. If the assignment created is in a stage that does not have metareview enabled, a metareview cannot be created

6. If the assignment does not have any legal meta reviews, then the "Request a new metareview to perform" button is not visible

7. A student should not be able to request a metareview if they have reached the limit of their allowed reviews

8. A student should be able to request a metareview if they are above their required but below their allowed reviews

9. A student should not be able to request a metareview about their own work.

10. A student should not be able to request a metareview about themselves.

11. If there is a valid metareview to perform related to two other students, then the metareview should be able to be performed.

12. If a student has requested two metareviewes but have not submitted it, then they should not be able to request a new metareview.

13. If the metareview limit on the assignment is set to 1 then a student should not be able to request a second meta review

===Coverage===
Current test coverage is 41.348%. Code coverage will increase based on the test plan above and the failing test case described below will be fixed.

Travis CI is currently failing because of:
Failures:
1) assignment function general tab should edit quiz number available to students
Failure/Error:
child_nodes.each do |node|
initialize_fnode_update_children(params, node, tmp_res)
end
NoMethodError:
undefined method `each' for "":String
# ./app/controllers/tree_display_controller.rb:209:in `children_node_ng'
# ./config/diagnostic.rb:11:in `call'
# ------------------
# --- Caused by: ---
# Capybara::CapybaraError:
# Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
# /home/travis/build/expertiza/expertiza/vendor/bundle/ruby/2.2.0/gems/capybara-2.17.0/lib/capybara/session.rb:145:in `raise_server_error!'

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-25T16:15:28Z

Pjloheid: /* Proposed Changes */

== '''Overview of Project''' ==

In Expertiza, meta-review is a feature that enables a third party to evaluate the quality of feedback that a reviewer gives a reviewee. meta-review is an important feature that can help students become better reviewers for their peers by providing detailed feedback that is actionable and positively formulated. Unfortunately, this feature is broken and the following issues were identified. The task is to fix these issues so that this feature can be used again.

* An assignment is deemed to have meta-reviews if and only if there is a meta-review deadline for this assignment in the `assignments_questionnaires` table.
* If there is no meta-review deadline, then don’t show the question about whether there is a max meta-review limit.
* In the DB the default number of meta-reviews allowed and required should be null.
* As soon as someone checks the Has meta-review Limit box on the Review Strategy tab, the UI should fill in 3 and 3 as the required and allowed number.
* Student pages should show the list of reviews that have been performed.
* When a user requests a new meta-review, the field which says the number of meta-reviews left ( refer to above image “Numbers of Meta-reviews left:”) does not change and the button to request a new meta-review disappears.

===Previous Work===

This assignment is a repeat of E1997 which was assigned to students last semester, Fall 2019.

Details of the previous implementation can be found here:
[http://https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Fall_2019_-_E1997._Issues_related_to_meta-reviewing E1997]

Our goals are to update the logic, refactor non-DRY code, and fix bugs.

===Meta-Review Flow Design===

This diagram describes the interactions of the meta-review system.(From E1997)

[[File:E1997_1.PNG]]

== '''Issue 1 - Removing Debugging Comments''' ==

===Issue Description===

It was noted in the previous team's implementation that multiple debugging comments were showing up during their video testing.

Debug messages shown when viewing a completed meta review:
[[File:Debug_comments.png]]

===Changes===

There was a mistake in <code>app/views/response/_review.html.erb</code> where "<<nowiki/>br/><<nowiki/>hr/><<nowiki/>br/>" was being displayed under the review being meta-reviewed. This was due to being included as a string constant rather than html code.

[[File:E2025_debug_comments_response_review.PNG]]

There was an instance in <code>app/views/response/view.html.erb</code> where ruby code responsible for getting a review to be displayed was being interpreted and displayed, even though the response was being displayed as html in the line before. This resulted in the response details being listed in the UI erroneously.

[[File:E2025_debug_response_view.PNG]]

== '''[https://github.com/RPHolloway/expertiza/issues/49 #49 - "Request a new meta-review" button crashes when there are no available reviews]''' ==

===Issue Description===

When there are no valid metareviews to perform (either there are no reivews performed on the assignment, the only valid reviews concern your own work or own reviews, you have completed all allowed metareviews, or you have reviewed all valid reviews already) then the "Request a New Review" button still is visible and displays the following error (correctly) when pressed.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and check "Use meta-review deadline" checkbox

3. Select "Yes" for all submission allowed, Review allowed and Meta-review allowed in all of the deadline types

4. Navigate back to Assignment Tree View and click on "Add participant"

5. Add couple of students(say student1 and student2) to the assignment

6. Impersonate/login as student1 and make a submission

7. Log in as instructor6 as previously described and change the submission due date to be passed so we are currently in the review stage

8. Impersonate/login as student2 and Request for a new review under others work, submit the review

9. Log in as instructor6 as previously described and change the review due date to be passed so we are currently in the meta-review stage

10. Impersonate/login back as student1 (the one with the submission) and Request for a new meta review

[[File:E2025_outofreviews.png]]

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

Here we conditionally hide the button based on whether or not the participant has reviews available to perform. These checks are located in assignment.rb which is the controller for Assignment. The following method was added to assignment.rb and returns true when the participant has a valid meta-review available.

# Returns true if there is a meta-review task available for the user
def metareview_available?(user_id)
metareviewer = AssignmentParticipant.where(user_id: user_id, parent_id: self.id).first
begin
self.response_map_to_metareview(metareviewer)
true
rescue
false
end
end

== '''Issue 3 - Private Methods''' ==

===Issue Description===
Within the code for the meta-reviewing, it is noted that many statements could be moved to private methods to avoid conflating the purpose of the method with meta-reviewing specifics. We will use the code climate as a guide to reduce the block size and use existing or new private methods when possible to make the logic of the program easier to follow.

===Proposed Changes===
On first inspection, this will mainly apply to the code that is located in the Response Controller (response_conotroller.rb) and the view for "list.html.erb" under app/views/student_review.

For example: The "new" method has meta review checks that should probably be abstracted and placed as private methods.

We will also continue applying this to any changes that we make across the course of the project. This also will apply to the scripts that are used in the _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder. They should be extracted to somewhere else.

== '''Issue 5 - Defaulting Required and Allowed Meta-reviews to NULL in Assignment Database''' ==

===Issue Description===

Currently, the number of required and allowed meta-reviews is always being set to 3 in the database. Per the specs of the feature, it should default the database values to NULL. When checkbox "Use meta-review deadline", in the "Due Dates" tab, is selected then the view should populate required and allowed meta-reviews to 3.

===Reproducing Existing Issue===

1. Log in as instructor6, click on mange...-> Assignments

2. Click on edit assignment, navigate to Due Dates and confirm that "Use meta-review deadline" checkbox is unchecked

3. Use rails c to access the DB

4. Observe that all assignments contain the incorrect db value (3 for all) for Assignments in the db

===Proposed Change===

This will just require making a new migration in the Assignment database to set the default to NULL instead of 3, which we believe it is now on default. This may also bring up other issues in the code in the view by allowing assignments without meta-review limits, but we are unable to see that at this time.

file: schema.rb

create_table "assignments", force: :cascade do |t|
...
t.boolean "is_anonymous", default: true
t.integer "num_reviews_required", limit: 4, default: 3
t.integer "num_metareviews_required", limit: 4, default: '''NULL'''
t.integer "num_metareviews_allowed", limit: 4, default: '''NULL'''
t.integer "num_reviews_allowed", limit: 4, default: 3
t.integer "simicheck", limit: 4, default: -1
...
end

file: _review_strategy.html.erb

This file has all the logic to populate the view with a default value once the "Use meta-review deadline" is checked. No modifications needed.

== '''Issue 6 - Meta Review Limit is Not Functioning Correctly in Assignment Creation''' ==

===Issue Description===

The "has meta review limit" should determine whether a limit or requirement is placed on the student to complete meta reviews. If disabled, then they should be able to complete as many as they want. This is independent of the Meta Review STAGE that is enabled/disabled in the due dates tab. They should not impact each other and currently do within the assignment creation.

===Reproducing Existing Issue===

1. If the "Use meta-review deadline" is disabled, then the Meta Review Limit panel disappears

2. "Has meta-review limit?" is hidden on edit

3. Possibility the "Has meta-review limit?" not being saved on assignment creation?

4. If the "Use meta-review deadline" is toggled (disabled, enabled), then the "Has meta-review limit?" resets to disabled

===Proposed Changes===

The code in _due_dates.html.erb and the _review_strategy.html.erb within the /app/app/views/assignments/edit folder is lazily written and does not really adhere to the project specifications of how metareviews should work on assignment creation. These views need to be edited to work correctly and also respond to the changes in issue 5.

== '''[https://github.com/RPHolloway/expertiza/issues/48 #48 - Number of meta-reviews allowed for an assignment is always nil]''' ==

== '''[https://github.com/RPHolloway/expertiza/issues/45 #45 - "Request a new meta-review" button bugs]''' ==

===Issue Description===

The "Request new a meta-review" button should be hidden when:
# The number of meta-reviews in-progress is greater than the maximum allowed outstanding reviews.
# The number of reviews allowed is limited AND the total number of meta-reviews completed or in-progress is greater than the number allowed.
# There are no valid reviews available for the participant.

===Changes===

This issue deals with the view "list.html.erb" under /app/views/student_review. The code below shows the conditional used to display the "Request new meta-review" button.

<% if @num_metareviews_in_progress >= Assignment.max_outstanding_reviews %>
Note: You can't have more than 2 outstanding meta-reviews. You must complete one of your
outstanding meta-reviews before selecting another.
<% elsif @assignment.num_metareviews_allowed && @num_metareviews_total == @assignment.num_metareviews_allowed %>
Note: You can not do more than <%=@assignment.num_metareviews_allowed%> meta-reviews according
to assignment policy.
<% elsif (!@assignment.metareview_available?(session[:user].id)) %>
Meta-reviews cannot be performed at this time.
<% else %>
<%= render :partial => 'set_dynamic_metareview', :locals => {:assignment => @assignment} %>
<% end %>

[https://github.com/RPHolloway/expertiza/issues/31 #31 - Tests]

== '''[https://github.com/RPHolloway/expertiza/issues/30 #30 - “Numbers of Meta-reviews left:” does not update after a user requests a review.]''' ==
===Issue Description===
Number of meta-reviews left should decrement when a user requests a meta-review. Current behavior only decremented when a review was submitted.

===Changes===

Previous behavior:

Number of Meta-Reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_completed %>

Fixed behavior:

Number of meta-reviews left: <%= @assignment.num_metareviews_allowed - @num_metareviews_total %>

== '''[https://github.com/RPHolloway/expertiza/issues/43 #43 - "Review Allowed" flag is required to be set on a due_date for a user to perform a meta-review.]''' ==
===Issue Description===
When an assignment with a meta-review is created the round that meta-reviews will be performed in should have the "Meta-review allowed" option set to yes. This should be the only requirement for making meta-reviews available in that round. The problem with the current implementation is that meta-reviews can only be performed in rounds where the "Review allowed" option is set.

[[File:E2025 Issue43.png]]

===Changes===
Code changes were applied to app/views/student_review/_responses.html.erb. Since this partial is shared by both reviews and meta-reviews in list.html.erb the title variable was used to determine if the begin option should be active or not.

Previous behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" && @assignment.can_review(topic_id) %>
Current behavior:
<% elsif @assignment.get_current_stage(topic_id) != "Complete" &&
((@assignment.can_review(topic_id) && title == "Review") ||
(@assignment.metareview_allowed(topic_id) && title == "Meta-review")) %>

[https://github.com/RPHolloway/expertiza/issues/29 #29 - Tests]

== '''[https://github.com/RPHolloway/expertiza/issues/50 #50 - The submit button is not available for meta-reviews.]''' ==

== '''Testing''' ==

=== Manual Testing ===

1. Log in as super_administrator2. Password is "password". Click on mange...-> Assignments.

2. Create a new Assignment by clicking the "+" button. Give the assignment these fields: Name, Course and Submission Directory.

[[File:New_assignment_general.png]]

3. Go to "Due Dates" tab and check "Use meta-review deadline" and then set the due dates 1 into the FUTURE for each review round and meta-review. Leave everything else as default as shown below:

[[File:New_assignment_duedates.png]]

4. Go to the "Rubrics" tab and populate as shown below:

[[File:New_assignment_rubrics.png]]

5. OPTIONAL: Go to "Review strategy page and select "Has meta-review limit?" and set the number of allowed and required meta-reviews.

6. Once done, "Create" the assignment.

7. Go back to Assignments. manage...-> Assignments.

8. Find the newly created assignment and click on "Add participant". Add the students below:

[[File:add_participants.png]]

9. Go to manage...Impersonate User and impersonate "Student485"

10. Go to the newly created assignment and submit a link for the assignment.

11. Revert back to super_administrator2 and change the submission due date to a date in the PAST so we are currently in the review stage.

12. Impersonate student486 and request for a new review under others work, submit the review.

13. Revert back to super_administrator2 as previously described and change the review due date to be passed so we are currently in the meta-review stage.

14. Impersonate as student485 and Request for a new meta review.

15. Press "Begin" and complete the meta-review

16. When the review is complete, the "Begin" should disappear and be replaced with "View" and "Edit"

===Automated Test Plan===
1. If I create an assignment, then the number of meta-reviews is set to null.

2. If 'Use metareview deadline' is not enabled in the due dates tab, then the meta-review fields in the review strategy tab are not visible.

3. If the metareview limit on the assignment is set to 3, then a student will see they need to submit 3 meta reviews

4. If the metareview deadline is enabled, the assignment cannot be created without a metareview rubric

5. If the assignment created is in a stage that does not have metareview enabled, a metareview cannot be created

6. If the assignment does not have any legal meta reviews, then the "Request a new metareview to perform" button is not visible

7. A student should not be able to request a metareview if they have reached the limit of their allowed reviews

8. A student should be able to request a metareview if they are above their required but below their allowed reviews

9. A student should not be able to request a metareview about their own work.

10. A student should not be able to request a metareview about themselves.

11. If there is a valid metareview to perform related to two other students, then the metareview should be able to be performed.

12. If a student has requested two metareviewes but have not submitted it, then they should not be able to request a new metareview.

13. If the metareview limit on the assignment is set to 1 then a student should not be able to request a second meta review

===Coverage===
Current test coverage is 41.348%. Code coverage will increase based on the test plan above and the failing test case described below will be fixed.

Travis CI is currently failing because of:
Failures:
1) assignment function general tab should edit quiz number available to students
Failure/Error:
child_nodes.each do |node|
initialize_fnode_update_children(params, node, tmp_res)
end
NoMethodError:
undefined method `each' for "":String
# ./app/controllers/tree_display_controller.rb:209:in `children_node_ng'
# ./config/diagnostic.rb:11:in `call'
# ------------------
# --- Caused by: ---
# Capybara::CapybaraError:
# Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
# /home/travis/build/expertiza/expertiza/vendor/bundle/ruby/2.2.0/gems/capybara-2.17.0/lib/capybara/session.rb:145:in `raise_server_error!'

File:E2025 debug response view.PNG

2020-04-25T16:14:45Z

Pjloheid:

File:E2025 debug comments response review.PNG

2020-04-25T16:03:19Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2025: Issues Related to Meta - Reviewing

2020-04-04T23:35:39Z

Pjloheid: Creation of page

==Intro==

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-04-01T02:42:37Z

Pjloheid: /* Test Plan */

==E2006. Refactor tree_display_controller.rb==

__TOC__

==Project Description==

===Problem Statement===
[https://docs.google.com/document/d/1_-rgdE4Fh_Fz_Jhxw1Qj4vehbu84qiUKAiLWGcFUl5A/edit#heading=h.h37uqoqtoks4 E2006. Refactor tree_display_controller.rb]

===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

==Goals==

From the problem background three main goals were derived to focus on in the refactor of tree display controller:
# '''Remove unnecessary methods''' - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
# '''Save code and avoid DRY issues''' - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
# '''Improve understanding''' - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Changes==
The sections below include a link to the commits where the changes were made following the description. We believe that by providing the links to code changes that we will improve the flow of the wiki while giving a better idea of what the code changes look like in each commit. All code changes can be found in pull request [https://github.com/expertiza/expertiza/pull/1698 #1698]

=== Make all file scope methods private ===
Code that is only called within the <code>tree_display_controller.rb</code> was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39 Assign private designation to internal methods]

=== Combine folder_node_ng_getter with children_node_ng ===
<code>folder_node_ng_getter</code> was being called by the view to return <code>FolderNode.get</code>. The resulting json of this call was then passed, without modification, into <code>children_node_ng</code>. Since this was the only place <code>folder_node_ng_getter</code> was being called it was combined with <code>children_node_ng</code> to make things easier to read when when viewing controller code. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng===
<code>children_node_ng</code> was a misleading name. The method has been renamed <code>get_folder_contents</code> since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the <code>get_folder_contents</code> method was refactored to include the behavior of <code>folder_node_ng_getter</code>. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call <code>serialize_folder_to_json</code> makes the code easier to read instead of chasing nested function calls through <code>initialize_fnode_update_children</code> which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng_2===
<code>children_node_ng_2</code> was a misleading name. The method has been renamed <code>get_sub_folder_contents</code> since it returns the subfolder items for the folders obtained by <code>get_folder_contents</code> in json format. Like the <code>get_folder_contents</code> refactor <code>get_sub_folder_contents</code> was simplified to make a single call to <code>serialize_sub_folder_to_json</code> making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/d7a364edce289f0efb8487429ba8fec8038f3557 Refactor children_node_ng_2] [https://github.com/expertiza/expertiza/pull/1698/commits/9a1d5caec559d15e576faf10fb55952cf9d99bf7 Rename children_node_2_ng to get_sub_folder_contents] [https://github.com/expertiza/expertiza/pull/1698/commits/867fa9d3f9d5cc988824ce2890e5ebc90708bf79 Rename serialize_to_json to serialize_sub_folder_to_json]

=== Inline function calls to make json serialization more clear ===
Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. [https://github.com/expertiza/expertiza/pull/1698/commits/d7acad1904a5431d1714008b738bc127582d53a7 Inline function calls to make json serialization more clear]

=== Simplify checks for determining a users access to view a course ===
With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. [https://github.com/expertiza/expertiza/pull/1698/commits/2ea9a2286688ef173613efc1d19e1fdf460af4d0 Simplified is_available checks] [https://github.com/expertiza/expertiza/pull/1698/commits/a9dcbe459ffbab9effa0589b9f618b0a7fb19810 Removed unused is_available checks]

===Collapse 'goto' methods===
In the legacy code found in <code>tree_display_controller.rb</code>, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the <code>goto_controller()</code> method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). [https://github.com/expertiza/expertiza/pull/1698/commits/2732cef5eb3bcf258b61447277bd82d8d58f7c54 Compact gotos]

===Remove unnecessary code to sort assignments by instructor and creation date===
By default assignments are received from <code>FolderNode.get</code> sorted by instructor and creation date. There is no need to resort this object. [https://github.com/expertiza/expertiza/pull/1698/commits/81a401083e5a4fd2d9720fd015dd7d515b808f83 Remove unnecessary code to sort assignments by instructor and creation date]

===Remove unused functions===
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* <code>filter()</code>
* <code>filter_node_is_qan()</code>
These methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
* <code>courses_assignments_obj()</code>
* <code>confirm_notifications_access()</code>
* <code>child_nodes_from_params()</code>
* <code>res_node_for_child()</code>
These methods are not used in the app.
* <code>update_tmp_obj()</code>
This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the <code>serialize_to_json</code> method which called it there was no reason to write a new method to call. Especially since the <code>update_tmp_obj</code> method was only making minor changes to the json.
* <code>bridge_to_is_available()</code>
* <code>action()</code>
These methods are not called in the app. References to these methods were removed from <code>routes.rb</code>

== Test Plan ==

All tests described below have been implemented in the [https://github.com/RPHolloway/expertiza/blob/beta/spec/controllers/tree_display_controller_spec.rb tree_display_controller_spec.rb file]. Since this project was focused on refactoring the tree_display_controller.rb code, and not on adding any functionality, we have edited the spec file to accommodate for our changes. [https://github.com/expertiza/expertiza/pull/1698/files?file-filters%5B%5D=.rb#diff-daf992d845e06cb7f3dff81021736967 All changes to the spec file] correspond to the refactoring that we have done.
;<code>#list</code> should not redirect to tree_display#list if the current user is an instructor.
:A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list

;<code>#list</code> should redirect to student_task#list if current user is a student
:A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.

;<code>#list</code> should redirect to login page if current user is nil.
:This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.

;<code>#ta_for_current_mappings?</code> should return true if the current user is a TA for current course
:This test should check that the method properly returns true if it receives a ta as the user.

;<code>#drill</code> should redirect to list action
:The drill method should redirect to the tree_display controller with the list action.

;<code>GET #session_last_open_tab</code> should return a status of 200.
:The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.

;<code>GET #get_folder_contents</code> should return a list of course objects(private) as a json.
:When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents</code> should return an empty list if no courses
:When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'

;<code>GET #get_folder_contents</code> should return a list of course objects(public) as a json.
:When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents for TA</code> should return an empty array if the logged in user is not a TA.
:Edge case testing to ensure that a user with the role of student results in an empty array.

;<code>GET #get_folder_contents for TA</code> should return a list of course objects the TA is supposed to TA in as json.
:If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.

;<code>GET #get_folder_contents for TA</code> should return an empty list when there is no mapping between a ta and a course.
:If the TA is not mapped to a course then the return should be empty.

;<code>GET #get_folder_contents for TA</code> should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
:Edge case test for when the user is a student in some classes, but a TA in others.

;<code>GET #get_folder_contents for TA</code> should return only the course he/she is TA of when the TA is a student of the same course.
:Edge case test for a student in a course also being a TA, most likely because of an error in assignment.

;<code>GET #get_folder_contents for TA</code> returns only the assignments which belongs to the course he/she is TA of.
:All assignments in the courses that the TA is assigned to should be returned.

;<code>GET #get_folder_contents for TA</code> should return an empty list if none of the assignments belong to the course he/she is TA of.
:Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.

=== Test Results ===

[[File:E2006_test_results.png]]

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

File:E2006 test results.png

2020-04-01T02:41:30Z

Pjloheid: Pjloheid uploaded a new version of File:E2006 test results.png

File:E2006 test results.png

2020-04-01T02:37:30Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-31T23:12:07Z

Pjloheid: /* Changes */

==E2006. Refactor tree_display_controller.rb==

__TOC__

==Project Description==

===Problem Statement===
[https://docs.google.com/document/d/1_-rgdE4Fh_Fz_Jhxw1Qj4vehbu84qiUKAiLWGcFUl5A/edit#heading=h.h37uqoqtoks4 E2006. Refactor tree_display_controller.rb]

===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

==Goals==

From the problem background three main goals were derived to focus on in the refactor of tree display controller:
# '''Remove unnecessary methods''' - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
# '''Save code and avoid DRY issues''' - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
# '''Improve understanding''' - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Changes==
The sections below include a link to the commits where the changes were made following the description. We believe that by providing the links to code changes that we will improve the flow of the wiki while giving a better idea of what the code changes look like in each commit. All code changes can be found in pull request [https://github.com/expertiza/expertiza/pull/1698 #1698]

=== Make all file scope methods private ===
Code that is only called within the <code>tree_display_controller.rb</code> was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39 Assign private designation to internal methods]

=== Combine folder_node_ng_getter with children_node_ng ===
<code>folder_node_ng_getter</code> was being called by the view to return <code>FolderNode.get</code>. The resulting json of this call was then passed, without modification, into <code>children_node_ng</code>. Since this was the only place <code>folder_node_ng_getter</code> was being called it was combined with <code>children_node_ng</code> to make things easier to read when when viewing controller code. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng===
<code>children_node_ng</code> was a misleading name. The method has been renamed <code>get_folder_contents</code> since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the <code>get_folder_contents</code> method was refactored to include the behavior of <code>folder_node_ng_getter</code>. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call <code>serialize_folder_to_json</code> makes the code easier to read instead of chasing nested function calls through <code>initialize_fnode_update_children</code> which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng_2===
<code>children_node_ng_2</code> was a misleading name. The method has been renamed <code>get_sub_folder_contents</code> since it returns the subfolder items for the folders obtained by <code>get_folder_contents</code> in json format. Like the <code>get_folder_contents</code> refactor <code>get_sub_folder_contents</code> was simplified to make a single call to <code>serialize_sub_folder_to_json</code> making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/d7a364edce289f0efb8487429ba8fec8038f3557 Refactor children_node_ng_2] [https://github.com/expertiza/expertiza/pull/1698/commits/9a1d5caec559d15e576faf10fb55952cf9d99bf7 Rename children_node_2_ng to get_sub_folder_contents] [https://github.com/expertiza/expertiza/pull/1698/commits/867fa9d3f9d5cc988824ce2890e5ebc90708bf79 Rename serialize_to_json to serialize_sub_folder_to_json]

=== Inline function calls to make json serialization more clear ===
Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. [https://github.com/expertiza/expertiza/pull/1698/commits/d7acad1904a5431d1714008b738bc127582d53a7 Inline function calls to make json serialization more clear]

=== Simplify checks for determining a users access to view a course ===
With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. [https://github.com/expertiza/expertiza/pull/1698/commits/2ea9a2286688ef173613efc1d19e1fdf460af4d0 Simplified is_available checks] [https://github.com/expertiza/expertiza/pull/1698/commits/a9dcbe459ffbab9effa0589b9f618b0a7fb19810 Removed unused is_available checks]

===Collapse 'goto' methods===
In the legacy code found in <code>tree_display_controller.rb</code>, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the <code>goto_controller()</code> method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). [https://github.com/expertiza/expertiza/pull/1698/commits/2732cef5eb3bcf258b61447277bd82d8d58f7c54 Compact gotos]

===Remove unnecessary code to sort assignments by instructor and creation date===
By default assignments are received from <code>FolderNode.get</code> sorted by instructor and creation date. There is no need to resort this object. [https://github.com/expertiza/expertiza/pull/1698/commits/81a401083e5a4fd2d9720fd015dd7d515b808f83 Remove unnecessary code to sort assignments by instructor and creation date]

===Remove unused functions===
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* <code>filter()</code>
* <code>filter_node_is_qan()</code>
These methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
* <code>courses_assignments_obj()</code>
* <code>confirm_notifications_access()</code>
* <code>child_nodes_from_params()</code>
* <code>res_node_for_child()</code>
These methods are not used in the app.
* <code>update_tmp_obj()</code>
This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the <code>serialize_to_json</code> method which called it there was no reason to write a new method to call. Especially since the <code>update_tmp_obj</code> method was only making minor changes to the json.
* <code>bridge_to_is_available()</code>
* <code>action()</code>
These methods are not called in the app. References to these methods were removed from <code>routes.rb</code>

== Test Plan ==
;<code>#list</code> should not redirect to tree_display#list if the current user is an instructor.
:A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list

;<code>#list</code> should redirect to student_task#list if current user is a student
:A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.

;<code>#list</code> should redirect to login page if current user is nil.
:This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.

;<code>#ta_for_current_mappings?</code> should return true if the current user is a TA for current course
:This test should check that the method properly returns true if it receives a ta as the user.

;<code>#drill</code> should redirect to list action
:The drill method should redirect to the tree_display controller with the list action.

;<code>GET #session_last_open_tab</code> should return a status of 200.
:The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.

;<code>GET #get_folder_contents</code> should return a list of course objects(private) as a json.
:When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents</code> should return an empty list if no courses
:When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'

;<code>GET #get_folder_contents</code> should return a list of course objects(public) as a json.
:When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents for TA</code> should return an empty array if the logged in user is not a TA.
:Edge case testing to ensure that a user with the role of student results in an empty array.

;<code>GET #get_folder_contents for TA</code> should return a list of course objects the TA is supposed to TA in as json.
:If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.

;<code>GET #get_folder_contents for TA</code> should return an empty list when there is no mapping between a ta and a course.
:If the TA is not mapped to a course then the return should be empty.

;<code>GET #get_folder_contents for TA</code> should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
:Edge case test for when the user is a student in some classes, but a TA in others.

;<code>GET #get_folder_contents for TA</code> should return only the course he/she is TA of when the TA is a student of the same course.
:Edge case test for a student in a course also being a TA, most likely because of an error in assignment.

;<code>GET #get_folder_contents for TA</code> returns only the assignments which belongs to the course he/she is TA of.
:All assignments in the courses that the TA is assigned to should be returned.

;<code>GET #get_folder_contents for TA</code> should return an empty list if none of the assignments belong to the course he/she is TA of.
:Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T02:47:17Z

Pjloheid: /* Test Plan */

==E2006. Refactor tree_display_controller.rb==

__TOC__

==Project Description==

===Problem Statement===
[https://docs.google.com/document/d/1_-rgdE4Fh_Fz_Jhxw1Qj4vehbu84qiUKAiLWGcFUl5A/edit#heading=h.h37uqoqtoks4 E2006. Refactor tree_display_controller.rb]

===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

==Goals==

From the problem background three main goals were derived to focus on in the refactor of tree display controller:
# '''Remove unnecessary methods''' - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
# '''Save code and avoid DRY issues''' - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
# '''Improve understanding''' - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Changes==
The sections below include a link to the commits where the changes were made following the description. All code changes can be found in pull request [https://github.com/expertiza/expertiza/pull/1698 #1698]

=== Make all file scope methods private ===
Code that is only called within the <code>tree_display_controller.rb</code> was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39 Assign private designation to internal methods]

=== Combine folder_node_ng_getter with children_node_ng ===
<code>folder_node_ng_getter</code> was being called by the view to return <code>FolderNode.get</code>. The resulting json of this call was then passed, without modification, into <code>children_node_ng</code>. Since this was the only place <code>folder_node_ng_getter</code> was being called it was combined with <code>children_node_ng</code> to make things easier to read when when viewing controller code. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng===
<code>children_node_ng</code> was a misleading name. The method has been renamed <code>get_folder_contents</code> since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the <code>get_folder_contents</code> method was refactored to include the behavior of <code>folder_node_ng_getter</code>. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call <code>serialize_folder_to_json</code> makes the code easier to read instead of chasing nested function calls through <code>initialize_fnode_update_children</code> which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng_2===
<code>children_node_ng_2</code> was a misleading name. The method has been renamed <code>get_sub_folder_contents</code> since it returns the subfolder items for the folders obtained by <code>get_folder_contents</code> in json format. Like the <code>get_folder_contents</code> refactor <code>get_sub_folder_contents</code> was simplified to make a single call to <code>serialize_sub_folder_to_json</code> making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/d7a364edce289f0efb8487429ba8fec8038f3557 Refactor children_node_ng_2] [https://github.com/expertiza/expertiza/pull/1698/commits/9a1d5caec559d15e576faf10fb55952cf9d99bf7 Rename children_node_2_ng to get_sub_folder_contents] [https://github.com/expertiza/expertiza/pull/1698/commits/867fa9d3f9d5cc988824ce2890e5ebc90708bf79 Rename serialize_to_json to serialize_sub_folder_to_json]

=== Inline function calls to make json serialization more clear ===
Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. [https://github.com/expertiza/expertiza/pull/1698/commits/d7acad1904a5431d1714008b738bc127582d53a7 Inline function calls to make json serialization more clear]

=== Simplify checks for determining a users access to view a course ===
With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. [https://github.com/expertiza/expertiza/pull/1698/commits/2ea9a2286688ef173613efc1d19e1fdf460af4d0 Simplified is_available checks] [https://github.com/expertiza/expertiza/pull/1698/commits/a9dcbe459ffbab9effa0589b9f618b0a7fb19810 Removed unused is_available checks]

===Collapse 'goto' methods===
In the legacy code found in <code>tree_display_controller.rb</code>, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the <code>goto_controller()</code> method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). [https://github.com/expertiza/expertiza/pull/1698/commits/2732cef5eb3bcf258b61447277bd82d8d58f7c54 Compact gotos]

===Remove unnecessary code to sort assignments by instructor and creation date===
By default assignments are received from <code>FolderNode.get</code> sorted by instructor and creation date. There is no need to resort this object. [https://github.com/expertiza/expertiza/pull/1698/commits/81a401083e5a4fd2d9720fd015dd7d515b808f83 Remove unnecessary code to sort assignments by instructor and creation date]

===Remove unused functions===
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* <code>filter()</code>
* <code>filter_node_is_qan()</code>
These methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
* <code>courses_assignments_obj()</code>
* <code>confirm_notifications_access()</code>
* <code>child_nodes_from_params()</code>
* <code>res_node_for_child()</code>
These methods are not used in the app.
* <code>update_tmp_obj()</code>
This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the <code>serialize_to_json</code> method which called it there was no reason to write a new method to call. Especially since the <code>update_tmp_obj</code> method was only making minor changes to the json.
* <code>bridge_to_is_available()</code>
* <code>action()</code>
These methods are not called in the app. References to these methods were removed from <code>routes.rb</code>

== Test Plan ==
;<code>#list</code> should not redirect to tree_display#list if the current user is an instructor.
:A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list

;<code>#list</code> should redirect to student_task#list if current user is a student
:A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.

;<code>#list</code> should redirect to login page if current user is nil.
:This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.

;<code>#ta_for_current_mappings?</code> should return true if the current user is a TA for current course
:This test should check that the method properly returns true if it receives a ta as the user.

;<code>#drill</code> should redirect to list action
:The drill method should redirect to the tree_display controller with the list action.

;<code>GET #session_last_open_tab</code> should return a status of 200.
:The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.

;<code>GET #get_folder_contents</code> should return a list of course objects(private) as a json.
:When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents</code> should return an empty list if no courses
:When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'

;<code>GET #get_folder_contents</code> should return a list of course objects(public) as a json.
:When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

;<code>GET #get_folder_contents for TA</code> should return an empty array if the logged in user is not a TA.
:Edge case testing to ensure that a user with the role of student results in an empty array.

;<code>GET #get_folder_contents for TA</code> should return a list of course objects the TA is supposed to TA in as json.
:If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.

;<code>GET #get_folder_contents for TA</code> should return an empty list when there is no mapping between a ta and a course.
:If the TA is not mapped to a course then the return should be empty.

;<code>GET #get_folder_contents for TA</code> should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
:Edge case test for when the user is a student in some classes, but a TA in others.

;<code>GET #get_folder_contents for TA</code> should return only the course he/she is TA of when the TA is a student of the same course.
:Edge case test for a student in a course also being a TA, most likely because of an error in assignment.

;<code>GET #get_folder_contents for TA</code> returns only the assignments which belongs to the course he/she is TA of.
:All assignments in the courses that the TA is assigned to should be returned.

;<code>GET #get_folder_contents for TA</code> should return an empty list if none of the assignments belong to the course he/she is TA of.
:Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T02:46:05Z

Pjloheid: /* Test Plan */

==E2006. Refactor tree_display_controller.rb==

__TOC__

==Project Description==

===Problem Statement===
[https://docs.google.com/document/d/1_-rgdE4Fh_Fz_Jhxw1Qj4vehbu84qiUKAiLWGcFUl5A/edit#heading=h.h37uqoqtoks4 E2006. Refactor tree_display_controller.rb]

===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

The image below shows that specific questionnaires are nested under different questionnaire categories. This means that the node for a questionnaire has a parent_id that points to a node for a questionnaire category. Similarly, listings of courses and their assignments are hierarchical.

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller was 379 lines long, and contained 45 methods. Many of those methods had similar names and were performing very similar functions. In an effort to save code and avoid DRY issues this file was refactored.

This effort reduced the controller to less than 200 lines, 20 public methods (15 of which are necessary scaffold for menu navigation), and 6 private methods.

==Goals==

From the problem background three main goals were derived to focus on in the refactor of tree display controller:
# '''Remove unnecessary methods''' - Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
# '''Save code and avoid DRY issues''' - In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.
# '''Improve understanding''' - For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Changes==
The sections below include a link to the commits where the changes were made following the description. All code changes can be found in pull request [https://github.com/expertiza/expertiza/pull/1698 #1698]

=== Make all file scope methods private ===
Code that is only called within the <code>tree_display_controller.rb</code> was moved to the private scope. This makes the code easier to read and easier to understand what is getting called outside the controller. [https://github.com/expertiza/expertiza/pull/1698/commits/0f3cda31e9e16eb3b435ca8e2a80f26f0e094f39 Assign private designation to internal methods]

=== Combine folder_node_ng_getter with children_node_ng ===
<code>folder_node_ng_getter</code> was being called by the view to return <code>FolderNode.get</code>. The resulting json of this call was then passed, without modification, into <code>children_node_ng</code>. Since this was the only place <code>folder_node_ng_getter</code> was being called it was combined with <code>children_node_ng</code> to make things easier to read when when viewing controller code. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng===
<code>children_node_ng</code> was a misleading name. The method has been renamed <code>get_folder_contents</code> since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. In addition to the name change, the <code>get_folder_contents</code> method was refactored to include the behavior of <code>folder_node_ng_getter</code>. This addition simplifies the code because the method no longer needs to convert the object that was converted to json back to an object. A single call <code>serialize_folder_to_json</code> makes the code easier to read instead of chasing nested function calls through <code>initialize_fnode_update_children</code> which ultimately leads to the same result. Another key change was changing the method from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/1d6401807b0fee302b765ea67b8024ca0f1658a7 Refactor children_node_ng]

===Refactor children_node_ng_2===
<code>children_node_ng_2</code> was a misleading name. The method has been renamed <code>get_sub_folder_contents</code> since it returns the subfolder items for the folders obtained by <code>get_folder_contents</code> in json format. Like the <code>get_folder_contents</code> refactor <code>get_sub_folder_contents</code> was simplified to make a single call to <code>serialize_sub_folder_to_json</code> making the code easier to read. Again this method was changed from a post to a get, to better represent what it was doing. Changes to <code>tree_display.jsx</code> and <code>routes.rb</code> had to be made to account for the name change and method type change. [https://github.com/expertiza/expertiza/pull/1698/commits/d7a364edce289f0efb8487429ba8fec8038f3557 Refactor children_node_ng_2] [https://github.com/expertiza/expertiza/pull/1698/commits/9a1d5caec559d15e576faf10fb55952cf9d99bf7 Rename children_node_2_ng to get_sub_folder_contents] [https://github.com/expertiza/expertiza/pull/1698/commits/867fa9d3f9d5cc988824ce2890e5ebc90708bf79 Rename serialize_to_json to serialize_sub_folder_to_json]

=== Inline function calls to make json serialization more clear ===
Where it could be done without making the existing functions unwieldy functions that updated json properties were moved up one level. This makes it easier to see in one place what properties are being modified by the serialization functions. [https://github.com/expertiza/expertiza/pull/1698/commits/d7acad1904a5431d1714008b738bc127582d53a7 Inline function calls to make json serialization more clear]

=== Simplify checks for determining a users access to view a course ===
With guidance from Dr. Gehringer it was determined a TA should have access to view all items for a given course if and only if they are mapped to that course. Likewise a instructor should be able to view all of the items for courses they teach. Admins and super admins have access to all items on the website. These facts allowed us to greatly simplify the checks necessary to determine if a course was available to the current user and if it should be private or not. [https://github.com/expertiza/expertiza/pull/1698/commits/2ea9a2286688ef173613efc1d19e1fdf460af4d0 Simplified is_available checks] [https://github.com/expertiza/expertiza/pull/1698/commits/a9dcbe459ffbab9effa0589b9f618b0a7fb19810 Removed unused is_available checks]

===Collapse 'goto' methods===
In the legacy code found in <code>tree_display_controller.rb</code>, 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the <code>goto_controller()</code> method with different values. Those 11 methods were condensed into single line methods with one overarching comment to describe them. This change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2). [https://github.com/expertiza/expertiza/pull/1698/commits/2732cef5eb3bcf258b61447277bd82d8d58f7c54 Compact gotos]

===Remove unnecessary code to sort assignments by instructor and creation date===
By default assignments are received from <code>FolderNode.get</code> sorted by instructor and creation date. There is no need to resort this object. [https://github.com/expertiza/expertiza/pull/1698/commits/81a401083e5a4fd2d9720fd015dd7d515b808f83 Remove unnecessary code to sort assignments by instructor and creation date]

===Remove unused functions===
By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* <code>filter()</code>
* <code>filter_node_is_qan()</code>
These methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]
* <code>courses_assignments_obj()</code>
* <code>confirm_notifications_access()</code>
* <code>child_nodes_from_params()</code>
* <code>res_node_for_child()</code>
These methods are not used in the app.
* <code>update_tmp_obj()</code>
This method was used only once in the app, and it was in the tree display controller, since the purpose of the method was aligned with the purpose of the <code>serialize_to_json</code> method which called it there was no reason to write a new method to call. Especially since the <code>update_tmp_obj</code> method was only making minor changes to the json.
* <code>bridge_to_is_available()</code>
* <code>action()</code>
These methods are not called in the app. References to these methods were removed from <code>routes.rb</code>

== Test Plan ==
; <code>#list</code> should not redirect to tree_display#list if the current user is an instructor.
: A user is defined to be an instructor. When the <code>get "list"</code> method is called, the response should not redirect to the tree_display/list

; <code>#list</code> should redirect to student_task#list if current user is a student
: A user is defined to be a student. Instead of redirecting to the tree_display list, it should redirect to the student_task list.

; <code>#list</code> should redirect to login page if current user is nil.
: This is a catch all for if a user somehow directs to the list without signing in, they shouldn't be able to see anything without signing in first.

; <code>#ta_for_current_mappings?</code> should return true if the current user is a TA for current course
: This test should check that the method properly returns true if it receives a ta as the user.

; <code>#drill</code> should redirect to list action
: The drill method should redirect to the tree_display controller with the list action.

; <code>GET #session_last_open_tab</code> should return a status of 200.
: The HTTP status 200 is the OK SUCCESS status to show that the get request succeeds.

; <code>GET #get_folder_contents</code> should return a list of course objects(private) as a json.
: When there is a course (private) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

; <code>GET #get_folder_contents</code> should return an empty list if no courses
: When the public and private courses have been deleted/aren't present, then the list returned should be empty, consisting of 'Courses: []'

; <code>GET #get_folder_contents</code> should return a list of course objects(public) as a json.
: When there is a course (public) and an assignment defined, the get_folder_contents method should match the json file provided in the test.

; <code>GET #get_folder_contents for TA</code> should return an empty array if the logged in user is not a TA.
: Edge case testing to ensure that a user with the role of student results in an empty array.

; <code>GET #get_folder_contents for TA</code> should return a list of course objects the TA is supposed to TA in as json.
: If the logged in user is a TA, then return should be a json of all courses that the user is a ta for.

; <code>GET #get_folder_contents for TA</code> should return an empty list when there is no mapping between a ta and a course.
: If the TA is not mapped to a course then the return should be empty.

; <code>GET #get_folder_contents for TA</code> should return only courses that a TA is a TA for. It should not return any courses that the user is a student in, but not a TA in.
: Edge case test for when the user is a student in some classes, but a TA in others.

; <code>GET #get_folder_contents for TA</code> should return only the course he/she is TA of when the TA is a student of the same course.
: Edge case test for a student in a course also being a TA, most likely because of an error in assignment.

; <code>GET #get_folder_contents for TA</code> returns only the assignments which belongs to the course he/she is TA of.
: All assignments in the courses that the TA is assigned to should be returned.

; <code>GET #get_folder_contents for TA</code> should return an empty list if none of the assignments belong to the course he/she is TA of.
: Edge case test for when there are assignments, but they don't belong to courses which he/she is a TA.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T01:19:20Z

Pjloheid: /* Test Plan */

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T01:01:24Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T00:44:17Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-24T00:43:01Z

Pjloheid: /* Goals */

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-23T22:15:03Z

Pjloheid: /* Make all file scope methods private */

==E2006. Refactor tree_display_controller.rb==

__TOC__

===About Expertiza===

[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation Expertiza] is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.

==Project Description==

===Problem Statement===
E2006. Refactor tree_display_controller.rb
===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

You will see here that assignments are nested within courses. This means that the node for an assignment has a parent_id that points to a node for a course. Similarly, listings of questionnaires are hierarchical:

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller is 379 lines long, and contains 45 methods. Many of those methods have similar names, yet there may be good reasons for having separate methods. Look over how these methods are used, and then decide. Then analyze the rest of the methods and try to find ways to save code and avoid DRY issues. Even if you cannot find a way to improve a method, you should be able to come up with a 1-line or 2-line comment explaining what that method does.

==Goals==

From the problem background, we have derived three main goals to focus on in the tree display controller:
===Remove unnecessary methods===
Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
===Save code and avoid DRY issues===
In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.

===Improve understanding===
For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Proposed Changes==
=== Make all file scope methods private ===

We decided to make any code that is only called within the '''tree_display_controller.rb''' to be private. By doing this we are making the code easier to read and easier for future people looking at the code to understand what is getting called from outside the controller.

<pre style="color: green">
private

# Add assignment attributes to json
def serialize_assignment_to_json(node, json)
json.merge!(
"course_id" => node.get_course_id,
"max_team_size" => node.get_max_team_size,
"is_intelligent" => node.get_is_intelligent,
"require_quiz" => node.get_require_quiz,
"allow_suggestions" => node.get_allow_suggestions,
"has_topic" => SignUpTopic.where(['assignment_id = ?', node.node_object_id]).first ? true : false
)
end

# Creates a json object that can be displayed by the UI
def serialize_folder_to_json(folder_type, node)
json = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type
}

if folder_type == "Courses" or folder_type == "Assignments"
json.merge! ({
"directory" => node.get_directory,
"creation_date" => node.get_creation_date,
"updated_date" => node.get_modified_date,
"institution" => Institution.where(id: node.retrieve_institution_id),
"private" => assignment_or_course_is_available?(node)
})
json["instructor_id"] = node.get_instructor_id
json["instructor"] = node.get_instructor_id ? User.find(node.get_instructor_id).name(session[:ip]) : nil
json["is_available"] = assignment_or_course_is_available?(node)
if folder_type == "Assignments"
serialize_assignment_to_json(node, json)
end
end

return json
end

# Creates a json object that can be displayed by the UI
def serialize_sub_folder_to_json(node)
json = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type,
"key" => params[:reactParams2][:key],
"private" => node.get_private,
"creation_date" => node.get_creation_date,
"updated_date" => node.get_modified_date
}

if node.type == "Courses" or node.type == "Assignments"
json["directory"] = node.get_directory
json["instructor_id"] = node.get_instructor_id
json["instructor"] = node.get_instructor_id ? User.find(node.get_instructor_id).name(session[:ip]) : nil
json["is_available"] = assignment_or_course_is_available?(node)
if folder_type == "Assignments"
serialize_assignment_to_json(node, json)
end
end

return json
end

# Checks if the user is the instructor for the course or assignment node provided.
# Note: Admin and super admin users are considered instructors for all courses.
def instructor_for_assignment_or_course?(node)
is_available(session[:user], node.get_instructor_id)
end

# Check if the user is a TA for the course or assignment node provided.
def ta_for_assignment_or_course?(node)
ta_mappings = TaMapping.where(ta_id: session[:user].id)
course_id = node.is_a?(CourseNode) ? node.node_object_id : Assignment.find(node.node_object_id).course_id
ta_mappings.any? { |ta_mapping| ta_mapping.course_id == course_id }
end

# Check if the provided node is avaiable to the logged in user.
def assignment_or_course_is_available?(node)
instructor_for_assignment_or_course?(node) or ta_for_assignment_or_course?(node)
end
</pre>

=== Removal of unneeded HTML POST ===
===Refactor children_node_ng===

We determined that the '''children_node_ng''' method had a misleading name for its function. We propose that the method be renamed to '''get_folder_contents''' since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. We believe that the proposed name change and comment make the code easier to understand (Goal 3). In addition to the name change, the '''get_folder_contents''' method is refactored to make the method itself more understandable. We also changed the method from a post to a get, to better represent what it was doing, so changes had to be made to account for the name change and method type change. In total, changes were made to '''tree_display.jsx''', '''tree_display_controller.rb''', and '''routes.rb'''.

====Changes to tree_display_controller.rb====

[[File:Diff_get_folder_contents_png.PNG|Diff_get_folder_contents_png.PNG]]

====Changes to tree_display.jsx====

[[File:Diff_get_folder_js.PNG]]

====Changes to routes.rb====

[[File:Diff_config_routes_get_folder.PNG]]

===Collapse 'goto' methods===

In the legacy code found in '''tree_display_controller.rb''', 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the '''goto_controller()''' method with different values, so we decided that those 11 methods could be condensed into single line methods with one overarching comment to describe them. We believe that this change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2).
====Original Implementation====
<pre style="color: red">
# refactored method to provide direct access to parameters
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

# direct access to questionnaires
def goto_questionnaires
goto_controller('Questionnaires')
end

# direct access to review rubrics
def goto_review_rubrics
goto_controller('Review')
end

# direct access to metareview rubrics
def goto_metareview_rubrics
goto_controller('Metareview')
end

# direct access to teammate review rubrics
def goto_teammatereview_rubrics
goto_controller('Teammate Review')
end

# direct access to author feedbacks
def goto_author_feedbacks
goto_controller('Author Feedback')
end

# direct access to global survey
def goto_global_survey
goto_controller('Global Survey')
end

# direct access to surveys
def goto_surveys
goto_controller('Assignment Survey')
end

# direct access to course surveys
def goto_course_surveys
goto_controller('Course Survey')
end

# direct access to courses
def goto_courses
goto_controller('Courses')
end

def goto_bookmarkrating_rubrics
goto_controller('Bookmarkrating')
end

# direct access to assignments
def goto_assignments
goto_controller('Assignments')
end
</pre>
====Proposed Implementation====
<pre style="color: green">
# The goto_ methods listed below are used to traverse the menu system. It is
# hard to tell exactly where they are called from, but at least some (if not all)
# are necessary. These functions may be better suited for another controller.
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

def goto_questionnaires; goto_controller('Questionnaires') end
def goto_review_rubrics; goto_controller('Review') end
def goto_metareview_rubrics; goto_controller('Metareview') end
def goto_teammatereview_rubrics; goto_controller('Teammate Review') end
def goto_author_feedbacks; goto_controller('Author Feedback') end
def goto_global_survey; goto_controller('Global Survey') end
def goto_surveys; goto_controller('Assignment Survey') end
def goto_course_surveys; goto_controller('Course Survey') end
def goto_courses; goto_controller('Courses') end
def goto_bookmarkrating_rubrics; goto_controller('Bookmarkrating') end
def goto_assignments; goto_controller('Assignments') end
</pre>

===Remove unused functions===

By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* filter()
* filter_node_is_qan()
* courses_assignments_obj()
* confirm_notifications_access()
* child_nodes_from_params()
* res_node_for_child()
* bridge_to_is_available()
* update_tmp_obj()
* action()

====Changes to tree_display_controller.rb====

The '''filter''' and '''filter_node_is_qan''' methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]

<pre style="color: red">
def filter
qid = 'filter+'
search = params[:filter_string]
filter_node = params[:filternode]
if filter_node == 'QAN' # Questionaire Assignment Name
qid = filter_node_is_qan(search, qid)
elsif filter_node == 'ACN' # Assignment Course Name
session[:root] = 2
qid << search
end
qid
end
</pre>

<pre style="color: red">
# if filter node is 'QAN', get the corresponding assignment questionnaires
def filter_node_is_qan(search, qid)
assignment = Assignment.find_by(name: search)
if assignment
assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id)
if assignment_questionnaires
assignment_questionnaires.each { |q| qid << "#{q.questionnaire_id}+" }
session[:root] = 1
end
end
qid
end
</pre>

The '''course_assignments_obj''' method is not called anywhere else in the app.

<pre style="color: red">
def courses_assignments_obj(node_type, tmp_object, node)
update_tmp_obj(tmp_object, node)
# tmpObject["private"] = node.get_private
instructor_id = node.get_instructor_id
## if current user's role is TA for a course, then that course will be listed under his course listing.
update_in_ta_course_listing(instructor_id, node, tmp_object)
update_instructor(tmp_object, instructor_id)
update_is_available(tmp_object, instructor_id, node)
assignments_method(node, tmp_object) if node_type == "Assignments"
end
</pre>

The '''confirm_notifications_access''' method is not called anywhere else in the app.

<pre style="color: red">
def confirm_notifications_access
redirect_to controller: :notifications, action: :list if current_user.try(:student?)
end
</pre>

The '''child_nodes_from_params''' method is not called anywhere else in the app

<pre style="color: red">
# finding out child_nodes from params
def child_nodes_from_params(child_nodes)
if child_nodes.is_a? String and !child_nodes.empty?
JSON.parse(child_nodes)
else
child_nodes
end
end
</pre>

The '''res_node_for_child''' method is not called anywhere else in the app.

<pre style="color: red">
# getting result nodes for child
# Changes to this method were done as part of E1788_OSS_project_Maroon_Heatmap_fixes
#
# courses_assignments_obj method makes a call to update_in_ta_course_listing which
# separates out courses based on if he/she is the TA for the course passed
# by marking private to be true in that case
#
# this also ensures that instructors (who are not ta) would have update_in_ta_course_listing
# not changing the private value if he/she is not TA which was set to true for all courses before filtering
# in update_tmp_obj in courses_assignments_obj
#
# below objects/variable names were part of the project as before and
# refactoring could have affected other functionalities too, so it was avoided in this fix
#
# fix comment end
#
def res_node_for_child(tmp_res)
res = {}
tmp_res.each_key do |node_type|
res[node_type] = []
tmp_res[node_type].each do |node|
tmp_object = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type
}
courses_assignments_obj(node_type, tmp_object, node) if %w[Courses Assignments].include? node_type
res[node_type] << tmp_object
end
end
res
end
</pre>

The '''bridge_to_is_available''' method is not called anywhere else in the app.

<pre style="color: red">
def bridge_to_is_available
user = session[:user]
owner_id = params[:owner_id]
is_available(user, owner_id)
end
</pre>

The '''update_tmp_obj''' method was used only once in the app, and it was in the tree display controller. We decided that since the purpose of the method was aligned with the purpose of the '''serialize_to_json''' method which called it, there was no reason to write a new method to call, especially since the '''update_tmp_obj''' method seemed like it was only making minor changes to the json.

<pre style="color: red">
def update_tmp_obj(tmp_object, node)
tmp = {
"directory" => node.get_directory,
"creation_date" => node.get_creation_date,
"updated_date" => node.get_modified_date,
"institution" => Institution.where(id: node.retrieve_institution_id),
"private" => node.get_instructor_id == session[:user].id
}
tmp_object.merge!(tmp)
end
</pre>

====Changes to routes.rb====

We removed the call for '''bridge_to_is_available''' and found that the '''action''' method was also unused.

[[File:Diff_config_routes_remove_functions.PNG]]

=== Assume an object with FolderNode properties is passed to children_node_2_ng ===

=== Populate JSON object within serialize methods ===

==Code Coverage==
To be completed.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

==References==
To be completed.

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-23T13:10:41Z

Pjloheid: /* Save code and avoid DRY issues */

==E2006. Refactor tree_display_controller.rb==

__TOC__

===About Expertiza===

[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation Expertiza] is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.

==Project Description==

===Problem Statement===
E2006. Refactor tree_display_controller.rb
===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

You will see here that assignments are nested within courses. This means that the node for an assignment has a parent_id that points to a node for a course. Similarly, listings of questionnaires are hierarchical:

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller is 379 lines long, and contains 45 methods. Many of those methods have similar names, yet there may be good reasons for having separate methods. Look over how these methods are used, and then decide. Then analyze the rest of the methods and try to find ways to save code and avoid DRY issues. Even if you cannot find a way to improve a method, you should be able to come up with a 1-line or 2-line comment explaining what that method does.

==Goals==

From the problem background, we have derived three main goals to focus on in the tree display controller:
===Remove unnecessary methods===
Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
===Save code and avoid DRY issues===
In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller and easier to read.

===Improve understanding===
For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Proposed Changes==
=== Make all file scope methods private ===
=== Removal of unneeded HTML POST ===
===Refactor children_node_ng===

We determined that the '''children_node_ng''' method had a misleading name for its function. We propose that the method be renamed to '''get_folder_contents''' since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. We believe that the proposed name change and comment make the code easier to understand (Goal 3). In addition to the name change, the '''get_folder_contents''' method is refactored to make the method itself more understandable. We also changed the method from a post to a get, to better represent what it was doing, so changes had to be made to account for the name change and method type change. In total, changes were made to '''tree_display.jsx''', '''tree_display_controller.rb''', and '''routes.rb'''.

====Changes to tree_display_controller.rb====

[[File:Diff_get_folder_contents_png.PNG|Diff_get_folder_contents_png.PNG]]

====Changes to tree_display.jsx====

[[File:Diff_get_folder_js.PNG]]

====Changes to routes.rb====

[[File:Diff_config_routes_get_folder.PNG]]

===Collapse 'goto' methods===

In the legacy code found in '''tree_display_controller.rb''', 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the '''goto_controller()''' method with different values, so we decided that those 11 methods could be condensed into single line methods with one overarching comment to describe them. We believe that this change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2).
====Original Implementation====
<pre style="color: red">
# refactored method to provide direct access to parameters
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

# direct access to questionnaires
def goto_questionnaires
goto_controller('Questionnaires')
end

# direct access to review rubrics
def goto_review_rubrics
goto_controller('Review')
end

# direct access to metareview rubrics
def goto_metareview_rubrics
goto_controller('Metareview')
end

# direct access to teammate review rubrics
def goto_teammatereview_rubrics
goto_controller('Teammate Review')
end

# direct access to author feedbacks
def goto_author_feedbacks
goto_controller('Author Feedback')
end

# direct access to global survey
def goto_global_survey
goto_controller('Global Survey')
end

# direct access to surveys
def goto_surveys
goto_controller('Assignment Survey')
end

# direct access to course surveys
def goto_course_surveys
goto_controller('Course Survey')
end

# direct access to courses
def goto_courses
goto_controller('Courses')
end

def goto_bookmarkrating_rubrics
goto_controller('Bookmarkrating')
end

# direct access to assignments
def goto_assignments
goto_controller('Assignments')
end
</pre>
====Proposed Implementation====
<pre style="color: green">
# The goto_ methods listed below are used to traverse the menu system. It is
# hard to tell exactly where they are called from, but at least some (if not all)
# are necessary. These functions may be better suited for another controller.
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

def goto_questionnaires; goto_controller('Questionnaires') end
def goto_review_rubrics; goto_controller('Review') end
def goto_metareview_rubrics; goto_controller('Metareview') end
def goto_teammatereview_rubrics; goto_controller('Teammate Review') end
def goto_author_feedbacks; goto_controller('Author Feedback') end
def goto_global_survey; goto_controller('Global Survey') end
def goto_surveys; goto_controller('Assignment Survey') end
def goto_course_surveys; goto_controller('Course Survey') end
def goto_courses; goto_controller('Courses') end
def goto_bookmarkrating_rubrics; goto_controller('Bookmarkrating') end
def goto_assignments; goto_controller('Assignments') end
</pre>

===Remove unused functions===

By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* filter()
* filter_node_is_qan()
* courses_assignments_obj()
* confirm_notifications_access()
* child_nodes_from_params()
* res_node_for_child()
* bridge_to_is_available()
* update_tmp_obj()
* action()

====Changes to tree_display_controller.rb====

The '''filter''' and '''filter_node_is_qan''' methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]

<pre style="color: red">
def filter
qid = 'filter+'
search = params[:filter_string]
filter_node = params[:filternode]
if filter_node == 'QAN' # Questionaire Assignment Name
qid = filter_node_is_qan(search, qid)
elsif filter_node == 'ACN' # Assignment Course Name
session[:root] = 2
qid << search
end
qid
end
</pre>

<pre style="color: red">
# if filter node is 'QAN', get the corresponding assignment questionnaires
def filter_node_is_qan(search, qid)
assignment = Assignment.find_by(name: search)
if assignment
assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id)
if assignment_questionnaires
assignment_questionnaires.each { |q| qid << "#{q.questionnaire_id}+" }
session[:root] = 1
end
end
qid
end
</pre>

The '''course_assignments_obj''' method is not called anywhere else in the app.

<pre style="color: red">
def courses_assignments_obj(node_type, tmp_object, node)
update_tmp_obj(tmp_object, node)
# tmpObject["private"] = node.get_private
instructor_id = node.get_instructor_id
## if current user's role is TA for a course, then that course will be listed under his course listing.
update_in_ta_course_listing(instructor_id, node, tmp_object)
update_instructor(tmp_object, instructor_id)
update_is_available(tmp_object, instructor_id, node)
assignments_method(node, tmp_object) if node_type == "Assignments"
end
</pre>

The '''confirm_notifications_access''' method is not called anywhere else in the app.

<pre style="color: red">
def confirm_notifications_access
redirect_to controller: :notifications, action: :list if current_user.try(:student?)
end
</pre>

The '''child_nodes_from_params''' method is not called anywhere else in the app

<pre style="color: red">
# finding out child_nodes from params
def child_nodes_from_params(child_nodes)
if child_nodes.is_a? String and !child_nodes.empty?
JSON.parse(child_nodes)
else
child_nodes
end
end
</pre>

The '''res_node_for_child''' method is not called anywhere else in the app.

<pre style="color: red">
# getting result nodes for child
# Changes to this method were done as part of E1788_OSS_project_Maroon_Heatmap_fixes
#
# courses_assignments_obj method makes a call to update_in_ta_course_listing which
# separates out courses based on if he/she is the TA for the course passed
# by marking private to be true in that case
#
# this also ensures that instructors (who are not ta) would have update_in_ta_course_listing
# not changing the private value if he/she is not TA which was set to true for all courses before filtering
# in update_tmp_obj in courses_assignments_obj
#
# below objects/variable names were part of the project as before and
# refactoring could have affected other functionalities too, so it was avoided in this fix
#
# fix comment end
#
def res_node_for_child(tmp_res)
res = {}
tmp_res.each_key do |node_type|
res[node_type] = []
tmp_res[node_type].each do |node|
tmp_object = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type
}
courses_assignments_obj(node_type, tmp_object, node) if %w[Courses Assignments].include? node_type
res[node_type] << tmp_object
end
end
res
end
</pre>

The '''bridge_to_is_available''' method is not called anywhere else in the app.

<pre style="color: red">
def bridge_to_is_available
user = session[:user]
owner_id = params[:owner_id]
is_available(user, owner_id)
end
</pre>

The '''update_tmp_obj''' method was used only once in the app, and it was in the tree display controller. We decided that since the purpose of the method was aligned with the purpose of the '''serialize_to_json''' method which called it, there was no reason to write a new method to call, especially since the '''update_tmp_obj''' method seemed like it was only making minor changes to the json.

<pre style="color: red">
def update_tmp_obj(tmp_object, node)
tmp = {
"directory" => node.get_directory,
"creation_date" => node.get_creation_date,
"updated_date" => node.get_modified_date,
"institution" => Institution.where(id: node.retrieve_institution_id),
"private" => node.get_instructor_id == session[:user].id
}
tmp_object.merge!(tmp)
end
</pre>

====Changes to routes.rb====

We removed the call for '''bridge_to_is_available''' and found that the '''action''' method was also unused.

[[File:Diff_config_routes_remove_functions.PNG]]

=== Assume an object with FolderNode properties is passed to children_node_2_ng ===

=== Populate JSON object within serialize methods ===

==Code Coverage==
To be completed.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

==References==
To be completed.

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-23T13:09:51Z

Pjloheid: /* Remove unused functions */

==E2006. Refactor tree_display_controller.rb==

__TOC__

===About Expertiza===

[http://wiki.expertiza.ncsu.edu/index.php/Expertiza_documentation Expertiza] is a web application through which students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.

==Project Description==

===Problem Statement===
E2006. Refactor tree_display_controller.rb
===Background===
The tree_display_controller is responsible for rendering hierarchical lists in the instructor user interface. Here is a sample instructor homepage:

[[File:Example_instructor_homepage1.JPG]]

You will see here that assignments are nested within courses. This means that the node for an assignment has a parent_id that points to a node for a course. Similarly, listings of questionnaires are hierarchical:

[[File:Questionnaires_listing_capture.JPG]]

The various rubrics, as well as surveys, are types of questionnaires. So the parent_id of a particular rubric will be the type of rubric (e.g., teammate review) that it is.

The tree_display_controller is 379 lines long, and contains 45 methods. Many of those methods have similar names, yet there may be good reasons for having separate methods. Look over how these methods are used, and then decide. Then analyze the rest of the methods and try to find ways to save code and avoid DRY issues. Even if you cannot find a way to improve a method, you should be able to come up with a 1-line or 2-line comment explaining what that method does.

==Goals==

From the problem background, we have derived three main goals to focus on in the tree display controller:
===Remove unnecessary methods===
Search the 45 methods defined in the controller to see where and why they are called. Remove outdated and unused methods, and see where it is fit to merge or replace methods.
===Save code and avoid DRY issues===
In addition to removing unnecessary methods, code should be reduced wherever possible to make the controller smaller an easier to read.
===Improve understanding===
For all necessary, complex methods in the controller, comments should be added to improve understanding of what methods do. This way, future reviewers won't have to search for implementations to understand the functionality when working on other issues.

==Proposed Changes==
=== Make all file scope methods private ===
=== Removal of unneeded HTML POST ===
===Refactor children_node_ng===

We determined that the '''children_node_ng''' method had a misleading name for its function. We propose that the method be renamed to '''get_folder_contents''' since it returns the contents of each top level folder (assignments, courses, and questionnaires) in json format. We believe that the proposed name change and comment make the code easier to understand (Goal 3). In addition to the name change, the '''get_folder_contents''' method is refactored to make the method itself more understandable. We also changed the method from a post to a get, to better represent what it was doing, so changes had to be made to account for the name change and method type change. In total, changes were made to '''tree_display.jsx''', '''tree_display_controller.rb''', and '''routes.rb'''.

====Changes to tree_display_controller.rb====

[[File:Diff_get_folder_contents_png.PNG|Diff_get_folder_contents_png.PNG]]

====Changes to tree_display.jsx====

[[File:Diff_get_folder_js.PNG]]

====Changes to routes.rb====

[[File:Diff_config_routes_get_folder.PNG]]

===Collapse 'goto' methods===

In the legacy code found in '''tree_display_controller.rb''', 60 lines are dedicated to the 12 methods related to traversing the menu system. 11 of the methods call the '''goto_controller()''' method with different values, so we decided that those 11 methods could be condensed into single line methods with one overarching comment to describe them. We believe that this change makes the code easier to read, since each of the 11 methods condensed carry out the same functionality, and easier to understand since we have added in a better comment for understanding what the code does (Goal 3). This also saved 41 lines of code in the controller (Goal 2).
====Original Implementation====
<pre style="color: red">
# refactored method to provide direct access to parameters
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

# direct access to questionnaires
def goto_questionnaires
goto_controller('Questionnaires')
end

# direct access to review rubrics
def goto_review_rubrics
goto_controller('Review')
end

# direct access to metareview rubrics
def goto_metareview_rubrics
goto_controller('Metareview')
end

# direct access to teammate review rubrics
def goto_teammatereview_rubrics
goto_controller('Teammate Review')
end

# direct access to author feedbacks
def goto_author_feedbacks
goto_controller('Author Feedback')
end

# direct access to global survey
def goto_global_survey
goto_controller('Global Survey')
end

# direct access to surveys
def goto_surveys
goto_controller('Assignment Survey')
end

# direct access to course surveys
def goto_course_surveys
goto_controller('Course Survey')
end

# direct access to courses
def goto_courses
goto_controller('Courses')
end

def goto_bookmarkrating_rubrics
goto_controller('Bookmarkrating')
end

# direct access to assignments
def goto_assignments
goto_controller('Assignments')
end
</pre>
====Proposed Implementation====
<pre style="color: green">
# The goto_ methods listed below are used to traverse the menu system. It is
# hard to tell exactly where they are called from, but at least some (if not all)
# are necessary. These functions may be better suited for another controller.
def goto_controller(name_parameter)
node_object = TreeFolder.find_by(name: name_parameter)
session[:root] = FolderNode.find_by(node_object_id: node_object.id).id
redirect_to controller: 'tree_display', action: 'list'
end

def goto_questionnaires; goto_controller('Questionnaires') end
def goto_review_rubrics; goto_controller('Review') end
def goto_metareview_rubrics; goto_controller('Metareview') end
def goto_teammatereview_rubrics; goto_controller('Teammate Review') end
def goto_author_feedbacks; goto_controller('Author Feedback') end
def goto_global_survey; goto_controller('Global Survey') end
def goto_surveys; goto_controller('Assignment Survey') end
def goto_course_surveys; goto_controller('Course Survey') end
def goto_courses; goto_controller('Courses') end
def goto_bookmarkrating_rubrics; goto_controller('Bookmarkrating') end
def goto_assignments; goto_controller('Assignments') end
</pre>

===Remove unused functions===

By searching for the implementations of all functions in the controller we were able to recognize some of them as being unused and remove them (Goal 1). Others became unused once refactoring was completed.
* filter()
* filter_node_is_qan()
* courses_assignments_obj()
* confirm_notifications_access()
* child_nodes_from_params()
* res_node_for_child()
* bridge_to_is_available()
* update_tmp_obj()
* action()

====Changes to tree_display_controller.rb====

The '''filter''' and '''filter_node_is_qan''' methods were both removed because they were only used in the page_header partial for tree display, which was [https://github.com/expertiza/expertiza/commit/167cd7893e2c7eab660495ea29ff2678024d9c47 removed approximately 3 years prior.]

<pre style="color: red">
def filter
qid = 'filter+'
search = params[:filter_string]
filter_node = params[:filternode]
if filter_node == 'QAN' # Questionaire Assignment Name
qid = filter_node_is_qan(search, qid)
elsif filter_node == 'ACN' # Assignment Course Name
session[:root] = 2
qid << search
end
qid
end
</pre>

<pre style="color: red">
# if filter node is 'QAN', get the corresponding assignment questionnaires
def filter_node_is_qan(search, qid)
assignment = Assignment.find_by(name: search)
if assignment
assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: assignment.id)
if assignment_questionnaires
assignment_questionnaires.each { |q| qid << "#{q.questionnaire_id}+" }
session[:root] = 1
end
end
qid
end
</pre>

The '''course_assignments_obj''' method is not called anywhere else in the app.

<pre style="color: red">
def courses_assignments_obj(node_type, tmp_object, node)
update_tmp_obj(tmp_object, node)
# tmpObject["private"] = node.get_private
instructor_id = node.get_instructor_id
## if current user's role is TA for a course, then that course will be listed under his course listing.
update_in_ta_course_listing(instructor_id, node, tmp_object)
update_instructor(tmp_object, instructor_id)
update_is_available(tmp_object, instructor_id, node)
assignments_method(node, tmp_object) if node_type == "Assignments"
end
</pre>

The '''confirm_notifications_access''' method is not called anywhere else in the app.

<pre style="color: red">
def confirm_notifications_access
redirect_to controller: :notifications, action: :list if current_user.try(:student?)
end
</pre>

The '''child_nodes_from_params''' method is not called anywhere else in the app

<pre style="color: red">
# finding out child_nodes from params
def child_nodes_from_params(child_nodes)
if child_nodes.is_a? String and !child_nodes.empty?
JSON.parse(child_nodes)
else
child_nodes
end
end
</pre>

The '''res_node_for_child''' method is not called anywhere else in the app.

<pre style="color: red">
# getting result nodes for child
# Changes to this method were done as part of E1788_OSS_project_Maroon_Heatmap_fixes
#
# courses_assignments_obj method makes a call to update_in_ta_course_listing which
# separates out courses based on if he/she is the TA for the course passed
# by marking private to be true in that case
#
# this also ensures that instructors (who are not ta) would have update_in_ta_course_listing
# not changing the private value if he/she is not TA which was set to true for all courses before filtering
# in update_tmp_obj in courses_assignments_obj
#
# below objects/variable names were part of the project as before and
# refactoring could have affected other functionalities too, so it was avoided in this fix
#
# fix comment end
#
def res_node_for_child(tmp_res)
res = {}
tmp_res.each_key do |node_type|
res[node_type] = []
tmp_res[node_type].each do |node|
tmp_object = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type
}
courses_assignments_obj(node_type, tmp_object, node) if %w[Courses Assignments].include? node_type
res[node_type] << tmp_object
end
end
res
end
</pre>

The '''bridge_to_is_available''' method is not called anywhere else in the app.

<pre style="color: red">
def bridge_to_is_available
user = session[:user]
owner_id = params[:owner_id]
is_available(user, owner_id)
end
</pre>

The '''update_tmp_obj''' method was used only once in the app, and it was in the tree display controller. We decided that since the purpose of the method was aligned with the purpose of the '''serialize_to_json''' method which called it, there was no reason to write a new method to call, especially since the '''update_tmp_obj''' method seemed like it was only making minor changes to the json.

<pre style="color: red">
def update_tmp_obj(tmp_object, node)
tmp = {
"directory" => node.get_directory,
"creation_date" => node.get_creation_date,
"updated_date" => node.get_modified_date,
"institution" => Institution.where(id: node.retrieve_institution_id),
"private" => node.get_instructor_id == session[:user].id
}
tmp_object.merge!(tmp)
end
</pre>

====Changes to routes.rb====

We removed the call for '''bridge_to_is_available''' and found that the '''action''' method was also unused.

[[File:Diff_config_routes_remove_functions.PNG]]

=== Assume an object with FolderNode properties is passed to children_node_2_ng ===

=== Populate JSON object within serialize methods ===

==Code Coverage==
To be completed.

==Team Information==
Brooks Anderson

Rick Holloway

PJ Loheide

Special thanks to our mentor and professor: Dr. Edward Gehringer

==References==
To be completed.

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-23T02:12:33Z

Pjloheid:

File:Diff config routes remove functions.PNG

2020-03-23T02:10:59Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-22T23:23:06Z

Pjloheid: /* Refactor children_node_ng */

File:Diff config routes get folder.PNG

2020-03-22T23:21:21Z

Pjloheid:

File:Diff get folder js.PNG

2020-03-22T23:19:31Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-22T22:58:46Z

Pjloheid: /* Refactor children_node_ng */

File:Diff get folder contents png.PNG

2020-03-22T22:51:59Z

Pjloheid:

File:Diff get folder contents.JPG

2020-03-22T22:49:25Z

Pjloheid: Pjloheid uploaded a new version of File:Diff get folder contents.JPG

File:Diff get folder contents.JPG

2020-03-22T22:38:01Z

Pjloheid:

CSC/ECE 517 Spring 2020 - E2006. Refactor Tree Display Controller

2020-03-22T22:22:11Z

Pjloheid: Updating images

File:Questionnaires listing capture.JPG

2020-03-22T22:21:11Z

Pjloheid:

File:Questionnaires listing.JPG

2020-03-22T22:20:41Z

Pjloheid: Pjloheid uploaded a new version of File:Questionnaires listing.JPG

File:Questionnaires listing.JPG

2020-03-22T22:19:05Z

Pjloheid: Pjloheid uploaded a new version of File:Questionnaires listing.JPG

File:Example instructor homepage1.JPG

2020-03-22T22:15:56Z

Pjloheid:

File:Example instructor homepage.JPG

2020-03-22T22:14:59Z