Fix cop RedundantPresenceValidationOnBelongs

[cyrillefr]

What? Why?

• Contributes to #11482

• Cop: Rails::RedundantPresenceValidationOnBelongsTo

• removes presence: true

• NB: Since Rails 5.0, default implicit checking of presence went from false to true. To avoid breaking things, this default has been added to some files, to keep the current behaviour. See the PR here. So for each of the offenses to be addressed, I have remove that line and made the explicit, implicit , and vice versa. Which means:

  • where removing the active_record.belongs_to_required_by_default config, also removing the presence: true, but also adding an optional: true for the belongs_to that were not validated
  • Example: the app/models/spree/address.rb file: A state is not required, but a country is, for the Spre::Address model. Pre-Rails 5.0: validates country (with self.belongs_to_required_by_default = false) bc country mandatory but not state. Post-Rails 5.0: removes validates country BUT add optional: true to belongs_to :state otherwise state is mandatory (with belongs_to_required_by_default set to true by Rails by default). By default now each belongs_to will be checked for presence unless explicitly stating optional: true

• NB 2 Some fields might be tagged optional ( like user in the Spree::Order model) and not be valid if the field is missing, but it is the same behaviour as before (there is no null in the DB column).

• NB 3 Even though we have belongs_to optional: true, that does not deter to have one validates :specifib_context (Spree::Order model). And no offense raised.

• NB 4 I had 3 strange errors, it seems that on validation, there is no more "can't be blank", but "must exists" They are ActiveRecord messages, so not supposed to be displayed. Though, I do not know if this a problem or not. Cf. the conversation here on rubocop github.

One of the message I have got:

2) SubscriptionLineItem validations requires a variant
     Failure/Error: expect(subject).to validate_presence_of :variant
     
       Expected SubscriptionLineItem to validate that :variant cannot be
       empty/falsy, but this could not be proved.
         After setting :variant to ‹nil›, the matcher expected the
         SubscriptionLineItem to be invalid and to produce the validation error
         "can't be blank" on :variant. The record was indeed invalid, but it
         produced these validation errors instead:
     
         * subscription: ["must exist"]
         * variant: ["must exist"]
         * quantity: ["can't be blank", "is not a number"]
     
         You're getting this error because ActiveRecord is configured to add a
         presence validation to all `belongs_to` associations, and this
         includes yours. *This* presence validation doesn't use "can't be
         blank", the usual validation message, but "must exist" instead.
     
         With that said, did you know that the `belong_to` matcher can test
         this validation for you? Instead of using `validate_presence_of`, try
         the following instead:
     
             it { should belong_to(:variant) }

So, I have modified these 2 specs accordingly:

• spec/models/subscription_line_item_spec.rb
• spec/models/tag_rule_spec.rb

What should we test?

Nothing. All offenses should be corrected and therefore raise nothing. All tests should pass and so show no sign of regression.

Release notes

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

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

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

[rioug]

I understand that using optional: true in the files below reflects the current behaviour, and that the database will enforce the links because null is set to false on the column . But I think it's confusing to have the code like this, as it doesn't really reflect our intentions. So I think we should remove the optional: true and add spec. It shouldn't break anything because the database enforce these links anyway ( Famous last word :laughing: )

These are the relations that jumped out to me, there might others :

LineItem
   belongs_to :order, class_name: "Spree::Order", inverse_of: :line_items, optional: true

Order
  belongs_to :user, class_name: "Spree::User", optional: true
  belongs_to :created_by, class_name: "Spree::User", optional: true
  belongs_to :order_cycle, optional: true
  belongs_to :distributor, class_name: 'Enterprise', optional: true
  belongs_to :customer, optional: true

ProductProperty
  belongs_to :product, class_name: "Spree::Product", touch: true, optional: true

@openfoodfoundation/core-devs what do you think ?

[dacook]

Thanks Gaetan, yes those ones that you've listed look like they should be required, we just haven't yet completed the work that was started in https://github.com/openfoodfoundation/openfoodnetwork/pull/11297.

Maybe Maikel can share more insight from when he worked on it, but I think we just need to work through each file one-by-one. Sorry Cyrille, as you've found this is not a simple one!

It might be helpful to separate the simpler ones (like the first few files up to and including address) into a separate commit, then deal with the harder ones step by step, in further commits or PRs.

[cyrillefr]

Hello @dacook ,

Following your advice, I have split the work in many parts. I have begun by the first 5 files here in #12407 . This PR is for me to remember what I have done in the first place.

[cyrillefr]

This PR has been chunked in smaller ones. Each is listed here. PR #12548 closes the case as there are no more files under that cop to be handled.

So, I can now close it.