Fix RedundantPresenceValidationOnBelongs on some files (part II) #12414
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. - corresponding migration files to match AR Models & DB tables - rake tasks to check corrupt data (ie: NULL/nil in id fields) - updated the todo
app/models/spree/product_property.rb
Outdated
| self.belongs_to_required_by_default = false | ||
|
|
||
| belongs_to :product, class_name: "Spree::Product", touch: true | ||
| belongs_to :product, class_name: "Spree::Product", touch: true, optional: true |
There was a problem hiding this comment.
It don't think it makes sense to have the product optional here.
I checked the production server and none of them have a product property with a product_id set to null so it's safe to the change. @openfoodfoundation/core-devs what do you think ?
There was a problem hiding this comment.
Well if there are no null product_id, I guess I could also update the migration file.
There was a problem hiding this comment.
Indeed, ProductProperty is a join table between Product and Property, so the association is definitely required.
So both the product_id and property_id columns can become NOT NULL. I'm not sure if Gaetan checked all prod servers yet, so we might need to check that before deploying.
There was a problem hiding this comment.
I just did, and we are good, not funny data :)
There was a problem hiding this comment.
Cool. So I have made the update.
rioug
added
the
technical changes only
app/models/spree/product_property.rb
Outdated
| self.belongs_to_required_by_default = false | ||
|
|
||
| belongs_to :product, class_name: "Spree::Product", touch: true | ||
| belongs_to :product, class_name: "Spree::Product", touch: true, optional: true |
There was a problem hiding this comment.
Indeed, ProductProperty is a join table between Product and Property, so the association is definitely required.
So both the product_id and property_id columns can become NOT NULL. I'm not sure if Gaetan checked all prod servers yet, so we might need to check that before deploying.
- product id required - DB + AR model
|
Migration succeeded on au_staging: @rioug just double-checking, can you confirm we are good to merge? |
|
We are all good, merging. |
What? Why?
What should we test?
After migration, all tests should pass and no rubocop offense should be raised.
Run the 3 rake files, to check it any corrupt data is present.
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