Bug 38011

Summary: Add a foreign key link between vendors and subscriptions
Product: Koha Reporter: Matt Blenkinsop <matt.blenkinsop>
Component: Architecture, internals, and plumbingAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: Pushed to main --- QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: david, jonathan.druart, martin.renvoize, phil
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
**Sponsored by** *PTFS Europe* and *ByWater Solutions*
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on:    
Bug Blocks: 5334, 38010, 38389, 38390    
Attachments: Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Schema update
Bug 38011: (QA follow-up) Additional unit tests
Bug 38011: (QA follow-up) Improve database update
Bug 38011: Add a foreign key between vendors and subscriptions
Bug 38011: Schema update
Bug 38011: (QA follow-up) Additional unit tests
Bug 38011: (QA follow-up) Improve database update
Bug 38011: (QA follow-up) Improve database update output
Bug 38011: Fix ReNewSubscription.t

Description Matt Blenkinsop 2024-09-26 13:24:25 UTC
There is a FIXME to add a foreign key between vendors and subscriptions
Comment 1 Matt Blenkinsop 2024-09-26 13:36:19 UTC
Created attachment 171971 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t
Comment 2 Matt Blenkinsop 2024-09-26 13:40:56 UTC
Created attachment 171972 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t
Comment 3 David Nind 2024-10-31 22:21:46 UTC
The tests failed for me (using KTD). 

If I don't run the databaseupdate, the tests pass:

prove t/db_dependent/Koha/Subscription.t
t/db_dependent/Koha/Subscription.t .. ok     
All tests successful.
Files=1, Tests=10,  2 wallclock secs ( 0.01 usr  0.00 sys +  1.32 cusr  0.39 csys =  1.72 CPU)
Result: PASS

If I do a databaseupdate and restart everything, the tests fail:

prove t/db_dependent/Koha/Subscription.t
t/db_dependent/Koha/Subscription.t .. 1/10 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`subscription`, CONSTRAINT `subscription_ibfk_4` FOREIGN KEY (`aqbooksellerid`) REFERENCES `aqbooksellers` (`id`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/Koha/Object.pm line 172
    # No tests run!

#   Failed test 'No tests run for subtest "Koha::Subscription->biblio"'
#   at t/db_dependent/Koha/Subscription.t line 49.
Broken FK constraint# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Subscription.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 9/10 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Subscription.t (Wstat: 65280 (exited 255) Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 10 tests but ran 2.
Files=1, Tests=2,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.92 cusr  0.17 csys =  1.12 CPU)
Result: FAIL

Am I supposed to run the databaseupdate?
Comment 4 Matt Blenkinsop 2024-11-01 09:40:43 UTC
Created attachment 173859 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t
Comment 5 Matt Blenkinsop 2024-11-01 09:41:32 UTC
Missed something from the DB update file that I had in a forgotten commit in my local branch - added that back in and it should work now!
Comment 6 David Nind 2024-11-01 09:59:54 UTC
Created attachment 173860 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Martin Renvoize (ashimema) 2024-11-07 12:10:19 UTC
Created attachment 174180 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t
prove t/db_dependent/Koha/Acquisition/Booksellers.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2024-11-07 12:10:22 UTC
Created attachment 174181 [details] [review]
Bug 38011: Schema update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2024-11-07 12:10:25 UTC
Created attachment 174182 [details] [review]
Bug 38011: (QA follow-up) Additional unit tests

Add an additional unit test for the default null on the new constraint.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2024-11-07 12:10:27 UTC
Created attachment 174183 [details] [review]
Bug 38011: (QA follow-up) Improve database update

This patch ensure's we fix errant values in subscriptions prior to
adding the foreign key constraint.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Jonathan Druart 2024-11-07 12:41:54 UTC
Created attachment 174185 [details] [review]
Bug 38011: Add a foreign key between vendors and subscriptions

Test plan:
prove t/db_dependent/Koha/Subscription.t
prove t/db_dependent/Koha/Acquisition/Booksellers.t

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

Amended-by: Jonathan Druart
Rename $rs

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Jonathan Druart 2024-11-07 12:41:57 UTC
Created attachment 174186 [details] [review]
Bug 38011: Schema update

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2024-11-07 12:41:59 UTC
Created attachment 174187 [details] [review]
Bug 38011: (QA follow-up) Additional unit tests

Add an additional unit test for the default null on the new constraint.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2024-11-07 12:42:03 UTC
Created attachment 174188 [details] [review]
Bug 38011: (QA follow-up) Improve database update

This patch ensure's we fix errant values in subscriptions prior to
adding the foreign key constraint.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Katrin Fischer 2024-11-11 14:59:51 UTC
The nice output is not so nice when you have no subscriptions to update:

Updated 0E0 subscriptions with NULL aqbooksellerid where aqbooksellerid was not found in aqbooksellers
Comment 16 Katrin Fischer 2024-11-11 16:43:59 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 17 Martin Renvoize (ashimema) 2024-11-12 07:39:06 UTC
(In reply to Katrin Fischer from comment #15)
> The nice output is not so nice when you have no subscriptions to update:
> 
> Updated 0E0 subscriptions with NULL aqbooksellerid where aqbooksellerid was
> not found in aqbooksellers

0E0 is the 'non-error' zero for DBI.. it needs catching explicitly.. I've done a few updates where that's the case.
Comment 18 Martin Renvoize (ashimema) 2024-11-12 07:41:40 UTC
Created attachment 174397 [details] [review]
Bug 38011: (QA follow-up) Improve database update output

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Katrin Fischer 2024-11-12 16:04:24 UTC
Follow-up picked for main - thanks!
Comment 20 Katrin Fischer 2024-11-12 16:55:36 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 21 Jonathan Druart 2024-11-14 08:49:49 UTC
Created attachment 174492 [details] [review]
Bug 38011: Fix ReNewSubscription.t

t/db_dependent/Serials/ReNewSubscription.t .. Value not allowed for auto_incr id in Aqbookseller at /kohadevbox/koha/t/lib/TestBuilder.pm line 402.
Null value for subscriptionid in Subscriptionhistory not allowed at /kohadevbox/koha/t/lib/TestBuilder.pm line 415.
Can't call method "enddate" on an undefined value at t/db_dependent/Serials/ReNewSubscription.t line 102.

It's failing when no vendor with id=1 in DB.
Comment 22 Katrin Fischer 2024-11-14 09:49:00 UTC
Picked follow-up for main.