CSC/ECE 517 Fall 2022 - E2258. Refactor submitted content controller

E2258. Refactoring submitted content Controller

This wiki 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 code readability by updating the generic variable and function names to more specific naming based on its functionalities.
• Implemented DRY principle by eliminating redundant code.
• Eliminated unused blocks of code, following the principles of YAGNI(You Ain’t Gonna Need It).
• Updated existing comments and added new ones to provide better understanding of written code.
• Divided large functions into atomic modules ensuring one function is responsible to a single task.

About Submitted Content Controller

Submitted content controller controls everything a user submits to expertiza. This function is mainly used to add hyperlinks and upload files to an assignment.

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.

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

1. Expertiza on GitHub
2. GitHub Project Repository Fork
3. The live Expertiza website
4. Demo link
5. Expertiza project documentation wiki
6. Rspec Documentation
7. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin