Enhance data integrity by adding foreign key constraints to the database

[lentschi]

Current state
Currently there are no foreign key constraints in the db.

Expected state
Adding some would have the following advantages:

• Prevent unforeseen runtime errors caused by broken references in future changes
• Enhance manual crawling: Crawling through the DB is difficult due to broken references
• Prevent bloated DB size
• Prevent data from partially not being removed, when removal was specifically requested by a user due to GDPR requests
• Tooling: Lots of DB viewers (such as DBeaver) support following foreign keys by ctrl clicking on them and other features like automated ERD generation.

Difficulty
Looking through the DB dump of my own foodcoop I found a lot of inconsistencies. (also see #158) Those will first have to be cleaned up by either deleting the records or setting foreign keys to NULL. (Which of the two depends on the way each of them is used in the application and on whether they are nullable.)
Also it will have to be decided how to best set which of the constraints (on delete cascade/set null/restrict).

[lentschi]

I had a first go at it but didn't create a PR yet, as there is an open matter to discuss:

Due to the inconsistent state of our foodcoop's SQL dump, I had to come up with a way to first clean the DB (see this migration). But I've no idea if these SQL statements will be sufficient for all foodcoops and it will be hard (near impossible) to get them right for anyone.
Still, I think this would add a lot of stability to the application, so we should strive to get them as good as it gets and at some point take risk that migrations will fail for some foodcoops. (We'll need to analzye their error logs then and provide patches)

Btw. this provided me with the first ER diagram of the foodsoft db (I removed users and groups from it, so there aren't too many connection lines):

[paroga]

i support adding foreign keys. why do you DELETE the entities in the migration? why can't you just nullify them?

[lentschi]

@paroga

i support adding foreign keys. why do you DELETE the entities in the migration? why can't you just nullify them?

I agree, it might be better to nullify some of them:
E.g. I'm pretty sure that DELETE FROM order_articles WHERE article_id NOT IN (SELECT id FROM articles) makes sense as this is just an extended join table, right? But DELETE FROM articles WHERE article_category_id NOT IN (SELECT id FROM article_categories) on the other hand is probably too drastic. Maybe these articles should just be assigned to some default category...?

Being unsure about this was part of the reason why I haven't created a PR for this yet (see my above comment): I don't know which foreign keys should allowed to be null and which shouldn't...?

In other words: As of now I'm uncertain which records still have a purpose despite being (partially) orphaned.

[paroga]

If I didn't forget something 464c9a3 should make clear what can and cannot be null, but don't trust it 100% and expect some review comments when we see the actual migration. ;-)

[lentschi]

@paroga Thanks, but I only found a single relation (messages n->1 groups) that was deleted by my cleanup script (db/migrate/20200718074330_delete_orphaned_records_from_all_tables.rb), but was mentioned as optional in the commit you referenced (464c9a3), which I've now changed to an UPDATE.

I also fixed another relation that I had analyzed incorrectly and added another fix for #158

Like I said earlier though: I've no idea if these SQL updates/deletes will be sufficient for all foodcoops.
Maybe you could try them in a local copy of all foodsofts you have access to? (That might give us an idea about how useful these migrations might be.)

Also I'll need to find a way to fix the rspec tests creating an MR: Many FactoryBot let statements now fail, because required foreign keys are missing. (For example factory :supplier is missing a supplier_category, but supplier_category_id is set to null: false in the schema.)

[paroga]

the commit i mentioned also includes the fixes for the tests (maybe you want to rebase your work on it for now, my plan is to merge it in the next few days into master after some broader testing deployment).
can you create PRs for your changes so I can comment on it? at least changing the scope :stockit (8655c0e) should get it's owen PR.