Fix RedundantPresenceValidationOnBelongs on two files #12493
Conversation
|
Hello @sigmundpetersen , @dacook I would like a rerun of the checks due to some strange error that do not occur locally. |
|
I have re-run the spec a couple of times @cyrillefr but it seems consistent on CI |
|
Seems there's a fix in #12490, you could wait for that |
|
Hi @cyrillefr , you can rebase this now to get the fixes from #12490 |
- 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.
- corresponding migration files to match AR Models &
DB tables
- rake tasks to check corrupt data (ie: NULL/nil in id fields)
- updated the todo
cyrillefr
force-pushed
the
RedundantPresenceValidationOnBelongs_part_IV
branch
from
3317b1f
to
46cd60a
Compare
|
Hello @sigmundpetersen , Thank you. |
| @@ -0,0 +1,5 @@ | |||
| class RequireTaxCategoryOnTaxRate < ActiveRecord::Migration[7.0] | |||
| def change | |||
| change_column_null :spree_tax_rates, :tax_category_id, false | |||
There was a problem hiding this comment.
I double-checked the assumption on all prod servers:
sql='select count(*) from spree_tax_rates where tax_category_id is null;'
ansible all_prod -u openfoodnetwork -a "psql -h localhost openfoodnetwork ofn_user -c '$sql'"
All clear! ✔️
| desc 'Checking missing required ids in Spree::TaxRate' | ||
| task check_missing_required_missing_ids_in_spree_tax_rates: :environment do | ||
| puts 'Checking for null tax_category_id' | ||
| ids = Spree::TaxRate.where(tax_category_id: nil).pluck(:id) | ||
|
|
||
| if ids.empty? | ||
| puts 'No NULL tax_category_id found in spree_tax_rates' | ||
| else | ||
| puts 'NULL tax_category_ids s have been found in spree_tax_rates:' | ||
| print ids | ||
| end | ||
| end |
There was a problem hiding this comment.
Thank your for providing this. But having it in this commit doesn't really help. The rake task can only be run once the code has been deployed. But if you deploy this commit then the migration is run already.
I did a pre-flight check querying all servers via Ansible. Only admins can do that, of course. I'm not sure if there's any better approach.
Can you remove this file again? Or maybe I quickly do it to save time.
There was a problem hiding this comment.
Hello @mkllnk , I will make these scripts in a separate commits ahead of migrations for next time.
Also, I will make a PR to delete these one-short rake taskes I have created.
|
Fixed conflict with GitHub UI. |
dacook
added
pr-staged-au
What? Why?
What should we test?
After migration, all tests should pass and no rubocop offense should be raised.
Run the rake file, to check it any corrupt data is present.
Note: only 1 migration file here because the null were already enforced at DB level for the
variant_overridestable.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