Enhancement: Attachment Checksum Validation (#699)

* change checksum validation to allow same checksum but different filename, prevent duplicat filename, fix and add tests

* revert or conditional

* cleanup

* fix validation and associated testing

* remove rubocop disable that is no longer needed

* change selectors for possible flaky tests

* remove redundant assert that sometimes flakes

app/validators/attachment_checksum_validator.rb

@@ -8,12 +8,14 @@ def validate(record)
     klass = record.class
 
     # add error if another Attachment associated with the Attachable exists with the same checksum
+    # no error occurs if the files have same name but different checksum or same checksum but different filenames
     if klass.joins(:file_blob)
             .where.not(id: record.id)
             .exists?(attachable_id: record.attachable_id,
                      attachable_type: record.attachable_type,
                      deleted_at: nil,
-                     file_blob: { checksum: record.file.checksum })
+                     file_blob: { checksum: record.file.checksum, filename: record.file.filename.to_s })
+
       record.errors.add(:file, :checksum_uniqueness)
     end
   end

test/graphql/attach_files_to_sample_test.rb

@@ -165,9 +165,9 @@ def setup
     assert_equal :error, data['status'][blob_file.signed_id]
   end
 
-  test 'attachFilesToSample mutation attach file with same checksum' do
+  test 'attachFilesToSample mutation attach file with same checksum but different names' do
     sample = samples(:sampleJeff)
-    # These files have the same md5 sum
+
     blob_file_a = active_storage_blobs(:attachment_md5_a_test_blob)
     blob_file_b = active_storage_blobs(:attachment_md5_b_test_blob)
 
@@ -200,14 +200,101 @@ def setup
     data = result['data']['attachFilesToSample']
 
     assert_not_empty data, 'attachFilesToSample should be populated when no authorization errors'
-    expected_status = { blob_file_b.signed_id => :error }
+    expected_status = { blob_file_b.signed_id => :success }
+    assert_equal expected_status, data['status']
+    assert_not_empty data['sample']
+    assert_equal 2, sample.attachments.count
+  end
+
+  test 'attachFilesToSample mutation attach file with same checksum and same names' do
+    sample = samples(:sampleJeff)
+
+    blob_file_a = active_storage_blobs(:attachment_md5_a_test_blob)
+    blob_file_a2 = active_storage_blobs(:attachment_md5_a_test_blob)
+
+    assert_equal 0, sample.attachments.count
+
+    result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION,
+                                 context: { current_user: @user, token: @api_scope_token },
+                                 variables: { files: [blob_file_a.signed_id],
+                                              sampleId: sample.to_global_id.to_s })
+
+    assert_nil result['errors'], 'should work and have no errors.'
+
+    data = result['data']['attachFilesToSample']
+
+    assert_not_empty data, 'attachFilesToSample should be populated when no authorization errors'
+    expected_status = { blob_file_a.signed_id => :success }
+    assert_equal expected_status, data['status']
+    assert_not_empty data['sample']
+    assert_equal 1, sample.attachments.count
+    assert_equal 'md5_a', sample.attachments[0].filename.to_s
+
+    # Second file
+    result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION,
+                                 context: { current_user: @user, token: @api_scope_token },
+                                 variables: { files: [blob_file_a2.signed_id],
+                                              sampleId: sample.to_global_id.to_s })
+
+    assert_nil result['errors'], 'should work and have no errors.'
+
+    data = result['data']['attachFilesToSample']
+
+    assert_not_empty data, 'attachFilesToSample should be populated when no authorization errors'
+    expected_status = { blob_file_a2.signed_id => :error }
     assert_equal expected_status, data['status']
     assert_not_empty data['sample']
-    expected_error = { blob_file_b.signed_id => ['File checksum matches existing file'] }
+    expected_error = { blob_file_a2.signed_id => ['File checksum matches existing file'] }
     assert_equal expected_error, data['errors']
     assert_equal 1, sample.attachments.count
   end
 
+  test 'attachFilesToSample mutation attach file with different checksum and same names' do
+    sample = samples(:sampleJeff)
+
+    blob_file_a = active_storage_blobs(:attachment_md5_a_test_blob)
+    blob_file_b = active_storage_blobs(:attachment_attach_files_to_sample_test_blob)
+
+    # match blob_file_b.filename to blob_file_a.filename
+    blob_file_b.filename = 'md5_a'
+    blob_file_b.save
+    assert_equal 0, sample.attachments.count
+
+    result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION,
+                                 context: { current_user: @user, token: @api_scope_token },
+                                 variables: { files: [blob_file_a.signed_id],
+                                              sampleId: sample.to_global_id.to_s })
+
+    assert_nil result['errors'], 'should work and have no errors.'
+
+    data = result['data']['attachFilesToSample']
+
+    assert_not_empty data, 'attachFilesToSample should be populated when no authorization errors'
+    expected_status = { blob_file_a.signed_id => :success }
+    assert_equal expected_status, data['status']
+    assert_not_empty data['sample']
+    assert_equal 1, sample.attachments.count
+    assert_equal 'md5_a', sample.attachments[0].filename.to_s
+
+    # Second file
+    result = IridaSchema.execute(ATTACH_FILES_TO_SAMPLE_BY_SAMPLE_ID_MUTATION,
+                                 context: { current_user: @user, token: @api_scope_token },
+                                 variables: { files: [blob_file_b.signed_id],
+                                              sampleId: sample.to_global_id.to_s })
+
+    assert_nil result['errors'], 'should work and have no errors.'
+
+    data = result['data']['attachFilesToSample']
+
+    assert_not_empty data, 'attachFilesToSample should be populated when no authorization errors'
+    expected_status = { blob_file_b.signed_id => :success }
+    assert_equal expected_status, data['status']
+    assert_not_empty data['sample']
+    sample.reload
+    assert_equal 2, sample.attachments.count
+    assert_equal 'md5_a', sample.attachments[1].filename.to_s
+  end
+
   test 'attachFilesToSample mutation attach 2 files at once' do
     sample = samples(:sampleJeff)
     blob_file_a = active_storage_blobs(:attachment_attach_files_to_sample_test_blob)

test/models/attachment_test.rb

@@ -25,6 +25,25 @@ def setup
     assert new_attachment.errors.added?(:file, :checksum_uniqueness)
   end
 
+  test 'file checksum matches another Attachment associated with the Attachable but with different filename' do
+    assert_equal 2, @sample.attachments.count
+    new_attachment = @sample.attachments.build(file:
+      { io: Rails.root.join('test/fixtures/files/test_file.fastq').open,
+        filename: 'copy_of_test_file.fastq' })
+    assert new_attachment.valid?
+    @sample.save
+    assert_equal 3, @sample.attachments.count
+  end
+
+  test 'file checksum differs from another Attachment associated with the Attachable but with same filename' do
+    new_attachment = @sample.attachments.build(file:
+      { io: Rails.root.join('test/fixtures/files/test_file_C.fastq').open,
+        filename: 'test_file.fastq' })
+    assert new_attachment.valid?
+    @sample.save
+    assert_equal 3, @sample.attachments.count
+  end
+
   test 'metadata fastq file types' do
     new_fastq_attachment_ext_fastq =
       @sample.attachments.build(file: { io: Rails.root.join('test/fixtures/files/test_file_1.fastq').open,

test/system/data_exports_test.rb

@@ -686,7 +686,7 @@ def setup
     assert_selector 'button.pointer-events-none.cursor-not-allowed.bg-slate-100.text-slate-600',
                     text: I18n.t('projects.samples.index.create_export_button.label')
 
-    within %(#samples-table) do
+    within first('tbody') do
       find("input[type='checkbox'][value='#{@sample30.id}']").click
     end
 
@@ -701,17 +701,18 @@ def setup
       click_button I18n.t('data_exports.new_linelist_export_dialog.submit_button')
     end
 
-    assert_selector 'dl', count: 1
-    assert_selector 'div:nth-child(2) dd', text: 'test csv export'
-    assert_selector 'div:nth-child(4) dd', text: 'csv'
+    within first('dl') do
+      assert_selector 'div:nth-child(2) dd', text: 'test csv export'
+      assert_selector 'div:nth-child(4) dd', text: 'csv'
+    end
   end
 
   test 'create xlsx export from group samples page' do
     visit group_samples_url(@group1)
     assert_selector 'button.pointer-events-none.cursor-not-allowed.bg-slate-100.text-slate-600',
                     text: I18n.t('projects.samples.index.create_export_button.label')
 
-    within %(#samples-table) do
+    within first('tbody') do
       find("input[type='checkbox'][value='#{@sample1.id}']").click
     end
 
@@ -727,9 +728,10 @@ def setup
       click_button I18n.t('data_exports.new_linelist_export_dialog.submit_button')
     end
 
-    assert_selector 'dl', count: 1
-    assert_selector 'div:nth-child(2) dd', text: 'test xlsx export'
-    assert_selector 'div:nth-child(4) dd', text: 'xlsx'
+    within first('dl') do
+      assert_selector 'div:nth-child(2) dd', text: 'test xlsx export'
+      assert_selector 'div:nth-child(4) dd', text: 'xlsx'
+    end
   end
 
   test 'linelist export with ready status does not have preview tab' do