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

Fix: Fix issue with incorrect error messages #209

Conversation

jozzi05
Copy link

@jozzi05 jozzi05 commented Oct 31, 2023

This PR fixes issues with randomly generated error codes when using extra options like :if, :on etc. Additionally, it adds a new extension to the size matcher that enables users to define custom validation context.

Error messages:

  1. on example
  • before:
    Size with context translation missing: en.activerecord.errors.models.size/portfolio.attributes.size_with_context.file_size_not_on
  • after:
    Size with context file size must be less than 2 KB (current size is 10 KB)
  1. if example
  • before:
    Size with if translation missing: en.activerecord.errors.models.size/portfolio.attributes.size_with_if.file_size_not_if
  • after:
    Size with if file size must be less than 2 KB (current size is 10 KB)

@igorkasyanchuk
Copy link
Owner

@Mth0158 @gr8bit do you have any comments about this PR? do you approve?

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 3, 2023

@igorkasyanchuk I'll have a look at it this weekend :)

Copy link
Collaborator

@Mth0158 Mth0158 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work @jozzi05, here are some improvements and it works for me to merge it!
@igorkasyanchuk @gr8bit let us know if something is missing

@jozzi05 jozzi05 requested a review from Mth0158 November 3, 2023 17:30
@igorkasyanchuk
Copy link
Owner

looks good for me, @Mth0158 do you approve it too?

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 6, 2023

@igorkasyanchuk fine with me, thanks again @jozzi05!

@jozzi05
Copy link
Author

jozzi05 commented Nov 6, 2023

@Mth0158 No worries, I'm happy I was able to help. Thank you for providing such a lovely gem to the open-source community. 🍻

@gr8bit
Copy link
Collaborator

gr8bit commented Nov 6, 2023

Looks fine, I like the context addition!

@jozzi05
Copy link
Author

jozzi05 commented Nov 8, 2023

@igorkasyanchuk any update on this PR? 🙏

@igorkasyanchuk
Copy link
Owner

oh, sorry, I merged a different PR first and now this has conflicts. Can you please fix and I'll merge ASAP

@jozzi05
Copy link
Author

jozzi05 commented Nov 10, 2023

Apologies, but changes from the master branch are gigantic. I don't really have time to investigate again how the current code handles validations and where all tests are placed. Please feel free to take inspiration from my test cases for a way to reproduce issues from the latest release to know if the issue still exists on the master branch. I'm closing this PR.

@Mth0158
Copy link
Collaborator

Mth0158 commented Nov 10, 2023

@jozzi05 sorry for the conflicts, it's due to my PR.
Reading your work, it should be simple to fix though, you could create a Contextable module to add the on matcher method in it & change the validate method in the Validatable concern. Then you could spread easily this behaviour to all our matchers (currently it only applies to the size validator).
Let me know what you think of this

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.

4 participants