Bug 23064

Summary: Cannot edit subscription with strict SQL modes turned on
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: SerialsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, colin.campbell, hayleypelham, jonathan.druart, katrin.fischer, lucas, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21761
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22812
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24010
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03, 19.05.08, 18.11.15
Bug Depends on:    
Bug Blocks: 17258    
Attachments: Bug 23064: Add tests
Bug 23064: Fix mod subscription with strict sql modes
Bug 23064: Use Koha::Subscription in ModSubscription
Bug 23064: Use Koha::Subscription in ModSubscription
Bug 23064: Use Koha::Subscription in ModSubscription
[18.11.x] Bug 23064: Use Koha::Subscription in ModSubscription

Description Nick Clemens (kidclamp) 2019-06-06 11:39:15 UTC
bug 22812 fixed the values when adding a new subscription, however, editing a subscription throws errors like:
DBD::mysql::st execute failed: Incorrect integer value: '' for column 'innerloop1'

if all numbering pattern values are not set
Comment 1 Jonathan Druart 2019-06-06 17:41:06 UTC
Created attachment 90386 [details] [review]
Bug 23064: Add tests

This param list is scary!
Comment 2 Jonathan Druart 2019-06-06 17:41:10 UTC
Created attachment 90387 [details] [review]
Bug 23064: Fix mod subscription with strict sql modes
Comment 3 Jonathan Druart 2019-06-06 17:44:06 UTC
I have lost too much time with the argument list! They differ from NewSubscription for no good reasons, it's insane.

This patch is not ready for 2 reasons:
- The returned value is not correct ($modified is not correctly named) and is a change in the behavior (not sure it matters are it is never checked). However the tests is wrong as it does not test anything.

- There is a warning:
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018.

I think it's related to bug 21761, that need to be fixed ASAP.
Comment 4 Jonathan Druart 2019-11-20 13:06:52 UTC
Created attachment 95611 [details] [review]
Bug 23064: Use Koha::Subscription in ModSubscription

We must use Koha::Subscription instead of raw SQL.
It will fix issue with default and integer values.

Test plan:
Edit a subscription and set number of issues = "f"
Save
=> Without this patch there is a SQL error in the log:
  Incorrect integer value: 'f' for column 'numberlength'
=> With this patch the other changes are effective.

Note: We also could change the type attribute of the input to "number",
to have a client-side check

Also, the return value of ModSuggestion is never used, so we are safe
with that.
Comment 5 Nick Clemens (kidclamp) 2019-12-02 14:46:02 UTC
With this patch I can save the subscription, however, if I enter 'f' then the number of issues is NULLed, so I can have a subscription with no length or end date, which should be impossible.

I think we must have some validation of the form to prevent non-numeric values entered
Comment 6 Jonathan Druart 2019-12-03 08:57:25 UTC
Hi Nick,

Since we are using Koha::Subscription for adding a subscription I guess the problem already exists (bug 22812).
I would consider it out of the scope of this bug. Form validation is not perfect in Koha and the effort is much bigger.

See also bug 24010 that is trying to deal with enforcing integer validation.
Comment 7 Jonathan Druart 2019-12-11 08:57:57 UTC
Switching back to NSO.
Comment 8 Hayley Pelham 2020-01-22 02:32:56 UTC
Hi Jonathan,

I am unable to reproduce this bug on master.

Steps I have taken:
1) Define a budget, active fund, and vendor
2) Create new subscription
3) Edit that subscription
4) Put 'f' in the 'Number of issues to display to staff' field
(I also tried putting 'f' in the Subscription length field, which saves with no errors and is NULLed)
5) Kept an eye on the plack-error.log and the intranet-error.log during the testing, did not see any error.

Would you please tell me what I have missed here?
Comment 9 Jonathan Druart 2020-01-22 09:00:49 UTC
Hi Hayley,
Did you turn the strict_sql_modes config flag on?
- edit $KOHA_CONF
- make sure you have <strict_sql_modes>1</strict_sql_modes> in the config section
- restart all the things

Then try again.
Comment 10 Hayley Pelham 2020-01-22 20:55:11 UTC
Created attachment 97753 [details] [review]
Bug 23064: Use Koha::Subscription in ModSubscription

We must use Koha::Subscription instead of raw SQL.
It will fix issue with default and integer values.

Test plan:
Edit a subscription and set number of issues = "f"
Save
=> Without this patch there is a SQL error in the log:
  Incorrect integer value: 'f' for column 'numberlength'
=> With this patch the other changes are effective.

Note: We also could change the type attribute of the input to "number",
to have a client-side check

Also, the return value of ModSuggestion is never used, so we are safe
with that.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
The <strict_sql_modes> flag must be present and set to 1 in the <config> section of koha-conf.xml
Comment 11 Katrin Fischer 2020-01-22 22:03:06 UTC
Created attachment 97754 [details] [review]
Bug 23064: Use Koha::Subscription in ModSubscription

We must use Koha::Subscription instead of raw SQL.
It will fix issue with default and integer values.

Test plan:
Edit a subscription and set number of issues = "f"
Save
=> Without this patch there is a SQL error in the log:
  Incorrect integer value: 'f' for column 'numberlength'
=> With this patch the other changes are effective.

Note: We also could change the type attribute of the input to "number",
to have a client-side check

Also, the return value of ModSuggestion is never used, so we are safe
with that.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
The <strict_sql_modes> flag must be present and set to 1 in the <config> section of koha-conf.xml

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2020-01-22 22:03:35 UTC
Reads nice and works well!
Comment 13 Martin Renvoize 2020-01-23 09:10:43 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-02-06 23:04:44 UTC
Pushed to 19.11.x for 19.11.03
Comment 15 Lucas Gass 2020-02-13 20:43:58 UTC
backported to 19.05.x for 19.05.08
Comment 16 Hayley Pelham 2020-02-20 23:17:35 UTC
Can this please be rebased for 18.11.x?
Comment 17 Jonathan Druart 2020-02-21 09:09:39 UTC
Created attachment 99359 [details] [review]
[18.11.x] Bug 23064: Use Koha::Subscription in ModSubscription

We must use Koha::Subscription instead of raw SQL.
It will fix issue with default and integer values.

Test plan:
Edit a subscription and set number of issues = "f"
Save
=> Without this patch there is a SQL error in the log:
  Incorrect integer value: 'f' for column 'numberlength'
=> With this patch the other changes are effective.

Note: We also could change the type attribute of the input to "number",
to have a client-side check

Also, the return value of ModSuggestion is never used, so we are safe
with that.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
The <strict_sql_modes> flag must be present and set to 1 in the <config> section of koha-conf.xml

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Jonathan Druart 2020-02-21 09:16:26 UTC
(In reply to Hayley Mapley from comment #16)
> Can this please be rebased for 18.11.x?

Done, see last patch.
Worth noting, when I tested it I saw that ergonomic issue: https://snipboard.io/tHK4ao.jpg
Comment 19 Hayley Pelham 2020-02-23 23:52:08 UTC
Backported to 18.11.x for 18.11.15.