Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation_errors_to option - customize copying of errors #26

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

brendon
Copy link

@brendon brendon commented Jul 20, 2020

This is an old PR that I had for Paperclip. See: thoughtbot#2347 for discussion.

I'd love to see it receiving the love it deserves :D

A couple of recent updates came through from my inclusion of recent Paperclip updates (README) but these can be reversed out by you if you like :)


Implicitly, errors are added to both base and the attribute, but we can now use add_validation_errors_to and set it to either :attribute or :base.
If it's not set, then errors would be added to both as below:
Screenshot from 2020-08-04 11-20-53

When we set it to :base, it will add error to the base only as:
Screenshot from 2020-08-04 11-20-16

And when it's set to :attribute, it will add it to attribute as:
Screenshot from 2020-08-04 11-19-58

@brendon
Copy link
Author

brendon commented Jul 20, 2020

I've updated this to resolve the conflicts. The test suite may need some work as it looks like some stuff with testing was changed between now and when I wrote this. I'm planning to migrate away from Paperclip at some point so probably can't commit too much time to this, but feel free to take it further and merge it if you like it :)

@ssinghi ssinghi requested a review from sbhawsingka July 30, 2020 14:56
@ssinghi
Copy link

ssinghi commented Jul 30, 2020

Thanks @brendon for this PR. We will review and discuss it, and get back to you soon.

Copy link
Collaborator

@sbhawsingka sbhawsingka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendon can you please rebase your branch with the master? Then it'll be easy for you to fix the tests.

Valid values for `add_validation_errors_to` are `:attribute`, `:base`,
or `:both`.

The option is globally set to `:both` that retains the existing
behaviour of adding validation errors to both the attribute and the
model base.
@brendon brendon force-pushed the add_validation_errors_to-option branch from 1348267 to 255d555 Compare August 1, 2020 02:00
end

context "with add_validation_errors_to set to :base" do
it "only adds error to base not attribute" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :base" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

context "with add_validation_errors_to set to :attribute" do
it "only adds error to attribute not base" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :attribute" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to base attribute"

assert @dummy.errors[:avatar_file_size].blank?,
"Error added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar].present?,
"Error not added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

Paperclip.options[:add_validation_errors_to] = :both
end

it "only adds error to base not attribute" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :base globally" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to attribute"

assert @dummy.errors[:avatar].blank?,
"Error added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar_file_size].present?,
"Error not added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@brendon
Copy link
Author

brendon commented Aug 1, 2020

Thanks @shalinibhawsingka-kreeti. Let me know if I've done the rebase wrong :)

@sbhawsingka
Copy link
Collaborator

Thanks @brendon, can you please use rspec mocks? For instance, if you have @dummy.stubs(:avatar_file_size).returns(11.kilobytes), you can write allow(@dummy).to receive(:avatar_file_size).and_return(11.kilobytes).

Paperclip.options[:add_validation_errors_to] = :both
end

it "only adds error to attribute not base" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :attribute globally" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to attribute"

assert @dummy.errors[:avatar].present?,
"Error not added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar_file_size].present?,
"Error not added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -205,6 +205,94 @@ def self.should_not_allow_attachment_file_size(size, options = {})
end
end

context "with add_validation_errors_to not set (implicitly :both)" do
it "adds error to both attribute and base" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -205,6 +205,94 @@ def self.should_not_allow_attachment_file_size(size, options = {})
end
end

context "with add_validation_errors_to not set (implicitly :both)" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

it "only adds error to base not attribute" do
build_validator matches: /.*\.png$/, allow_nil: false,
add_validation_errors_to: :base
allow(@dummy).to receive_messages(avatar_file_name: "data.txt")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

context "with add_validation_errors_to set to :base" do
it "only adds error to base not attribute" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :base" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

it "only adds error to attribute not base" do
build_validator matches: /.*\.png$/, allow_nil: false,
add_validation_errors_to: :attribute
allow(@dummy).to receive_messages(avatar_file_name: "data.txt")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@brendon
Copy link
Author

brendon commented Aug 2, 2020

Thanks @shalinibhawsingka-kreeti, I've updated the tests. Not sure what you want to do about the hound messages. They're kind of annoying and they're requesting changes to strings that bring them out of line with the rest of the file :)

@sbhawsingka
Copy link
Collaborator

Thanks @brendon, please use 4 space indentation for indenting a condition in an if statement spanning multiple lines, this is in regard to your code - if record.errors.include?(attribute) && [:both, :base].include?(options[:add_validation_errors_to])
Also, please align the arguments in the specs.

@brendon
Copy link
Author

brendon commented Aug 4, 2020

Have a go now :)

"Error not added to base attribute"

assert @dummy.errors[:avatar_file_size].blank?,
"Error added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar].present?,
"Error not added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to attribute"

assert @dummy.errors[:avatar].blank?,
"Error added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar_file_size].present?,
"Error not added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to base attribute"

assert @dummy.errors[:avatar_file_name].blank?,
"Error added to attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@validator.validate(@dummy)

assert @dummy.errors[:avatar].present?,
"Error not added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.


it "only adds error to base not attribute" do
build_validator matches: /.*\.png$/, allow_nil: false
allow(@dummy).to receive_messages(avatar_file_name: "data.txt")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

Paperclip.options[:add_validation_errors_to] = :both
end

it "only adds error to base not attribute" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end
end

context "with add_validation_errors_to set to :base globally" do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

"Error not added to attribute"

assert @dummy.errors[:avatar].blank?,
"Error added to base attribute"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@brendon
Copy link
Author

brendon commented Aug 4, 2020

Maybe turn off the string literal rule given most of the strings in Paperclip are double-quoted?

@sbhawsingka
Copy link
Collaborator

sbhawsingka commented Aug 5, 2020

Thanks @brendon, we are changing the string literal to use double-quotes.

@ssinghi ssinghi changed the title Add validation errors to option Add validation_errors_ to options - customize copying of errors Aug 5, 2020
@ssinghi ssinghi changed the title Add validation_errors_ to options - customize copying of errors Add validation_errors_to options - customize copying of errors Aug 5, 2020
@ssinghi ssinghi merged commit ce84dbf into kreeti:master Aug 5, 2020
@ssinghi ssinghi changed the title Add validation_errors_to options - customize copying of errors Add validation_errors_to option - customize copying of errors Aug 5, 2020
@brendon
Copy link
Author

brendon commented Aug 5, 2020

Thanks @shalinibhawsingka-kreeti, great to see this finally merged! :D

@brendon
Copy link
Author

brendon commented Aug 7, 2020

Would it be possible to release a new gem version? Then I can finally get off my fork branch after all these years! :D

@ssinghi
Copy link

ssinghi commented Aug 10, 2020

@brendon
Copy link
Author

brendon commented Aug 10, 2020

Thank you so much @ssinghi :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants