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.
• Added new tests to increase code coverage

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

• Only a user who is also the participant of the assignment can view the assignment

Any other user is redirected to the main page of the system.

• Participant cannot upload a file/hyperlink if the assignment submission deadline has crosses

After the deadline, participants can just view the assignment.

• Files of particular type and max 5 mb allowed

No file beyond 5mb is allowed to be uploaded and a error message is displayed.

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: A method performing multiple functions:

The submit_file method in the controller app/controllers/submitted_content_controller.rb was performing multiple functions such as checking the file size, validating the file integrity by checking its type and then using its content to upload the file. This resulted in the controller being lengthy and complex. Ideally, there should be a modular approach for the above requirement.

• Solution: We have made the controller modular by introducing the below methods:
  • validate_file_size_type
  • get_sanitized_file_path
  • get_curr_directory

• Problem 3: Convention over configuration:

The method view is performing similar to that of show function which is the convention used by Rails for naming the function. Though in the code, the method was called 'view' and the related variables were named according to that.

• Solution: The method was renamed to show and the related variables were renamed accordingly.

• Problem 4: Violation of the DRY principle:

The function of authorization was performed in each of the functions that were defined in the controller. This caused repetition of the code in each of the method. A change in the common functionality would mean changes at multiple places.

• Solution: The common functionality was implemented in a common method called ensure_current_user_is_participant and that function is called in the before_action of each of the methods defined.

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 (Refer to this commit: https://github.com/expertiza/expertiza/pull/2459/commits/eb9353fccfef245dab13915e80616bc46cca723a)

• 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

Test Plan

The following plan is considered to ensure thorough testing of submitted_content_controller:

• Automated Tests with both positive and negative scenarios to ensure all functions of controller are working as expected.
• Manual Tests using UI to ensure the functionality of overall system isn't affected by adding new changes. These tests mainly involve the steps of adding/removing files and hyperlinks from assignment submission page.

Further details on above tests can be found in below sections.

Automated Testing using RSPEC

The submitted_content_controller has existing automated rspec tests in place. Furthermore following tests were added to the code to ensure the functionality is working as expected and thereby increasing code coverage:

# Test to verify is one_team_can_submit_work? function is working as expected
  describe '#one_team_can_submit_work' do
    it 'participant cannot submit work if team doesnt hold a topic' do
      allow(AssignmentParticipant).to receive(:find).and_return(participant)
      allow(SignUpTopic).to receive(:where).and_return([nil])
      params = { id: 21 }
      allow(controller).to receive(:params).and_return(params)
      expect(controller.send(:one_team_can_submit_work?)).to eql(false)
    end
    it 'participant can submit work if team holds a topic' do
      allow(AssignmentParticipant).to receive(:find).and_return(participant)
      allow(SignUpTopic).to receive(:where).and_return([signup_topic])
      allow(SignedUpTeam).to receive(:topic_id).and_return(1)
      params = { id: 21 }
      allow(controller).to receive(:params).and_return(params)
      expect(controller.send(:one_team_can_submit_work?)).to eql(true)
    end
  end

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)
51 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 (ID: instructor6, Password: password), 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