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

Add task to check for invalid address #12413

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

rioug
Copy link
Collaborator

@rioug rioug commented Apr 24, 2024

What? Why?

As part of #12407, we added some migration that tighten the data validation at the database level, in particular for addresses.
These migrations will fail deployment of the database contains invalid data.

This task check for invalid data addresses and check if they are in use, ie linked to another model. It will return :

  • a list of invalid address in in use
  • a list of invalid address not in use, ie that we can delete

Example output :

$ rails ofn:data:check_invalid_address_used
Checking for invalid address
Checking if any of 66 invalid addresses are in use
"Customers []"
"Subscriptions []"
"EnterpriseGroup []"
"User []"
"Order []"
"Shipments []"
"Enterprises []"
"66 addresses can be deleted:"
[8965, 8669, 8670, 8752, 8753, 8872, 8873, 8914, 8915, 8958, 8959, 8961, 8962, 8964, 8967, 8968, 8991, 8992, 9015, 9016, 9048, 9049, 9128, 9129, 9088, 9089, 9158, 9159, 9193, 9194, 9418, 9419, 9327, 9328, 9527, 9528, 9563, 9564, 9570, 9571, 9572, 9573, 9586, 9587, 9591, 9592, 9594, 9595, 9620, 9621, 9624, 9625, 9658, 9659, 9716, 9717, 9719, 9720, 9726, 9727, 9730, 9731, 9734, 9735, 9762, 9763]
"0 addresses need to be fixed:"
[]

What should we test?

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

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

It checks for invalid addresses, ie address with address1, city, phone
or country_id set to null.
It will return:
- a list of invalid address in use (associated to another
model)
- a list of invalid address not in use, ie that we can delete
@rioug rioug self-assigned this Apr 24, 2024
@rioug rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Apr 24, 2024
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Very cool. We could add it to the migration to automatically delete.... too risky?

@dacook
Copy link
Member

dacook commented Apr 24, 2024

We could add it to the migration to automatically delete.... too risky?

Sounds like a good idea to me. I think that we'd do the same thing manually before the release (eg manually copy the code to each affected server, then run it, then construct a delete statement.)
It's quicker to deploy it automatically, then if it fails we'll need to deal with any issues manually anyway.

@dacook dacook self-requested a review April 24, 2024 06:09
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

If my reading is right, I think we could potentially count some addresses as unused, when they are still used. Can you please check?

Comment on lines 65 to 67
next e.address_id if check_correct_address_id(e.address_id, invalid_addresses)

e.business_address_id if check_correct_address_id(e.business_address_id, invalid_addresses)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that an Enterprise has both invalid address and business_address?

Comment on lines 92 to 94
next o.ship_address_id if check_correct_address_id(o.ship_address_id, addresses)

o.bill_address_id if check_correct_address_id(o.bill_address_id, addresses)
Copy link
Member

Choose a reason for hiding this comment

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

As above, I think we could miss invalid addresses here.

@rioug
Copy link
Collaborator Author

rioug commented Apr 24, 2024

Very cool. We could add it to the migration to automatically delete.... too risky?

Seems a bit risky, as an alternative I can printout the code to be run.

@rioug rioug requested review from dacook and mkllnk April 24, 2024 06:53
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Cool, that should work 👍

@rioug rioug merged commit 442991a into openfoodfoundation:master Apr 29, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants