Skip to content

Commit

Permalink
Fix validation context for filename uniqueness
Browse files Browse the repository at this point in the history
In 7075279, the decision was made
to scope attachment validations to user submissions on attachment
models only:

> This updates the Attachment model to scope the AttachmentValidator so
> that it's only part of the model validations when a user creates or
> updates an attachment.

This was to prevent an issue whereby invalid attachments would be
'shed' when creating new editions. We only want the validations to
run when editing the attachments themselves.

The 'shedding' bug hasn't returned, but we do have a new one: when
creating a new edition on a document that has multiple attachments
of the same filename, we hit this validation error: 'Validation
failed: This <document type> already has a file called
"<filename>"'.

The fix was to only call the validation methods when making an
edit to an attachment, not to its associated document. It looks
as though the filename uniqueness validation check was accidentally
missed from the commit above.

Trello: https://trello.com/c/FJuN46qU/3217-unable-to-create-new-edition-in-whitehall-validation-error
  • Loading branch information
ChrisBAshton committed Nov 27, 2024
1 parent 4ebd734 commit f3e26c8
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion app/models/file_attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class FileAttachment < Attachment

accepts_nested_attributes_for :attachment_data, reject_if: ->(attributes) { attributes[:file].blank? && attributes[:file_cache].blank? }

validate :filename_is_unique
validate :filename_is_unique, on: :user_input

def filename_changed?
previous_attachment_data_id = attachment_data_id_was
Expand Down
2 changes: 1 addition & 1 deletion test/unit/app/models/file_attachment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def assert_delegated(attachment, method)
attachable = create(:policy_group, attachments: [build(:file_attachment, file: file_fixture("whitepaper.pdf"))])
duplicate = build(:file_attachment, file: file_fixture("whitepaper.pdf"), attachable:)

assert_not duplicate.valid?
assert_not duplicate.valid?(:user_input)
assert_match %r{This policy group already has a file called "whitepaper.pdf"}, duplicate.errors[:base].first
end

Expand Down

0 comments on commit f3e26c8

Please sign in to comment.