E1914 Refactor users controller

About Expertiza

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

This document contains the description of the changes that we made to users_controller, how we refactored it, our testing results and some design decisions that we took while making these changes.

Separate all methods related to the workflow of a RequestedUser object

Problem:
The users_controller.rb file included the standard CRUD methods for a User model along with methods for other workflows. The users_controller.rb file also handled the creation and management of a RequestedUser object in addition to handling the user object.
Solution:
Earlier, there was no controllers for a RequestedUser object and these were handled by the Users controller itself. We separated the RequestedUser workflow from the Users and we renamed the RequestedUser model to AccountRequest. As a part of this refactoring activity, we created a new controller called the account_requests_controller to handle the workflow of an AccountRequest. The following methods were moved from users_controller to account_requests_controller.
1) created_approved_user
2) list_pending_requested
3) request_new
4) created_requested_user_record
5) roles_for_request_sign_up
6) requested_user_params

Writing comments to make methods more understandable

Problem:
The users_controller included a few methods which lacked documentation.
Solution:

Wrote comments for the 'foreign' method. The comments include:

1) What the method does: This method is used to find the list of roles the current user can embody.
2) Where the method is used: Used to display a drop-down selection of roles for the current user in the views.

Wrote comments for the show and show_selection methods:

1) What the method does:
show(): If the current user is a student, they should only be able to see information about themselves. All other people should be able to see information about themselves or other students. If the request to show() passes these checks, then they are shown the view 'show'. Otherwise, they are redirected to the home page.
show_selection(): If the role of a user's parent is less than the current user or if the current user is requesting to see itself or if user's parent_id does not exist then the show() method is called. All these conditions boil down to whether the current user of the system is authorized to see/edit the information about the user specified in the params. If the requested user does not exist or if the current user is not authorized to see the requested user, then the current user is redirected back to the list page.
2) Scenarios in which show_selection is called: in users/list.html.erb, a list of users is shown. On top of the list there is a functionality to search the list. When a person searches for a particular student, and selects that student, the show_selection method is called. If the person is allowed to see that student, the user is directed to the show() method. Otherwise the person stays on the list view.
3) Scenarios in which show() is called: From the edit.html.erb, if the person wants to see the information instead of editing it. From show_selection() as described above.

Renaming methods to reflect their actual behavior

Problem:
Methods in the Users_controller were not named to convey exactly what they did.
Solution:
The get_role method was named more like it would be in Java. It was renamed to 'role' because that is how it would be named in ruby. We chose to keep the method in the controller instead of moving it to the model because it does not alter the data structure and it also has selection logic.

Make GUI changes in the form displayed after "Request Account" button is clicked

Problem:
The forms that come after "Request Account" button is clicked need to be changed.
Solution: Only instructor accounts can be created, so the drop-down was removed.
All form labels were bold-faced.
The “Self Introduction” label was re-named to “Self-Introduction”.
The text-box for the self-introduction field now includes a useful hint of what is expected in the text-box. The hint is "Please include a website name".

Before it looked like this :

After it looks like this:

Paginating the list of users

Problem:
When a list of all users are shown, the list is not paginated. So the instructors see a list of all users that have enrolled for a course over all the years that they have offered the course.
Solution:
The existing project does not have the feature of pagination for the user list. So, currently, all the users are shown to the instructor.
The paginate_list method is supposed to paginate the list of users. The method was not called anywhere and also the method logic was incorrect.
With the latest implementation, the pagination is added with dropdown option - "25", "50", "100" and "ALL".
The method "paginate_list" is corrected and called at right place.
The default option is kept to "25" to reduce the web-page loading time.

Before it looked like this:

After it looks like this:

As can be clearly seen, different pagination options are now provided at the lower part of the view.

Test Plan

RSpec testing for account_requests_controller

Problem:
Since we created a new controller, tests for this controller did not exist. All those tests were written in the users_controller since that was where the methods were located earlier.
Solution:
We moved the tests from the users_controller_spec.rb file to the newly created accounts_request_controller.rb file. We also changed some tests in account_request_controller to route to their appropriate tests.
It can be seen that all 11 test cases passed for account_request_controller.

RSpec testing for users_controller

Problem:
The pagination for users_controller was not tested correctly in the previous implementation. We fixed this issue by writing a test for this.

Solution:
It can now be seen that all 18 test cases pass for users_controller.

Manual Testing

The link to our project instance.

1) To view pending account requests:
Login as the administrator.
Username = administrator5
Password = password
Navigate to Manage(in the horizontal tab at the top of the page) -> Pending Requests.
You will be able to see all the pending requests. The method that serves this view is in the newly created controller. So this is one way to test if the controller we wrote and the routes we configured are working.

2) To test the changes in the form for Account Requests:
Go to the login page. But do not log in.
Click the Request Account button.
You will be able to see the labels in bold and a hint of what should be entered in the last textbox.

3) To test the pagination:
Login as an instructor.
username: instructor6
password : password
Navigate to Manage(in the horizontal tab at the top of the page) -> users.
You will be able to see a list of users and page numbers where the list ends. You will also be able to select the number of users to show on each page using a dropdown at the top of the list.

4) Other changes:
To check if the build passed or not, this is the repository which we forked from expertiza. If you navigate to this and scroll down to the README, you should be able to see the build status and test case coverage statistics. You should also be ble to see the build status and all checks at the end of the pull request.
The other changes we made like renaming methods and documenting code are not so visible in the GUI and will have to be seen in the code.

Travis CI

During the second round review, we discovered that we had not synced the Main Expertiza beta branch with our forked beta branch, so we have synced our branch with the main Expertiza branch and made some commits to fix issues in Travis CI builds. Eventually, the Travis CI builds pass and can be seen in our pull request.

Design choices

1) Partial Views:
There are some partial views which are related to displaying forms to create/edit a user in the Users view. The same partials were earlier being used for displaying forms for requesting a new account. Now that we separated the two controllers, we had the following options to deal with these duplicated partials.
Option 1:
Keep accessing the partials in the users view from the newly created Accounts_request view. This would make the code DRY but harder to understand.
Option 2:
Duplicate the partials in the accounts request view.This would make some render partial statements hard to read but would make the code DRY.
Option 3:
Create partial files in the views folder. Have Users_controller, Account_requests_controller access these partials. The code becomes DRY and also readable.

We decided to go with the second option. Even though these forms contain the same code right now, due to changing requirements in the future, these views may need to be changed which would allow us to add more functionality in the future. This option gives some flexibility to the accounts_controller to modify the form without affecting the User's form. This design decision was taken after discussions with our mentor. The partials that are now duplicated are: _email, _name, _institution, _password, and _prefs which can be found in user and account_request views.

2) Refactoring variable names:
We refactored the variables names from requested_users to account_requests (in views, controller actions, etc.) since requested_users is a bad naming choice for our variable. As we are referring to AccountRequest model in our controllers, such renaming makes sense here.
For example:

 account_request = AccountRequest.all 

makes more sense than

 requested_users = AccountRequest.all

Important Links

1. Expertiza Documentation: Expertiza Main Page
2. To show our design changes, we have hosted our implementation on VCL. The link is: VCL instance
3. Link to the pull request Pull request.