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

Getting MissingAttributeError when submitting a value in the form with trailing whitespace #97

Closed
jmuheim opened this Issue Sep 16, 2017 · 20 comments

Comments

Projects
None yet
2 participants
@jmuheim

jmuheim commented Sep 16, 2017

I have a Page model that translates :title, :navigation_title, :lead, :content.

class Page < ApplicationRecord
  extend Mobility
  translates :title, :navigation_title, :lead, :content
end

I'm using the following config:

Mobility.configure do |config|
  config.default_backend = :column
  config.accessor_method = :translates
  config.query_method    = :i18n
  config.default_options = {
    fallbacks: { de: :en }
  }
end

I noticed a strange behaviour: whenever I submit a value using the form with a trailing whitespace (e.g. _x, _x_, or x_, where whitespace is substituted with an underscore) to a translated field (e.g. lead) I get the following error:

ActiveModel::MissingAttributeError at /de/pages/1
can't write unknown attribute `lead`

If I submit any other value (e.g. x`), it works perfectly.

This only happens to translated fields, not to other fields.

Any idea what's going on? I'm happy to provide more details if needed.

Here's the app I'm currently working on: jmuheim/base#86

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 16, 2017

Owner

My guess would be that it's paper trail. What if you remove it and see if you see the same issue?

Owner

shioyama commented Sep 16, 2017

My guess would be that it's paper trail. What if you remove it and see if you see the same issue?

@shioyama shioyama added the bug label Sep 16, 2017

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 16, 2017

Owner

Hmm I'm not sure what's going wrong. I can see the error running your code, but removing all other stuff from the model it still occurs. However I don't see it in isolated Mobility specs, so something in your projects setup or gemfile is conflicting I would suppose...

Owner

shioyama commented Sep 16, 2017

Hmm I'm not sure what's going wrong. I can see the error running your code, but removing all other stuff from the model it still occurs. However I don't see it in isolated Mobility specs, so something in your projects setup or gemfile is conflicting I would suppose...

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 17, 2017

Owner

Ah ok it's compatibility with the strip_attributes gem (should be obvious). This is not a bug but a compatibility issue.

Owner

shioyama commented Sep 17, 2017

Ah ok it's compatibility with the strip_attributes gem (should be obvious). This is not a bug but a compatibility issue.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 17, 2017

Owner

I'm probably going to have to close this since I can't really guarantee compatibility with every other gem.

Owner

shioyama commented Sep 17, 2017

I'm probably going to have to close this since I can't really guarantee compatibility with every other gem.

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@jmuheim

jmuheim Sep 17, 2017

Thanks a lot for finding the problem.

Do you have a more specific guess on what's the problem with strip_attributes gem? I will create an issue on its side then.

jmuheim commented Sep 17, 2017

Thanks a lot for finding the problem.

Do you have a more specific guess on what's the problem with strip_attributes gem? I will create an issue on its side then.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Oct 8, 2017

Owner

@jmuheim can you try pointing your Gemfile to the branch in #102 ?

gem 'mobility', github: 'shioyama/mobility', branch: 'convert_attributemethods_to_plugin'

This branch extracts the code overriding the attributes hash into a plugin which is not enabled by default. I believe this should fix the problem with strip_attributes.

Owner

shioyama commented Oct 8, 2017

@jmuheim can you try pointing your Gemfile to the branch in #102 ?

gem 'mobility', github: 'shioyama/mobility', branch: 'convert_attributemethods_to_plugin'

This branch extracts the code overriding the attributes hash into a plugin which is not enabled by default. I believe this should fix the problem with strip_attributes.

@shioyama shioyama added the bug label Oct 8, 2017

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Nov 24, 2017

Owner

@jmuheim See above, can you please try using the branch in #102? I am going to merge that branch to fix the issue here.

Owner

shioyama commented Nov 24, 2017

@jmuheim See above, can you please try using the branch in #102? I am going to merge that branch to fix the issue here.

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@jmuheim

jmuheim Nov 24, 2017

I'll do soon, sorry for the delay.

jmuheim commented Nov 24, 2017

I'll do soon, sorry for the delay.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Nov 27, 2017

Owner

I've merged #102. If you use the latest on master now, you should not have any conflict with strip_attributes anymore.

This will be released shortly as a change in 0.3.

Owner

shioyama commented Nov 27, 2017

I've merged #102. If you use the latest on master now, you should not have any conflict with strip_attributes anymore.

This will be released shortly as a change in 0.3.

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@jmuheim

jmuheim Nov 27, 2017

Thanks a lot for your effort! 👍

jmuheim commented Nov 27, 2017

Thanks a lot for your effort! 👍

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@jmuheim

jmuheim Dec 5, 2017

Hey there!

I tried to use the master branch, and while the problem disappeared, a spec failed.

The spec checks that each translated column offers a small icon next to the input. This icon is added in a custom SimpleForm input:

    if object.respond_to?(:translated_attribute_names) && object.translated_attribute_names.include?(attribute_name.to_s)
      help_blocks << template.content_tag(:span, nil, class: 'fa fa-globe') + ' ' + t('simple_form.inputs.better_text.multi_language')
    end

It seems that in the master branch the object doesn't respond to :translated_attribute_names anymore. But in the source the method still exists, so I'm unsure why it doesn't work like this anymore.

I'm using 0.2.3 until I can fix this.

Any help is highly appreciated.

jmuheim commented Dec 5, 2017

Hey there!

I tried to use the master branch, and while the problem disappeared, a spec failed.

The spec checks that each translated column offers a small icon next to the input. This icon is added in a custom SimpleForm input:

    if object.respond_to?(:translated_attribute_names) && object.translated_attribute_names.include?(attribute_name.to_s)
      help_blocks << template.content_tag(:span, nil, class: 'fa fa-globe') + ' ' + t('simple_form.inputs.better_text.multi_language')
    end

It seems that in the master branch the object doesn't respond to :translated_attribute_names anymore. But in the source the method still exists, so I'm unsure why it doesn't work like this anymore.

I'm using 0.2.3 until I can fix this.

Any help is highly appreciated.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 5, 2017

Owner

Hmm, I've checked and for me, using the latest (0.3.3) the model instance responds to translated_attribute_names.

Are you sure that your model instance really doesn't respond to translated_attribute_names? Maybe it's something to do with simple form.

Owner

shioyama commented Dec 5, 2017

Hmm, I've checked and for me, using the latest (0.3.3) the model instance responds to translated_attribute_names.

Are you sure that your model instance really doesn't respond to translated_attribute_names? Maybe it's something to do with simple form.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 5, 2017

Owner

I notice the problem is (possibly) connected to paper trail. I was actually thinking of creating a small gem for using mobility with paper trail.

I'm suspecting the issue might be because the attributes method on the model no longer includes translated attributes in 0.3.x.

Can you try setting attribute_methods to true in your config, like this:

Mobility.configure do |config|
  config.default_backend = :column
  config.accessor_method = :translates
  config.query_method    = :i18n
  config.default_options[:fallbacks] = { de: :en }
  config.default_options[:attribute_methods] = true
end

?

Owner

shioyama commented Dec 5, 2017

I notice the problem is (possibly) connected to paper trail. I was actually thinking of creating a small gem for using mobility with paper trail.

I'm suspecting the issue might be because the attributes method on the model no longer includes translated attributes in 0.3.x.

Can you try setting attribute_methods to true in your config, like this:

Mobility.configure do |config|
  config.default_backend = :column
  config.accessor_method = :translates
  config.query_method    = :i18n
  config.default_options[:fallbacks] = { de: :en }
  config.default_options[:attribute_methods] = true
end

?

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama
Owner

shioyama commented Dec 5, 2017

btw, this is a wrap-up of recent changes: http://dejimata.com/2017/12/5/mobility-0-3-ready-for-prime-time

jmuheim added a commit to jmuheim/base that referenced this issue Dec 5, 2017

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 5, 2017

Owner

This is the view code you're talking about right?

Owner

shioyama commented Dec 5, 2017

This is the view code you're talking about right?

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 6, 2017

Owner

Ok I've confirmed this issue in your repository, and indeed adding attribute_methods: true to the config fixes it (but brings back the original issue you posted here). I'm debugging now, very strange...

Owner

shioyama commented Dec 6, 2017

Ok I've confirmed this issue in your repository, and indeed adding attribute_methods: true to the config fixes it (but brings back the original issue you posted here). I'm debugging now, very strange...

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 6, 2017

Owner

Ah, never mind, I see the issue. translated_attribute_methods is still in Mobility, but only on the model class, not on the instance, see here. This should really not be extracted into the plugin.

I'll fix this and release a new patch version. Thanks for noticing the issue!

Owner

shioyama commented Dec 6, 2017

Ah, never mind, I see the issue. translated_attribute_methods is still in Mobility, but only on the model class, not on the instance, see here. This should really not be extracted into the plugin.

I'll fix this and release a new patch version. Thanks for noticing the issue!

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Dec 6, 2017

Owner

@jmuheim ok issue is fixed now, try 0.3.4.

Owner

shioyama commented Dec 6, 2017

@jmuheim ok issue is fixed now, try 0.3.4.

@jmuheim

This comment has been minimized.

Show comment
Hide comment
@jmuheim

jmuheim Dec 6, 2017

It's working, thank you so much. I love mobility gem. ❤️

jmuheim commented Dec 6, 2017

It's working, thank you so much. I love mobility gem. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment