If column budget_period_id in aqbudgets table is NULL lateorders.pl, Koha dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157. Of course solution for this is to fix missing values, but I think we should still prevent Koha dying on this.
Created attachment 143988 [details] [review] Bug 32132: Return undef if budget is not found If budget_period_id column is set as NULL, "Late orders" page dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157 Prevent this happening by returning undef from method "budget". To test: 1. Navigate to "Late orders" page. 2. Choose any order, take a note in which budget it belongs to. 3. Update chosen budget from database: UPDATE aqbudgets SET budget_period_id WHERE budget_id = <budget_id>; 4. Reload "Late orders" page. => Page dies on error 500. 5. Apply this patch. 6. Reload page again. => Late orders should now be displayed correctly. Also prove t/db_dependent/Koha/Acquisition/Fund.t. Sponsored-by: Koha-Suomi Oy
There is the ON DELETE CASCADE clause on this FK so I think the correct fix is to make budget_period_id NOT NULL instead.
Setting this assigned to remind myself to look into this again at some point.
Created attachment 161721 [details] [review] Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id If budget_period_id column is set as NULL, "Late orders" page dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157 This patch makes changes to column aqbudgets.budget_period_id so that it no longer allows NULL values. This is in line with aqbudgets tables foreign key "budget_period_id" which has ON DELETE CASCADE clause. To test: 1. Run prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should fail 2. Run updatedatabase.pl and restart services if needed 3. Rerun prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should now pass Sponsored-by: Koha-Suomi Oy
Created attachment 162232 [details] [review] Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id If budget_period_id column is set as NULL, "Late orders" page dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157 This patch makes changes to column aqbudgets.budget_period_id so that it no longer allows NULL values. This is in line with aqbudgets tables foreign key "budget_period_id" which has ON DELETE CASCADE clause. To test: 1. Run prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should fail 2. Run updatedatabase.pl and restart services if needed 3. Rerun prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should now pass Sponsored-by: Koha-Suomi Oy
Created attachment 162233 [details] [review] Bug 32132: DO NOT PUSH! Schema changes
Added changes to kohastructure.sql and schema changes.
Created attachment 162241 [details] [review] Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id If budget_period_id column is set as NULL, "Late orders" page dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157 This patch makes changes to column aqbudgets.budget_period_id so that it no longer allows NULL values. This is in line with aqbudgets tables foreign key "budget_period_id" which has ON DELETE CASCADE clause. To test: 1. Run prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should fail 2. Run updatedatabase.pl and restart services if needed 3. Rerun prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should now pass Sponsored-by: Koha-Suomi Oy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 162584 [details] [review] Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id If budget_period_id column is set as NULL, "Late orders" page dies on error 500. Logs read: Template process failed: undef error - DBIC result _type isn't of the _type Aqbudget at /home/koha/Koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/lateorders.tt line 157 This patch makes changes to column aqbudgets.budget_period_id so that it no longer allows NULL values. This is in line with aqbudgets tables foreign key "budget_period_id" which has ON DELETE CASCADE clause. To test: 1. Run prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should fail 2. Run updatedatabase.pl and restart services if needed 3. Rerun prove t/db_dependent/Koha/Acquisition/Fund.t => Tests should now pass Sponsored-by: Koha-Suomi Oy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Question: I believe the database update will fail if the library has data with budget_period_id = NULL. Could we handle this a bit more gracefully in the database update maybe?
Pushed for 24.05! Well done everyone, thank you!
This makes a lot of tests fail as the test data seems not to be correct. See Jenkins reports on the dashboard. Example: prove t/db_dependent/Acquisition/NewOrder.t t/db_dependent/Acquisition/NewOrder.t .. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Field 'budget_period_id' doesn't have a default value at /kohadevbox/koha/C4/Budgets.pm line 657 # Looks like your test exited with 11 before it could output anything. t/db_dependent/Acquisition/NewOrder.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 8/8 subtests prove t/db_dependent/Letters.t t/db_dependent/Letters.t .. 53/100 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Field 'budget_period_id' doesn't have a default value at /kohadevbox/koha/C4/Budgets.pm line 657 # Looks like your test exited with 11 just after 56. t/db_dependent/Letters.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 44/100 subtests
(In reply to Katrin Fischer from comment #12) > This makes a lot of tests fail as the test data seems not to be correct. See > Jenkins reports on the dashboard. Example: > > prove t/db_dependent/Acquisition/NewOrder.t > t/db_dependent/Acquisition/NewOrder.t .. > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Field 'budget_period_id' doesn't have a default value at > /kohadevbox/koha/C4/Budgets.pm line 657 > # Looks like your test exited with 11 before it could output anything. > t/db_dependent/Acquisition/NewOrder.t .. Dubious, test returned 11 (wstat > 2816, 0xb00) > Failed 8/8 subtests > > prove t/db_dependent/Letters.t > t/db_dependent/Letters.t .. 53/100 > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Field 'budget_period_id' doesn't have a default value at > /kohadevbox/koha/C4/Budgets.pm line 657 > # Looks like your test exited with 11 just after 56. > t/db_dependent/Letters.t .. Dubious, test returned 11 (wstat 2816, 0xb00) > Failed 44/100 subtests Budget periods id has to be added at least to those tests which use C4::Budgets::AddBudget. Will look into this now.
(In reply to Katrin Fischer from comment #10) > Question: I believe the database update will fail if the library has data > with budget_period_id = NULL. Could we handle this a bit more gracefully in > the database update maybe? Should I make changes to atomicupdate file in these patches or should I provide new patch?
(In reply to Emmi Takkinen from comment #14) > (In reply to Katrin Fischer from comment #10) > > Question: I believe the database update will fail if the library has data > > with budget_period_id = NULL. Could we handle this a bit more gracefully in > > the database update maybe? > > Should I make changes to atomicupdate file in these patches or should I > provide new patch? A new patch please, but you can put it on this bug. Please switch to "PQA" - that way it will show top of my queue. Thanks!
I was able to fix all tests failing with DBI error, but qa-tool is styill unhappy with some of them: Can't call method "currency" on an undefined value at /home/emmi/Koha/C4/Acquisition.pm line 1388. It seems some tests Jenkins is marking as failed also fail on this error. Not sure where that stems from though.
If you post what you have Emmi, I'm happy to try and take a look at the rest.. problem shared and all that ;)
Created attachment 165373 [details] [review] Bug 32132: Fix failing tests Some tests failed due changes made to aqbudgets.budget_period_id. Tests failed with error: DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Field 'budget_period_id' doesn't have a default value at... This patch fixes those tests. To test run following lines: prove t/db_dependent/Acquisition/CancelReceipt.t prove t/db_dependent/Acquisition/GetBasketAsCSV.t prove t/db_dependent/Acquisition/GetBasketGroupAsCSV.t prove t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t prove t/db_dependent/Acquisition/GetOrdersByBiblionumber.t prove t/db_dependent/Acquisition/Invoices.t prove t/db_dependent/Acquisition/NewOrder.t prove t/db_dependent/Acquisition/OrderUsers.t prove t/db_dependent/Acquisition/TransferOrder.t prove t/db_dependent/Acquisition/close_reopen_basket.t prove t/db_dependent/Koha/Acquisition/Funds.t prove t/db_dependent/Letters.t Sponsored-by: Koha-Suomi Oy
Created attachment 165374 [details] [review] Bug 32132: Handle funds without budget_period_id gracefully Sponsored-by: Koha-Suomi Oy
Thanks Emmi, it's looking good to me.. I've pushed in Katrins absence as RM backup. It'll be great to see the dashboard go green shortly :)
I've added another patch, will push shortly.. we missed a last case in Acquisitions.t
(In reply to Martin Renvoize from comment #21) > I've added another patch, will push shortly.. we missed a last case in > Acquisitions.t Looks OK now with in main : 364ada88b0 Bug 32132: (RM follow-up) Merge supplimentary db update d5eff4f462 Bug 32132: (RM follow-up) Use testbuilder in Acquisitions.t
I prefer not to backport this complex DB change
Hi Emmi and all, please have a look at bug 32132 - we have issues on update with the database update here. Maybe we need to add some additional checks. We have had similar issues in the past, I don't remember if depending on DBMS version but it could be a factor.
(In reply to Katrin Fischer from comment #24) > Hi Emmi and all, please have a look at bug 32132 - we have issues on update > with the database update here. Maybe we need to add some additional checks. > We have had similar issues in the past, I don't remember if depending on > DBMS version but it could be a factor. ... bug 36993!
Bug fix, nothing to add/edit in the Koha manual.
There is a report on Mattermost: Upgrade to 23.12.00.023 [15:26:18]: Bug 32132 - Set aqbudgets.budget_period_id as NOT NULL ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Can't DROP FOREIGN KEY `aqbudgetperiods_ibfk_1`; check that it exists at /usr/share/koha/lib/C4/Installer.pm line 741 Please ALWAYS check for the existence of a FK/column/table before dropping it!
See https://chat.koha-community.org/koha-community/pl/b4yi6dzowbyrjexqc3ynf6h7xe
(In reply to Jonathan Druart from comment #27) > There is a report on Mattermost: > > > Upgrade to 23.12.00.023 [15:26:18]: Bug 32132 - Set > aqbudgets.budget_period_id as NOT NULL > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Can't DROP > FOREIGN KEY `aqbudgetperiods_ibfk_1`; check that it exists at > /usr/share/koha/lib/C4/Installer.pm line 741 > > > Please ALWAYS check for the existence of a FK/column/table before dropping > it! Dropping and adding that foreign key was added in bug 36993 to fix upgrade failure. However I missed the cases where it doesn't exist to begin with. I'll file a new bug report for this.
Filed bug 37820.