Bug 29818

Summary:	Cannot save subscription frequency without display order
Product:	Koha	Reporter:	Andrew Fuerste-Henry <andrew>
Component:	Serials	Assignee:	Phil Ringnalda <phil>
Status:	Pushed to main ---	QA Contact:	Aleisha Amohia <aleisha>
Severity:	normal
Priority:	P5 - low	CC:	aleisha, dcook, phil
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38378
Change sponsored?:	---	Patch complexity:	---
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	24.11.00
Circulation function:
Bug Depends on:
Bug Blocks:	17258
Attachments:	Bug 29818: Cannot save subscription frequency without display order Bug 29818: Cannot save subscription frequency without display order Bug 29818: Cannot save subscription frequency without display order

Description Andrew Fuerste-Henry 2022-01-07 13:34:49 UTC

To recreate:
1 - Go to Manage Frequencies (/cgi-bin/koha/serials/subscription-frequencies.pl)
2 - create a new frequency, don't enter a value for display order
3 - try to save, get an error:
C4::Serials::Frequency::AddSubscriptionFrequency(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column 'displayorder' at row 1 at /kohadevbox/koha/serials/subscription-frequencies.pl line 89
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Comment 1 Katrin Fischer 2022-01-08 21:45:16 UTC

Confirmed on master. Same issue for numbering patterns without display order.

I have also tested on 20.11 and there it's possible to save the entries with the display order being empty. So something must have changed and the field should not be mandatory or cause such an ugly error.

Comment 2 David Cook 2024-11-13 02:07:12 UTC

Yep, still an issue on main. Just bumped into it while testing bug 38378.

Comment 3 David Cook 2024-11-13 03:34:47 UTC

I've got a full plate at the moment, but if someone wants to take this one, I'd say the solution would just be to make it a required field in the template. 

Not a perfect solution but certainly a practical one.

Comment 4 Phil Ringnalda 2024-11-13 04:20:23 UTC

And as you might expect since it's only us here seeing it, it's from strict-sql-modes. Since numbering patterns were already fixed in bug 28012 I can crib the fix from there.

What I can't do is resist the temptation to point out that numbering patterns are currently displayed in a DataTable, which does not have Display order set as the default sort.

Comment 5 Phil Ringnalda 2024-11-13 05:51:52 UTC

Created attachment 174445 [details] [review]
Bug 29818: Cannot save subscription frequency without display order

The schema says that subscription_frequencies.displayorder can be null, and
everything else deals with it being null just fine, but if you try to save
a new frequency without specifying display order with strict_sql_modes set,
you get an error.

Test plan:
 1. Without the patch, Serials - Manage frequencies - New frequency
 2. Description is mandatory, so fill it in, then click Save
 3. Boom! Apply patch, restart_all
 4. Repeat steps 1-2, and verify that no error is thrown and the new
    frequency shows up (at the top of the list since nothing comes before
    something)
 5. New frequency, fill in Description, try typing something other than a
    number in Display order and saving. You should be told to follow the
    directions that only numeric characters are allowed

Sponsored-by: Chetco Community Public Library

Comment 6 Katrin Fischer 2024-11-13 07:57:12 UTC

Thanks for the patch Phil, I was going to protest making the display order required.

Comment 7 David Nind 2024-11-15 13:15:56 UTC

Created attachment 174577 [details] [review]
Bug 29818: Cannot save subscription frequency without display order

The schema says that subscription_frequencies.displayorder can be null, and
everything else deals with it being null just fine, but if you try to save
a new frequency without specifying display order with strict_sql_modes set,
you get an error.

Test plan:
 1. Without the patch, Serials - Manage frequencies - New frequency
 2. Description is mandatory, so fill it in, then click Save
 3. Boom! Apply patch, restart_all
 4. Repeat steps 1-2, and verify that no error is thrown and the new
    frequency shows up (at the top of the list since nothing comes before
    something)
 5. New frequency, fill in Description, try typing something other than a
    number in Display order and saving. You should be told to follow the
    directions that only numeric characters are allowed

Sponsored-by: Chetco Community Public Library
Signed-off-by: David Nind <david@davidnind.com>

Comment 8 Aleisha Amohia 2024-11-18 00:40:24 UTC

Created attachment 174654 [details] [review]
Bug 29818: Cannot save subscription frequency without display order

The schema says that subscription_frequencies.displayorder can be null, and
everything else deals with it being null just fine, but if you try to save
a new frequency without specifying display order with strict_sql_modes set,
you get an error.

Test plan:
 1. Without the patch, Serials - Manage frequencies - New frequency
 2. Description is mandatory, so fill it in, then click Save
 3. Boom! Apply patch, restart_all
 4. Repeat steps 1-2, and verify that no error is thrown and the new
    frequency shows up (at the top of the list since nothing comes before
    something)
 5. New frequency, fill in Description, try typing something other than a
    number in Display order and saving. You should be told to follow the
    directions that only numeric characters are allowed

Sponsored-by: Chetco Community Public Library
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Comment 9 Katrin Fischer 2024-11-18 16:45:32 UTC

Pushed for 24.11!

Well done everyone, thank you!