== Introduction - Purpose & Problem ==
Currently, reviews of other student’s work can only be performed by individual students in Expertiza, not by groups of students. Since doing reviews together could help students learn more than by doing them alone, it has been requested that reviews must now have the option to be done by teams instead of individual participants. Therefore, there should be an option when creating assignments that allows the creator to select whether the assignment will use team reviewers or individual reviewers. For simplification, we were allowed to assume that the teams that worked on an assignment together would review together. Each participant should be able to make individual changes to the review (while logged in from their account), but these changes should apply to the team’s collective review. This creates an issue where teammates could accidentally overwrite each other’s work if they edit the review at once. Therefore, it has been decided that the review should be locked while one edits it so that only one participant can edit it at once. Locking the review presents its own challenges.

== Proposed Solution ==
* Modification to Response Map Classes
** A field should be added to ResponseMap which indicates whether responses are done by teams or by individuals.
* Modification to Assignment Class
** A field indicating if the assignment is to be done with team or individuals. This is necessary because part of the suggested requirements is to add a drop down on the review strategy section of the assignment.
* Locking Solution
** Research needs to be done as to whether a rails mechanism already exists to facilitate a lock on page edits
** A solution can be implemented from scratch by storing a flag in the database on the review’s table entry. This would require an additional migration.
**The ability to have a lock on a review requires the implementation of some kind of auto-unlock feature. If a user never unlocks a review, his/her teammates still need to be able to modify the review.
*** We should be able to use the “updated at” field to check if the lock has been held for too long and needs to be released.

== More on Locks ==
The locking solution we added works as a general locking solution. It adds a new table in the database called locks which create a mapping between a user and a resource. This database change does not force a lock on the resource. Just because there exists a lock between some resource and some user, does not mean that other users cannot edit that resource. The actual prevention of edits, be it redirecting or preventing access to controller methods, is the responsibility of you, the programmer. This class just provides an easy interface to facilitate that behavior.

=== Database Changes ===
Locks created the following changes:
* locks
** timeout_period
** created_at
** updated_at
** user_id
** lockable_id
** lockable_type

=== Code Changes ===
[[File:E1973_lock_uml.png]]

=== How to implement ===
Whichever model needs to be able to be locked must include this line:
include Lockable
See app/models/response.rb

To check to see if a resource is locked, use
resource = Lock.get_lock(resource, user, timeout_period)
If the resource is nil, it has been locked. If it's not nil, the given user owns the lock over the current resource.
See app/controllers/response_controller.rb#edit

To unlock a resource after it is done being used, use
Lock.release_lock(resource)
See app/controllers/lock_controller.rb#release_lock

To check to see if a lock exists between a user and a resource, use
Lock.lock_between?(resource, user)
See app/controllers/response_controller.rb#update
Because locks can time out, if user1 makes changes and stalls on a page, user2 could get the lock, make edits, and release it. If that's the case, we may not want to keep user1's changes. The way to check for that scenario is by seeing if the user still has a lock on this object.

=== About lock timeouts ===
Lock.get_lock takes a timeout_period. Timeout_period minutes after a user gets a lock, any user who calls get_lock on a resource will acquire a lock.

As the code stands, you MUST include a timeout period if you wish to get a lock. The rationale being that permanently preventing access to a resource is a heavy price for forgetting to unlock a resource. If you wish to do away with this feature, you will need to add the code yourself. (Perhaps using -1 for no time limit). Though I would heavily recommend using the timeout feature.

== Changes to Code ==
* review_response_map.rb (migration required)
** Field - reviewer_is_team: boolean
* review_mapping_controller.rb - update the following methods to use AssignmentTeam as well as AssignmentParticipant as the reviewer
** automatic_review_mapping()
** add_calibration()
** assign_reviewer_dynamically()
** get_reviewer()
* assignment.rb (migration required)
** Field - has_team_reviews: boolean
* assignment_controller.rb
** updated new() and create() methods to handle new field has_team_reviews
* assignments/edit/_review_strategy.html.erb - added check box for has_team_reviews
<tr>
<td id='reviewer_is_team'>
<input name="assignment_form[assignment][reviewer_is_team]" type="hidden" value="false"/>
<%= check_box_tag('assignment_form[assignment][reviewer_is_team]', 'true', @assignment_form.assignment.reviewer_is_team) %>
<%= label_tag('assignment_form[assignment][reviewer_is_team]', 'Is Review done by Teams?') %>
<img src="/assets/info.png" title='You can select whether the reviews should be done by individual students or teams'>
</td>
</tr>
* assignment_participant.rb
** Method - get_reviewer
*** Returns the participant's team if the reviews for the assignment are done by teams.
*** Several lines of code treated reviewers explicitly as participants. In order to avoid changing too much functionality, and to go with Dr. Gehringer's request that the changes be polymorphic, we just inserted this method whenever the code treated a participant as a reviewer.
*** Example (review_mapping_controller.rb):
*** Reviewer used to be retrieved by the call to AssignmentParticipant.where. Now, reviewer and participant are treated seperately.
def assign_reviewer_dynamically
assignment = Assignment.find(params[:assignment_id])
participant = AssignmentParticipant.where(user_id: params[:reviewer_id], parent_id: assignment.id).first
reviewer = participant.get_reviewer
...
* Added lock.rb, lock_controller.rb, and lockable.rb

== Changes Represented in UML ==
[[File:E1973_Uml_1.png]]

== UI Changes ==
=== Assignment Review Strategy ===
==== Before ====
[[File:E1973_Editing_before.png]]

==== After ====
[[File:E1973_Editing_after.png]]

=== Assign Reviewers ===
==== Before ====
[[File:E1973_Participants_before.png]]

==== After ====
[[File:E1973_Participants_after.png]]

== Test Plan ==
=== Rspec ===
Our primary purpose for rspec testing will involve ensuring that we haven't broken any existing functionality.
Since our new functionality doesn't involve any model/view/controller additions, our tests will be appended to existing rspec tests inside of:

* assignments_controller_spec.rb
* response_controller_spec.rb
* review_mapping_controller_spec.rb

The new functionality we need to test should primarily ensure that:

* Students on the same team can see each other's review responses
* No two students can edit a review at the same time
* Reviews submitted by teams are treated identically to reviews submitted by individuals

=== UI Testing ===

Our UI tests aim to capture the following core pieces of functionality: 

'''Students on the same team can view/edit the same response: '''

''Prerequisites: we use our fixture to ensure that an assignment has been created, and that students 9 and 10 are on a team together. ''

# Login as student10.
# Navigate to assignments, and click on an assignment.
# Request a new submission to review.
# In the review, leave the comment "Excellent work done!" and click save.
# Logout and login as student 9. Repeat step 2.
# Click View. Notice that the review already says "Excellent work done!".
# Go back to the review and click Edit. Change the message to "Decent work here".
# Logout and login as student 10. Repeat step 2.
# Click View. Notice that the review now says "Decent work here".

=== Manual Testing ===
 
'''Students cannot edit the response at the same time: '''
''Prerequisites: Same as the above test.''
# Repeat steps 1-4 (inclusive) from the above test. However, remain on the edit page. 
# Open another browser and navigate to Expertiza. Then, login as student9 in another browser. 
# Navigate to the assignment and click "Edit". Verify that you are redirected and unable to edit the review response since student10 is already editing it.

CSC/ECE 517 Fall 2019 - E1945. Refactor users controller.rb

__TOC__

== '''About Expertiza''' ==
Expertiza is an open-source project based on Ruby on Rails framework. It allows the instructor to create new assignments and customize new or existing assignments. The instructor is allowed to create a list of topics for the students to which they can sign up for. For working on different projects and assignments the students can form teams in Expertiza. Peer review is another feature where students can review other students' submissions. This feature is available in Expertiza. Furthermore, Expertiza supports submission across various document types, including URLs and wiki pages.

== '''Abstract''' ==
The UserController is a controller used for managing the creation, modification, and destruction of users in the Expertiza system. Instructor users must be added by creating a request for a new account. A key part of our team’s work was moving methods associated with managing a new account request from the UserController to a new controller named AccountRequestController. This removed coupling between account requests and user objects. Furthermore, it allowed an account request to be properly associated with its own controller, model, and view. Additionally, the team refactored and documented some methods in the UserController.

== '''Problem Statement''' ==

The ''users_controller.rb'' file included the standard CRUD methods for a ''User'' model along with methods for other workflows. Most notably, the ''users_controller.rb'' file handled the creation and management of a ''RequestedUser'' object. The file also included a few methods which had a bad name or lack documentation. Thus it required to make the following changes to make the code more readable and change certain views.

The following tasks were required to be done for the project by our team:

* Separate all methods related to the workflow of a ''RequestedUser'' object
* Move below-mentioned methods to a new file named ''account_request_controller.rb''
# created_approved_user
# list_pending_requested
# request_new
# created_requested_user_record
# roles_for_request_sign_up
# Requested_user_params
* The ''RequestedUser'' model should be renamed to ''AccountRequest'' and it’s lifetime must be managed by the new ''AccountRequestController''
* The form that was currently displayed when the “Request Account” button is clicked from the Expertiza login page. It had to be edited with the following changes
** Only instructor accounts can be created, so the dropdown had to be removed
** All form labels had to be bold-faced
** The “Self Introduction” label should be re-named to “Self-Introduction”
** The textbox for the self-introduction field should include some hint (“Please include a link to your website”)
* Comments had to be written for the following methods
# get_role
# show_selection
# foreign
* The paginate_list method had to be invoked at the correct location (in the list method) so that it paginates the users list correctly. Presently all users being shown on a single page by default on clicking the user list.

== '''Our Work''' ==

=== Changing the Request Account Page ===

This is the page that was loading originally when '''request account''' button is clicked in the login page:

[[File:Newuser.png|center|frame|none]]

==== Removing Dropdown ====
Only the Instructor account can be created and not TA which is there as an option originally.
For this in the '''request_new.html.erb''' file, the selection tag has been changed to label tag in which the '''Instructor''' is put on the label

==== All form labels had to be bold-faced ====
For this in the individual forms inside the view are visited and bold tag has been added individually.

==== Renaming Self Introduction ====
Inside the '''_self_introduction.html.erb''' file the label is edited to the required one.

==== Adding hints in text-box ====
Initially, the text-box was blank and some hints had to be added. This was done by adding a place_holder attribute inside the text area inside the '''_self_introduction.html.erb''' file

This is the current page that gets loaded after fixing the above issues:

[[File:Expertiza.png|center|frame|none]]

=== Adding comments for methods ===
The following methods had comments written or renamed for better understanding of the working of the methods and to reflect their actual behaviour.
#get_role - The method was renamed to '''role''', a comment was added explaining that it finds the role of a given user object
#show_selection - The method was renamed to '''show_if_authorized''', as this method should only display the users if the current user is authorized. Also changed it from a GET to a POST request since it more accurately reflects its working.
#foreign - A comment was added for this method, explaining that it stores all roles possible and that it gets role id of the session’s user.

[[File:Foreign.png|center|caption]]

=== Invoking paginate_list method ===

Invoked the paginate_list method at the correct location(in the list method) so that it paginates the users list correctly. The number of users per-page has currently been set to 100, which was showing all users in a single page by default. Also added a section at the bottom of the '''list.html.erb''' page for navigating between the paginated list of users.

[[File:Paginate.png|center|]]

As it can be seen now that there are page numbers and not all the users are being showed on the same page as was the case beforehand.

== '''Results''' ==
The refactoring of the user_controller made the view for the account request more clearer and easy to understand for the user i.e. the UI was improved. Also, the code was made more cleaner and easy to read by for people who works on this later as the methods are segregated for performing their corresponding tasks and they comments are given for those which were not clear. Furthermore, the paginate users is now repaired and is working properly which lets the students list to be shown page wise instead of all the students in a single page which was not readable.

'''Video of Account Request Process:''' 
https://youtu.be/ZlUZWbpYvAI

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

Tests for UserController were updated to account for the fact that some methods now use the AccountRequestController. The tests for the methods that were moved to AccountRequestController were moved to a new spec in the file ''account_request_spec.rb''. The team did this because it made sense to make a new spec for a new controller. Nevertheless, most test functionalities remained the same. The tests were also updated to not select the user role from the “User Role” dropdown. This was done because the dropdown was removed from the UI since only instructor accounts can be created.
 

'''RSpec:'''
AccountRequestController
context 'request account feature'
it 'works correctly'
context 'on users#list_pending_requested page'
it 'allows super-admin and admin to communicate with requesters by clicking email addresses'
context 'when super-admin or admin rejects a requester'
it 'displays \'Rejected\' as status'
context 'when super-admin or admin accepts a requester'
it 'displays \'Accept\' as status and sends an email with randomly-generated password to the new user'
context 'using name as username and password in the email'
it 'allows the new user to login Expertiza'
UsersController
context '#index'
it 'redirects if user is student'
it 'renders list if user is instructor'
context '#set_anonymized_view'
it 'redirects to back'
context "#show_if_authorized"
it 'user is nil'
it 'user is not nil and user is available for editing'
it 'user is not nil but is not available for editing'
context '#show'
it 'when params[:id] is not nil'
it 'when params[:id] is not nil but role_id is nil'
it 'when params[:id] is nil'
context "#new"
it '1'

'''Manual Testing'''
AccountRequestController
ApproveTest
1. Navigate to Expertiza home page.
2. Click Request User button.
3. Fill in user information and submit.
4. Login as super_administrator2 (password is "password").
5. Click Administration -> Show -> Pending Requests
6. Click Approve for the bottom-most request.
7. Observe that the user's account request has been approved.
(This test checks the flow of an AccountRequest's life cycle)
RejectTest
1. Repeat steps 1-5 from ApproveTest.Use different information for the user being created.
2. Click Reject for the bottom-most request.
3. Observe that the user's account request has been rejected.

PaginateUsers
1. Login as instructor6 (password is"password").
2. Click Manage -> Users.
3. Scroll to the bottom of the user list page and observe that it is paginated.
(This test checks that the list of users is paginated)

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

'''Github Repo:''' https://github.com/deepayanbardhan/expertiza
 
'''Pull Request:''' https://github.com/expertiza/expertiza/pull/1541

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

'''Project Mentor:'''
 Ramya Vijayakumar

'''Project Members:''' 
Benjamin Fisher 
Deepayan Bardhan 
Sanket Pai

CSC/ECE 517 Fall 2019 - E1945. Refactor users controller.rb

2019-11-07T03:13:29Z

2019-11-07T02:29:26Z

Bjfisher: /* Test Plan */