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

2788 [Subscriptions] Fix address issues when syncing subscriptions and orders #3589

Conversation

Projects
None yet
4 participants
@kristinalim
Copy link
Member

commented Mar 7, 2019

Fix shipping and billing address issues when syncing subscriptions and orders.

What? Why?

This fixes remaining issues in #2788, related to how changes to subscriptions' bill address, ship address, and shipping method are carried over to the orders.

This PR includes changes in PR #3560, and cherry-picks commits for v1 in #3605, so should not be merged until those have been approved and merged to their respective base branches.

It turned out that many spec assertions were passing only by accident in v1. See more info here: #3605 The specs had to be fixed in v1 so we could make sure to preserve the behaviour in v2. The fix specific to v2 is only in these commits:

What should we test?

  1. Create a subscription with no end date for an open OC. Set pickup as the shipping method, but set billing details that is different from the shipping details.
  2. "Edit" the subscription order generated. This will create the order. The order ship address should be: bill address name and phone number, and shop address.
  3. Edit the subscription bill address. This should affect the bill and ship address of the order.
  4. Change the order bill address.
  5. Edit the subscription bill address again. This should not affect the order bill and ship address.
  6. Change back order bill address to be the same as subscription bill address.
  7. Switch subscription to delivery.
  8. Edit the subscription bill address again. Check that the order bill address is updated.
  9. Change the order bill address.
  10. Edit the subscription bill address again. The order bill addresses should not be updated.

Dependencies

@kristinalim kristinalim self-assigned this Mar 7, 2019

@kristinalim kristinalim force-pushed the kristinalim:fix/2788-ship_address_changed branch from 9933b7b to 23cf072 Mar 7, 2019

shipping_method = shipment.shipping_method
next if shipping_method.require_ship_address
return if order.shipment.andand.shipping_method.blank?
return if order.shipment.shipping_method.require_ship_address

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Mar 8, 2019

Contributor

I was just discussing the use of andand in another PR with @mkllnk
maybe:

    return if order.shipment.blank?
    return if order.shipment.shipping_method.blank?
    return if order.shipment.shipping_method.require_ship_address

or simply?

    return if order.shipment.andand.shipping_method.andand.require_ship_address

This comment has been minimized.

Copy link
@mkllnk

mkllnk Mar 11, 2019

Member

Or we implement the law of Demeter and then write:

return if order.require_ship_address?

😄

@kristinalim kristinalim force-pushed the kristinalim:fix/2788-ship_address_changed branch 3 times, most recently from d96b302 to 3fc8e70 Mar 12, 2019

@kristinalim kristinalim force-pushed the kristinalim:fix/2788-ship_address_changed branch 4 times, most recently from 63da6da to eb1961c Mar 21, 2019

@kristinalim kristinalim changed the title 2788 [BLOCKED] [Subscriptions] Fix address issues when syncing subscriptions and orders 2788 [Subscriptions] Fix address issues when syncing subscriptions and orders Mar 22, 2019

@kristinalim

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

I've rebased this upon #3640 which already includes the changes in #3560 and #3605. So now, this is blocked only by #3640. When that is merged into 2-0-stable, this should be rebased again.

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

fyi #3640 has been merged

@kristinalim kristinalim force-pushed the kristinalim:fix/2788-ship_address_changed branch from 98f133f to b956b4e Mar 27, 2019

@kristinalim

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Rebased and green, @luisramos0. Thanks.

@luisramos0
Copy link
Contributor

left a comment

nice 💪

I think update_associations_for can be simplified.

unless pickup_to_delivery && order.shipment.present?
order.updater.__send__(:shipping_address_from_distributor)
end

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Mar 27, 2019

Contributor

this is a bit brain damaging, isnt it? :-)

I didnt manage to improve it much but I think this looks a bit easier to follow:

    pickup_to_delivery = force_ship_address_required?(order)
    if !pickup_to_delivery || order.shipment.present?
        update_ship_address_for(order) if (ship_address.changes.keys & relevant_address_attrs).any?
    end
    if !pickup_to_delivery || order.shipment.blank?
      order.updater.__send__(:shipping_address_from_distributor)
    end

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Mar 27, 2019

Contributor

I'd extract this block above including the comment to a separate method, maybe into update_ship_address_for.

This comment has been minimized.

Copy link
@mkllnk

mkllnk Mar 29, 2019

Member

I see that making this method public would mean touching another class but I think it's worth it.

This comment has been minimized.

Copy link
@kristinalim

kristinalim Mar 29, 2019

Author Member

this is a bit brain damaging, isnt it? :-)

Yes, I had a difficult time following code paths for this. 😓

Thanks @luisramos0 and @mkllnk. I agree with your suggestions and have appended the commits. 🙂

@mkllnk
Copy link
Member

left a comment

I'm pretty happy with this PR. I would like to know your opinion about public vs private before I approve though.


order.ship_address = order.distributor.address
end
order.ship_address = order.__send__(:address_from_distributor)

This comment has been minimized.

Copy link
@mkllnk

mkllnk Mar 29, 2019

Member

Shouldn't we make that method public if we use it?

This comment has been minimized.

Copy link
@luisramos0

luisramos0 Mar 29, 2019

Contributor

yeah, address_from_distributor is also a candidate for a nice little service that could also help below in shipping_address_from_distributor

This comment has been minimized.

Copy link
@kristinalim

kristinalim Mar 29, 2019

Author Member

I've made this method public as well. 🙂

unless pickup_to_delivery && order.shipment.present?
order.updater.__send__(:shipping_address_from_distributor)
end

This comment has been minimized.

Copy link
@mkllnk

mkllnk Mar 29, 2019

Member

I see that making this method public would mean touching another class but I think it's worth it.

@luisramos0
Copy link
Contributor

left a comment

super!

@mkllnk

mkllnk approved these changes Apr 2, 2019

Copy link
Member

left a comment

Nice!

@RachL RachL added pr-staged-fr and removed pr-staged-fr labels Apr 4, 2019

@RachL RachL self-assigned this Apr 4, 2019

@RachL RachL added the pr-staged-fr label Apr 4, 2019

@luisramos0 luisramos0 added pr-staged-au and removed pr-staged-fr labels Apr 4, 2019

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I think this is the last subs v2 PR and it closes #2786 as well, correct @kristinalim ?

@RachL

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@kristinalim so far in my tests my order bill address never gets updated when I edit the subscription bill address.
I'm using enterprise 2 and I'm logged in as super admin all along on AU staging.

Maybe I don't look at the right place?

For instance:

"Edit" the subscription order generated. This will create the order. The order ship address should be: bill address name and phone number, and shop address.

What do you mean by This will create the order ? I used the order cycle closing time in order to create the order.

Also where do you exactly see the order ship/bill address?

I went to the "customer details" of the edit order submenu. But I guess it should show up some place else but I couldn't find where.... the invoice is showing the shipping address of my customer... Sorry if this is obvious :(

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I am unstaging this one from AUS to put the new test data there. this one can go next.

@luisramos0

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

this is now back to aus staging.

kristinalim added some commits Mar 7, 2019

Uncomment and update OrderSyncer failing specs
This still achieves a logic issue before the Spree upgrade, where
switching from pick-up to delivery affects whether simultaneous changes
to shipping address are ignored or not.

This behaviour can be fixed in a separate PR.
Update check for pickup address manipulation
The way that the bill contact details and distributor address are set
for pickup orders has changed. At this point, after before_save_hook is
called, the bill contact details have already been set as well.
Rename method OrderSyncer#update_ship_address_for
This method has become only a small part of the logic for updating the
order's ship address. This no longer follows the method naming
convention within OrderSyncer where update_*_for describes almost the
complete logic for updating the association.

This is renamed in preparation for extraction of the complete update
logic for ship_address. The new method will be named
update_ship_address_for.

@kristinalim kristinalim force-pushed the kristinalim:fix/2788-ship_address_changed branch from 5e8bdaa to 1972606 Apr 30, 2019

@kristinalim

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Apologies, my comment on this is really late.

This will create the order.

@RachL Immediately after creating a subscription, there are "orders" (proxy orders) listed under the subscription, but the real orders are not actually created until a background task that generates these is run.

I checked just now and see that this is actually only 5 minutes (I don't have this running automatically in my local machine), so you can just wait 5 minutes too. If you don't want to wait, clicking the link to "edit the order" would create the order. The order needs to exist for the rest of the testing steps.

Also where do you exactly see the order ship/bill address?

I went to the "customer details" of the edit order submenu. But I guess it should show up some place else but I couldn't find where....

This screen recording shows where the subscription order's addresses should show up, @RachL. Are we looking at different places?

20190502015400-subscription-order-addresses

I went through all the testing steps in my dev server and don't see a problem. I will also check the data you are using @RachL. Thank you!

@RachL RachL removed the pr-staged-au label May 2, 2019

@RachL

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@kristinalim thank you for the tip! I didn't know the shortcut, but I was looking at the right place. It didn't worked last time, maybe something with staging it because now I could see each step more clearly!

However 2 things that did not worked:

  • Edit the subscription bill address. This should affect the bill and ship address of the order. this only affected the bill address. Not the shipping address. Why should it update the ship address as well?
  • The email title was not updated but I know there is a PR somewhere for this
  • There is a translation key also missing here (but maybe not due to this PR?):
    image

Updated testing notes:
https://docs.google.com/document/d/1DNbA0L2dLpmYLZrEkQx9GKI044bAgEcZDsFweYpUXPo/edit#

@kristinalim

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Edit the subscription bill address. This should affect the bill and ship address of the order. this only affected the bill address. Not the shipping address. Why should it update the ship address as well?

Sorry, my bad, Rachel! I was actually editing the non-address part of the subscription billing details (not address) here. So if you edit the subscription's billing name or phone number, it should be copied over to the orders' billing and shipping details. The address part would not be copied to the orders' shipping details because the order is pickup and would still use the hub's address.

@RachL

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Thank you @kristinalim as #3782 is created I'm moving this PR foward!

@RachL RachL removed the pr-staged-au label May 3, 2019

@luisramos0 luisramos0 merged commit 4afa0cd into openfoodfoundation:2-0-stable May 3, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.