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

• Remove the comment “# hence use team count for the check”. The code it refers to is no longer there.
• Change the method name view to something more informative of the method.
• Line 79 Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.
• If we are not using this method, then remove it. We want to follow YAGNI (You Ain’t Gonna Need It)
• [Important] submit_file method is long. Try to break it into multiple methods.
• Also, check if variables defined as instance variables can be made local variables?
• Inside folder_action, Too many nested if-else statements inside this method, can we do something better?
• The code needs method commenting and better commenting in general.
• Some of the actions in the code (from line 187 to 247) could perhaps be moved to another class. Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?
• If there is any dead code (not being used anymore ) delete it

Solution

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.

Code improvements

• To reduce redundancy and ensure DRY principle, the following function "ensure_current_user_is_participant" was created and called in before_action instead of calling it individually in multiple functions.

before_action :ensure_current_user_is_participant, only: %i[edit show submit_hyperlink folder_action]

def ensure_current_user_is_participant
   @participant = AssignmentParticipant.find(params[:id])
   return unless current_user_id?(@participant.user_id)
end

• Renamed the :view function to :show. This was done to make it more relatable to the code written within that function.

• Divided the submit_file function into multiple atomic functions to ensure each function is responsible for one task.

   # Check file content size and file type
   def validate_file_size_type(file, file_size_limit, file_content)
   .
   .
   end

   # Sanitize and return the file name
   def get_sanitized_file_path(file, curr_directory)
   .
   .
   .
   end

   # Get current directory path
   def get_curr_directory(participant)
    .
    .
    .
   end

   def submit_file
   .
   .
   if (!validate_file_size_type(file, file_size_limit, file_content))
   .
   .
   curr_directory = get_curr_directory(participant)
   .
   .
   sanitized_file_path = get_sanitized_file_path(file, curr_directory)
   .
   .
   end

• Removed existing unused code for copy, move, rename file and create folder.

• Added new comments and updated old ones. Example:

   # Validate whether a particular action is allowed by the current user or not based on the privileges
   def action_allowed?

   # submit_hyperlink is called when a new hyperlink is added to an assignment
   def submit_hyperlink

Automated Testing using RSPEC

The submitted_content_controller has automated rspec tests in place. All the tests currently pass after pushing the updates to the code. The tests can be executed "rspec spec/controllers/submitted_content_controller_spec.rb" command as shown below.

rspec spec/controllers/submitted_content_controller_spec.rb
.
.
.

Finished in 15.47 seconds (files took 10 seconds to load)
49 examples, 0 failures, 15 pending

Testing from UI

Following are a few testcases with respectto our code changes that can be tried from UI:

1. To go to assignments submission page, type in the following url: http://yoururl.tech/expertiza

2. After logging in as student/instructor or admin, go to Assignments (http://152.7.176.53:8080/student_task/list) and open the Test1 assignment, and try to submit a new file or hyperlink.

3. Following test cases were checked:

• Upload a file greater than 5 mb and it shouldn't be allowed and an error should be visible
• Upload a txt file and it shouldn't be allowed and an error should be visible
• Upload a pdf file less than 5 mb and it should be allowed to be uploaded
• Uploaded file can be deleted
• New hyperlinks can be added
• Existing hyperlinks can be deleted

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