Spike out adding custom error messages via locales #8356
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidgisbey
force-pushed
the
spike-out-adding-custom-error-messages-as-translations
branch
2 times, most recently
from
01bfc97 to
cfa6d49
Compare
This is a spike commit to test the feasibility of adding custom error messages for specific models/attributes to locale files, while retaining the existing behaviour for those without custom messages. This is made slightly more difficult by presence of all the nested error messages that can exist. For example, in this PR govspeak_content error messages are present on the attachment object. I've got round this for now by checking if the error is a nested error and if that is the case is splits the string on "." and gets the second to last element in the array. I've had to do this rather than grab the first element in the array because sometimes we have multiple layers of nesting. For example, the attribute for a nested error for review reminders would be: "document.review_reminder.attribute" This would mean it would grab "review_reminder" and use that as the model name it looks for in the locales files.
davidgisbey
commented
Oct 11, 2023
|
|
||
| def error_message_text(error) | ||
| if custom_error_present_in_locales?(error) | ||
| custom_error_message(error) |
There was a problem hiding this comment.
i actually think this should just be error.message tbh
I ran into an issue with this being overwritten by this custom error message.
I think the right thing to do though is add custom messages for each affected attribute in the locales files. Having an overwritten error message on the object itself is bad.
davidgisbey
force-pushed
the
spike-out-adding-custom-error-messages-as-translations
branch
2 times, most recently
from
73f2628 to
6979985
Compare
davidgisbey
force-pushed
the
spike-out-adding-custom-error-messages-as-translations
branch
from
6979985 to
b600197
Compare
davidgisbey
changed the title
davidgisbey
changed the title
|
THere's a wayyy more elegant solution for this so we'll go with that. https://edgeguides.rubyonrails.org/configuring.html#configuring-active-model |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The problem
A common ask from design is that we give more descriptive error messages. At the moment the implementation uses default rails messages. We use
#full_messagehttps://api.rubyonrails.org/v6.1.4/classes/ActiveModel/Error.html#method-i-full_message in the error summary componentEssentially this combines the attribute and standard error message to construct the error message
For example a blank title would produce
as
error.attribute == :titleand `error.message == "can't be blank"Eventually we want to end up with a full set of error messages which live in locales rather than the model itself. Getting to that point is actually slightly tricky as we have many models, all of which need custom error messages. Replacing all of these in one go in one PR would be a pretty insurmountable task.
It feels like we need a way to slowly add these in. Most likely 1 section or even model at a time.
Due to this we need to ensure the original implementation continues to work when custom error messages aren't present in the locales for that specific error.
Proposed solution
This attempts to solve this issue by extending the error summary component. For each error we check whether an error message is present in the locales. If one is, then we use that error message in place of the error found on the object. If not, then we continue to use
#full_message.There's actually some more hidden complexity here as often there are nested errors which require some additional work to figure out where the locale should exist.
Originally, if one was found in the locales, i just used
error.messagebut i noticed that errors added in the model itself take precedent. This can be fixed by removing the message from the model itself, but for shared errors (for example theinvalid_dateone added recently by Ryan it's used in many places.I do think that probably the right thing to do is update all those messages in 1 go and remove the error message from the
DateValidationconcern.### Error text above inputs
I've not worried about extending this implementation to the ErrorsHelper which handles rendering the error message above the relevant field. If people are happy with this approach it could quite easily be extended to this helper though
Follow these steps if you are doing a Rails upgrade.