counter_culture icon indicating copy to clipboard operation
counter_culture copied to clipboard

Fixing for mobility gem

Open michaelvu812 opened this issue 2 years ago • 7 comments

michaelvu812 avatar Feb 28 '22 04:02 michaelvu812

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.

magnusvk avatar Oct 12 '22 02:10 magnusvk

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.

MRoc avatar Oct 12 '23 08:10 MRoc

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

magnusvk avatar Oct 12 '23 17:10 magnusvk

I can confirm this bug and the solution provided works.

hishammalik avatar Dec 01 '23 15:12 hishammalik

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

magnusvk avatar Dec 04 '23 22:12 magnusvk

@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.

hishammalik avatar Dec 05 '23 15:12 hishammalik

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.

magnusvk avatar Dec 05 '23 17:12 magnusvk