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

fieldnames in error messages not found via i18n scope #83

Closed
schorsch opened this issue Jun 3, 2020 · 6 comments · Fixed by #85
Closed

fieldnames in error messages not found via i18n scope #83

schorsch opened this issue Jun 3, 2020 · 6 comments · Fixed by #85

Comments

@schorsch
Copy link

schorsch commented Jun 3, 2020

first: thanks for the update and everybodys work!

I dont know if this ticket belongs here or in the reform main gem, but since i am using ActiveRecord validations i suppose this is right here.

With reform-rails ( v0.2.0 + reform v2.3.1) using model.errors.full_messages the fieldnames with errors are not found within their 'normal' i18n scope which is either de.attributes.a_name or de.activerecord.attributes.my_model.a_name

it seems as if the method here needs to be overridden to make an i18n lookup instead of a simple capitalize https://github.com/trailblazer/reform/blob/master/lib/reform/errors.rb#L29

Does anybody else has this problem? I would probalby go with a monkey patch as a quick solution but this does not feel right.

@valscion
Copy link

Yeah, we've got the same issue. Seems like changes done in #44 have caused this, I think it might be due to how errors is now overridden here:

# DISCUSS @FRAN: not sure this is 100% compatible with AMV::Errors?
def errors
self
end

"not sure this is 100% compatible with AMV::Errors?" comment would point to to this direction, in that overriding errors is not compatible with ActiveModel validations.

just_start_rails_and_fieldnames_in_error_messages_not_found_via_i18n_scope_·Issue__83·_trailblazer_reform-rails

The controller with bad translations showing up is fairly simple (authorization code removed from here):

# frozen_string_literal: true
module API
  module Catering
    class InquiriesController < ApplicationController
      def update
        catering_inquiry = ::Catering::Inquiry.find(params[:id])
        form = ::Catering::UpdateInquiryForm.new(catering_inquiry)

        if form.validate(params[:inquiry])
          form.sync
          form.model.save!
          respond_to do |format|
            format.json { render(json: { success: true }) }
          end
        else
          respond_to do |format|
            format.json do
              render(
                json: { message: form.errors.full_messages.to_sentence },
                status: :unprocessable_entity
              )
            end
          end
        end
      end
    end
  end
end

and the form itself is also fairly simple:

# frozen_string_literal: true
module Catering
  class UpdateInquiryForm < Reform::Form
    model :inquiry

    property :company, from: :author_company
    property :author_name
    property :event_at, type: Types::Form::Date

    property :status
    property :rent_fee, type: Types::Form::Float
    property :catering_fee, type: Types::Form::Float
    property :other_fees, type: Types::Form::Float

    validates :status, inclusion: { in: Catering::Inquiry.statuses }
    validates :author_name, presence: true
    validates :event_at, presence: true, date: true
  end
end

It seems like this translation isn't getting applied like it did in reform-rails v0.1.7:

fi:
  attributes:
    author_name: Asiakkaan nimi

Proof that it worked in reform-rails v0.1.7:

just_start_rails

@marcelolx
Copy link
Contributor

We've this problem too, i send a PR only overriding full_messages to reform-rails delegate the call to AM #85, but this doesn't solves the problem for nested model attributes, also I haven't looked at the comment on the code which @valscion highlighted, tomorrow I will take another look to see if that could be handled changing errors behavior.

@marcelolx
Copy link
Contributor

@schorsch @valscion Could you guys make some tests with the branch of this PR #85 to see if the changes solve your problems? Tomorrow I'll do some tests in our application, but I would appreciate if other folks made some tests to make sure I'm not breaking anything on reform-rails.

@valscion
Copy link

Yeah, we can test it :) — a few little things are currently in progress so we might not be able to get to it today already, but hopefully we'll get to testing it during this working week still.

@valscion
Copy link

Based on a quick test #85 would seem to work ☺️.

Sanitized_Author_-_Venuu_fi

For reference, here's the same page with reform-rails using v0.2.0:

broken_reform-rails

@marcelolx
Copy link
Contributor

@valscion Thanks for the tests ❤️

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 a pull request may close this issue.

3 participants