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

find/where through association isn't using translation table #303

Closed
rvrm opened this issue Nov 15, 2013 · 18 comments · Fixed by #305
Closed

find/where through association isn't using translation table #303

rvrm opened this issue Nov 15, 2013 · 18 comments · Fixed by #305
Labels

Comments

@rvrm
Copy link

rvrm commented Nov 15, 2013

Using find() or where() directly on my model generates a query which looks up the slug in the page_translations table:

Page.find('my-title')
SELECT FROM "pages" LEFT OUTER JOIN "page_translations" ...
-> #<Page id: 1 ...>

When I use find through a association, the translation table isn't being used. The original table is being used instead.

@site.pages.find('my-title')
SELECT "pages".* FROM "pages" WHERE "pages"."site_id" = $1 AND "pages"."slug" = 'my-title' LIMIT 1  [["site_id", 1]]
-> ActiveRecord::RecordNotFound

Adding the scope manually works, but feels more like a hack:

Page.where(site_id: @site.id, title: 'my-title').first
SELECT "pages"."id" AS t0_r0, "pages"."title" AS t0_r1 ... AND (("pages"."site_id" = 1 AND "page_translations"."title" = 'my-title'))
-> #<Page id: 1 ...>

Rails 4.0.1 — Globalize 4.0.0.alpha.2

@shioyama
Copy link
Contributor

Oh, interesting. Globalize::ActiveRecord::Relation does not currently handle that case but perhaps it should. I'll look into it.

You're using FriendlyId I assume?

@rvrm
Copy link
Author

rvrm commented Nov 15, 2013

Yes, I have the FriendlyId gem installed, but it works the same way for where-conditions, which aren't altered by FriendlyId as far as I know...

@site.pages.where(title: 'my-title').first
SELECT "pages".* FROM "pages" WHERE "pages"."shop_id" = $1 AND "pages"."title" = 'my-title' LIMIT 1  [["site_id", 1]]

@shioyama
Copy link
Contributor

Yes I know, it's not supported either way at the moment, but I just wanted to check since you mentioned searching by slug.

@shioyama
Copy link
Contributor

Created a branch with a simple failing test to get the ball rolling.

@yadoga
Copy link

yadoga commented Nov 16, 2013

Great! Good to hear you're on top of it.

@shioyama
Copy link
Contributor

Hmm... this problem is more tricky than expected. I'm trying to avoid monkeypatching ActiveRecord but it's looking to be difficult/impossible. Will keep digging but just wanted to let anyone who is reading this know.

@shioyama
Copy link
Contributor

Ok, got something which seems to work in 435d78c, all tests (including the new one) pass. Would anybody like to try this for a spin?

gem 'globalize', github: 'globalize/globalize', ref: '435d78c'

See the full set of changes here.

@rvrm
Copy link
Author

rvrm commented Nov 18, 2013

Wow, it works like a charm with where() trough associations! Even the new find_or_create_by() methods work great.

The only thing that doesn't seem to be supported is find() (and the deprecated find_by_* methods). I mention this because the friendly_id-globalize gem uses find() and will still not work properly with this improvement.

@shioyama
Copy link
Contributor

Yes sorry, I realize that and I should be able to fix it. Also I think chaining will not currently work, i.e. blog.posts.where(title: 'a translated title').where(summary: 'a translated summary') while choke on the second where. But I know how to fix those, just give me a bit more time.

The key insight was using the delegated relation classes to patch query methods locally for translated models only.

@rvrm
Copy link
Author

rvrm commented Nov 18, 2013

Yay, thanks Chris, big joy here on our side! Would like to support your efforts by donating via Gittip, if ok with you? Let me know, if you'd prefer another way alternatively. We're all looking forward to your upcoming commit.

@shioyama
Copy link
Contributor

Thanks that's very kind, but I'm happy to do this for no monetary gain. Handling translated queries on associations is actually really important and something that I'd totally looked over, so glad that you noticed it.

shioyama added a commit that referenced this issue Nov 19, 2013
…r chaining. Fixes chaining issue mentioned in #303.
@shioyama
Copy link
Contributor

Ok the chaining issue was easily solved.

@shioyama
Copy link
Contributor

@parndt just to check, is friendly_id-globalize supposed to work with find on associations (see above)? It doesn't look to me like it was ever supported.

If that's the case, then the issue here should be moved to the issue on that repository.

@parndt
Copy link
Member

parndt commented Nov 19, 2013

FriendlyId doesn't patch find directly, but through a friendly proxy. If it doesn't work through that, then it's a bug.

@shioyama
Copy link
Contributor

Yeah that's what I thought.

@rvrm can you try testing with the friendly proxy, i.e. @site.pages.friendly.find('my-title')?

@rvrm
Copy link
Author

rvrm commented Nov 19, 2013

Confirming the patch now works via find as well! Thank you @shioyama @parndt, that's great news!

@rvrm
Copy link
Author

rvrm commented Nov 19, 2013

Used a global config.use :finders via the friendly_id initializer, so calling friendly on the association does the job as advertised now. Cheers!

@shioyama
Copy link
Contributor

Great! I'll clean up the branch a bit and then merge into master. This is a bit experimental so hoping to get some thoughts from @norman before releasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants