Checkout addresses #5703

Matt-Yorkley · 2020-06-30T07:16:09Z

What? Why?

Closes #5702
Related to #5663 and possibly to #5705

Set timestamps to not be nullable again.
Fixes problems with updating default shipping / billing addresses during checkout processing.

So I think a nasty error was happening (due to the way we were updating default address records) part-way through the transaction for placing and order, which was then aborted/rolled back in a messy way (causing timestamps on associated objects to be NULL), and possibly triggering other issues within postgres itself (see below). Basically there was some really, really nasty stuff happening here that can only be descried as a fail cascade.

There's some related background in this Rails issue in a case where an exception is raised during a transaction at the point that postgres is dealing with caching important values relating to it's internal state (which we actually see some evidence of in the stacktraces on Katuma) and essentially all hell breaks loose: rails/rails#17607

Other related reading: Rails 4 has introduced breaking changes to the behavior of #clone (apparently without mentioning it in the upgrade guide 😒 ), which I think is what caused the problematic exception in this case. See: https://github.com/rails/rails/blob/4-0-stable/activerecord/lib/active_record/core.rb#L217-L220

We didn't see this in the build before, mostly because of this: #5663 (comment) and partly because of a tiny bit of missing spec coverage for a particular case.

What should we test?

We need to very carefully test the "save as default billing address" and "save as default shipping address" checkout options with:

guest checkout
registered user checkout where the user does not already have default addresses saved
registered user checkout where the user does have default addresses saved

Update: it looks like it's actually just the scenario where the user already has default addresses saved, and they select the "save as default shipping/billing address" option again, which updates their existing default addresses.

Release notes

Fixed bug with updating default addresses during checkout in v3

sauloperez

This is looking good 👍

Matt-Yorkley · 2020-06-30T08:47:20Z

I just added some explanatory notes in each commit 👍

luisramos0

From my comment here:
#5702 (comment)

I am convinced that we can make this code change and still keep the constraints relaxed. I dont understand what serious problems the relaxed constraints can create.

Matt-Yorkley · 2020-06-30T09:18:59Z

I now have feature specs that replicate this bug, and fail at checkout submission, with the expected error (when timestamps are not nullable in the database). The failing specs are fixed by the commits in this PR.

spec/features/consumer/shopping/checkout_spec.rb

These specs fail with the "NULL values for created_at / updated_at" errors we've been seeing.

In Rails 4, #clone behaves differently. The attributes hash of the cloned object is shared with the original, it's not a separate object! https://github.com/rails/rails/blob/4-0-stable/activerecord/lib/active_record/core.rb#L217-L220

Doing #clone or #dup of an object's attributes then passing it to an #update/#update_attributes call means we are manually passing values for created_at and updated_at, which can cause problems, especially if the object being duped hasn't been persisted yet: in this case we would be manually attempting to save timestamps with nil values, which is not a good idea. Here they are blacklisted from the attributes hash.

I considered deleting these tests, as they're not very good and are testing Rails functionality. I decided to leave them in case something explodes in a future upgrade. For reference: there are issues in Rails 4 when using `object.clone.attributes`, and with assigning a hash containing `created_at` and `updated_at` values with `object.update(attributes_hash)`.

luisramos0 · 2020-06-30T09:36:45Z

wow!!!! I had to give it a try, I can confirm, you are right!!!! 👏

I added the spec to master and see it passing!
I added the constraints and see the spec failing

and then added your fix and it's green again.

👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏

luisramos0 · 2020-06-30T09:38:41Z

This will be the cause and fix to the multiple stripe payments right Matt?
#5705

Matt-Yorkley · 2020-06-30T09:40:09Z

This will be the cause and fix to the multiple stripe payments right Matt?

I really, really hope so.

sauloperez · 2020-06-30T10:25:49Z

But the root cause was the issue with #clone and the unordered rollback made it just hard to spot, right? That's what caused the rollbacks in the first place.

Matt-Yorkley · 2020-06-30T10:29:15Z

But the root cause was the issue with #clone and the unordered rollback made it just hard to spot, right?

I think problems due to the #clone issue caused an exception, which triggered a rollback, which fucked up, and the combination of some or all of the above possibly then caused an internal issue in postgres itself, on top of all that.

What made it hard to spot was this: #5663 (comment) and a missing spec.

sigmundpetersen · 2020-06-30T10:33:57Z

So this is Test ready then? I guess we move it to the top as well?

Matt-Yorkley · 2020-06-30T15:57:37Z

I've adjusted the migration here to guard against NULL values whilst the table's NULL constraint is being altered. It needs some re-review and then it would be good to test deploying it to a staging server that does have some NULL timestamps in the database.

luisramos0 · 2020-06-30T16:27:10Z

In uk live there are for example orders without these values, I think we will be ok 👍

Let's get another dev to review this before we test. What can go wrong?

Matt-Yorkley · 2020-06-30T16:45:32Z

Interesting, there is a single order in UK Prod with NULL values in timestamps. I assume that order could create problems...

sauloperez · 2020-07-01T07:47:40Z

db/migrate/20200630070422_require_timestamps.rb

@@ -0,0 +1,253 @@
+class RequireTimestamps < ActiveRecord::Migration
+  def up
+    current_time = Time.now


Have you checked what's the actual value that the ALTER TABLE statement gets called with? It might not matter much in this context but we should always use Rails' Time.zone.now. It's a bit fuzzy in my memory but I remember Time.now was a no. What I do know is that calling zone is the only way to include time-zone information for date times in Rails. According to https://thoughtbot.com/blog/its-about-time-zones we should let AR do the UTC conversion.

It might be fine as it is, but I'd still check this to be fully confident.

sauloperez

Nice! great finding the fact that #change_column_null allows this option! Before that method, you had to change the schema in graduall and backward-compatible steps.

…straint This argument in #change_column_null assigns this value where any NULL values are found, but it doesn't alter the table's `:default` value

Matt-Yorkley · 2020-07-01T08:02:09Z

Before that method, you had to change the schema in graduall and backward-compatible steps.

Yeah, it's new in Rails 4 🎉

It would have been a huge pain to do this nicely without this option...

sauloperez · 2020-07-01T08:04:59Z

It would have been a huge pain to do this nicely without this option...

well, it required patience more than anything

sauloperez · 2020-07-01T08:44:10Z

I tested this locally and it works as intended. An order without timestamps (I set them to null from psql) ends up with timestamps set to the current time in UTC and not null column modifiers 👌

filipefurtad0 · 2020-07-01T17:33:51Z

Hey @Matt-Yorkley ,

Checked the 3 scenarios you mentioned, focusing on Stripe-SCA payments:

i) guest checkout - OK
ii) registered user checkout where the user does not already have default addresses saved - OK
iii) registered user checkout where the user does have default addresses saved - OK

In iii) I tested 8 different options/combinations of the checkboxes on checkout, inserting new addresses when required

Save as default billing address
Shipping address same as billing address?
Save as default shipping address

These appear as expected in the dashboard on Stripe.

Also inserted a bit random and unusual characters (on addresses, names). In all cases checkout proceeded as expected.

Ready to go.

Require timestamps

57ba248

Matt-Yorkley self-assigned this Jun 30, 2020

luisramos0 mentioned this pull request Jun 30, 2020

Multiple successful stripe payments captured for the same order #5705

Closed

Matt-Yorkley force-pushed the checkout-addresses branch from a1c423c to 48f8e39 Compare June 30, 2020 08:43

sauloperez reviewed Jun 30, 2020

View reviewed changes

luisramos0 reviewed Jun 30, 2020

View reviewed changes

Matt-Yorkley force-pushed the checkout-addresses branch from 48f8e39 to 13eecab Compare June 30, 2020 09:23

Matt-Yorkley marked this pull request as ready for review June 30, 2020 09:23

luisramos0 reviewed Jun 30, 2020

View reviewed changes

spec/features/consumer/shopping/checkout_spec.rb Outdated Show resolved Hide resolved

Matt-Yorkley added 5 commits June 30, 2020 11:30

Add specs for updating existing default addresses during checkout

abe96c6

These specs fail with the "NULL values for created_at / updated_at" errors we've been seeing.

Use #dup when copying attributes hash

ce8908f

In Rails 4, #clone behaves differently. The attributes hash of the cloned object is shared with the original, it's not a separate object! https://github.com/rails/rails/blob/4-0-stable/activerecord/lib/active_record/core.rb#L217-L220

Enable pending checkout spec

11da385

Matt-Yorkley force-pushed the checkout-addresses branch from 13eecab to 11da385 Compare June 30, 2020 09:30

luisramos0 approved these changes Jun 30, 2020

View reviewed changes

sauloperez approved these changes Jun 30, 2020

View reviewed changes

Matt-Yorkley added the v3-regression label Jun 30, 2020

filipefurtad0 self-assigned this Jun 30, 2020

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jun 30, 2020

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jun 30, 2020

sauloperez reviewed Jul 1, 2020

View reviewed changes

sauloperez approved these changes Jul 1, 2020

View reviewed changes

Add assign default value of current time whilst altering NOT NULL con…

49fb01a

…straint This argument in #change_column_null assigns this value where any NULL values are found, but it doesn't alter the table's `:default` value

Matt-Yorkley force-pushed the checkout-addresses branch from 1f708df to 49fb01a Compare July 1, 2020 08:00

sauloperez added the dev-test A dev need to test this one label Jul 1, 2020

sauloperez removed the dev-test A dev need to test this one label Jul 1, 2020

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 1, 2020

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jul 1, 2020

luisramos0 merged commit 7dcc2bb into openfoodfoundation:master Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkout addresses #5703

Checkout addresses #5703

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

sauloperez left a comment

Matt-Yorkley commented Jun 30, 2020

luisramos0 left a comment

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

luisramos0 commented Jun 30, 2020

luisramos0 commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020

sauloperez commented Jun 30, 2020 •

edited

Loading

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

sigmundpetersen commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

luisramos0 commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020

sauloperez Jul 1, 2020 •

edited

Loading

sauloperez left a comment

Matt-Yorkley commented Jul 1, 2020

sauloperez commented Jul 1, 2020

sauloperez commented Jul 1, 2020

filipefurtad0 commented Jul 1, 2020

Checkout addresses #5703

Checkout addresses #5703

Conversation

Matt-Yorkley commented Jun 30, 2020 • edited Loading

What? Why?

What should we test?

Release notes

sauloperez left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Jun 30, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Jun 30, 2020 • edited Loading

luisramos0 commented Jun 30, 2020

luisramos0 commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020

sauloperez commented Jun 30, 2020 • edited Loading

Matt-Yorkley commented Jun 30, 2020 • edited Loading

sigmundpetersen commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020 • edited Loading

luisramos0 commented Jun 30, 2020

Matt-Yorkley commented Jun 30, 2020

sauloperez Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

sauloperez left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Jul 1, 2020

sauloperez commented Jul 1, 2020

sauloperez commented Jul 1, 2020

filipefurtad0 commented Jul 1, 2020

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

sauloperez commented Jun 30, 2020 •

edited

Loading

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

Matt-Yorkley commented Jun 30, 2020 •

edited

Loading

sauloperez Jul 1, 2020 •

edited

Loading