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 I18n locale for validator error message #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Saleh-Salem
Copy link

@Saleh-Salem Saleh-Salem commented May 18, 2022

Description

These changes will allow changing the message default for the validator and customizing with any other locales

@ifellinaholeonce
Copy link
Collaborator

Thanks for this @Saleh-Salem. Please give me some time to review and consider this. Thanks for the contribution, I like the idea.

@Saleh-Salem
Copy link
Author

@ifellinaholeonce did you check this PR?

@ifellinaholeonce
Copy link
Collaborator

Thanks for this @Saleh-Salem. I've been trying to find other examples of gems that us i18n to handle error messages from within the gem. I haven't been able to find any, do you know other gems that do this?

I'm trying to understand a bit more the problem this is trying to solve. Why not have your application handle the errors that this gem raises and use an appropriate locale in the app for delivering an error message to the end user?

@iMacTia have you seen this done before or have any thoughts on the topic?

@Saleh-Salem
Copy link
Author

@ifellinaholeonce you can check devise gem, and see it handle locale for end-user

@ifellinaholeonce
Copy link
Collaborator

Thanks for that example @Saleh-Salem .

I am still a little hesitant to merge this. Let me share my thoughts and I'd be interested in what you think.

By adding translations into the gem as you've done in this PR, we are introducing complexity that will make it harder to update the gem in the future. If error messages change, the gem would require input for the changes in every language that is included in it. This is tough to do for a gem of this size.

However, I do see possibilities for rethinking how errors are handled in the gem. Currently, if a developer wanted to translate the error messages, they would need to do some regex checks on the error message from the one generic error that this gem raises (InvalidParameterError) in order to determine the issue and return a translated response to the end user.

One path forward could be returning error codes instead of strings. This has the benefit of being easier for developers to interact with while also not really adding much complexity for future maintenance on this gem.

I kind of think of https://stripe.com/docs/error-codes as an example of this.

@Saleh-Salem
Copy link
Author

Thanks, @ifellinaholeonce. For this point not understand you, bro.

One path forward could be returning error codes instead of strings. This has the benefit of being easier for developers to interact with while also not really adding much complexity for future maintenance on this gem.

In my PR there, I think no issue with refactoring in the future.

@Saleh-Salem
Copy link
Author

@ifellinaholeonce @nicolasblanco what is the update please?

@iMacTia
Copy link
Collaborator

iMacTia commented Aug 4, 2022

@Saleh-Salem I stand by @ifellinaholeonce's point.
devise is a fairly different use-case from this gem, as that is a Rails engine that outputs the messages directly into views which are exposed to users through a browser. Localisation makes total sense there!

rails_param is a much more low-level gem, and it's not exposed to the user directly.
Instead, the error messages are raised with an InvalidParameterError exception managed programmatically.
At best, the recipient of that message is supposed to be the developer, so we don't have any reason (or resources, for what's worth) to translate those.

I do agree though that these messages are often rescued and returned in the API response, which I presume is the reason for this request.
For that use-case, we could add error codes to the exception so that applications that want to provide localisation will be able to do so more easily.

For example, instead of this:

if params[name].nil? && options[:required]
  raise InvalidParameterError.new(
  I18n.t('rails_param.validator.default.required', name: parameter_name),
  param: parameter_name,
  options: options
)

We could do:

if params[name].nil? && options[:required]
  raise InvalidParameterError.new(
  "Parameter #{parameter_name} is required",
  param: parameter_name,
  options: options,
  error_code: 'rails_param.validator.default.required'
)

That way, when you rescue the exception you'll be able to do the following:

begin
  ...
rescue RailsParam::InvalidParameterError => exc
  message = I18n.t(exc.error_code, name: exc.param)
end

Please let me know if the above makes sense

@Saleh-Salem
Copy link
Author

@iMacTia imagine we have more than one parameter and need to add a custom message for every param, how we can handle that?

@iMacTia
Copy link
Collaborator

iMacTia commented Sep 21, 2022

This whole discussion only applies to the default messages, you can already override the message to whatever you want in whatever language with the :message option.
If that's not enough, we could also introduce a new :error_code option that allows you to override the default error_code

@Saleh-Salem
Copy link
Author

@iMacTia good, you mean error_code will be for one validator only.

example

param! :order, String, in: ["ASC", "DESC"], transform: :upcase, default: "ASC"

how can add a message for every validate in, transform, default

@iMacTia
Copy link
Collaborator

iMacTia commented Oct 18, 2022

I don't think you need custom messages for transform or default, but I do see your point.

Let's say for example the user sends an Integer, you may want an error like "wrong type".
If the user sends a string, but not one of the values specified in the in option, then the error should be something like "value not allowed".

The way the DSL is built right now does not allow you to provide custom messages with such granularity, but the solution proposed by this PR would also not allow for such customisation.

This PR adds additional localisation for each Error, so it's still keeping the granularity at the global validator level, not the individual parameter validator level.

Let me try to further explain why I think error codes would be enough.
rails_param currently raises an error in case of validation errors.
This error message is built based on the validation that failed.

Taking your example above:

param! :order, String, in: ["ASC", "DESC"], transform: :upcase, default: "ASC"

Passing an incorrect type will raise a RailsParam::InvalidParameterError with a certain message, while passing "ASCENDING" (not an allowed value) will raise the same error but with a different message.

Bubbling up this exception will not look nice for the end user, so you probably have something like the following in your application:

rescue_from RailsParam::InvalidParameterError, with: invalid_param

def invalid_param(exc)
  render json: {
    error: 'validation_error',
    error_message: exc.message
  }, status: 400
end

This catches the RailsParam::InvalidParameterError and renders it in a nice way for your API users.
If you're not building an API, the implementation would be similar, but you'd probably set the error message in the flash or session objects.

Now, exc.message is an English error message, so you're suggesting adding translations into other languages, but that would bring maintenance burden to the rails_param team to keep the translations up-to-date if we add new validators in future.

Instead, we could use error codes to allow applications to provide their own translations.
Imagine each validator adds an error_code to the exception they raise, that you can access with exc.error_code.
This way, you can change the implementation in your application to be:

rescue_from RailsParam::InvalidParameterError, with: invalid_param

def invalid_param(exc)
  render json: {
    error: 'validation_error',
    error_code: exc.error_code,
    error_message: I18n.t(exc.error_code, name: exc.param)
  }, status: 400
end

You can then localise each error code into any language you like, depending on how many languages your application supports.

I hope this helps clarify our error_codes proposal. Would this work for you on your particular use-case?

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