Database consistency

[lentschi]

The current foodsoft DB has multiple db inconsistencies (broken foreign keys mainly).

I've made an attempt to address the issue by adding foreign key constraints, however this PR is now long since deprecated. (We've never gotten around to merging it.)

However, in the light of the extend article units PR and of the issue found by RayOei we might think about having another go at this before merging the PR. (This would of course be quite some effort as the PR would then need to update and extend those constraints)

Any thoughts?

[RayOei]

Technically I am in favour of getting the database sorted out, it is one of the things that hurt my eyes, so to speak. But I would also consider it another scope creep which will postpone this large PR once again, making it even more complex.

So: yes we should do it. But NOT with this PR.

Better to have a focused task for improving the structure/constraints/cascade update & delete etc. as this will undoubtedly unveil all kinds of issues.
Another option is to add improving parts of the database when working on a specific feature, which that feature touches?

[lentschi]

But I would also consider it another scope creep which will postpone this large PR once again, making it even more complex.

Yes, of course that's the downside, which is why we had decided to leave it out initially.

But on the other hand, it would then be easier to identify which db inconsistencies were caused by the migrations in the article units fork (I hope none, but you never know) and which had been in there before.

I personally still lean towards postponing it as you suggest - I just wanted everybody's opinion first.

[RayOei]

As to the mentioned issue #1114, see comment: #1114 (comment)

[lentschi]

Just to mention another DB inconsistency: In the current production foodsoft, whenever an Article is created, an ArticlePrice is created. Analyzing some foodcoops DBs however, that for some reason in the past this wasn't always the case (either due to a bug or a changed feature).
See #1132 (comment)

Solution might be to either a) remove the affected articles or to b) populate ArticleVersion from Article directly instead of from ArticlePrice in such cases.

I guess, even though the last affected records I could find were from 2017, I guess b) would still be the best option to not cause more inconsistencies. What do you think?

[RayOei]

Do you know what happens with those articles which miss that ArticlePrice?
In general I agree with your option b) as it is better to keep data than to remove it. Unless it is crystal clear the data is unusable or too old to be of any use.
You can also argue that data more then 5 years old is not of much use? Which opens the door to option a) 😁

I don't think a cleanup function exists? With our FC we discussed removing old data (order/articles/etc) when merging upstream but left it for now. I guess having such a feature may remove issues like this over time? Just a thought...

[lentschi]

Do you know what happens with those articles which miss that ArticlePrice?

No, not really - that's why I'm asking. We'd have to test that through, unless someone already has experience with that type of inconsistency...?
The question is, if it is already causing errors somewhere in the application (with the production code). I guess for article CRUDL it currently does not, but there might be other features where it does.

I don't think a cleanup function exists?

No - nothing current. I tried to introduce such a cleanup with said PR, but that wasn't merged and didn't address this specific issue either.