CSC/ECE 517 Fall 2021 - E2130. Refactor submitted content controller.rb

E2130: Refactor 'submitted_content_controller'

This page provides detailed explanation of the Submitted Content Controller which is a part of the Expertiza project. The aim of the project is to refactor the 'submitted_content_controller', which 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 refactoring some parts of the controller to incorporate the OODD principles so that the readability of code could be improved.

About Expertiza

The Expertiza platform employs a divide-and-conquer strategy for creating reusable learning objects via active-learning exercises built entirely on Ruby on Rails framework. Students get to choose from a list of tasks to complete either individually or in teams. They then prepare their work and submit it to a peer-review mechanism. On submission, other students can assess their peers work and provide feedback. Expertiza encourages students to collaborate in order to improve the learning experiences from one another. It aids their learning by making them translate what is taught in the lectures and apply those concepts to a real-world issue.

Problem Statement

submitted_content_controller had some problems that violate essential Rails design principles which needed to be rectified. Issues included some methods being too long which needed to be broken down, a few methods needed better naming and a few that were no longer needed.

Broadly, the following issues were addressed as a part of refactoring this controller:

• Renaming methods to more appropriate and functionality specific names.
• The existing code was reused to perform either the same function or re-purposed to do a similar function adhering to standards and improving overall quality of the code.
• Introduction of modular code in order to make each module easier to understand, test and refactor independently of others.

Problems and Solutions

• Problem 1: action_allowed method should be changed to use new_access_control methods.

The original code does not make use of the new access control methods which are a part of the authorization_helper.rb

def action_allowed?
    ['Instructor',
     'Teaching Assistant',
     'Administrator',
     'Super-Administrator',
     'Student'].include? current_role_name and
    ((%w[edit].include? action_name) ? are_needed_authorizations_present?(params[:id], "reader", "reviewer") : true) and
    one_team_can_submit_work?
  end

• Solution: On using the new access control methods:

  def action_allowed?
    if %w[edit update list_submissions].include? params[:action]
      current_user_has_admin_privileges? || current_user_teaching_staff_of_assignment?(params[:id])
    else
      current_user_has_ta_privileges?
    end

    case params[:action]
    when 'edit'
      current_user_has_student_privileges? &&
      are_needed_authorizations_present?(params[:id], "reader", "reviewer")
    when 'submit_file', 'submit_hyperlink'
      current_user_has_student_privileges? &&
      one_team_can_submit_work?
    else
      current_user_has_student_privileges?
    end
  end

2. Problem 2: Remove the comment “# hence use team count for the check”.

The current code no longer checks for the team count to see if a participant belongs to a team. Comment on line #19 removed.

3. Problem 3: 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: We found changing the name to disable_submission to be more apt in this case

 def disable_submission
    @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

4. Problem 4: submit_hyperlink is filled with a lot of logging code.

Logging is essential to understand the behavior of the application and to debug unexpected issues or for simply tracking events as in the production environment, we can’t debug issues without proper log files.

• Solution: We added a function for the sole purpose of logging so that it can be reused wherever required.

 def log_info(controller_name, participant_name, message, request)
    ExpertizaLogger.info LoggerMessage.new(controller_name, participant_name, message, request)
  end

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

• Solution: The function to remove_hyperlink performs as expected, hence the comment is no longer valid.

6. Problem 6: submit_file method is long.

• Solution: Implemented modular code by separating the function into simpler functions.

  def tested
    @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
    return curr_directory
  end

7. Problem 7: Move mail_assigned_reviewers to a mailer class.

• Solution: Moved the mail_assigned_reviewers to mailer_helper.rb

 def self.mail_assigned_reviewers(team)
    maps = ResponseMap.where(reviewed_object_id: @participant.assignment.id, reviewee_id: team.id, type: 'ReviewResponseMap')
    unless maps.nil?
      maps.each do |map|
        # Mailing function
        Mailer.general_email(
          to: User.find(Participant.find(map.reviewer_id).user_id).email,
          subject:  "Link to update the review for Assignment '#{@participant.assignment.name}'",
          cc: User.find_by(@participant.assignment.instructor_id).email,
          link: "Link: https://expertiza.ncsu.edu/response/new?id=#{map.id}",
          assignment: @participant.assignment.name
        ).deliver_now
      end
    end
  end

8. Problem 8: Change the method name of folder_action.

• Solution: Renamed folder_action to perform_folder_action.

 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]
      SubmittedFiles.delete_selected_files
    elsif params[:faction][:rename]
      SubmittedFiles.rename_selected_file
    elsif params[:faction][:move]
      SubmittedFiles.move_selected_file
    elsif params[:faction][:copy]
      SubmittedFiles.copy_selected_file
    elsif params[:faction][:create]
      create_new_folder
    end
    redirect_to action: 'edit', id: @participant.id
  end

9. Problem 9: Some of the actions in the code (from line 187 to 247) could perhaps be moved to another class

• Solution: Separated functions related to submission of files into a new helper class called submitted_files_helper.rb.

module SubmittedFiles

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


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



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

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

end

Testing Requirements

1. git clone https://github.com/Neelkanth7/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:

   i.  rspec spec/controllers/submitted_content_controller_spec.rb (Test file mentioned missing)

References

• Github link to the project.
• Link to the project description.