Bug 22368

Summary: Table suggestions lacks foreign key constraints
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: DatabaseAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle, marjorie.barry-vila, martin.renvoize, mtompset, nick, tomascohen, vasanth.k
Version: MainKeywords: Academy
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22008
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13515
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5334, 22348, 7811, 22905, 24244    
Attachments: Bug 22636: Add missing constraints to `suggestions`
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: Add tests
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: (QA follow-up) Mute db errors in test
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: (QA follow-up) Mute db errors in test
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql
Bug 22368: (QA follow-up) Switch to using foreign_key_exists
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: (QA follow-up) Mute db errors in test
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql
Bug 22368: (QA follow-up) Switch to using foreign_key_exists
Bug 22368: (QA follow-up) Changed to LEFT JOIN as requested
Bug 22368: Add tests
Bug 22368: Add tests
Bug 22368: Add missing constraints to `suggestions`
Bug 22368: (QA follow-up) Mute db errors in test
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql
Bug 22368: (QA follow-up) Switch to using foreign_key_exists
Bug 22368: (QA follow-up) Changed to LEFT JOIN as requested
Bug 22368: Make sure the tests will always pass

Description Martin Renvoize 2019-02-19 11:45:57 UTC
The suggestions table, and code that relies upon it, assumes a series of relationships exist to other tables, however few of these are enforced at the database level.. we should add foreign key constraints for these.
Comment 1 Martin Renvoize 2019-02-19 14:05:29 UTC
Created attachment 85278 [details] [review]
Bug 22636: Add missing constraints to `suggestions`
Comment 2 Martin Renvoize 2019-02-19 14:07:22 UTC
Created attachment 85279 [details] [review]
Bug 22368: Add missing constraints to `suggestions`
Comment 3 Martin Renvoize 2019-02-19 15:31:51 UTC
Created attachment 85282 [details] [review]
Bug 22368: Add missing constraints to `suggestions`
Comment 4 Martin Renvoize 2019-02-19 15:31:54 UTC
Created attachment 85283 [details] [review]
Bug 22368: Add tests
Comment 5 Martin Renvoize 2019-02-19 16:08:04 UTC
Created attachment 85286 [details] [review]
Bug 22368: Add missing constraints to `suggestions`
Comment 6 Martin Renvoize 2019-02-19 16:08:07 UTC
Created attachment 85287 [details] [review]
Bug 22368: Add tests
Comment 7 Mark Tompsett 2019-02-19 18:35:47 UTC
Only issue with this now: it's noisy.

Applied tests, and it failed as expected.
Applied patch, and it passed, but it was noisier than expected.
Is there any way to clean up the noise?
I'm still going to sign off, but the noise is my only concern.
Comment 8 Mark Tompsett 2019-02-19 18:36:25 UTC
Created attachment 85309 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Mark Tompsett 2019-02-19 18:36:28 UTC
Created attachment 85310 [details] [review]
Bug 22368: Add missing constraints to `suggestions`

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 Martin Renvoize 2019-02-19 18:39:41 UTC
I've solved that noise somewhere before.. I'll have a hunt around and remind myself how and add it as a follow-up. Thanks for the SO
Comment 11 Martin Renvoize 2019-02-20 08:56:07 UTC
OK, I have no idea how to quiet those warnings.. perhaps a friendly QA person might add a QA followup and teach me how to do it ?
Comment 12 Josef Moravec 2019-02-20 16:14:24 UTC
Created attachment 85377 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2019-02-20 16:14:29 UTC
Created attachment 85378 [details] [review]
Bug 22368: Add missing constraints to `suggestions`

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2019-02-20 16:14:34 UTC
Created attachment 85379 [details] [review]
Bug 22368: (QA follow-up) Mute db errors in test

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Martin Renvoize 2019-02-20 16:44:42 UTC
*** Bug 13533 has been marked as a duplicate of this bug. ***
Comment 16 Martin Renvoize 2019-02-20 16:45:59 UTC
Awesome, thanks for the followup.. I'll bank that one for next time :).
Comment 17 Martin Renvoize 2019-02-20 16:52:33 UTC
Created attachment 85388 [details] [review]
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-02-20 16:53:13 UTC
Madly enough.. I just found a typo in the second patch so I've fixed it with the QA follow-up..
Comment 19 Mark Tompsett 2019-02-20 17:11:52 UTC
Comment on attachment 85388 [details] [review]
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql

Review of attachment 85388 [details] [review]:
-----------------------------------------------------------------

You meant extra. :)
Comment 20 Martin Renvoize 2019-02-20 18:19:56 UTC
Hehe, well, one extra and one missing. 😉

It's so easy to make such a mistake in kohastructure during rebase.. I may try and write a test to catch such mistakes.. though that's for another bug and another day.
Comment 21 Martin Renvoize 2019-02-21 09:33:49 UTC
*** Bug 13484 has been marked as a duplicate of this bug. ***
Comment 22 Jonathan Druart 2019-02-21 14:23:21 UTC
C4::Installer::foreign_key_exists should be used here.
Comment 23 Jonathan Druart 2019-02-21 14:25:31 UTC
NOT IN could (must?) be replaced with LEFT JOIN table SET attr_a = NULL WHERE attr_b IS NULL
Comment 24 Jonathan Druart 2019-02-21 14:29:39 UTC
Regarding C4::Biblio::DelBiblio, suggestions_ibfk_biblionumber should ON DELETE CASCADE (instead of SET NULL).
Same for suggestions_ibfk_branchcode?

Marking as FQA/In discussion
Comment 25 Martin Renvoize 2019-02-22 16:51:30 UTC
(In reply to Jonathan Druart from comment #22)
> C4::Installer::foreign_key_exists should be used here.

Happy to use this, though I used bug 21065 as an example.. do we need to fixup that one to use foreign_key_exists too? What do we win with the function vs my version..
Comment 26 Martin Renvoize 2019-02-22 16:52:47 UTC
(In reply to Jonathan Druart from comment #23)
> NOT IN could (must?) be replaced with LEFT JOIN table SET attr_a = NULL
> WHERE attr_b IS NULL

Personally, I always find the NOT IN clearer than a LEFT JOIN.. and I thought the performance was pretty much the same as the mysql optimiser accounts for it.
Comment 27 Martin Renvoize 2019-02-22 16:56:10 UTC
(In reply to Jonathan Druart from comment #24)
> Regarding C4::Biblio::DelBiblio, suggestions_ibfk_biblionumber should ON
> DELETE CASCADE (instead of SET NULL).
> Same for suggestions_ibfk_branchcode?
> 
> Marking as FQA/In discussion

I did wonder about these too... I was hoping to spark such discussion with the submission of this bug, so thanks.

I'm happy to agree that 'deleting a biblio should also delete any suggestions attached to it' and 'deleting a branch should also delete any suggestions attached'.. Josef do you concur/have any opinion?
Comment 28 Katrin Fischer 2019-02-24 12:18:37 UTC
(In reply to Martin Renvoize from comment #27)
> (In reply to Jonathan Druart from comment #24)
> > Regarding C4::Biblio::DelBiblio, suggestions_ibfk_biblionumber should ON
> > DELETE CASCADE (instead of SET NULL).
> > Same for suggestions_ibfk_branchcode?
> > 
> > Marking as FQA/In discussion
> 
> I did wonder about these too... I was hoping to spark such discussion with
> the submission of this bug, so thanks.
> 
> I'm happy to agree that 'deleting a biblio should also delete any
> suggestions attached to it' and 'deleting a branch should also delete any
> suggestions attached'.. Josef do you concur/have any opinion?

I think I'd prefer NULL in both cases as suggestions are perfectly fine without a branch and a biblionumber, both are optional right now.

Deleting library:
Should be a really rare case. I think at the moment we warn if there are patrons and items for the library before deleting. Should this be extended to cover suggestions as well? 
I think I'd prefer resetting to NULL (any library) to deleting. It seems the less dangerous and a patron from another library could have made the suggestion to begin with (or anonymous) and another library still might want to process it or inform the patron.

Deleting biblio:
This one is harder, I think I like NULL better, because the suggestion doesn't need the link, it's optional as it's only added when the suggestion is ordered.
A use case where this might happen is: A patron makes a suggestion, it's ordered, it's 'sold out' and the order is cancelled deleting the record along with it. You might want to reset the status of the suggestion and try another vendor or inform the patron.

I might think about this from the wrong direction, but maybe it adds to the discussion :)
Comment 29 Josef Moravec 2019-02-25 06:35:48 UTC
(In reply to Martin Renvoize from comment #26)
> (In reply to Jonathan Druart from comment #23)
> > NOT IN could (must?) be replaced with LEFT JOIN table SET attr_a = NULL
> > WHERE attr_b IS NULL
> 
> Personally, I always find the NOT IN clearer than a LEFT JOIN.. and I
> thought the performance was pretty much the same as the mysql optimiser
> accounts for it.

I do find the NOT IN syntax more readable too..
Comment 30 Josef Moravec 2019-02-25 06:44:35 UTC
(In reply to Katrin Fischer from comment #28)
> (In reply to Martin Renvoize from comment #27)
> > (In reply to Jonathan Druart from comment #24)
> > > Regarding C4::Biblio::DelBiblio, suggestions_ibfk_biblionumber should ON
> > > DELETE CASCADE (instead of SET NULL).
> > > Same for suggestions_ibfk_branchcode?
> > > 
> > > Marking as FQA/In discussion
> > 
> > I did wonder about these too... I was hoping to spark such discussion with
> > the submission of this bug, so thanks.
> > 
> > I'm happy to agree that 'deleting a biblio should also delete any
> > suggestions attached to it' and 'deleting a branch should also delete any
> > suggestions attached'.. Josef do you concur/have any opinion?
> 
> I think I'd prefer NULL in both cases as suggestions are perfectly fine
> without a branch and a biblionumber, both are optional right now.
> 
> Deleting library:
> Should be a really rare case. I think at the moment we warn if there are
> patrons and items for the library before deleting. Should this be extended
> to cover suggestions as well? 
> I think I'd prefer resetting to NULL (any library) to deleting. It seems the
> less dangerous and a patron from another library could have made the
> suggestion to begin with (or anonymous) and another library still might want
> to process it or inform the patron.
> 
> Deleting biblio:
> This one is harder, I think I like NULL better, because the suggestion
> doesn't need the link, it's optional as it's only added when the suggestion
> is ordered.
> A use case where this might happen is: A patron makes a suggestion, it's
> ordered, it's 'sold out' and the order is cancelled deleting the record
> along with it. You might want to reset the status of the suggestion and try
> another vendor or inform the patron.
> 
> I might think about this from the wrong direction, but maybe it adds to the
> discussion :)

I do prefer to SET NULL on both, as Katrin said, somebody can reuse it then... for example deleting it after deleting order and biblio is a side effect from my point of view - we are not working with suggestions and some suggestion can be deleted, even without a notice - it is weird.
Comment 31 Martin Renvoize 2019-02-25 12:40:11 UTC
Created attachment 85608 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Martin Renvoize 2019-02-25 12:40:14 UTC
Created attachment 85609 [details] [review]
Bug 22368: Add missing constraints to `suggestions`

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 33 Martin Renvoize 2019-02-25 12:40:18 UTC
Created attachment 85610 [details] [review]
Bug 22368: (QA follow-up) Mute db errors in test

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 34 Martin Renvoize 2019-02-25 12:40:22 UTC
Created attachment 85611 [details] [review]
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2019-02-25 12:40:26 UTC
Created attachment 85612 [details] [review]
Bug 22368: (QA follow-up) Switch to using foreign_key_exists

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2019-02-26 17:36:11 UTC
Created attachment 85723 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 37 Martin Renvoize 2019-02-26 17:36:16 UTC
Created attachment 85725 [details] [review]
Bug 22368: Add missing constraints to `suggestions`

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 38 Martin Renvoize 2019-02-26 17:36:20 UTC
Created attachment 85726 [details] [review]
Bug 22368: (QA follow-up) Mute db errors in test

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 39 Martin Renvoize 2019-02-26 17:36:24 UTC
Created attachment 85727 [details] [review]
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2019-02-26 17:36:30 UTC
Created attachment 85728 [details] [review]
Bug 22368: (QA follow-up) Switch to using foreign_key_exists

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2019-02-26 17:36:35 UTC
Created attachment 85729 [details] [review]
Bug 22368: (QA follow-up) Changed to LEFT JOIN as requested

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Kyle M Hall 2019-02-26 20:02:22 UTC
Created attachment 85739 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Kyle M Hall 2019-02-26 20:09:02 UTC
Created attachment 85742 [details] [review]
Bug 22368: Add tests

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Kyle M Hall 2019-02-26 20:09:33 UTC
Created attachment 85743 [details] [review]
Bug 22368: Add missing constraints to `suggestions`

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Kyle M Hall 2019-02-26 20:09:37 UTC
Created attachment 85744 [details] [review]
Bug 22368: (QA follow-up) Mute db errors in test

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Kyle M Hall 2019-02-26 20:09:41 UTC
Created attachment 85745 [details] [review]
Bug 22368: (QA follow-up) Missing comma in kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall 2019-02-26 20:09:45 UTC
Created attachment 85746 [details] [review]
Bug 22368: (QA follow-up) Switch to using foreign_key_exists

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 Kyle M Hall 2019-02-26 20:09:49 UTC
Created attachment 85747 [details] [review]
Bug 22368: (QA follow-up) Changed to LEFT JOIN as requested

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Jonathan Druart 2019-02-26 20:26:00 UTC
Created attachment 85754 [details] [review]
Bug 22368: Make sure the tests will always pass

Who knows!

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Nick Clemens 2019-03-11 13:13:10 UTC
Awesome work all!

Pushed to master for 19.05
Comment 51 Martin Renvoize 2019-03-12 09:41:45 UTC
Enhancement will not be backported to 18.11.x series.
Comment 52 Katrin Fischer 2019-04-23 11:38:58 UTC
*** Bug 22756 has been marked as a duplicate of this bug. ***
Comment 53 Katrin Fischer 2019-04-23 12:10:13 UTC
*** Bug 22758 has been marked as a duplicate of this bug. ***
Comment 54 Jonathan Druart 2019-05-14 15:57:48 UTC
I think I have a blocker for 19.05, see bug 22905.

suggestions.branchcode may be set to __ANY__ prior to this patch, and so the constraint will not be created.
Comment 55 Jonathan Druart 2019-05-14 16:00:05 UTC
(In reply to Jonathan Druart from comment #54)
> I think I have a blocker for 19.05, see bug 22905.
> 
> suggestions.branchcode may be set to __ANY__ prior to this patch, and so the
> constraint will not be created.

Nevermind, it will be set to NULL. Sorry for the noise!
Comment 56 Jonathan Druart 2019-05-14 16:01:02 UTC
However bug 22905 is still valid, the suggestion will not be accepted.