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

Alternative implementation of handling translations in ransack #602

Merged
merged 16 commits into from Jul 31, 2015

Conversation

Projects
None yet
6 participants
@mvz

mvz commented Jul 9, 2015

Based on #595. See there for discussion.

huoxito and others added some commits Mar 24, 2015

Let ransack know about translations
Since globalize 5 won't store translated data on default tables we need
to tweak the table / attribute name to let it fetch the data on the
right place.

This is just a proof of concept on what we could do to
improve the spree / ransack / globalize love here. Not sure this will
work with associations yet or even if it will work at all in all
scenarios
alias ransack / search again
perhaps the original link is lost once you override the method?
Point to rails/4-2-stable on build
And add example for fetching products via ransack

mvz added some commits Jul 14, 2015

Improve ransack translation specs
This ensures no more than the desired result is returned.
accepts_nested_attributes_for :translations
klass.class_eval do
class << self
alias_method_chain :ransack, :translations

This comment has been minimized.

@huoxito

huoxito Jul 15, 2015

Contributor

mostly curious why don't we stick with regular ruby and use super? or do we need alias_method_chain for some particular reason? we could kill the klass_eval here with super

This comment has been minimized.

@mvz

mvz Jul 15, 2015

I needed alias_method_chain for my first implementation, but I can now move back to using super which is indeed cleaner.

it 'fetches variant from product via translation table' do
variants = described_class.includes(:product).ransack(name_cont: 'globalize').result.to_a
expect(variants.last).to eq variant

This comment has been minimized.

@huoxito

huoxito Jul 15, 2015

Contributor

I wonder if we need the same code as in https://github.com/spree-contrib/spree_i18n/pull/595/files#diff-5d3646eb5b08660b00d9f57891773b27R20.

I'd expect the build to fail though since this will only be fixed on next rails release but it feels relevant to leave the spec here (could point back to 4-2-stable) so we know whether variants search would work or not. Can you tell if the variants search on admin/orders work on latest rails gem release and with this patch?

This comment has been minimized.

@mvz

mvz Jul 15, 2015

Yes, I took that part out because it needs Rails 4-2-stable and it looked like a pre-condition check.

I'll take another look at this part.

@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 15, 2015

hey @mvz thanks a ton for taking this to another level. I tried to investigate this mostly for fun and can't remember all the ransack / globalize details anymore unless I spend many hours on it again :) anyway I do see you've improved it so much more thanks!

@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 23, 2015

@mvz just a heads up I'm planning to merge this as is tonight so we can all move on, will also submit another PR to the new spree_globalize repo, thanks a lot for your work

@mvz

This comment has been minimized.

mvz commented Jul 24, 2015

I'm pretty sure variant select is not working yet with this or the previous solution.

I've locally added a test to demonstrate with the exact query used by Spree, and it fails. I haven't had time to find a solution yet, but I could take a look at it today.

In any case, the current PR will still be valid as it fixes search in all other places in admin, so if you want to merge it I can always open a follow-up PR.

@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 24, 2015

yep you're totally right @mvz the variants search is still not working and the patch present in rails/4-2-stable seems to make no difference (I'm confused .. might try to give a look another day, a bit late here)

I hadn't tried this myself via browser until just now =/ and I think the test I had setup is misleading

# setup stuff
# ...
variants = described_class.includes(:product).ransack(name_cont: 'globalize').result

^^ it seems to go through the translations table but it doesn't filter anything it brings all records .. and when I try via product_name_or_sku_cont it doesn't hit the translations table at all. sorry for all the false positives here guys

@mvz

This comment has been minimized.

mvz commented Jul 25, 2015

I fixed ransack for Variant as well.

model, attrib_name = locate_attribute name
prefix = name.chomp(attrib_name)
if (model.try(:translated_attribute_names) || []).include? attrib_name.to_sym

This comment has been minimized.

@huoxito

huoxito Jul 27, 2015

Contributor

I think changing this line to this could fix the errors:

      if model && model.translated_attribute_names.include?(attrib_name.to_s.to_sym)

I can't say I get exactly whats going on with this locate_attribute, I suppose there's no other check we could add to avoid hitting this stack when we dont have too right? seems a bit expensive and very complicated

@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 27, 2015

great stuff @mvz very impressive

@mvz

This comment has been minimized.

mvz commented Jul 28, 2015

I fixed the remaining spec failures.

I haven't investigated whether bailing out early, e.g., for known attributes, will speed things up. For now, I rely on Ransack::Context#bind to be reasonably fast at finding the correct model and attribute.

Caching the list of translated attributes per model may speed things up as well., but I think translated_attribute_names should be pretty fast. Eliminating try will be a boost as well but it requires adding translated_attribute_names to all models.

@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 29, 2015

by your comment on the other thread I'm assuming you're not done yet here @mvz, let us know when it's fine to merge pls thanks

@mvz

This comment has been minimized.

mvz commented Jul 29, 2015

No, I'm done. That comment was from before my last fixes. Ready to merge as far as I'm concerned!

huoxito added a commit that referenced this pull request Jul 31, 2015

Merge pull request #602 from mvz/3-0-ransack-translations
Alternative implementation of handling translations in ransack

@huoxito huoxito merged commit b0f49a3 into spree-contrib:3-0-stable Jul 31, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@huoxito

This comment has been minimized.

Contributor

huoxito commented Jul 31, 2015

sweet thanks @mvz !!! impressive work here great stuff, do you want to submit another PR to spree_globalize so next major version also has this working? (I can do it myself too just double checking if you'd rather have the honors)

I'm thinking all spree models would need this new version of ransack method for situations like this one we have in Variant but I guess this is safer for now cheers

@sepastian

This comment has been minimized.

Contributor

sepastian commented Jul 31, 2015

Thanks a lot! 👍

How do I use this now?

@JDutil

This comment has been minimized.

Member

JDutil commented Jul 31, 2015

@sepastian I believe you should just need to update spree_i18n: bundle update spree_i18n

@stefatkins

This comment has been minimized.

stefatkins commented Aug 26, 2015

Hi, i'm trying to get the admin product filter to work on spree 3.0 stable
This is my gemfile :
gem 'spree', github: 'spree/spree', branch: '3-0-stable'
gem 'spree_gateway', github: 'spree/spree_gateway', branch: '3-0-stable'
gem 'spree_auth_devise', github: 'spree/spree_auth_devise', branch: '3-0-stable'
gem 'spree_i18n', git: 'https://github.com/mvz/spree_i18n.git', branch: '3-0-ransack-translations'

I ran bundle install and bundle update spree_i18n but The admin product filter is still returning all of the products. Is there a step that I missed ?

@mvz

This comment has been minimized.

mvz commented Aug 26, 2015

@stefan694 since this has been merged you should point to spree_i18n/3-0-stable again. I'm removing this branch from my repo.

To solve your problem, you may want to check the logs to see what queries are run.

@mvz mvz deleted the mvz:3-0-ransack-translations branch Aug 26, 2015

@stefatkins

This comment has been minimized.

stefatkins commented Aug 26, 2015

Just did a fresh install of Spree 3.0.5.beta and the product filter problem occurs as soon as I install the spree_i18n gem.The query params seem to be correct. Any idea of where it could be coming from mvz?


Started GET "/admin/products?utf8=%E2%9C%93&q%5Bname_cont%5D=mug&q%5Bvariants_including_master_sku_cont%5D=&q%5Bdeleted_at_null%5D=1" for 127.0.0.1 at 2015-08-26 15:19:36 +0200
Processing by Spree::Admin::ProductsController#index as HTML
  Parameters: {"utf8"=>"✓", "q"=>{"name_cont"=>"mug", "variants_including_master_sku_cont"=>"", "deleted_at_null"=>"1"}}
  Spree::User Load (0.4ms)  SELECT  "spree_users".* FROM "spree_users" WHERE "spree_users"."deleted_at" IS NULL AND "spree_users"."id" = $1  ORDER BY "spree_users"."id" ASC LIMIT 1  [["id", 1]]
   (0.3ms)  SELECT COUNT(*) FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = $1 AND "spree_roles"."name" = $2  [["user_id", 1], ["name", "admin"]]
   (0.4ms)  SELECT DISTINCT COUNT(DISTINCT "spree_products"."id") FROM "spree_products" WHERE "spree_products"."deleted_at" IS NULL
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_index_table_options.html.erb (113.9ms)
   (0.4ms)  SELECT COUNT(DISTINCT count_column) FROM (SELECT  DISTINCT "spree_products"."id" AS count_column FROM "spree_products" WHERE "spree_products"."deleted_at" IS NULL LIMIT 10 OFFSET 0) subquery_for_count
  Spree::Product Load (0.5ms)  SELECT  DISTINCT "spree_products".* FROM "spree_products" WHERE "spree_products"."deleted_at" IS NULL  ORDER BY "spree_products"."name" ASC LIMIT 10 OFFSET 0
  Spree::Variant Load (0.4ms)  SELECT "spree_variants".* FROM "spree_variants" WHERE "spree_variants"."deleted_at" IS NULL AND "spree_variants"."is_master" = $1 AND "spree_variants"."product_id" IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)  ORDER BY "spree_variants".position ASC  [["is_master", "f"]]
  Spree::Image Load (0.5ms)  SELECT "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_type" = 'Spree::Variant' AND "spree_assets"."viewable_id" IN (17, 18, 19, 20, 21, 22, 23, 24, 25, 26)  ORDER BY "spree_assets"."position" ASC
  Spree::Variant Load (0.3ms)  SELECT "spree_variants".* FROM "spree_variants" WHERE "spree_variants"."deleted_at" IS NULL AND "spree_variants"."is_master" = $1 AND "spree_variants"."product_id" IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)  [["is_master", "t"]]
  Spree::Image Load (0.4ms)  SELECT "spree_assets".* FROM "spree_assets" WHERE "spree_assets"."type" IN ('Spree::Image') AND "spree_assets"."viewable_type" = 'Spree::Variant' AND "spree_assets"."viewable_id" IN (1, 2, 3, 4, 5, 7, 6, 10, 9, 8)  ORDER BY "spree_assets"."position" ASC
  Spree::Price Load (0.3ms)  SELECT "spree_prices".* FROM "spree_prices" WHERE "spree_prices"."deleted_at" IS NULL AND "spree_prices"."currency" = $1 AND "spree_prices"."variant_id" IN (1, 2, 3, 4, 5, 7, 6, 10, 9, 8)  [["currency", "USD"]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 1]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 2]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 3]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 4]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 5]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 6]]
  Spree::Product::Translation Load (0.4ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 7]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 8]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 9]]
  Spree::Product::Translation Load (0.2ms)  SELECT "spree_product_translations".* FROM "spree_product_translations" WHERE "spree_product_translations"."spree_product_id" = $1  [["spree_product_id", 10]]
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_index_table_options.html.erb (120.4ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/products/index.html.erb within spree/layouts/admin (360.9ms)
Deface: 1 overrides found for 'spree/admin/shared/_head'
Deface: 'override' matched 1 times with 'title'
Deface: [WARNING] No :original defined for 'override', you should change its definition to include:
 :original => 'b94dd9df96e085d9a869128fa811ee3aaf55fab1' 
Deface: 1 overrides found for 'spree/admin/shared/_translations'
Deface: 'translation' matched 1 times with 'script'
Deface: [WARNING] No :original defined for 'translation', you should change its definition to include:
 :original => '6d879f5c231ff848b4e1023dc0f8b271f922269b' 
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_translations.html.erb (28.8ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_head.html.erb (957.0ms)
Deface: 1 overrides found for 'spree/admin/shared/_header'
Deface: 'auth_admin_login_navigation_bar' matched 1 times with '[data-hook='admin_login_navigation_bar'], #admin_login_navigation_bar[data-hook]'
Deface: [ERROR] The original source for 'auth_admin_login_navigation_bar' has changed, this override should be reviewed to ensure it's still valid.
  CACHE (0.0ms)  SELECT COUNT(*) FROM "spree_roles" INNER JOIN "spree_roles_users" ON "spree_roles"."id" = "spree_roles_users"."role_id" WHERE "spree_roles_users"."user_id" = $1 AND "spree_roles"."name" = $2  [["user_id", 1], ["name", "admin"]]
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree_auth_devise-2e8e08759c4d/lib/views/backend/spree/layouts/admin/_login_nav.html.erb (1.4ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_header.html.erb (16.4ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/sub_menu/_product.html.erb (3.8ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/sub_menu/_promotion.html.erb (1.3ms)
  Spree::Store Load (0.4ms)  SELECT  "spree_stores".* FROM "spree_stores" WHERE (url like '%localhost%')  ORDER BY "spree_stores"."id" ASC LIMIT 1
  Spree::Store Load (0.3ms)  SELECT  "spree_stores".* FROM "spree_stores" WHERE "spree_stores"."default" = $1  ORDER BY "spree_stores"."id" ASC LIMIT 1  [["default", "t"]]
  Spree::Country Load (0.3ms)  SELECT  "spree_countries".* FROM "spree_countries" WHERE "spree_countries"."id" = $1 LIMIT 1  [["id", 232]]
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/sub_menu/_configuration.html.erb (8.4ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_main_menu.html.erb (113.3ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_content_header.html.erb (0.3ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_alert.html.erb (0.0ms)
  Rendered /Users/stefan/.rvm/gems/ruby-2.2.1/bundler/gems/spree-a39d06ab644e/backend/app/views/spree/admin/shared/_table_filter.html.erb (0.8ms)
Completed 200 OK in 1690ms (Views: 1674.4ms | ActiveRecord: 7.1ms)
@calvinl

This comment has been minimized.

calvinl commented Aug 26, 2015

I'm seeing this issue too, after upgrading from spree 3.0.0 to 3.0.4.

@JDutil

This comment has been minimized.

Member

JDutil commented Aug 26, 2015

The issue is probably related to https://spreecommerce.com/blog/security-updates-2015-8-19 we now whitelist ransack searches spree/spree@f7e00dc so the translation assocations will need to be whitelisted.

@calvinl

This comment has been minimized.

calvinl commented Aug 26, 2015

@JDutil I thought so too, but I tried it and it didn't work. It worked (with whitelisting) after removing spree_i18n for me. I could have been doing something wrong -- but I realized I wasn't going to need translations for my project. I can dig into it a bit more later.

@stefatkins

This comment has been minimized.

stefatkins commented Aug 26, 2015

Thanks JDutil ! It worked for me :)
just put this line in initializers/spree.rb :

Spree::Product.whitelisted_ransackable_associations |= ['translations']
@mvz

This comment has been minimized.

mvz commented Aug 27, 2015

The translation whitelisting needs to happen for all translated models, so it's best to include it in spree_i18n rather than individual initializers.

@stefan694: putting it in the initializer works fine for Spree::Product, but in general this turns out to be dangerous since the spree models may not be fully loaded yet. In particalar, Spree.user_class is not set yet at that point meaning adding whitelists for Spree::Order leads to breakage.

TL;DR: Better put that kind of thing in a decorator.

@stefatkins

This comment has been minimized.

stefatkins commented Aug 27, 2015

Sorry for the newbie question but what's a decorator in rails ? In which file would you recommend putting this line in ?

@mvz

This comment has been minimized.

mvz commented Aug 27, 2015

It's a Spree thing (although it would work with other engine-based apps). You can customize behavior of the spree models and controllers and such by creating a file like app/models/spree/product_decorator.rb, containing something like this:

Spree::Product.class_eval do
  self.whitelisted_ransackable_associations |= ['translations']
end

Plugins for Spree use this trick as well, see for example general_settings_controller_decorator.rb for an example for a controller.

@JDutil

This comment has been minimized.

Member

JDutil commented Aug 27, 2015

I think it'd be best to put in the the translateable concorn to effect all models https://github.com/spree-contrib/spree_i18n/blob/3-0-stable/app/models/spree_i18n/translatable.rb

@mvz

This comment has been minimized.

mvz commented Aug 27, 2015

I think it'd be best to put it in the translateable concern.

In order to fix this in spree_i18n, definitely. I was only considering what was needed to fix it in a current app.

@JDutil

This comment has been minimized.

Member

JDutil commented Aug 27, 2015

Could you guys try the fix-ransack branch out from this PR #618 I've added to the translatable concern, but I still need to figure out what's wrong with a few specs.

@JDutil

This comment has been minimized.

Member

JDutil commented Aug 27, 2015

Should be fixed by #618

@mvz

This comment has been minimized.

mvz commented Sep 21, 2015

@huoxito I don't know what the status is, but I'd be happy to create a PR for spree_globalize once the remaining issues with ransackable attribute and relation whitelisting have been ironed out.

@huoxito

This comment has been minimized.

Contributor

huoxito commented Sep 21, 2015

awesome thanks @mvz feel free to open the PR there any time though, I see @alepore has been pretty active there so he can probably help confirm things are working with spree master as well

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