Add edge case handling to file rename feature

Why are these changes being introduced:

* Creating duplicate filenames or renaming the wrong file should be
  detected and prevented

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-643

How does this address that need:

* Adds checks to prevent creating a duplicate filename when renaming
  a thesis attachment
* Ensures the attachment being renamed is correctly associated with the
  thesis in the URL path or form submission or prevents renaming

Document any side effects to this change:

* report related test numbers changed when number of theses changed
* some rubocop autoformatting in thesis_controller_test

app/controllers/file_controller.rb

@@ -1,26 +1,74 @@
 class FileController < ApplicationController
   before_action :require_user
   before_action :authenticate_user!
+  before_action :load_thesis_and_attachment
   load_and_authorize_resource
   protect_from_forgery with: :exception
 
   def rename_form
-    @thesis = Thesis.find(params[:thesis_id])
-    @attachment = ActiveStorage::Attachment.find(params[:attachment_id])
+    return if appropriate_attachment_for_thesis?
+
+    error_nonmatching_thesis_message
+    redirect_to thesis_process_path(@thesis)
   end
 
   def rename
-    thesis = Thesis.find(params[:thesis_id])
-    attachment = ActiveStorage::Attachment.find(params[:attachment_id])
+    prerename_validations
+    rename_attachment unless flash.key?(:error)
+
+    redirect_to thesis_process_path(@thesis)
+  end
+
+  private
+
+  def prerename_validations
+    error_nonmatching_thesis_message unless appropriate_attachment_for_thesis?
+    error_duplicate_name_message if new_duplicate?
+  end
+
+  def new_duplicate?
+    return false if @thesis.files.count == 1
+    return true if attachment_names_except_current.include?(params[:attachment][:filename])
+
+    false
+  end
+
+  def attachment_names_except_current
+    @thesis.files.map { |x| x.filename if x != @attachment }.compact
+  end
 
-    attachment.blob.filename = params[:attachment][:filename]
+  def rename_attachment
+    @attachment.blob.filename = params[:attachment][:filename]
 
-    if attachment.blob.save
-      flash[:success] = "#{thesis.title} file #{attachment.filename} been updated."
+    if @attachment.blob.save
+      flash[:success] = success_message
     else
-      flash[:error] = "#{thesis.title} file was unable to be updated"
+      flash[:error] = error_rename_failed_message
     end
+  end
+
+  def appropriate_attachment_for_thesis?
+    @thesis.files.include?(@attachment)
+  end
+
+  def load_thesis_and_attachment
+    @thesis = Thesis.find(params[:thesis_id])
+    @attachment = ActiveStorage::Attachment.find(params[:attachment_id])
+  end
+
+  def error_duplicate_name_message
+    flash[:error] = 'The new name you chose is the same as an existing name of a file attached to this thesis.'
+  end
+
+  def success_message
+    "#{@thesis.title} file #{@attachment.filename} been updated."
+  end
+
+  def error_rename_failed_message
+    "#{@thesis.title} file was unable to be updated"
+  end
 
-    redirect_to thesis_process_path(thesis)
+  def error_nonmatching_thesis_message
+    flash[:error] = 'The file to be renamed was not associated with the thesis being edited.'
   end
 end

test/controllers/files_controller_test.rb

@@ -154,4 +154,92 @@ class FilesControllerTest < ActionDispatch::IntegrationTest
     assert_equal('renamed_a_pdf_too.pdf', @attachment.blob.filename.to_s)
     assert_equal('KADsjJnGD1sVUgvqyZOaRg==', @attachment.checksum)
   end
+
+  test 'creating a duplicate filename flashes a warning and does not rename file' do
+    sign_in users(:thesis_admin)
+    @thesis = theses(:rename_attachment_tests)
+    assert_equal(2, @thesis.files.count)
+    @attachment_one = @thesis.files.first
+    @attachment_two = @thesis.files.last
+
+    # unique filenames
+    assert(@thesis.files.map { |f| f.filename.to_s }.uniq.count == 2)
+
+    # rename one file to same as other
+    post rename_file_path(@thesis, @attachment_one),
+         params: { attachment: { filename: @attachment_two.filename.to_s } }
+    follow_redirect!
+    assert_equal thesis_process_path(theses(:rename_attachment_tests)), path
+
+    assert_select '.alert-banner.error',
+                  text: 'The new name you chose is the same as an existing name of a file attached to this thesis.', count: 1
+
+    @attachment_one.reload
+    @attachment_two.reload
+
+    # unique filenames (still)
+    assert(@thesis.files.map { |f| f.filename.to_s }.uniq.count == 2)
+  end
+
+  test 'renaming a file on a thesis with multiple files only renames the intended file' do
+    sign_in users(:thesis_admin)
+    @thesis = theses(:rename_attachment_tests)
+    assert_equal(2, @thesis.files.count)
+    @attachment_one = @thesis.files.first
+    @attachment_two = @thesis.files.last
+
+    # unique filenames
+    assert(@thesis.files.map { |f| f.filename.to_s }.uniq.count == 2)
+
+    # rename one file to same as other
+    post rename_file_path(@thesis, @attachment_one),
+         params: { attachment: { filename: 'hallo.pdf' } }
+    follow_redirect!
+    assert_equal thesis_process_path(theses(:rename_attachment_tests)), path
+
+    assert_select '.alert-banner.success',
+                  text: 'rename_my_attachments file hallo.pdf been updated.', count: 1
+
+    @attachment_one.reload
+    @attachment_two.reload
+
+    # unique filenames (still)
+    assert(@thesis.files.map { |f| f.filename.to_s }.uniq.count == 2)
+  end
+
+  test 'accessing rename form with non-matching thesis and attachment redirects to thesis page with warning' do
+    sign_in users(:thesis_admin)
+    @thesis = theses(:publication_review)
+    @different_thesis = theses(:rename_attachment_tests)
+    @attachment = @different_thesis.files.first
+    get "/file/rename/#{@thesis.id}/#{@attachment.id}"
+    assert_response :redirect
+
+    follow_redirect!
+    assert_equal thesis_process_path(theses(:publication_review)), path
+    assert_select '.alert-banner.error',
+                  text: 'The file to be renamed was not associated with the thesis being edited.', count: 1
+  end
+
+  test 'submitting rename form with non-matching thesis and attachment redirects to thesis page with warning' do
+    sign_in users(:thesis_admin)
+    @thesis = theses(:publication_review)
+    @different_thesis = theses(:rename_attachment_tests)
+    @attachment = @different_thesis.files.first
+
+    assert_equal('b_pdf.pdf', @attachment.blob.filename.to_s)
+
+    post rename_file_path(@thesis, @attachment),
+         params: { attachment: { filename: 'renamed_a_pdf_too.pdf' } }
+    follow_redirect!
+    assert_equal thesis_process_path(theses(:publication_review)), path
+
+    assert_select '.alert-banner.error',
+                  text: 'The file to be renamed was not associated with the thesis being edited.', count: 1
+
+    @attachment.reload
+
+    refute_equal('renamed_a_pdf_too.pdf', @attachment.blob.filename.to_s)
+    assert_equal('b_pdf.pdf', @attachment.blob.filename.to_s)
+  end
 end

test/controllers/thesis_controller_test.rb

@@ -254,8 +254,8 @@ def attach_files_to_records(tr, th)
     # options (two specific terms, and the "all terms" option)
     sign_in users(:processor)
     get thesis_select_path
-    assert_select 'table#thesisQueue tbody tr', count: 6
-    assert_select 'select[name="graduation"] option', count: 3
+    assert_select 'table#thesisQueue tbody tr', count: 7
+    assert_select 'select[name="graduation"] option', count: 4
     # Now request the queue with a term filter applied, and see three records
     get thesis_select_path, params: { graduation: '2018-06-01' }
     assert_select 'table#thesisQueue tbody tr', count: 3
@@ -698,7 +698,8 @@ def attach_files_to_records(tr, th)
     sign_in users(:processor)
     get thesis_publish_preview_path
     assert_select 'div#publish-to-dspace', count: 1
-    assert_select 'div#publish-to-dspace a[href=?]', thesis_publish_to_dspace_path, text: 'Publish theses to DSpace@MIT', count: 0
+    assert_select 'div#publish-to-dspace a[href=?]', thesis_publish_to_dspace_path,
+                  text: 'Publish theses to DSpace@MIT', count: 0
   end
 
   test 'publication preview, with a term specified, includes a button to actually publish that includes the term parameter' do
@@ -755,19 +756,22 @@ def attach_files_to_records(tr, th)
     get thesis_publish_to_dspace_path
     assert_redirected_to thesis_select_path
     follow_redirect!
-    assert_select '.alert-banner.warning', text: 'Please select a term before attempting to publish theses to DSpace@MIT.', count: 1
+    assert_select '.alert-banner.warning',
+                  text: 'Please select a term before attempting to publish theses to DSpace@MIT.', count: 1
     get thesis_publish_to_dspace_path, params: { graduation: 'all' }
-    assert_redirected_to thesis_select_path( params: { graduation: 'all' })
+    assert_redirected_to thesis_select_path(params: { graduation: 'all' })
     follow_redirect!
-    assert_select '.alert-banner.warning', text: 'Please select a term before attempting to publish theses to DSpace@MIT.', count: 1
+    assert_select '.alert-banner.warning',
+                  text: 'Please select a term before attempting to publish theses to DSpace@MIT.', count: 1
   end
 
   test 'publishing a specific term to dspace ends back at the processing queue with a flash success' do
     sign_in users(:processor)
     get thesis_publish_to_dspace_path, params: { graduation: '2018-09-01' }
-    assert_redirected_to thesis_select_path( params: { graduation: '2018-09-01' })
+    assert_redirected_to thesis_select_path(params: { graduation: '2018-09-01' })
     follow_redirect!
-    assert_select '.alert-banner.success', text: 'The theses you selected have been added to the publication queue. Status updates are not immediate.', count: 1
+    assert_select '.alert-banner.success',
+                  text: 'The theses you selected have been added to the publication queue. Status updates are not immediate.', count: 1
   end
 
   # ~~~~~~~~~~~~~~~~~~~~~ proquest export preview ~~~~~~~~~~~~~~~~~~~~~
@@ -876,7 +880,8 @@ def attach_files_to_records(tr, th)
     assert_enqueued_jobs 1
     assert_redirected_to thesis_proquest_export_preview_path
     follow_redirect!
-    assert_select '.alert-banner.success', text: 'The theses you selected will be exported. Status updates are not immediate.', count: 1
+    assert_select '.alert-banner.success',
+                  text: 'The theses you selected will be exported. Status updates are not immediate.', count: 1
   end
 
   test 'exporting to proquest with no eligible theses redirects to proquest export preview path with a flash message' do
@@ -960,7 +965,7 @@ def attach_files_to_records(tr, th)
     sign_in users(:admin)
     get reset_all_publication_errors_path
     assert_redirected_to thesis_select_path
-    
+
     error_count = Thesis.where(publication_status: 'Publication error').count
     assert error_count == 0
   end

test/fixtures/active_storage/attachments.yml

@@ -56,3 +56,15 @@ sip_one_bag_attachment:
   name: bag
   record: sip_one (SubmissionInformationPackage)
   blob: doctor_files_blob
+
+rename_files_attachment_a:
+  name: files
+  record: rename_attachment_tests (Thesis)
+  purpose: thesis_pdf
+  blob: rename_files_blob_a
+
+rename_files_attachment_b:
+  name: files
+  record: rename_attachment_tests (Thesis)
+  purpose: signature_page
+  blob: rename_files_blob_b

test/fixtures/active_storage/blobs.yml

@@ -69,3 +69,21 @@ engineer_files_blob:
   byte_size: 19333
   checksum: KADsjJnGD1sVUgvqyZOaRg==
   service_name: test
+
+rename_files_blob_a:
+  key: <%= ActiveStorage::Blob.generate_unique_secure_token %>
+  filename: a_pdf.pdf
+  content_type: application/pdf
+  metadata: '{"identified":true,"analyzed":true}'
+  byte_size: 19333
+  checksum: KADsjJnGD1sVUgvqyZOaRg==
+  service_name: test
+
+rename_files_blob_b:
+  key: <%= ActiveStorage::Blob.generate_unique_secure_token %>
+  filename: b_pdf.pdf
+  content_type: application/pdf
+  metadata: '{"identified":true,"analyzed":true}'
+  byte_size: 19333
+  checksum: KADsjJnGD1sVUgvqyZOaRg==
+  service_name: test

test/fixtures/theses.yml

@@ -370,3 +370,13 @@ publication_error_too:
   degrees: [one]
   departments: [two]
   publication_status: 'Publication error'
+
+rename_attachment_tests:
+  title: rename_my_attachments
+  abstract: MyText
+  grad_date: 2017-09-01
+  departments: [one]
+  degrees: [one]
+  advisors: [first]
+  copyright: mit
+  license: nocc

test/models/report_test.rb

@@ -22,9 +22,9 @@ class ReportTest < ActiveSupport::TestCase
     returned_rows = r.index_data['license'].count
     assert_equal expected_rows, returned_rows
     # This includes a row of all zeros, which could only have been generated by populate_category
-    assert r.index_data['license'][3][:data].values.all? { |v| v == 0 }
+    assert(r.index_data['license'][3][:data].values.all? { |v| v == 0 })
     # Also check that a row with expected data is represented accurately
-    assert_equal r.index_data['license'][1][:data].values, [3, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+    assert_equal r.index_data['license'][1][:data].values, [4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
   end
 
   # pad_terms is a private method that gets called by each reporting row, to ensure that all rows have some value for
@@ -48,7 +48,7 @@ class ReportTest < ActiveSupport::TestCase
     result = r.index_data
     assert_equal Department.count, result['departments'].pluck(:label).length
     assert_includes result['departments'].pluck(:label), Department.first.name_dw
-    assert_equal result['departments'][0][:data].values, [0, 2, 0, 0, 0, 0, 0, 3, 0, 0, 1, 0, 0, 0, 0]
+    assert_equal result['departments'][0][:data].values, [1, 2, 0, 0, 0, 0, 0, 3, 0, 0, 1, 0, 0, 0, 0]
   end
 
   test 'index includes summary data of authors not graduated' do
@@ -70,7 +70,7 @@ class ReportTest < ActiveSupport::TestCase
 
     r = Report.new
     result = r.index_data
-    assert_equal result['summary'][5][:data].values, [0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0]
+    assert_equal result['summary'][5][:data].values, [1, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0]
   end
 
   test 'authors not graduated summary data dedups theses with multiple files' do
@@ -92,7 +92,7 @@ class ReportTest < ActiveSupport::TestCase
     r = Report.new
     result = r.index_data
     assert_not_equal result['summary'][5][:data].values, [0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
-    assert_equal result['summary'][5][:data].values, [0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+    assert_equal result['summary'][5][:data].values, [1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
   end
 
   # ~~~~ Term detail report