CSC/ECE 517 Spring 2022 - E2221. Refactor submitted content controller.rb

E2221. Refactoring of submitted_content_controller

This page provides a description of the refactoring done on 'submitted_content_controller', a part of the Open Source project - Expertiza, at North Carolina State University.

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, enforce deadlines etc. It also allows the instructor to create list of topics from which students can bid for their project requirements. Students can form teams in Expertiza to work on various projects and assignments. Students can also review other student's or team's work and provide useful information on the scope of improvement of their project. Expertiza supports submission across various document types, including the URLs and wiki pages.

Background on submitted_content_controller

The submitted_content_controller controls everything a user submits to expertiza. It contains the methods to submit assignment related information such as submit & remove hyperlinks, files and other relevant information that could be part of the assignment. It also handles views based on the user roles and permissions they have. The project involved working out on few issues of designing in the controller to incorporate the OODD principles and ruby design principles so that the readability of code could be improved.

Problem Statement

submitted_content_controller had some problems that violate essential Rails design principles which needed to be rectified.

The following issues were addressed as a part of refactoring this controller:

• Renaming methods to more appropriate and functionality specific names.
• some methods being too long which needed to be broken down.
• Remove or change the comment for the methods to be more meaningful and reflective on the actual functionality.

List of Issues in submitted_content_controller.rb

• Remove the comment “# hence use team count for the check”.

• Change the method name view to something more informative of the method.

• Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

• submit_file method is long. Try to break it into multiple methods.

• Change the method name folder_action to something like perform_folder_action.

• Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

Files modified and created in the project

We worked on the 'submitted_content_controller.rb' and created a new controller file while refactoring and named it 'submitted_folder_controller'.

Solutions devised

• Issue 1 : Remove the comment “# hence use team count for the check”.

Removed the comment on line 19 because the code no longer checks the team count.

• Issue 2 : Change the method name view to something more informative of the method.

The original code uses the generic 'view' method name to display a view corresponding to a case when submissions cannot be accepted, for instance in the case when a deadline is passed.

def view
    @participant = AssignmentParticipant.find(params[:id])
    return unless current_user_id?(@participant.user_id)
    @assignment = @participant.assignment
    # @can_submit is the flag indicating if the user can submit or not in current stage
    @can_submit = false
    @stage = @assignment.get_current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
    redirect_to action: 'edit', id: params[:id], view: true
  end

• Solution : The function name view is changed to prevent_submission, according to the ruby method naming conventions, as this is called when @assignment.submission_allowed(topic_id) is false which essentially means user should not be able to submit

def prevent_submission
    @participant = AssignmentParticipant.find(params[:id])
    # check if the current user id is same as participant user id
    return unless current_user_id?(@participant.user_id)

    # @assignment is used to store the assignment of the participant
    @assignment = @participant.assignment
    # @can_submit is the flag indicating if the user can submit or not in current stage
    @can_submit = false
    @stage = @assignment.current_stage(SignedUpTeam.topic_id(@participant.parent_id, @participant.user_id))
    redirect_to action: 'edit', id: params[:id], view: true
  end

• Issue 3 : Check the validity of the comment ”# Note: This is not used yet in the view until we all decide to do so”.

remove_hyperlink is used in the view - expertiza/app/views/submitted_content/_hyperlink.html.erb

Therefore, the comment is invalid. So we removed the comment on line 67 because it was redundant.

• Issue 4 : submit_file method is long. Try to break it into multiple methods.

• Solution : The changes made for submit_file and reasons for them:

def get_file_upload(participant, file, file_content) : The main reason for creating this new method from submit_file is that in order to submit a file, we need to first upload the file for submission, so that's a two related but different actions. Therefore, this method handles the work needed to be done for uploading a file for submission. The refactor is needed for making the submit_file shorter and more readable.

def notify_reviewers(participant): Originally, submit_file has a final step to notify all reviewers assigned to this reviewee/submission after the file is submitted. The action of notify reviewers can be refactored into its own method from the submit_file method since they are two different actions, and now notify_reviewers method can be reused if needed for other type of submission such as hyperlink.

Also, added some error codes in submit_file method additionally for authentication of user, for content size and content integrity in order to actually proceed with submitting a file.

def submit_file
    participant = AssignmentParticipant.find(params[:id])
    # check if the current user id is same as participant user id
    # Validate the user and on failure redirect to edit function for Submitting a file.
    unless current_user_id?(participant.user_id)
      flash[:error] = "Authentication Error"
      redirect_to action: 'edit', id: participant.id
      return
    end

    file = params[:uploaded_file]
    file_size_limit = 5

    # Check if file size is greater than the specified limit then redirect to edit function
    unless check_content_size(file, file_size_limit)
      flash[:error] = "File size must smaller than #{file_size_limit}MB"
      redirect_to action: 'edit', id: participant.id
      return
    end

    file_content = file.read

    # check if file type is not among specified then redirect to edit function
    unless check_content_type_integrity(file_content)
      flash[:error] = 'File type error'
      redirect_to action: 'edit', id: participant.id
      return
    end

    # Submit the file successfully
    full_filename = get_file_upload(participant, file, file_content)
    assignment = Assignment.find(participant.parent_id)
    team = participant.team
    SubmissionRecord.create(team_id: team.id,
                            content: full_filename,
                            user: participant.name,
                            assignment_id: assignment.id,
                            operation: "Submit File")
    ExpertizaLogger.info LoggerMessage.new(controller_name, participant.name, 'The file has been submitted.', request)
    notify_reviewers(participant)
  end

def get_file_upload(participant, file, file_content)
    participant.team.set_student_directory_num
    @current_folder = DisplayOption.new
    @current_folder.name = '/'
    @current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
    curr_directory = if params[:origin] == 'review'
                       participant.review_file_path(params[:response_map_id]).to_s + @current_folder.name
                     else
                       participant.team.path.to_s + @current_folder.name
                     end
    FileUtils.mkdir_p(curr_directory) unless File.exist? curr_directory
    safe_filename = file.original_filename.tr('\\', '/')
    safe_filename = FileHelper.sanitize_filename(safe_filename) # new code to sanitize file path before upload*
    full_filename = curr_directory + File.split(safe_filename).last.tr(' ', '_') # safe_filename #curr_directory +
    File.open(full_filename, 'wb') { |f| f.write(file_content) }
    if params['unzip']
      SubmittedContentHelper.unzip_file(full_filename, curr_directory, true) if file_type(safe_filename) == 'zip'
    end
    return full_filename
  end

def notify_reviewers(participant)
    participant.mail_assigned_reviewers

    if params[:origin] == 'review'
      redirect_to :back
    else
      redirect_to action: 'edit', id: participant.id
    end
  end

• Issue 5 : Change the method name folder_action to something like perform_folder_action.

Too many nested if-else statements inside this method, can we do something better?

• Solution : Renamed the folder_action method to perform_folder_action so that method name is more indicative of the actions performed by it.

Since we are not working with objects and params is a method in ruby, changing the if-else statements would not give any advantage. Even changing into case statements would not be advantageous to the code.

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

     @current_folder = DisplayOption.new
     @current_folder.name = '/'
     @current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
     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
     redirect_to action: 'edit', id: @participant.id
   end

• Issue 6 : We have commented the code in both submitted_content_controller and submitted_folder_container where necessary to describe the function a method is performing for better understanding of code. We kept all the comments in sync with the code.

• Issue 7 : Some of the actions in the code (from line 218 to 285) could perhaps be moved to another class.

• Solution : The perform_folder_action method in submitted_content_controller and the methods following it are all related to actions such as delete, move, rename, copy or create folders. Since all these methods are related to folder actions, we created a new class and controller to perform these actions. We moved all the related code to a new submitted_folder_controller. The submitted_folder_controller has single responsibility of handling folder related actions. This will made the previously long code more readable and understandable.

submitted_folder_controller.rb looks like below:

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

    @current_folder = DisplayOption.new
    @current_folder.name = "/"
    @current_folder.name = FileHelper.sanitize_folder(params[:current_folder][:name]) if params[:current_folder]
    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
    redirect_to action: 'edit', id: @participant.id
  end

  # Raises error if there are issues with downloading of current selected folder and file in it
  def download
    begin
      folder_name = params['current_folder']['name']
      file_name = params['download']
      raise "Folder_name is nil." if folder_name.nil?
      raise "File_name is nil." if file_name.nil?
      raise "Cannot send a whole folder." if File.directory?(folder_name + "/" + file_name)
      raise "File does not exist." unless File.exist?(folder_name + "/" + file_name)

      send_file(folder_name + "/" + file_name, disposition: 'inline')
    rescue StandardError => e
      flash[:error] = e.message
    end
  end

  def controller_locale
    locale_for_student
  end

  # Moves file from current location/directory to specified directory/location and raises error if any problem occurs
  def move_selected_file
    old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    newloc = @participant.dir_path
    newloc += "/"
    newloc += params[:faction][:move]
    begin
      FileHelper.move_file(old_filename, newloc)
      flash[:note] = "The file was successfully moved from \"/#{params[:filenames][params[:chk_files]]}\" to \"/#{params[:faction][:move]}\""
    rescue StandardError => e
      flash[:error] = "There was a problem moving the file: " + e.message
    end
  end

  # To rename a selected file, checks any discrepancies in new file name, if new filename is same as some existing filename in current directory
  # error is flashed.
  def rename_selected_file
    old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:rename])
    begin
      raise "A file already exists in this directory with the name \"#{params[:faction][:rename]}\"" if File.exist?(new_filename)

      File.send("rename", old_filename, new_filename)
    rescue StandardError => e
      flash[:error] = "There was a problem renaming the file: " + e.message
    end
  end

  # Function to delete selected file
  def delete_selected_files
    filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    FileUtils.rm_r(filename)
    participant = Participant.find_by(id: params[:id])
    assignment = participant.try(:assignment)
    team = participant.try(:team)
    SubmissionRecord.create(team_id: team.try(:id),
                            content: filename,
                            user: participant.try(:name),
                            assignment_id: assignment.try(:id),
                            operation: "Remove File")
    ExpertizaLogger.info LoggerMessage.new(controller_name, @participant.name, 'The selected file has been deleted.', request)
  end

  # Function to copy selected file
  def copy_selected_file
    old_filename = params[:directories][params[:chk_files]] + "/" + params[:filenames][params[:chk_files]]
    new_filename = params[:directories][params[:chk_files]] + "/" + FileHelper.sanitize_filename(params[:faction][:copy])
    begin
      raise "A file with this name already exists. Please delete the existing file before copying." if File.exist?(new_filename)
      raise "The referenced file does not exist." unless File.exist?(old_filename)

      FileUtils.cp_r(old_filename, new_filename)
    rescue StandardError => e
      flash[:error] = "There was a problem copying the file: " + e.message
    end
  end

  # Function to create new folder
  def create_new_folder
    newloc = @participant.dir_path
    newloc += "/"
    newloc += params[:faction][:create]
    begin
      FileHelper.create_directory_from_path(newloc)
      flash[:note] = "The directory #{params[:faction][:create]} was created."
    rescue StandardError => e
      flash[:error] = e.message
    end
  end
end

Would it be helpful and clean to separate this controller into a submitted_files_controller and a submitted_hyperlinks_controller?

• The refactor is not necessary or relatively minor because there are only two methods(remove_hyperlink and submit_hyperlink) in submitted_content_controller that are mainly on hyperlinks. Moreover, submission via hyperlink is as simple as it is now, and it doesn't need a controller of its own. It will just overcomplicate things. It is better to keep all methods related to submission under one controller.

Testing Plan

Rspec Testing:

1. git clone https://github.com/LeooHsiang/expertiza

2. Change directory to expertiza. Run "bundle install" and rails db:migrate.

3. Start the rails server.

4. Run the following command in a new terminal of the expertiza directory:

rspec spec/controllers/submitted_content_controller_spec.rb

Manual Testing: Manual Testing Video with voice over

Edge Cases:

• Return error when the user who is not the assignment's participant trying to do the submission
• Return 'You or your teammate(s) have already submitted the same hyperlink.' when the same hyperlink already submitted
• Return error when the URL/hyperlink is invalid
• Return error when the uploading file is smaller than 5 MB
• Return error when the uploading file is not pdf or file type is not among specified

References

1. Expertiza on GitHub
2. Forked Repository Link
3. Expertiza website
4. Expertiza Documentation