Bug 32132 - Missing budget_period_id in aqbudgets kills lateorders.pl
Summary: Missing budget_period_id in aqbudgets kills lateorders.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-08 11:59 UTC by Emmi Takkinen
Modified: 2024-04-24 09:10 UTC (History)
2 users (show)

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


Attachments
Bug 32132: Return undef if budget is not found (2.57 KB, patch)
2022-11-17 10:00 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id (2.98 KB, patch)
2024-02-02 08:00 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id (4.19 KB, patch)
2024-02-16 06:12 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 32132: DO NOT PUSH! Schema changes (4.08 KB, patch)
2024-02-16 06:12 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id (4.23 KB, patch)
2024-02-16 15:38 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 32132: Do not allow NULL value in column aqbudgets.budget_period_id (4.31 KB, patch)
2024-02-29 10:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32132: Fix failing tests (13.95 KB, patch)
2024-04-23 10:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 32132: Handle funds without budget_period_id gracefully (2.16 KB, patch)
2024-04-23 10:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2022-11-08 11:59:29 UTC
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.
Comment 1 Emmi Takkinen 2022-11-17 10:00:21 UTC
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
Comment 2 Jonathan Druart 2023-05-04 11:53:36 UTC
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.
Comment 3 Emmi Takkinen 2023-08-23 06:12:27 UTC
Setting this assigned to remind myself to look into this again at some point.
Comment 4 Emmi Takkinen 2024-02-02 08:00:08 UTC
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
Comment 5 Emmi Takkinen 2024-02-16 06:12:38 UTC
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
Comment 6 Emmi Takkinen 2024-02-16 06:12:58 UTC
Created attachment 162233 [details] [review]
Bug 32132: DO NOT PUSH! Schema changes
Comment 7 Emmi Takkinen 2024-02-16 06:13:21 UTC
Added changes to kohastructure.sql and schema changes.
Comment 8 Lucas Gass 2024-02-16 15:38:23 UTC
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>
Comment 9 Jonathan Druart 2024-02-29 10:35:06 UTC
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>
Comment 10 Katrin Fischer 2024-04-19 16:59:17 UTC
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?
Comment 11 Katrin Fischer 2024-04-19 17:55:40 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Katrin Fischer 2024-04-22 06:13:56 UTC
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
Comment 13 Emmi Takkinen 2024-04-22 09:44:56 UTC
(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.
Comment 14 Emmi Takkinen 2024-04-22 09:52:23 UTC
(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?
Comment 15 Katrin Fischer 2024-04-22 10:25:30 UTC
(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!
Comment 16 Emmi Takkinen 2024-04-22 11:47:37 UTC
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.
Comment 17 Martin Renvoize 2024-04-23 06:44:13 UTC
If you post what you have Emmi, I'm happy to try and take a look at the rest.. problem shared and all that ;)
Comment 18 Emmi Takkinen 2024-04-23 10:54:10 UTC
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
Comment 19 Emmi Takkinen 2024-04-23 10:54:39 UTC
Created attachment 165374 [details] [review]
Bug 32132: Handle funds without budget_period_id gracefully


Sponsored-by: Koha-Suomi Oy
Comment 20 Martin Renvoize 2024-04-23 15:49:01 UTC
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 :)
Comment 21 Martin Renvoize 2024-04-24 09:10:19 UTC
I've added another patch, will push shortly.. we missed a last case in Acquisitions.t