CSC/ECE 517 Fall 2022 - E2258. Refactor submitted content controller
E2258. Refactoring submitted content Controller
This project is a description of Expertiza OSS project E2258. Refactor submitted_content_controller.rb
About Expertiza
Expertiza is an open source project based on Ruby on Rails framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.
Problem Statement
The following tasks were accomplished in this project:
- Improved the clarity of code by improving the variable and parameter names.
- Implemented DRY principles by eliminating redundant code.
- Removed unused blocks of code, following the principles of YAGNI(You Ain’t Gonna Need It).
- Improvised the comments and made code more readable.
- Divided large function into Atomic function and made them more modular.
- Fixed security bugs as suggested by Automated Code Analysis
- Renamed the methods and variables to be more sensible
About Submitted Content Controller
submitted_content_controller controls everything a user submits to expertiza. This controller has some problems that violate some essential Rails design principles. There are few methods in this controller that are far too long and need to be broken down into multiple methods.
Current Implementation
Functionality
- Any user irrespective of his/ her privileges can view all the versions.
- The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
- Any user can delete any version
- The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
- Filtering of versions were restricted to the current user
- The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.
Drawbacks and Solutions
- Problem 1: The method paginate_list is doing more than one thing.
- The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
# For filtering the versions list with proper search and pagination. def paginate_list(id, user_id, item_type, event, datetime) # Set up the search criteria criteria = '' criteria = criteria + "id = #{id} AND " if id && id.to_i > 0 if current_user_role? == 'Super-Administrator' criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0 end criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0 criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any') criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any') criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND " criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND " if current_role == 'Instructor' || current_role == 'Administrator' end # Remove the last ' AND ' criteria = criteria[0..-5] versions = Version.page(params[:page]).order('id').per_page(25).where(criteria) versions end
- Solution: The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
- An administrator can see all the versions
- An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
- A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
- A Student can see all the versions created by him/ her.
- Problem 2: The search criteria created in the method paginate_list was difficult to comprehend.
- The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
- Solution: The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
- Problem 3: The paginate method can be moved to a helper class.
- VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
- Solution: The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.
New Implementation
Code improvements
- To reduce redundancy, we have created a new function "ensure_current_user_is_participant" and we have called this function before_action.
before_action :ensure_current_user_is_participant, only: %i[edit show submit_hyperlink folder_action]
- Changed the variable name :view to :show because "view" is generic variable and "show" is more sensible variable name
def show
- We have changed a larger function def submit_file into small function ,which is now more readable and simple to understand.
Automated Testing using RSPEC
The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
user-expertiza $rspec spec . . . Finished in 5.39 seconds (files took 25.33 seconds to load) 66 examples, 0 failures Randomized with seed 19254 . .
Testing from UI
Following are a few testcases with respectto our code changes that can be tried from UI: 1. To go to versions index page, type in the following url after logging in:
http://152.46.16.81:3000/versions
2. After logging in as student/instructor or admin : Try accessing the new, create, edit, update actions. These actions are not allowed to any of the users.
http://152.46.16.81:3000/versions/new This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.
3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
http://152.46.16.81:3000/versions/search
4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.
References
- Expertiza on GitHub
- GitHub Project Repository Fork
- The live Expertiza website
- Demo link
- Expertiza project documentation wiki
- Rspec Documentation
- Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin