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

Fixing for mobility gem #344

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

Conversation

michaelvu812
Copy link

No description provided.

@magnusvk
Copy link
Owner

Sorry for the long radio silence here, but do you have any extra info for why this is required? If you wanted me to merge this I'd love to see a failing test added, too.

@MRoc
Copy link

MRoc commented Oct 12, 2023

It seems this fixes a problem with the gems mobility and counter_culture. The problem occurs when having a model with translated properties and a counter cache, something like this:

class Book < ApplicationRecord
  extend Mobility
  translates :title

  belongs_to :category
  counter_culture :category
end

Mobility adds additional properties for each language, e.g. for title it also adds title_en and title_de. When then updating a model like book.update(title_de: "German Title"), and having a counter_culture on the same object, the following error is raised:

ActiveModel::MissingAttributeError:
       can't write unknown attribute `title_de`

It seems counter_culture overwrites these properties. The PR tries to fix this.

@magnusvk
Copy link
Owner

That makes sense. I'd be happy to merge this, but would still like to see a failing spec.

@hishammalik
Copy link

I can confirm this bug and the solution provided works.

@magnusvk
Copy link
Owner

magnusvk commented Dec 4, 2023

This PR is still missing a test unfortunately—any chance you can add that here?

@hishammalik
Copy link

@magnusvk its a bit tricky to create a test case for this as the conflict happens when mobility gem is added with reader settings. So The test scenario would be a new rails application with counter_culture and mobility gem added, and then a counter field added to a model that is also using translations using mobility. Not sure how such a test can be added into the PR but the one-liner does indeed solve the problem..

My application has mobility configured with 'container' backend, and then this error occurred in the scenario as above. Patch was added to sort this as per above PR to make it work.

@magnusvk
Copy link
Owner

magnusvk commented Dec 5, 2023

Maybe you don't have to go all the way to including the mobility gem, but instead just do what the gem does and declare an attribute the same way mobility does? I just don't have any confidence this won't break again if there's not some sort of failing test for 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.

None yet

5 participants