Migrate default billing addresses to address book #3838

mamhoff · 2020-11-11T13:36:58Z

Description

Spree::User records have a bill_address_id that denotes their saved
billing address to be reused in the next checkout. That column, however,
is not in use since Solidus 2.11.0. Instead, we rely on a
default_billing Boolean on the spree_user_addresses join table.

This migration sets the default_billing flag on that join table for
addresses matching the user's default billing address and the user's ID.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

core/db/migrate/20201111133031_migrate_default_billing_addresses_to_address_book.rb

kennyadsl · 2020-11-12T17:42:55Z

Ref: #3563

tvdeyen

Approving although this ideally should be a data migration that runs in a potential solidus:upgrade rake task. We had this rake task once (back in 1.x times) but this was unfortunately removed.

Anyhow. Thanks 👍

tvdeyen · 2020-11-13T09:33:48Z

core/db/migrate/20201111133031_migrate_default_billing_addresses_to_address_book.rb

+
+class MigrateDefaultBillingAddressesToAddressBook < ActiveRecord::Migration[5.2]
+  def up
+    adapter_type = connection.adapter_name.downcase.to_sym


Thinking about this again, I think we should add a safe guard here. Since we already shipped this code maybe someone already migrated their data.

This is true also for people that already updated to 2.11 and has some address with default_billing to true, isn't it?

Yes. I've added a safeguard so that the migration does not do anything if any default_billing flags are already set.

But that only avoids the migration (and the fix) to run for people that used 2.11 for some time before this will be released, isn't it? The ideal would be something that fixes their database as well. Do you think it's possible?

@mamhoff any thoughts on this? Not sure I was clear enough, I meant that I think this will prevent the issue to happen only for users upgrading from Solidus 2.10.x to 2.11.4 (assuming 2.11.4 will contain this fix), but doesn't cover who upgrades from 2.11.3 to 2.11.4. Does it make sense?

ikraamg · 2020-11-28T13:41:11Z

Hi, thank you for this fix 🐛 👍 !

Below I have a suggestion, tested locally, in addition to your fix, to account for any codebase that already uses the default_billing (Solidus >=2.11.0). This should be able to safely migrate and rollback the DB:

def up
    # Safely migrate to the use of `default_billing` if the boolean was already used for the default address.
    if Spree::UserAddress.where(default_billing: true).any?
      (Spree::User.select { |user| user.bill_address_id && !user.bill_address })
        .each { |user| Spree::UserAddress.find_by(user_id: user.id, address_id: user.bill_address_id).update_attribute(:default_billing, true) }
      return true
    end

    adapter_type = connection.adapter_name.downcase.to_sym
    if adapter_type == :mysql2
      sql = <<~SQL
        UPDATE spree_user_addresses
        JOIN spree_users ON spree_user_addresses.user_id = spree_users.id
          AND spree_user_addresses.address_id = spree_users.bill_address_id
        SET spree_user_addresses.default_billing = true
      SQL
    else
      sql = <<~SQL
        UPDATE spree_user_addresses
        SET default_billing = true
        FROM spree_users
        WHERE spree_user_addresses.address_id = spree_users.bill_address_id
          AND spree_user_addresses.user_id = spree_users.id;
          AND spree_users.bill_address != true
      SQL
    end
    execute sql
  end

  def down
    # Safely rollback the use of `default_billing` if the boolean was used for the default address.
    if Spree::UserAddress.where(default_billing: true).any?
      (Spree::User.select { |user| user.bill_address })
        .each { |user| user.update_attribute(:bill_address_id, user.bill_address.id) }
    end
    Spree::UserAddress.update_all(default_billing: false) # rubocop:disable Rails/SkipsModelValidations
  end

mamhoff · 2020-11-30T10:45:31Z

Hey @ikraamg thank you - However, for a very large Solidus installation, this will be prohibitively slow. I've purposefully set this to run in pure SQL, and I'm sure it's possible to do this without iterating over ActiveRecord collections. Sometime this week :)

ikraamg · 2020-12-11T12:49:04Z

Hi @mamhoff, @DanielePalombo helped out with a solution where we exclude the user_addresses that are already using the new format by setting those bill_address_id to nil before running the query:

if Spree::UserAddress.where(default_billing: true).any?
  Spree::User.joins(:bill_address).update_all(bill_address_id: nil)
end

....

It probably also should be moved to a dedicated rake task as Thomas mentioned.
I can work on it if your hands are full right now.

ikraamg · 2021-01-22T13:27:43Z

The task is created in a PR to this branch, however, I am unsure where to run the task as we do not have the upgrade task anymore that was previously used for similar tasks.

mamhoff#1

core/spec/lib/tasks/migrations/migrate_default_billing_addresses_to_address_book_spec.rb

core/lib/tasks/upgrade.rake

core/spec/lib/spree/event/subscriber_registry_spec.rb

backend/app/views/spree/admin/stock_movements/index.html.erb

core/lib/spree/awesome_nested_set_override.rb

core/lib/spree/core/importer/order.rb

core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb

core/app/models/spree/taxon.rb

core/lib/spree/core/controller_helpers/strong_parameters.rb

mamhoff · 2021-02-01T09:25:09Z

@ikraamg @DanielePalombo Thank you for moving this ahead! I've merged @ikraamg s Pull Request and fixed a few whitespace issues, but I think this should be good to go now :)

core/lib/tasks/upgrade.rake

kennyadsl · 2021-02-02T09:21:51Z

CHANGELOG.md

@@ -1,5 +1,13 @@
 ## Solidus 3.0.0 (master, unreleased)

+Migrated default billing addresses to address book


This should not go into the 3.0 changelog, we need to backport this task into 2.11.x. I'll take care of the changelog, you can remove this entirely.

Okay, I will remove this when I add the test case.

core/spec/lib/tasks/migrations/migrate_default_billing_addresses_to_address_book_spec.rb

ikraamg · 2021-02-02T11:59:15Z

@mamhoff I have made the fixes that @kennyadsl requested in a new PR: mamhoff#2.

Then after a squash here, I think we are ready to merge this PR 👍

kennyadsl · 2021-02-14T09:35:16Z

@mamhoff any chance to take a look at the other PR that @ikraamg submitted on this branch? Let me know if you don't have time and we'll merge this and apply the patches directly with another PR against the main repo. Thanks!

`Spree::User` records have a `bill_address_id` that denotes their saved billing address to be reused in the next checkout. That column, however, is not in use since Solidus 2.11.0. Instead, we rely on a `default_billing` Boolean on the `spree_user_addresses` join table. This migration sets the `default_billing` flag on that join table for addresses matching the user's default billing address and the user's ID. MySQL has different SQL syntax than standard SQL, which is why we need two SQL statements. For stores that have run this or a similar migration before, there's a safeguard so they remember to fix their migrations when they upgrade. Co-authored-by: mamhoff <[email protected]> Co-authored-by: DanielePalombo <[email protected]> Co-authored-by: Ikraam Ghoor <[email protected]>

mamhoff · 2021-02-15T09:28:14Z

@ikraamg @DanielePalombo @kennyadsl merged and squashed!

kennyadsl

Thanks @mamhoff @ikraamg @DanielePalombo !

houndci-bot reviewed Nov 11, 2020

View reviewed changes

core/db/migrate/20201111133031_migrate_default_billing_addresses_to_address_book.rb Outdated Show resolved Hide resolved

core/db/migrate/20201111133031_migrate_default_billing_addresses_to_address_book.rb Outdated Show resolved Hide resolved

mamhoff force-pushed the migrate-default-billing-addresses branch 2 times, most recently from 06eef4f to f83707b Compare November 11, 2020 15:49

tvdeyen approved these changes Nov 13, 2020

View reviewed changes

tvdeyen reviewed Nov 13, 2020

View reviewed changes

tvdeyen self-requested a review November 13, 2020 09:34

mamhoff force-pushed the migrate-default-billing-addresses branch from f83707b to 686b9a1 Compare November 13, 2020 09:37

houndci-bot reviewed Feb 1, 2021

View reviewed changes

mamhoff force-pushed the migrate-default-billing-addresses branch from d31e393 to c09dc03 Compare February 1, 2021 09:21

houndci-bot reviewed Feb 1, 2021

View reviewed changes

mamhoff force-pushed the migrate-default-billing-addresses branch from c09dc03 to ca3ea85 Compare February 1, 2021 09:23

kennyadsl reviewed Feb 2, 2021

View reviewed changes

mamhoff force-pushed the migrate-default-billing-addresses branch from 34eb833 to c91b470 Compare February 15, 2021 09:27

kennyadsl added the Needs Backport label Feb 15, 2021

kennyadsl added this to the 2.11 milestone Feb 15, 2021

kennyadsl approved these changes Feb 15, 2021

View reviewed changes

kennyadsl merged commit 0e58e06 into solidusio:master Feb 15, 2021

kennyadsl removed the Needs Backport label Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate default billing addresses to address book #3838

Migrate default billing addresses to address book #3838

mamhoff commented Nov 11, 2020

kennyadsl commented Nov 12, 2020

tvdeyen left a comment

tvdeyen Nov 13, 2020

kennyadsl Nov 13, 2020

mamhoff Nov 13, 2020

kennyadsl Nov 13, 2020

kennyadsl Nov 18, 2020

ikraamg commented Nov 28, 2020 •

edited

Loading

mamhoff commented Nov 30, 2020

ikraamg commented Dec 11, 2020 •

edited

Loading

ikraamg commented Jan 22, 2021

mamhoff commented Feb 1, 2021

kennyadsl Feb 2, 2021

ikraamg Feb 2, 2021

ikraamg commented Feb 2, 2021 •

edited

Loading

kennyadsl commented Feb 14, 2021

mamhoff commented Feb 15, 2021

kennyadsl left a comment

		@@ -1,5 +1,13 @@
		## Solidus 3.0.0 (master, unreleased)

		Migrated default billing addresses to address book

Migrate default billing addresses to address book #3838

Migrate default billing addresses to address book #3838

Conversation

mamhoff commented Nov 11, 2020

kennyadsl commented Nov 12, 2020

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Nov 13, 2020

Choose a reason for hiding this comment

kennyadsl Nov 13, 2020

Choose a reason for hiding this comment

mamhoff Nov 13, 2020

Choose a reason for hiding this comment

kennyadsl Nov 13, 2020

Choose a reason for hiding this comment

kennyadsl Nov 18, 2020

Choose a reason for hiding this comment

ikraamg commented Nov 28, 2020 • edited Loading

mamhoff commented Nov 30, 2020

ikraamg commented Dec 11, 2020 • edited Loading

ikraamg commented Jan 22, 2021

mamhoff commented Feb 1, 2021

kennyadsl Feb 2, 2021

Choose a reason for hiding this comment

ikraamg Feb 2, 2021

Choose a reason for hiding this comment

ikraamg commented Feb 2, 2021 • edited Loading

kennyadsl commented Feb 14, 2021

mamhoff commented Feb 15, 2021

kennyadsl left a comment

Choose a reason for hiding this comment

ikraamg commented Nov 28, 2020 •

edited

Loading

ikraamg commented Dec 11, 2020 •

edited

Loading

ikraamg commented Feb 2, 2021 •

edited

Loading