Skip to content
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

Remove user address reference when removing address from the address #3482

Conversation

SamuelMartini
Copy link
Contributor

@SamuelMartini SamuelMartini commented Jan 22, 2020

Description

The address book should provide the user the ability to manage all his addresses.

ship_address_id and bill_address_id are persisted on the Spree::User record after transitioning from address: Spree::User#persist_order_address.

def persist_order_address(order)
if order.ship_address
address = save_in_address_book(
order.ship_address.attributes,
Spree::Config.automatic_default_address
)
self.ship_address_id = address.id if address && address.persisted?
end

These address fields are used to assign user default addresses to the order before transitioning to address: Spree::Order#assign_default_user_addresses.
def assign_default_user_addresses
if user
bill_address = (user.bill_address || user.default_address)
ship_address = (user.ship_address || user.default_address)
# this is one of 2 places still using User#bill_address
self.bill_address ||= bill_address if bill_address.try!(:valid?)
# Skip setting ship address if order doesn't have a delivery checkout step
# to avoid triggering validations on shipping address
self.ship_address ||= ship_address if ship_address.try!(:valid?) && checkout_steps.include?("delivery")
end
end

Currently, when a user empties his address book the user record still has the ship_address_id and bill_address_id references.
In the address checkout step because of assign_default_user_addresses the order addresses are filled with addresses linked on the user record.

As a user I expect that when I empty my address book no address will be shown during my next checkout.

As a developer I expect to use the address book to provide the user full management capability on his addresses.

This commit removes the address reference from the user record when an address is removed from the address book.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

@SamuelMartini SamuelMartini marked this pull request as ready for review January 24, 2020 10:18
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamuelMartini thank you for this fix! I left a minor note on tests, hope it makes sense

@@ -242,6 +242,35 @@ module Spree
it "returns false if the addresses is not there" do
expect(user.remove_from_address_book(0)).to be false
end

context 'when user has previous order addresses' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that besides all these cases when the attributes match and so they are then changed to nil, it would be nice to have at least one test for the other scenarios when they don't match and they are not changed... what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @spaghetticode, thank you for the comment!
Added example when the address does not match any user addresses: https://github.com/solidusio/solidus/pull/3482/files#diff-e930e6c49d94b00d1756dfaf242e4d68R252-R264

…book

The address book should provide the user the ability to manage all his addresses.

`ship_address_id` and `bill_address_id` are persisted on the `Spree::User`
record after transitioning from `address`: `Spree::User#persist_order_address`.
This address fields are used to assign user default addresses to the order
before transitioning to `address`: `Spree::Order#assign_default_user_addresses`.

When a user empties the address book he does not expect the removed address
being shown again during the checkout or elsewhere.

This commit removes the address reference from the user record when an address
is removed from the address book.
@SamuelMartini SamuelMartini force-pushed the SamuelMartini/address_book_user_management branch from 2da3a9c to 63d02a9 Compare February 7, 2020 08:52
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamuelMartini thank you 👍

@spaghetticode spaghetticode merged commit d04ff90 into solidusio:master Feb 28, 2020
@spaghetticode spaghetticode deleted the SamuelMartini/address_book_user_management branch February 28, 2020 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants