Fix RedundantPresenceValidationOnBelongs on some files

[cyrillefr]

What? Why?

• Contributes to Fix Rubocop Rails issues #11482
• First attempt was made by Fix cop RedundantPresenceValidationOnBelongs #12380 , but some choices will have to be made file by file. So, here 5 files deemed the simpler ones (cf. conversation). Hopefully the rest will follow.
• See also Require belongs_to associations by default #11297 that started the work

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):

• 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

[dacook]

Great, thanks for ensuring the DB schema is synchronised with the app validation rules 💯

[rioug]

Nice one !

[mkllnk]

We should test the migrations with production databases. If we have corrupt data then the deployment may fail.

[mkllnk]

Bad news with au-prod:

== 20240422150502 RequireAddress1AndCityAndPhoneAndCountryAndOnAddress: migrating 
-- change_column_null(:spree_addresses, :address1, false)
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::NotNullViolation: ERROR:  column "address1" of relation "spree_addresses" contains null values
/home/maikel/code/openfoodnetwork/db/migrate/20240422150502_require_address1_and_city_and_phone_and_country_and_on_address.rb:3:in `change'
<internal:/home/maikel/.rbenv/versions/3.1.4/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
<internal:/home/maikel/.rbenv/versions/3.1.4/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
-e:1:in `<main>'

[mkllnk]

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

[cyrillefr]

Mmh, Can/Should I remove this migration file now that this is merged ?

[rioug]

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

[mkllnk]

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.

[mkllnk]

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.

[cyrillefr]

Hello @mkllnk , @dacook ,

I am starting to wonder if I should continue with this task, or at least try to advance it little by little, parallel with other tasks...

[mkllnk]

if I should continue with this task, or at least try to advance it little by little, parallel with other tasks...

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.