Bug 33053 - Tables item_groups and recalls have a biblio_id column with a default of 0
Summary: Tables item_groups and recalls have a biblio_id column with a default of 0
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Aleisha Amohia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-02-23 13:29 UTC by Kyle M Hall
Modified: 2023-06-15 14:50 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06


Attachments
Bug 33053: Remove default value for tables item_groups and recalls (3.21 KB, patch)
2023-02-23 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33053: Remove default value for tables item_groups and recalls (3.26 KB, patch)
2023-04-02 19:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 33053: Remove default value for tables item_groups and recalls (3.32 KB, patch)
2023-04-28 04:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33053: (QA follow-up) fix syntax error (1.24 KB, patch)
2023-04-28 04:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33053: Handle invalid biblio_id more robustly (1.59 KB, patch)
2023-05-11 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-02-23 13:29:01 UTC
These default biblio_ids of 0 are harmless but incorrect. The default values should be removed.
Comment 1 Kyle M Hall 2023-02-23 13:34:55 UTC
Created attachment 147232 [details] [review]
Bug 33053: Remove default value for tables item_groups and recalls

These default biblio_ids of 0 are harmless but incorrect. The default values should be removed.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Biblio.t
3) prove t/db_dependent/Koha/Recalls.t
Comment 2 David Nind 2023-04-02 19:48:20 UTC
Created attachment 149075 [details] [review]
Bug 33053: Remove default value for tables item_groups and recalls

These default biblio_ids of 0 are harmless but incorrect. The default values should be removed.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Biblio.t
3) prove t/db_dependent/Koha/Recalls.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Aleisha Amohia 2023-04-28 04:08:59 UTC
Created attachment 150355 [details] [review]
Bug 33053: Remove default value for tables item_groups and recalls

These default biblio_ids of 0 are harmless but incorrect. The default values should be removed.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Biblio.t
3) prove t/db_dependent/Koha/Recalls.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 4 Aleisha Amohia 2023-04-28 04:09:02 UTC
Created attachment 150356 [details] [review]
Bug 33053: (QA follow-up) fix syntax error

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 5 Tomás Cohen Arazi 2023-05-05 18:43:04 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 6 Tomás Cohen Arazi 2023-05-10 19:34:10 UTC
This patchset highlighted an issue, see:

I commented like this:

$ git diff
diff --git a/t/db_dependent/api/v1/item_groups.t b/t/db_dependent/api/v1/item_groups.t
index 3506eae582d..8a823d121ec 100755
--- a/t/db_dependent/api/v1/item_groups.t
+++ b/t/db_dependent/api/v1/item_groups.t
@@ -112,11 +112,11 @@ subtest 'add() tests' => sub {
 
     # Invalid biblio id
     {   # hide useless warnings
-        local *STDERR;
-        open STDERR, '>', '/dev/null';
+        #local *STDERR;
+        #open STDERR, '>', '/dev/null';
         $t->post_ok( "//$auth_userid:$password@/api/v1/biblios/XXX/item_groups" => json => $item_group )
             ->status_is( 404 );
-        close STDERR;
+        #close STDERR;
     }
 
     $schema->storage->txn_rollback;

And then:

$ prove t/db_dependent/api/v1/item_groups.t

t/db_dependent/api/v1/item_groups.t .. 1/5 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'biblio_id' cannot be null at /kohadevbox/koha/Koha/Object.pm line 170

    #   Failed test '404 Not Found'
    #   at t/db_dependent/api/v1/item_groups.t line 117.
    #          got: '500'
    #     expected: '404'
    # Looks like you failed 1 test of 6.

#   Failed test 'add() tests'
#   at t/db_dependent/api/v1/item_groups.t line 123.

Please provide a fix!
Comment 7 Tomás Cohen Arazi 2023-05-11 19:57:23 UTC
Created attachment 151094 [details] [review]
Bug 33053: Handle invalid biblio_id more robustly

This patch addresses the fact the invalid FK error might differ under
some circumstances. We could try to catch the exception adding another
case, but I think this pattern is cleaner and the authors didn't provide
a fix.

We can discuss it later, as this controller class has several things to
review.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Martin Renvoize 2023-05-12 15:36:20 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 9 Lucas Gass 2023-06-15 14:50:32 UTC
Patchset won't apply cleanly to 22.05.x, no backport.