[split checkout] Fix bug preventing user from reaching payment step

[rioug]

When using a "pick up" shipping method, with a user who doesn't have a shipping address it was impossible to proceed to the payment step because shipping address was invalid.

To fix this, we ensure that "ship_address_same_as_billing" parameter is set to true when using a "pick up" shipping method.

What? Why?

• Closes [Split checkout] Users without shipping address are not able to reach step2 if there is only a pickup shipping method in step 1 #10479

In a very specific scenario, a user can be blocked from proceeding to the payment step on checkout :

• User had no shipping address
• Shop only has a "pick up" shipping method
  It this case the user will get a shipping address error when clicking on "Next - Payment Method" button as the shipping address is invalid

I am not sure how a user can end up in this scenario, I didn't get to check the affected user. As far as I can see, there is no way to delete a user/customer shipping address. I managed to reproduce the error by manually delete the customer shipping address ( see the test section below )
I fixed the issue by setting ship_address_same_as_billing to true when the selected shipping method is a "pick up" one. It's a bit hacky but it seems like the easiest solution.
Ideally we would skip validating the order's shipping address in this case, but that would mean getting rid of accepts_nested_attributes_for :ship_address in Spree::Order which would probably cause other issue. I don't think there is a way to skip/conditionally skip validation with accepts_nested_attributes_for.

Filipe did mention in this comment that

Since we're dealing with a pick-up delivery method the hub's address should become the ship address for that order. Don't know why this is happening

I don't if this correct or where in the code it's supposed to happen.

What should we test?

• Create or set up a shop with only a "pick up" shipping method
• Make sure there is an order cycle open for said shop

• Create a new user

• Place an order with the new user

• Remove user shipping address via rails console x :

  • Load placed order: o = Spree::Order.find_by number: 'R313071156'

  • Load customer c = o.customer, it should have a ship_address_id

  • Remove ship_address,

    c.ship_address = nil
    c.save!
    

  • Check in back office that address has been remove properly

  • Go to customer tab, choose shop you placed an order with , you should see "Edit" under "Shipping Address" for your use:

    

• Start an order with the new user and go to checkout

• Click on Next - Payment method -> you should be redirected to the payment step

Alternatively, you might be able to do the following, similar to what the system spec does, I ran out of time to test that :

• Set up a shop with two shipping methods, one requiring a shipping address, and one pick up only
• Start an order and go to checkout
• First choose the shipping method requiring an address, untick "Shipping address same as billing address?"
• Make sure the shipping address is empty
• Then choose the pick up shipping method
• Click on Next - Payment method -> you should be redirected to the payment step

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

[dacook]

Hmm sounds like a tricky one. I agree, this seems like a hacky fix but it's not clear how else it could be fixed.

I was wondering if theres' any other way so jumped in to try.
I tried to clear the shipping address from the params (which I think the frontend JS should be doing), but the validation continues to execute and fail regardless
I discovered the temporary var Order#save_ship_address, so I tried disabling this. I think this disabled some validation, but not all.

So I can't find a better way either, I give up, and approve this fix :)

[dacook]

@jibees as you're intimately familiar with split checkout, it would be great to get your feedback.

[jibees]

As @dacook already said, this may seem a bit hacky, but it does the job.
💪

[filipefurtad0]

Hey @rioug,
Thanks so much for all the details on how to reproduce and test 🙌

Some preliminary comments:

Filipe did mention in this https://github.com/openfoodfoundation/openfoodnetwork/issues/9056#issuecomment-1087845901 that

    Since we're dealing with a pick-up delivery method the hub's address should become the ship address for that order. Don't know why this is happening

I don't if this correct or where in the code it's supposed to happen.

I remember observing this behavior at some point, but the app has indeed changed quite a bit over time. This could be related with this code/PR, although I'm not sure this is still active - I'll need to double check.

I'll proceed to test the workaround proposed in this PR.

I'm not sure though, it is strictly correct to set the shipping to the billing address of the customer, for pick up methods.

For example, for tax calculation, as we usually do this based on shipping address, with this tax setting:

Spree::Config.set(tax_using_ship_address: true)

I don't know how instances are set, but I assume this is the case for most -> something else for me to double check.

Looking at the userguide:
https://ofn-user-guide.gitbook.io/ofn-super-admin-guide/ofn-platform-configuration/taxes-and-tax-zones#how-zones-interact-with-tax

Note: If the shipping method is a ‘collection’ method, the tax is based on the location of the shop. If the shipping method is a ‘delivery’ method, the tax will show according to the location of the customer (their shipping address).

So, this can lead to incorrect tax charges, in cases which the hub is located in another state (relevant for CA) or country with differing (business?) address.

[filipefurtad0]

Ok I think I've found it now:

openfoodnetwork/engines/order_management/app/services/order_management/order/updater.rb

Lines 147 to 154 in 890ab67

	# Sets the distributor's address as shipping address of the order for those
	# shipments using a shipping method that doesn't require address, such us
	# a pickup.
	def shipping_address_from_distributor
	return if order.shipping_method.blank? || order.shipping_method.require_ship_address
	order.ship_address = order.address_from_distributor
	end

And a spec here:

openfoodnetwork/engines/order_management/spec/services/order_management/order/updater_spec.rb

Lines 147 to 154 in 890ab67

	updater.update_payment_state
	expect(order.payment_state).to eq('failed')
	end
	end
	context "when the order has a payment that requires authorization" do
	let!(:payment) { create(:payment, order: order, state: "requires_authorization") }

I've deleted this code snippet and ran our system test on split checkout:
spec/system/consumer/split_checkout_spec.rb

Indeed some tests fail, because it uses before_save_hook. So I don't know why it is not fetching the shipping_address_from_distributor in this case, but I think this would be the expected behavior.

So this seems to be the case for the legacy checkout - I did not test manually. But I guess we would need to decide whether we keep this behavior for split checkout or change it the proposal in this PR of

ensure that "ship_address_same_as_billing" parameter is set to true when using a "pick up" shipping method.

I'll pose the question on the #split-checkout channel.

[RachL]

I feel like we should keep the same rule as legacy checkout as it looks like it has fixed the previous bug and never introduced side effects.

But if there are strong opinions that we should move forward with the current proposal, I'm following.

[mkllnk]

I agree on keeping the old behaviour and copying the distributor's address as shipping address. It's wrong to ship the delivery to the customer's home or business address when pickup is selected. The shipping address is also used for tax calculations and, while I'm no tax expert, I would expect the law of the store to be applied when picking up an order there and potentially paying in cash. This logic has worked for us so far.

[rioug]

Thanks for the feedback, I'll rework my solution

[rioug]

shipping_address_from_distributor mentioned by Filipe above is called here but updater.before_save_hook :

openfoodnetwork/app/models/spree/order.rb

Lines 341 to 365 in 890ab67

	def finalize!
	touch :completed_at
	all_adjustments.update_all state: 'closed'
	# update payment and shipment(s) states, and save
	updater.update_payment_state
	shipments.each do |shipment|
	shipment.update!(self)
	shipment.finalize!
	end
	updater.update_shipment_state
	updater.before_save_hook
	save
	deliver_order_confirmation_email
	state_changes.create(
	previous_state: 'cart',
	next_state: 'complete',
	name: 'order',
	user_id: user_id
	)
	end

So I think my previous solution would have worked. Anyway I reworked the solution to use the distributor address for "pick up" shipping method. It introduces a little bit of duplication as I couldn't reuse existing code, but I believe the Order::Updater still has the last word.

[dacook]

So I think my previous solution would have worked. Anyway I reworked the solution to use the distributor address for "pick up" shipping method. It introduces a little bit of duplication as I couldn't reuse existing code, but I believe the Order::Updater still has the last word.

So it sounds like the fix here (whether the old or new solution) is simply just to get past validation. Then regardless, when the order is saved, it will copy the distributor. I wonder why this wasn't an issue with the old checkout process.

The second fix seems ideal because it is in line with the Order::Updater behaviour, but as you say it duplicates it. I'm wondering why you can't re-use existing code?
It looks to me like this could be solved in the Order model. Could there be a before_validation that calls updater.shipping_address_from_distributor?

[rioug]

It looks to me like this could be solved in the Order model. Could there be a before_validation that calls updater.shipping_address_from_distributor?

I didn't think of that, I'll give it a try. My main issue was the updater.shipping_address_from_distributor requires shipping method and address data to be set.

[rioug]

@dacook So using updater.shipping_address_from_distributor? in a before_validation callback doesn't work as order.shipments are not yet defined, it's needed by the updater.
And interestingly line 144 below is useless :

openfoodnetwork/app/controllers/split_checkout_controller.rb

Lines 141 to 151 in 5dd2737

	def update_order
	return if params[:confirm_order] || @order.errors.any?
	@order.select_shipping_method(params[:shipping_method_id])
	@order.update(order_params)
	@order.updater.update_totals_and_states
	validate_current_step!
	@order.errors.empty?
	end

as select_shipping_method returns early if there is no shipments :

openfoodnetwork/app/models/concerns/order_shipment.rb

Lines 39 to 50 in 5dd2737

	def select_shipping_method(shipping_method_id)
	return if shipping_method_id.blank? || shipments.empty?
	shipment = shipments.first
	shipping_rate = shipment.shipping_rates.find_by(shipping_method_id: shipping_method_id)
	return unless shipping_rate
	shipment.selected_shipping_rate_id = shipping_rate.id
	shipment.shipping_method
	end
	end

[dacook]

Thanks for looking into that. I have one more review sorry! I wonder if we can simplify a couple of bits.

[dacook]

Great, thank you!

It would be good to squash the final solution down into one commit for the next reviewer.

[jibees]

I've squashed into one commit (hope this is fine).

Thanks for this one @rioug 🙏 This looks good to me!!

[RachL]

Managed to reproduce in master with the following scenario (thanks @rioug 🙏 ) :

• Set up a shop with two shipping methods, one requiring a shipping address, and one pick up only
• Start an order and go to checkout
• First choose the shipping method requiring an address, untick "Shipping address same as billing address?"
• Make sure the shipping address is empty
• Then choose the pick up shipping method

With the PR this scenario is fixed: I can go to payment method screen with no error + if I go back I see shipping address has been correctly field up with distributor address.

Marvelous 💪

[drummer83]

@RachL Great! Thanks for testing this! This bug was a mystery in the beginning and we solved it in great team work! 💪

Just curious: When you jumped back to the 'step 1 - details', was there a pre-selected shipping method? I believe it should still have been Pick-up and no shipping address was shown, right? In your screenshot it's not active anymore, so you switched to 'home delivery'. And then the hub address was used as shipping address? Is this really what we want? You would want your own shipping address in that case, wouldn't you?

[RachL]

I believe it should still have been Pick-up and no shipping address was shown, right?

Yes that's what happened, I've selected the delivery method after seeing pick-up was correctly saved because I was curious to see what the system saved as a shipping address.

You would want your own shipping address in that case, wouldn't we?

Yes but in that particular case I've removed my shipping address from the form, I don't see how we can retrieve it. Let's see if this scenario happens in real life. For now cases that came up on production were cases where there was only a pick-up method and we have no idea how the user has end up with no shipping address...

[audez]

Hi! Some users don't have shipping address because normally if you don't select "Save shipping address", no shipping address should be saved. But there's a recent bug: even if you don't check the box, the shipping address is saved (and clears out the preceding one if there already was a saved one).

The bug occurs each time a user with no shipping address selects a pickup method (even if the shop also has a shipping method).

Steps to reproduce the prod scenario are here: #10479 (comment)