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

From Expertiza_Wiki
Revision as of 00:24, 27 October 2022 by Avmeher (talk | contribs) (Added Problem 1 in Drawback)
Jump to navigation Jump to search

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: Violation of the YAGNI principle:
Functions related to moving, copying, renaming and deleting an uploaded file were present in the controller, javascript file as well as the view. Though in the view, only functionality related to delete was uncommented whereas the other functionalities were disabled. This resulted in unused code being accumulated for each of these functionalities in the javascript file, which increased the size of the javascript file.
# app/views/submitted_content/_submitted_files.html.erb

    <% if files.length > 0 and participant.team.directory_num != nil and participant.team.directory_num >= 0 %>
      <% if same_team_flag && stage != "Finished" %>
        <div style="padding-bottom:0.6em"><h5>File actions:</h5>
        <!--<input type="button" onclick="createNewFolder()"     value="Create new folder"    />
          <input type="button" onclick="copySelectedFile()"    value="Copy selected file"   />
          <input type="button" onclick="moveSelectedFile()"    value="Move selected file"   />
          <input type="button" onclick="renameFile()"          value="Rename selected file" />-->
          <input type="button" onclick="deleteSelectedFile();" value="Delete selected file" />
          <input type="reset" value="Reset">
        </div>

#app/assets/javascripts/submissions.js

function createNewFolder(){
	var new_folder = prompt("Enter a name for a new folder","");
	.
        .
        .
}

function moveSelectedFile(){	
	var old_filename = getSelectedName();
	.
        .
        .
}

function copySelectedFile(){	
	var old_filename = getSelectedName();
	.
        .
        .
}

function renameFile(){        
	var old_filename = getSelectedName();
	.
        .
        .
}
For handling the action based on the user input for the above functionality, a nested if else loop was also present which was executed according to the value passed by the view (delete, create, copy, move, rename). As the functions were already commented, this nested if else loop was also unnecessary and increasing the program complexity.
#app/controllers/submitted_content_controller.rb

'''# if else loop based on the actions'''

    if params[:faction][:delete]
      delete_selected_files
    elsif params[:faction][:rename]
      rename_selected_file
    elsif params[:faction][:move]
      move_selected_file
    elsif params[:faction][:copy]
      copy_selected_file
    elsif params[:faction][:create]
      create_new_folder
    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

  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
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2022_-_E2258._Refactor_submitted_content_controller&oldid=146024"