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

Migrate variants with nil unit_value #6792

Conversation

andrewpbrett
Copy link
Contributor

@andrewpbrett andrewpbrett commented Feb 2, 2021

What? Why?

Addresses #6737

Variants with a unit_value of nil and whose product uses items cannot be changed to use weight as their variant_unit. We have some assumptions in the code that unit_value is never nil.

What should we test?

The issue described in #6737 will no longer be reproducible without manually altering the database directly even if you try to alter the database directly.

Release notes

Changelog Category: User facing changes | Technical changes

Dependencies

Documentation updates

@sauloperez
Copy link
Contributor

We have some assumptions in the code that unit_value is never nil.

is there something preventing us from enforcing that at DB-level? without a DB constraint, we can't guarantee so.

@andrewpbrett
Copy link
Contributor Author

is there something preventing us from enforcing that at DB-level?

I don't think so, I added a line to the migration that puts the constraint there.

@sauloperez
Copy link
Contributor

Now I noticed we haven't updated the db/schema's timestamp. This needs the be executed locally and the schema updated. I move it to Dev test in case you want to play with it in a staging server @andrewpbrett

@andrewpbrett andrewpbrett added pr-staged-uk staging.openfoodnetwork.org.uk pr-staged-es and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Feb 3, 2021
@andrewpbrett
Copy link
Contributor Author

@sauloperez I tested it out; I created a Variant with a nil unit_value and confirmed that the migration set it to be 1; further, it's no longer possible to set unit_value to be nil after this migration. 👍 Do we want a second review or good to merge now?

@sauloperez
Copy link
Contributor

sauloperez commented Feb 4, 2021

That's enough IMO.

@andrewpbrett andrewpbrett added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-es labels Feb 4, 2021
@andrewpbrett
Copy link
Contributor Author

I beefed this up a bit, let me know if you still think it’s okay to merge @sauloperez. I did a dev test and will deploy it to a staging server as well. After running it locally I can no longer force a value of NaN into the DB even by setting it directly.

change_column_null :spree_variants, :unit_value, false, 1
change_column_null :spree_variants, :weight, false, 0.0
execute "ALTER TABLE spree_variants ADD CONSTRAINT check_unit_value_for_nan CHECK (unit_value <> 'NaN')"
execute "ALTER TABLE spree_variants ADD CONSTRAINT check_weight_for_nan CHECK (weight <> 'NaN')"
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 finally we leverage PostgreSQL features

spree_products: { variant_unit: "items" },
spree_variants: { unit_value: [nil, Float::NAN] }
).find_each do |variant|
Spree::Variant.where(unit_value: [nil, Float::NAN]).find_each do |variant|
Copy link
Contributor

Choose a reason for hiding this comment

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

lol I didn't know NAN existed

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it exists in Rails, but I'm surprised Postgres allows saving it as a value on a decimal field in the DB...

Copy link
Contributor

Choose a reason for hiding this comment

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

you gotta love the 🐘 ❤️

@andrewpbrett andrewpbrett removed the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 5, 2021
@andrewpbrett andrewpbrett merged commit d3ad997 into openfoodfoundation:master Feb 5, 2021
@andrewpbrett andrewpbrett deleted the migrate-variant-unit-values branch February 5, 2021 02:00
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.

3 participants