Fix RedundantPresenceValidationOnBelongs on two files

[cyrillefr]

What? Why?

• Contributes to Fix Rubocop Rails issues #11482
• First attempt was made by Fix cop RedundantPresenceValidationOnBelongs #12380
• Continuation of Fix RedundantPresenceValidationOnBelongs on some files #12407
• See also Require belongs_to associations by default #11297 that started the work

There were a presence: true for the StockMovement model and I have taken on me to add a not null constraint at the DB level.

What should we test?

After migration, all tests should pass and no rubocop offense should be raised.
Run the 2 rake files, to check it any corrupt data is present.

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.

Dependencies

Documentation updates

[cyrillefr]

Hello @sigmundpetersen or @drummer83 ,
Some failed front-end task that runs ok locally.
Could it be possible to re-run the checking ?

[rioug]

I am not too familiar with the stock system but I feel like this shouldn't be optional @openfoodfoundation/core-devs ?

[cyrillefr]

Without the optional: true, around 10 specs are in error, all related to originator.

[mkllnk]

Maybe the specs need to set the originator? Is it just incomplete test data?

[cyrillefr]

Hello @mkllnk ,

I have no idea if the originator nulleable is a good thing or not.
I have watched a bit one example and cannot make my mind.
I also have no data for stock movement locally.

But, I have looked at the adjustment model that belongs too to originator (There are only 2)
(Btw for now with the self.belongs_to_required_by_default = false)

There are various scope defined, amongst them this one:
scope :admin, -> { where(originator_type: nil) }
Which indicates that for the adjustment model, the originator may be null.
So my guess is that nulleable is a good choice.

May be someone can help us with the data model and what are the exact intentions of this particular model ?

[rioug]

I looked at the production server, and we have some stock movement without originator :

sql=`select count(*) from spree_stock_movements where originator_id is null;'

ansible all_prod -u openfoodnetwork -a "psql -h localhost openfoodnetwork ofn_user -c '$sql'"
openfoodnetwork.org.au | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.org.nz | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.ca | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.org.uk | CHANGED | rc=0 >>
 count 
-------
     3
(1 row)
openfoodnetwork.ie | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.net | CHANGED | rc=0 >>
 count 
-------
     3
(1 row)
app.katuma.org | CHANGED | rc=0 >>
 count 
-------
    26
(1 row)
csicsoka.org | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.de | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.in | CHANGED | rc=0 >>
 count 
-------
     0
(1 row)
openfoodnetwork.be | CHANGED | rc=0 >>
 count 
-------
     1
(1 row)
coopcircuits.fr | CHANGED | rc=0 >>
 count 
-------
    16
(1 row)

So maybe we want to keep it as it is ? If I remember correctly we are trying to not change inventory, so that's another reason not to change this ?

I am not sure if an admin user can do manual stock movement ? but from what I can infer from the data, all stock movement orginator are Spree::Shipment. In the case of an adjustment an adjustment with no originator is manual adjustment, ie made by an admin. So it would be fair to assume the same here ?

@mkllnk what do you think ?

[mkllnk]

This pull request is addressing style rules. So let's keep the logic as it is and go ahead with this.

[rioug]

I checked staging and prod for the updated column and we are good

[mkllnk]

Let's test the checkout with stock controlled products just in case.

[filipefurtad0]

Hey @cyrillefr @mkllnk ,

I've tested the scenario in which some items are added to the cart by the customer while in the meantime the stock is reduced to zero, by the admin - imitating a sort of race condition. This triggers the out-of-stock warning as usual:

Also checked out an order with a variant which had only 2 on hand stock: in this case the UI reflected this limitation, allowing the customer to place only two line items in the cart.

I think this looks good. Merging!