Methods for Spree::Taxon for all products/variants from descendants #1761

dgra · 2017-03-08T23:40:06Z

This adds 2 primary methods to Spree::Taxon to allow users to retreive
all products and all variants that are under the taxon instance. Helpful for
a taxon page to show an overview of everything that is under that
taxon. Also, included is a shortcut method for
taxon.descendants.pluck(:id).

cbrunsdon · 2017-03-15T02:13:44Z

core/app/models/spree/taxon.rb

+    end
+
+    # @return [ActiveRecord::Relation<Spree::Variant>] all self and descendant variants, including master variants.
+    def all_variants


This can get cleaned up to

Variant.where(product_id: all_products.pluck(:id))

@cbrunsdon I will change that, my original thinking for the other way was to save database calls but it only saves us one db call. Thank you for your feedback.

cbrunsdon · 2017-03-15T02:14:40Z

core/app/models/spree/taxon.rb

@@ -79,12 +79,32 @@ def to_param
      permalink
    end

+    # @return [Array<Integer>] the ids of descendant taxons.
+    def descendant_ids


I don't think this is really worth being a public method, its more likely anyone that wants this will not know it exists and call descendants.pluck(:id)

jhawthorn · 2017-03-16T21:02:24Z

core/app/models/spree/taxon.rb

@@ -85,6 +85,16 @@ def active_products
      products.not_deleted.available
    end

+    # @return [ActiveRecord::Relation<Spree::Product>] all self and descendant products
+    def all_products
+      Product.joins(:taxons).where(spree_taxons: { id: descendant_ids.push(id) })


It would be better to use descendant_ids + [id].

Push would append to the array descendant_ids returns which we want to avoid (I don't know whether or not that would cause a real issue, but I can't know without reading its source, so we want to avoid it)

Good eye. Changing now. Thanks.

dgra · 2017-04-03T18:41:30Z

@jhawthorn @cbrunsdon I just wanted to checkup on the state of this PR. Thank you guys.

dgra · 2017-05-23T13:36:36Z

@jhawthorn @cbrunsdon Is there any update on this PR?

gmacdougall

Hi Dustin,

Sorry for the slow response on this one. I have some feedback in regards to query optimization that I think makes sense here.

gmacdougall · 2017-07-01T11:24:49Z

core/app/models/spree/taxon.rb

@@ -85,6 +85,16 @@ def active_products
      products.not_deleted.available
    end

+    # @return [ActiveRecord::Relation<Spree::Product>] all self and descendant products
+    def all_products
+      Product.joins(:taxons).where(spree_taxons: { id: descendant_ids + [id] })


I have some query optimizations I think would be suitable here. Any time we use pluck, we take back the identifiers and put them in a ruby array which means a round trip to the database.

Currently all_products is required to do 2 queries, and all_variants is required to do 3, but we can reduce that to one each pretty easily.

Example of current behaviour:

[1] pry(main)> Spree::Taxon.first.all_variants.count Spree::Taxon Load (0.1ms) SELECT "spree_taxons".* FROM "spree_taxons" ORDER BY "spree_taxons"."id" ASC LIMIT ? [["LIMIT", 1]] (0.2ms) SELECT "spree_taxons"."id" FROM "spree_taxons" WHERE ("spree_taxons"."lft" >= 1) AND ("spree_taxons"."lft" < 12) AND ("spree_taxons"."id" != 1) ORDER BY "spree_taxons"."lft" (0.2ms) SELECT "spree_products"."id" FROM "spree_products" INNER JOIN "spree_products_taxons" ON "spree_products_taxons"."product_id" = "spree_products"."id" INNER JOIN "spree_taxons" ON "spree_taxons"."id" = "spree_products_taxons"."taxon_id" WHERE "spree_products"."deleted_at" IS NULL AND "spree_taxons"."id" IN (3, 4, 5, 6, 7, 1) (0.1ms) SELECT COUNT(*) FROM "spree_variants" WHERE "spree_variants"."deleted_at" IS NULL AND "spree_variants"."product_id" IN (1, 2, 8, 9, 4, 3, 5, 7, 6)

But if we try this:

# @return [ActiveRecord::Relation<Spree::Product>] all self and descendant # products def all_products scope = Product.joins(:taxons) scope.where( spree_taxons: { id: descendants.select(:id) } ).or(scope.where(spree_taxons: { id: id } )) end # @return [ActiveRecord::Relation<Spree::Variant>] all self and descendant # variants, including master variants. def all_variants Variant.where(product_id: all_products.select(:id)) end

Everything can be done in a single subquery:

Spree::Taxon Load (0.2ms) SELECT "spree_taxons".* FROM "spree_taxons" ORDER BY "spree_taxons"."id" ASC LIMIT ? [["LIMIT", 1]] (0.3ms) SELECT COUNT(*) FROM "spree_variants" WHERE "spree_variants"."deleted_at" IS NULL AND "spree_variants"."product_id" IN (SELECT "spree_products"."id" FROM "spree_products" INNER JOIN "spree_products_taxons" ON "spree_products_taxons"."product_id" = "spree_products"."id" INNER JOIN "spree_taxons" ON "spree_taxons"."id" = "spree_products_taxons"."taxon_id" WHERE ("spree_products"."deleted_at" IS NULL AND "spree_taxons"."id" IN (SELECT "spree_taxons"."id" FROM "spree_taxons" WHERE ("spree_taxons"."lft" >= 1) AND ("spree_taxons"."lft" < 12) AND ("spree_taxons"."id" != 1) ORDER BY "spree_taxons"."lft") OR "spree_products"."deleted_at" IS NULL AND "spree_taxons"."id" = ?)) [["id", 1]] => 19

This means only one round trip to the DB, less work done in Ruby and hopefully happy developers!

dgra · 2017-07-03T20:37:12Z

@gmacdougall No problem and thank you for looking at this. I knew it had a few db calls but I should have done a better job of reducing those. Thank you again for looking at this and reducing those calls.

gmacdougall · 2017-07-03T21:01:17Z

core/app/models/spree/taxon.rb

@@ -124,5 +137,10 @@ def touch_ancestors_and_taxonomy
      # Have taxonomy touch happen in #touch_ancestors_and_taxonomy rather than association option in order for imports to override.
      taxonomy.try!(:touch)
    end
+
+    # @return [Array<Integer>] the ids of descendant taxons.
+    def descendant_ids


This method is no longer used and can be removed.

dgra · 2017-07-03T21:31:21Z

@gmacdougall You are right. I removed that method and I will rebase these commits into a single commit if/when this gets approved or when prompted to.

gmacdougall · 2017-07-03T23:18:28Z

Looks reasonable to me. Thanks for addressing the feedback.

Rebase away!

dgra · 2017-07-03T23:36:12Z

Thank you so much for providing feedback on this!

dgra · 2017-09-05T17:34:34Z

@gmacdougall Just checking up on the status of this PR. Any update?

jhawthorn · 2017-10-04T18:44:54Z

core/app/models/spree/taxon.rb

+      scope = Product.joins(:taxons)
+      scope.where(
+        spree_taxons: { id: descendants.select(:id) }
+      ).or(scope.where(spree_taxons: { id: id } ))


This could probably be simpler as where(spree_taxons: { id: self_and_descendants.ids } )

jhawthorn · 2017-10-04T20:09:54Z

I modified all_products to be a little cleaner using self_and_descendants.

Thanks for the change 👍

This adds 2 primary methods to Spree::Taxon to allow users to retreive all products and all variants that are under the taxon instance. Helpful for a taxon page to show an overview of everything that is under that taxon.

gmacdougall

👍

gmacdougall · 2017-10-04T20:14:32Z

Thanks for the work on this Dustin! Sorry for the slow response.

cbrunsdon · 2017-03-15T02:25:20Z

core/app/models/spree/taxon.rb

    # @return [ActiveRecord::Relation<Spree::Product>] the active products the
    #   belong to this taxon
    def active_products
      products.not_deleted.available
    end

+    # @return [ActiveRecord::Relation<Spree::Product>] all self and descendant products
+    def all_products
+      Product.joins(:taxons).where(spree_taxons: { id: descendant_ids.push(id) })


self_and_descendants is provided by acts_as_nested_sets so this can get cleaned up with

self_and_descendants.pluck(:id)

I think the whole thing can be done in one query with:

Spree::Product.joins(:classifications).joins(:taxons).merge(self_and_descendants).distinct

dgra changed the title ~~Add helper methods to Spree::Taxon model for all products/variants~~ Methods for Spree::Taxon for all products/variants from descendants Mar 9, 2017

cbrunsdon reviewed Mar 15, 2017

View reviewed changes

dgra force-pushed the feature/all_products_taxon branch from f498768 to 50833f7 Compare March 15, 2017 15:37

jhawthorn reviewed Mar 16, 2017

View reviewed changes

dgra force-pushed the feature/all_products_taxon branch 2 times, most recently from 4812786 to 57559b9 Compare March 16, 2017 22:51

gmacdougall requested changes Jul 1, 2017

View reviewed changes

gmacdougall requested changes Jul 3, 2017

View reviewed changes

dgra force-pushed the feature/all_products_taxon branch from 978d749 to 9a24fd9 Compare July 3, 2017 23:35

jhawthorn reviewed Oct 4, 2017

View reviewed changes

jhawthorn force-pushed the feature/all_products_taxon branch 2 times, most recently from 3069036 to 568049c Compare October 4, 2017 20:09

Add helper methods to Spree::Taxon model for products/variants

b494168

This adds 2 primary methods to Spree::Taxon to allow users to retreive all products and all variants that are under the taxon instance. Helpful for a taxon page to show an overview of everything that is under that taxon.

jhawthorn force-pushed the feature/all_products_taxon branch from 568049c to b494168 Compare October 4, 2017 20:12

gmacdougall approved these changes Oct 4, 2017

View reviewed changes

cbrunsdon approved these changes Oct 4, 2017

View reviewed changes

gmacdougall merged commit 00bad3a into solidusio:master Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Methods for Spree::Taxon for all products/variants from descendants #1761

Methods for Spree::Taxon for all products/variants from descendants #1761

dgra commented Mar 8, 2017

cbrunsdon Mar 15, 2017

dgra Mar 15, 2017

cbrunsdon Mar 15, 2017

jhawthorn Mar 16, 2017

dgra Mar 16, 2017

dgra commented Apr 3, 2017

dgra commented May 23, 2017

gmacdougall left a comment

gmacdougall Jul 1, 2017

dgra commented Jul 3, 2017

gmacdougall Jul 3, 2017

dgra commented Jul 3, 2017 •

edited

Loading

gmacdougall commented Jul 3, 2017

dgra commented Jul 3, 2017

dgra commented Sep 5, 2017

jhawthorn Oct 4, 2017

jhawthorn commented Oct 4, 2017

gmacdougall left a comment

gmacdougall commented Oct 4, 2017

cbrunsdon Mar 15, 2017

cbrunsdon Mar 15, 2017

Methods for Spree::Taxon for all products/variants from descendants #1761

Methods for Spree::Taxon for all products/variants from descendants #1761

Conversation

dgra commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgra commented Apr 3, 2017

dgra commented May 23, 2017

gmacdougall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgra commented Jul 3, 2017

Choose a reason for hiding this comment

dgra commented Jul 3, 2017 • edited Loading

gmacdougall commented Jul 3, 2017

dgra commented Jul 3, 2017

dgra commented Sep 5, 2017

Choose a reason for hiding this comment

jhawthorn commented Oct 4, 2017

gmacdougall left a comment

Choose a reason for hiding this comment

gmacdougall commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgra commented Jul 3, 2017 •

edited

Loading