Bug 23064 - Cannot edit subscription with strict SQL modes turned on
Summary: Cannot edit subscription with strict SQL modes turned on
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2019-06-06 11:39 UTC by Nick Clemens
Modified: 2021-06-14 21:28 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 23064: Add tests (1.44 KB, patch)
2019-06-06 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23064: Fix mod subscription with strict sql modes (5.96 KB, patch)
2019-06-06 17:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23064: Use Koha::Subscription in ModSubscription (4.73 KB, patch)
2019-11-20 13:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23064: Use Koha::Subscription in ModSubscription (4.89 KB, patch)
2020-01-22 20:55 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 23064: Use Koha::Subscription in ModSubscription (4.94 KB, patch)
2020-01-22 22:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[18.11.x] Bug 23064: Use Koha::Subscription in ModSubscription (4.87 KB, patch)
2020-02-21 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 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 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.