-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Fix RedundantPresenceValidationOnBelongs on some files #12407
Fix RedundantPresenceValidationOnBelongs on some files #12407
Conversation
- presence: true is redundant since Rails 5.0 BUT applies with new default config of belongs_to_required_by_default to true Lots of files with belongs_to_required_by_default = false (backward compatibility) So: deleting this setting implies to adding optional: true - added 'NOT NULL' constraints so model constraints match with contraints on DB tables. - updated the todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for ensuring the DB schema is synchronised with the app validation rules 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one !
We should test the migrations with production databases. If we have corrupt data then the deployment may fail. |
Bad news with au-prod:
|
change_column_null :spree_addresses, :address1, false | ||
change_column_null :spree_addresses, :city, false | ||
change_column_null :spree_addresses, :phone, false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually have to enforce these three. If they are null, the records are invalid in Rails but we have some database violations. I ran this:
sql='select count(*) from spree_addresses where address1 is null;'
ansible all_prod -u openfoodnetwork -a "psql -h localhost openfoodnetwork ofn_user -c '$sql'"
Most databases are okay, but some are not:
openfoodnetwork.org.au | CHANGED | rc=0 >>
count
-------
66
csicsoka.org | CHANGED | rc=0 >>
count
-------
1236
Same numbers for city
.
And for phone
we have the same in AU but two other results:
openfoodnetwork.org.uk | CHANGED | rc=0 >>
count
-------
4
csicsoka.org | CHANGED | rc=0 >>
count
-------
0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh, Can/Should I remove this migration file now that this is merged ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking at this, we are going to try fix the problematic data, but if it's too much of an issue we might revert some of the migrations
change_column_null :spree_addresses, :address1, false | ||
change_column_null :spree_addresses, :city, false | ||
change_column_null :spree_addresses, :phone, false | ||
change_column_null :spree_addresses, :country_id, false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we got a problem here, too:
csicsoka.org | CHANGED | rc=0 >>
count
-------
21
This one should be quite easy to fix by setting the default country on this instance, manually in the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be good to check if these invalid addresses are actually used. Unfortunately, we have 13 references from other tables to addresses. We should probably script that.
You can continue with this task. One thing that would help a lot is if you write SQL queries to check the database for corrupt data. Then we can run those queries against all production servers at once and see if we need to fix anything up. |
- in relation to: Fix Rubocop Rails issues openfoodfoundation#11482 - and Require belongs_to associations by default openfoodfoundation#11297 - and openfoodfoundation#12407 openfoodfoundation#12428 etc.
What? Why?
Here, I added some migration so that the required fields in AR models match with what is in the DB.
What should we test?
After migration, all tests should pass and no rubocop offense should be raised.
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates