Remove HABTM relationships

[BenMorganIO]

Please refer to spree/spree#6627.

• shipping methods/zones (see use HABTM for zones and shipping methods #317)
• taxons/prototypes (see HABTM to HMT for prototype taxons #327)
• properties/prototypes
• taxons/promotion rules
• products/promotion rules (see habtm for product promotion rules #318)
• option types/prototypes
• establish rails table naming convention (see Rename tables for rails convention #328)

[BenMorganIO]

This needs to be changed here. Made a mistake with sorting my ABCs.

[BenMorganIO]

I would prefer to move this to the model as a scope later on or in this PR to help refactor this code.

Also, this line right here was originally:

DISTINCT (spree_products.id)

Arel now generates:

DISTINCT ON (spree_products.id)

It may partially be because I'm using Arel::Nodes::DistinctOn instead of Arel::Nodes::Distinct. However, I couldn't find a way for Distinct to be able to accept a table and a column, but DistinctOn did.

@gmacdougall WDYT?

[BenMorganIO]

@gmacdougall For instances such as this, what are your thoughts?

[gmacdougall]

Do you mean the table rename here? I think its fine.

[BenMorganIO]

kk

[BenMorganIO]

If anyone happens to know how to update this, it would be appreciated. :)

[BenMorganIO]

Does anyone know how to get this to be arel friendly? I tried the Spree::Classification.are_table[:taxon_id], however I didn't realize how some of Arels internals would hiccup when #to_sym was called onto it; the usage of #to_sym is probably so that Rails can lower the amount of string allocations and know the key type.

Anywho, for those who want a deeper understanding of the problem, here it is:

When you call Foo.arel_table[:bar] you have called into the Arel::Table and then Arel hands you back this lovely Arel::Attributes::Attributes (also aliased to Arel::Attribute). This is all nice and pretty and you go "I have an Arel object back, yippie, now lets get the SQL!" and you end up doing Foo.arel_table[:bar].to_sql and you get UndefinedMethodError. Wat?

But wait, if we stop for a second and start doing some work with the code:

Foo.arel_table[:bar].lower.to_sql
# => "LOWER(\"foo\".\"bar\"")

OK, so somehow there is a reference to the table. Its there. It exists. All we did was call #lower and then it just wrapped it. It turns out that you actually need to setup the code so that there's a reference to a node. Why? Cause that's where #to_sql is defined.

Its unfortunate that the docs for #to_sql start with FIX ME and its not because people want Foo.arel_table[:bar].to_sql but because of the AST traversal to find a relation.

If we want to be able to pump a string out, this is what the code started to look life for me after a half hour of hacking:

collector = Arel::Collectors::SQLString.new
collector = ActiveRecord::Base.connection.visitor.accept Foor.arel_table[:bar], collector
collector.value
# => "\"foo\".\"bar\""

TBH, if we did a lot of this and changed up the app to use Arel in a lot more spots, we could start (badly) supporting more DBs than just MySQL, PGSQL, and SQLite. But, hey, lets just stick to doing the Rails-Way™.

[gmacdougall]

Give this a try:

      add_search_scope :in_taxon do |taxon|
        joins(:classifications).
        where(Spree::Classification.table_name => {
          taxon_id: taxon.self_and_descendants
        }).
        order(Spree::Classification.arel_table[:position].asc)
      end

Comparison:

Before:

[1] pry(main)> Spree::Product.in_taxon Spree::Taxon.find_by!(permalink: "categories/bags")
  Spree::Taxon Load (0.2ms)  SELECT  "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."permalink" = ? LIMIT 1  [["permalink", "categories/bags"]]
   (0.2ms)  SELECT "spree_taxons"."id" FROM "spree_taxons" WHERE ("spree_taxons"."lft" >= 2) AND ("spree_taxons"."lft" < 3)  ORDER BY "spree_taxons"."lft"
=>   SQL (0.3ms)  SELECT "spree_products"."id" AS t0_r0, "spree_products"."name" AS t0_r1, "spree_products"."description" AS t0_r2, "spree_products"."available_on" AS t0_r3, "spree_products"."deleted_at" AS t0_r4, "spree_products"."slug" AS t0_r5, "spree_products"."meta_description" AS t0_r6, "spree_products"."meta_keywords" AS t0_r7, "spree_products"."tax_category_id" AS t0_r8, "spree_products"."shipping_category_id" AS t0_r9, "spree_products"."created_at" AS t0_r10, "spree_products"."updated_at" AS t0_r11, "spree_products"."promotionable" AS t0_r12, "spree_products"."meta_title" AS t0_r13, "spree_classifications"."product_id" AS t1_r0, "spree_classifications"."taxon_id" AS t1_r1, "spree_classifications"."id" AS t1_r2, "spree_classifications"."position" AS t1_r3 FROM "spree_products" LEFT OUTER JOIN "spree_classifications" ON "spree_classifications"."product_id" = "spree_products"."id" WHERE "spree_products"."deleted_at" IS NULL AND "spree_classifications"."taxon_id" = 3  ORDER BY spree_classifications.position ASC

After:

[2] pry(main)> Spree::Product.in_taxon Spree::Taxon.find_by!(permalink: "categories/bags")
  Spree::Taxon Load (0.4ms)  SELECT  "spree_taxons".* FROM "spree_taxons" WHERE "spree_taxons"."permalink" = ? LIMIT 1  [["permalink", "categories/bags"]]
=>   Spree::Product Load (0.4ms)  SELECT "spree_products".* FROM "spree_products" INNER JOIN "spree_classifications" ON "spree_classifications"."product_id" = "spree_products"."id" WHERE "spree_products"."deleted_at" IS NULL AND "spree_classifications"."taxon_id" IN (SELECT "spree_taxons"."id" FROM "spree_taxons" WHERE ("spree_taxons"."lft" >= 2) AND ("spree_taxons"."lft" < 3)  ORDER BY "spree_taxons"."lft")  ORDER BY "spree_classifications"."position" ASC

Query is simpler and easier to understand, plus we do it all in one shot query (with a subselect) rather than two. It functions a bit differently internally but should produce the intended results.

[BenMorganIO]

😍

[BenMorganIO]

I think this ready for another round of code review :)

[magnusvk]

Thanks for splitting out the migrations. This still doesn't allow me to deploy this one at a time, though. I was hoping for there to be one commit for each HABTM that is removed so that I can deploy and go live with each intermediary step. So that would require a significant history rewrite but I don't feel good deploying this without that.

[BenMorganIO]

@magnusvk Yeah, the cherry-picking will be very, picky... I could separate this into several PRs?

[magnusvk]

I think that would probably be optimal, yeah.

[BenMorganIO]

KK, I'll cherry-pick some of the commits out.

[magnusvk]

Given that this isn't a ton of code, maybe it makes more sense to just manually pick the code over one HABTM association at a time? (Rather than using git.) I mean, either way works for me of course, but that's probably what I would try here.

[magnusvk]

Also, thanks for doing that extra work!

[BenMorganIO]

Tag #317, #318

[tvdeyen]

Please don't forget to also port spree/spree#6654, because these changes introduced the bug fixed with spree #6654

Thanks

[cbrunsdon]

I pulled out the code in this one that I cared about that hasn't already been merged in #539 and #538.

That is the last of the HABTMs! So closing this, thanks for the work @BenMorganIO