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

Scoped support? #7

Closed
bricesanchez opened this issue Mar 29, 2018 · 9 comments
Closed

Scoped support? #7

bricesanchez opened this issue Mar 29, 2018 · 9 comments

Comments

@bricesanchez
Copy link
Contributor

Hi @shioyama !

Thanks for your awesome work on mobility!

I'm in the process of switching from globalize to mobility for refinerycms refinery/refinerycms#3360

I have a lot of fails specs due to this config: https://github.com/refinery/refinerycms/blob/master/pages/app/models/refinery/page.rb#L36-L37

I'm not sure if it's because friendly_id-mobility gem does not support scoped feature or if i have another bug.

Do you have any idea?

@shioyama
Copy link
Owner

shioyama commented Mar 29, 2018

I made a comment on refinery/refinerycms#3360, but basically FriendlyId::Mobility doesn't do anything in particular for scoped and I don't believe it should have to.

However, because Mobility doesn't monkey-patch ActiveRecord, the order in which things happen in your model is important. For history (as mentioned in the readme), you need to have :mobility come before :history in the array passed to use:, like this:

friendly_id :title, use: [:history, :mobility]

so I believe the same is true for scoped (if so I need to add this to the readme on this repo):

friendly_id :title, use: [:scoped, :mobility]

It seems that in that other PR, the order is reversed, which means that scoped wouldn't get the scope overrides that Mobility uses to handle translated attributes, which would explain the failures.

@shioyama
Copy link
Owner

I'm not sure though that that will fix it. If not, I'll add some specs here on scoped and check that it works with Mobility, and if not try and track down the problem. (Probably we should have those specs here anyway.)

@shioyama
Copy link
Owner

shioyama commented Mar 29, 2018

Actually, thinking about this again, I don't think that alone will fix it. The problem is that unlike Globalize, Mobility doesn't patch AR query methods globally, only through the i18n scope (available on translated models). So perhaps if after calling translates in the model, if you added default_scope { i18n } that might work. But this gem should probably handle this so you don't need to do that.

bricesanchez added a commit to refinery/refinerycms that referenced this issue Mar 29, 2018
@bricesanchez
Copy link
Contributor Author

Thanks for your quick answer !

Actually, thinking about this again, I don't think that alone will fix it. The problem is that unlike Globalize, Mobility doesn't patch AR query methods globally, only through the i18n scope (available on translated models). So perhaps if after calling translates in the model, if you added default_scope { i18n } that might work. But this gem should probably handle this so you don't need to do that.

It looks like this helps a little: refinery/refinerycms@836e050

It fixes 20 specs but not all slug specs! :)

@shioyama
Copy link
Owner

It fixes 20 specs but not all slug specs! :)

Ok that's good! Ideally though, this gem should allow you to use scoped without having to apply a default scope like that. I'll look into it.

@shioyama
Copy link
Owner

shioyama commented Apr 4, 2018

I'm pretty busy this week, will try to get to this next week. I don't think it should be much work to get scoped support. Ideally I'd like to just import scoped specs from FriendlyId and test against them, with changes to test that Mobility finds work (similar to how other tests are done in this gem right now).

@shioyama
Copy link
Owner

shioyama commented Apr 4, 2018

Of course if you want to give it a shot, that's also always welcome 😄

@bricesanchez
Copy link
Contributor Author

I'm made my early tests the last night, i will try to send you a PR if i find something interesting.

bricesanchez added a commit to refinery/refinerycms that referenced this issue Apr 10, 2018
parndt pushed a commit to refinery/refinerycms that referenced this issue May 25, 2018
@andreierdoss
Copy link
Contributor

Any chance to fix this issue? Scoped does not work for me. I am using it in a self referential relationship ie. categories with sub categories.

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

No branches or pull requests

3 participants