Uniform bill_address and ship_address behaviour in Spree::UserAddressBook module #3563

oldjackson · 2020-03-20T14:18:21Z

Responding to the issue #3539, the association has_one :bill_address provided by UserAddressBook to LegacyUser has been made similar to the one already in place for default_address alias ship_address.

Namely, user.bill_address is now the Address pointed by the only UserAddress having the new default_billing flag column set to true, which plays the same role as the default one (which retains the function of selecting the one and only default shipping address).

Being now possible to set the two default addresses independently,

#persist_order_address now calls #save_in_address_book in the same way for the two addresses, solving the issue.
the new public methods #mark_default_ship_address and #mark_default_bill_address are provided. #mark_default_address just calls the former for backward compatibility, and can be deprecated.

core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb

kennyadsl

Thanks, @oldjackson. I left a lot of comments but most of them are about code style and syntax. I think your work here has been fantastic, thanks.

Let me also add that you could improve how commits are organized by squashing some of them and adding some meaningful commit descriptions that explain the why of each change.

Thanks again.

core/app/models/concerns/spree/user_address_book.rb

core/spec/models/spree/concerns/user_address_book_spec.rb

kennyadsl · 2020-03-23T13:05:35Z

core/spec/models/spree/concerns/user_address_book_spec.rb

+          context "shipping address" do
+            it do
+              subject
+              expect(user.default_address).to eq address


I think you can use this syntax: it { is_expected.to eq address }

Not in this case, as I'm not doing an expect(subject).to, but I did it in the following one

core/spec/models/spree/concerns/user_address_book_spec.rb

The association 'belongs_to :bill_address' is superseded by 'has_one :bill_address, through: :default_user_bill_address' so to mimick what already happens for :ship_address and use the same logics around.

aldesantis

@oldjackson thanks, I just left a couple of comments! Also, it looks like tests are failing because they're calling some of the APIs you deprecated. Could you take a look at those?

aldesantis · 2020-03-26T06:57:31Z

core/spec/models/spree/concerns/user_address_book_spec.rb

-          expect(user.default_address).to eq address
+        context "sets it as default" do
+          context "shipping address" do
+            it do


it blocks should always have a description unless you're using one-liners, so that it's clear what you're testing in the example.

Done (long ago)

aldesantis · 2020-03-26T06:57:40Z

core/spec/models/spree/concerns/user_address_book_spec.rb

+          context "billing address" do
+            subject { user.save_in_address_book(address.attributes, true, :billing) }
+
+            it do


Same as for the other: can you add a description here?

Done (long ago)

kennyadsl

Left a couple of non-blocking things if you have time. Otherwise, I'm ok with this version as well. I still think we need to revisit commits and create a cleaner git history. Thanks again!

kennyadsl · 2020-03-30T14:02:35Z

core/app/models/concerns/spree/user_address_book.rb

    end

    def default_address=(address)
+      Spree::Deprecation.warn "This setter does not take into account Spree::Config.automatic_default_address and is deprecated. "\
+        "Please start using #ship_address="
      save_in_address_book(address.attributes, true) if address


Why not using ship_address = address here instead?

I think this is still not done actually, I meant:

def default_address=(address) Spree::Deprecation.warn "This setter does not take into account Spree::Config.automatic_default_address and is deprecated. "\ "Please start using ship_address = address" ship_address = address end

kennyadsl · 2020-03-30T14:03:44Z

core/app/models/concerns/spree/user_address_book.rb

+    end
+
+    def bill_address=(address)
+      # stow a copy in our address book too


Can we remove this comment, please? I think the code is already self-explanatory

kennyadsl · 2020-03-30T14:05:56Z

core/app/models/concerns/spree/user_address_book.rb

      end

      user_address.address
    end

    def mark_default_address(address)
+      Spree::Deprecation.warn "Hey, this method is deprecated and it sets the ship_address only! " \


I know I initially suggested how to formulate this sentence, but Hey, wasn't meant to end up in real code. :P
Can you please restyle this message in line with the tone of the other deprecations (just a little bit more formal).

kennyadsl · 2020-04-20T12:37:29Z

@oldjackson Any chance you could force push here? I'd love to merge this but CircleCI is stuck for some reason. I already tried to rerun specs there but the status is not updating on GitHub.

oldjackson · 2020-04-20T15:24:12Z

I've just force-pushed again. CircleCI seems to be working now.

spaghetticode

@oldjackson thanks for working on this, it surely was not an easy task especially considering the vast amount of tests you had to modify and add.

I've left a few notes, also I've noticed that the commit message bodies are on a single line. Can you re-wrap them to a similar length of the other ones in this project, so they become more readable and they don't look weird on the git log?

spaghetticode · 2020-04-22T15:41:19Z

core/app/models/concerns/spree/user_address_book.rb

          ActiveRecord::Base.transaction do
            (self - [user_address]).each do |ua| # update_all would be nice, but it bypasses ActiveRecord callbacks
-              ua.persisted? ? ua.update!(default: false) : ua.default = false
+              ua.persisted? ? ua.update!({ column_for_default => false }) : ua.write_attribute(column_for_default, false)


Since we're already changing these lines, I think it would make sense to replace the non-communicative variable name ua with something more meaningful... what do you think?

Also, I would remove reduntant curly braces from here update!({ column_for_default => false }) and from line 19 below

Possibly it may also be nice to not use ternary operator for better readability as well

spaghetticode · 2020-04-22T16:14:57Z

core/spec/models/spree/concerns/user_address_book_spec.rb

@@ -11,56 +11,98 @@ module Spree
    let!(:user) { create(:user) }

    describe "#save_in_address_book" do
-      context "saving a default address" do
+      context "saving a shipping address" do


I think we still need to keep the word default in the description, as this block subject is saving a default shipping address.

spaghetticode · 2020-04-22T16:40:56Z

core/spec/models/spree/concerns/user_address_book_spec.rb

        end
      end

-      context "when address is nil" do
+      context "when one order address is nil" do


I think when the order sounds more correct

I wanted to keep the emphasis on the case when one of the two addresses inside the order is nil. I rephrased it for better clarity (or did I?).

spaghetticode · 2020-04-22T16:42:52Z

core/spec/models/spree/concerns/user_address_book_spec.rb

-          order = build(:order)
-          order.ship_address = nil
+          it "does not call save_in_address_book on ship address" do
+            order = build(:order)


I think this line (which is repeated in the next test) may be extracted in a let

spaghetticode · 2020-04-22T16:44:00Z

core/spec/models/spree/concerns/user_address_book_spec.rb

+      let!(:address) { create(:address) }
+
+      # https://github.com/solidusio/solidus/issues/1241
+      it "resets the association and persists" do


I think that adding exactly what gets persisted may add clarity to the message

skukx

Looks good, just a couple non-necessary suggestions

skukx · 2020-04-22T19:05:08Z

core/spec/models/spree/concerns/user_address_book_spec.rb

+              subject
+              expect(user_address.default).to be_falsey
+              expect(user_address.default_billing).to eq true
+            end
          end

          it "adds the address to the user's the associated addresses" do


Extra the:

'adds the address to the user's the associated addresses' # extra the 'adds the address to the user's associated addresses' # cleaned up

Having the now symmetric :ship_address and :bill_address we can define also the specific methods to mark their defaults, #mark_default_ship_address and #mark_default_bill_address. The old mark_default_address is kept for compatibility and calls the former, but gets deprecated.

In #mark_default and #save_in_address_book the argument changes name. In #mark_default it also becomes a keyword argument for the sake of clarity. Other style changes are also made.

The association #default_address and #default_user_address get deprecated in favor of #ship_address #default_user_ship_address. The related setters are also deprecated. The setters maintain their present definition as they don't take into account Spree::Config.automatic_default_address.

For better clarity the scope ::default_shipping is defined along with ::default_billing. ::default is redefined in terms of the former, and deprecated.

Where suitable, expectations are rewritten to make use or RSpec one-liners

kennyadsl

Thanks @oldjackson, looking forward to merging this!

kennyadsl · 2020-05-12T14:18:40Z

@aldesantis you've broken it! 😆

aldesantis · 2020-05-12T17:17:55Z

@kennyadsl sorry! 🙏 Fixed it now.

kennyadsl · 2020-05-13T15:25:40Z

@aldesantis great, can you please change your review status? It's still Changes Requested

aldesantis · 2020-05-13T15:33:07Z

@kennyadsl done!

houndci-bot reviewed Mar 20, 2020

View reviewed changes

core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb Outdated Show resolved Hide resolved

oldjackson changed the title ~~Reorder #persist_order_address spec~~ Uniform bill_address and ship_address behaviour in Spree::UserAddressBook module Mar 20, 2020

houndci-bot reviewed Mar 20, 2020

View reviewed changes

core/db/migrate/20200320144521_add_default_billng_flag_to_user_addresses.rb Outdated Show resolved Hide resolved

kennyadsl requested changes Mar 23, 2020

View reviewed changes

kennyadsl mentioned this pull request Mar 24, 2020

Support Default Billing Address in Address Book #3539

Closed

oldjackson force-pushed the persist-order-address-fix branch from 0336f71 to ae300ca Compare March 24, 2020 22:16

Redefine bill_address association on user

f983978

The association 'belongs_to :bill_address' is superseded by 'has_one :bill_address, through: :default_user_bill_address' so to mimick what already happens for :ship_address and use the same logics around.

oldjackson force-pushed the persist-order-address-fix branch from ae300ca to f6fdf08 Compare March 24, 2020 22:29

aldesantis requested changes Mar 26, 2020

View reviewed changes

oldjackson force-pushed the persist-order-address-fix branch from f9c3ff0 to 0619618 Compare March 29, 2020 15:40

kennyadsl reviewed Mar 30, 2020

View reviewed changes

oldjackson force-pushed the persist-order-address-fix branch from 0619618 to decc624 Compare March 31, 2020 08:08

oldjackson force-pushed the persist-order-address-fix branch from decc624 to 5c9dafe Compare April 20, 2020 15:22

kennyadsl added the Important Change label Apr 21, 2020

oldjackson force-pushed the persist-order-address-fix branch from 5c9dafe to f2e2043 Compare April 21, 2020 21:12

spaghetticode reviewed Apr 22, 2020

View reviewed changes

skukx approved these changes Apr 22, 2020

View reviewed changes

oldjackson added 3 commits April 27, 2020 00:07

Test #save_in_address_book behaviour more thoroughly

00277e1

Change address_kind argument to address_type

708c402

In #mark_default and #save_in_address_book the argument changes name. In #mark_default it also becomes a keyword argument for the sake of clarity. Other style changes are also made.

oldjackson force-pushed the persist-order-address-fix branch 3 times, most recently from f5f106d to e3b1fbf Compare April 26, 2020 22:42

oldjackson added 4 commits May 1, 2020 13:27

Deprecate UserAddress::default scope

131404d

For better clarity the scope ::default_shipping is defined along with ::default_billing. ::default is redefined in terms of the former, and deprecated.

Use one-liners expectations in specs

b4e5616

Where suitable, expectations are rewritten to make use or RSpec one-liners

Remove calls to deprecated methods (in both code and specs)

2abbf81

oldjackson force-pushed the persist-order-address-fix branch from e3b1fbf to 2abbf81 Compare May 1, 2020 11:29

kennyadsl approved these changes May 4, 2020

View reviewed changes

kennyadsl requested a review from aldesantis May 4, 2020 15:11

jarednorman approved these changes May 4, 2020

View reviewed changes

aldesantis force-pushed the persist-order-address-fix branch from 72f4114 to 49347c3 Compare May 11, 2020 14:16

Streamline deprecation messages

d9a5df5

aldesantis force-pushed the persist-order-address-fix branch from 49347c3 to d9a5df5 Compare May 12, 2020 15:26

aldesantis approved these changes May 13, 2020

View reviewed changes

aldesantis added this to the 2.11 milestone May 22, 2020

aldesantis merged commit a7ac7d8 into solidusio:master May 22, 2020

kennyadsl mentioned this pull request Nov 12, 2020

Migrate default billing addresses to address book #3838

Merged

5 tasks

the-krg mentioned this pull request Nov 7, 2023

Add new Solidus/SpreeDefaultAddressDeprecated cop solidusio/rubocop-solidus#64

Open

13 tasks

Uniform bill_address and ship_address behaviour in Spree::UserAddressBook module #3563

Uniform bill_address and ship_address behaviour in Spree::UserAddressBook module #3563

Conversation

oldjackson commented Mar 20, 2020 • edited Loading

kennyadsl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldesantis left a comment • edited Loading

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

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl Apr 21, 2020 • edited Loading

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

Choose a reason for hiding this comment

kennyadsl commented Apr 20, 2020 • edited Loading

oldjackson commented Apr 20, 2020 • edited Loading

spaghetticode left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skukx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl commented May 12, 2020

aldesantis commented May 12, 2020

kennyadsl commented May 13, 2020

aldesantis commented May 13, 2020

oldjackson commented Mar 20, 2020 •

edited

Loading

kennyadsl left a comment •

edited

Loading

aldesantis left a comment •

edited

Loading

kennyadsl Apr 21, 2020 •

edited

Loading

kennyadsl commented Apr 20, 2020 •

edited

Loading

oldjackson commented Apr 20, 2020 •

edited

Loading