Bug 24010 - Number of issues to display to staff accepts non-integer values
Summary: Number of issues to display to staff accepts non-integer values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-11 14:48 UTC by Joonas Kylmälä
Modified: 2023-06-08 22:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds validation to the subscription entry form to check that the values for these fields are numbers: - Number of issues to display to staff - Number of issues to display to the public
Version(s) released in:
22.11.00, 22.05.04, 21.11.11


Attachments
Bug 24010: Use Koha::Subscription in ModSubscription (4.73 KB, patch)
2019-11-20 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: DB Changes (2.82 KB, patch)
2019-11-22 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount (2.97 KB, patch)
2019-11-22 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount (second try) (1.97 KB, patch)
2019-11-27 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount (second try) (1.98 KB, patch)
2019-12-03 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: DB Changes (2.95 KB, patch)
2022-05-13 17:21 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24010: Number of issues to display to staff accepts non-integer values (4.09 KB, patch)
2022-05-13 17:22 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 24010: DB Changes (2.78 KB, patch)
2022-05-18 07:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: Number of issues to display to staff accepts non-integer values (4.14 KB, patch)
2022-06-25 11:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 24010: DB Changes (2.83 KB, patch)
2022-06-25 11:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 24010: Number of issues to display to staff accepts non-integer values (4.22 KB, patch)
2022-07-04 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: DB Changes (2.90 KB, patch)
2022-07-04 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24010: (QA follow-up) Correct COMMENT syntax (2.32 KB, patch)
2022-07-05 14:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2019-11-11 14:48:53 UTC
When adding a new serial subscription in the page /cgi-bin/koha/serials/subscription-add.pl one can add to the field "Number of issues to display to staff" a non-integer value like "f" and then when you save this new subscription and go to the "Subscriptions" tab in /cgi-bin/koha/catalogue/detail.pl?biblionumber=XXXX you get the following error in plack-error.log:

>DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [for Statement "SELECT   serialid,serialseq, status, planneddate, publisheddate, notes
>                        FROM     serial
>                        WHERE    subscriptionid = ?
>                        AND      status IN (2,4,41,42,43,44)
>                        ORDER BY publisheddate DESC LIMIT 0,f
>                " with ParamValues: 0=2] at /home/koha/kohaclone/C4/Serials.pm line 784.
Comment 1 Jonathan Druart 2019-11-20 11:22:54 UTC
Same as bug 22812, we must use Koha::Subscription here.
Comment 2 Jonathan Druart 2019-11-20 11:28:39 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2019-11-20 12:05:34 UTC
This fix doesn't solve the original problem reported plus I'm not able to reproduce the error described in the patch:

> Incorrect integer value: 'f' for column 'numberlength'

When I edit "Subscription length" field (I assume this is the "number of issues") to "f" Regardless whether this patch is applied I only get in plack-error.log

> Use of uninitialized value $sub_on in concatenation (.) or string at /home/koha/kohaclone/serials/subscription-add.pl line 113.
> Argument "f" isn't numeric in numeric ne (!=) at /home/koha/kohaclone/serials/subscription-add.pl line 279.
Comment 4 Jonathan Druart 2019-11-20 13:01:02 UTC
Sorry, I read the description too quickly!
Comment 5 Jonathan Druart 2019-11-22 11:34:33 UTC
Created attachment 95717 [details] [review]
Bug 24010: DB Changes
Comment 6 Jonathan Druart 2019-11-22 11:34:37 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2019-11-22 11:37:18 UTC
The second patch does not work as expected. I am struggling to make the validation work on the form.

It seems that the validate jQuery function collides with the validated class put on the form.

Owen, is it clear for you what we should do in this case?
I have also tried type="number" but it does not block form submission.
Comment 8 Joonas Kylmälä 2019-11-22 11:41:01 UTC
Instead of failing the DB change could we just make the non-integer values NULL? The non-integer values never worked so it should not be a problem to modify them to NULL?
Comment 9 Jonathan Druart 2019-11-22 11:46:45 UTC
(In reply to Joonas Kylmälä from comment #8)
> Instead of failing the DB change could we just make the non-integer values
> NULL? The non-integer values never worked so it should not be a problem to
> modify them to NULL?

I can use an SQL regex to set to NULL non-numeric values, yes.
Comment 10 Jonathan Druart 2019-11-27 12:48:41 UTC
Owen, you suggested me to use the new version of jQuery Validate (from bug 24089), but it still does not work for me.

From IRC:
<Joubu> oleonard: does it work with the new version?
<oleonard> Yes. I suggest you favor the "validated" class and add 'data-rule-number="true"' to the inputs
Comment 11 Jonathan Druart 2019-11-27 12:49:28 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2019-12-02 15:28:00 UTC
It works for me if I change the input type to "number." I wouldn't have expected that to be necessary, but it's not an incorrect change to make.
Comment 13 Jonathan Druart 2019-12-03 09:00:19 UTC
Does it block the form submission?
Comment 14 Jonathan Druart 2019-12-03 09:00:45 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2019-12-07 10:56:17 UTC
(In reply to Jonathan Druart from comment #14)
> Created attachment 95935 [details] [review] [review]
> Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount
> (second try)

Should this be NSO?
Comment 16 Jonathan Druart 2019-12-10 11:18:36 UTC
No, it does not work.
Comment 17 Owen Leonard 2022-05-13 17:21:55 UTC
Created attachment 134985 [details] [review]
Bug 24010: DB Changes
Comment 18 Owen Leonard 2022-05-13 17:22:00 UTC
Created attachment 134986 [details] [review]
Bug 24010: Number of issues to display to staff accepts non-integer values

This patch modifies the subscription entry form so that it will perform
a check on the staffdisplaycount and opacdisplaycount fields before
proceding to the second step. It verifies that the values are numeric.

The changes are made in the style of the existing form validation, which
should be rewritten to either use the validation plugin or to peform
checks in a way that all checks are run before warning the user.
However, this smaller change will work in the meantime.

To test, apply the patch and go to Serials -> New subscription.

- Fill out the form with at least the required fields, but put something
  other than a number if the "Number of issues to display to staff" and
  "Number of issues to display to the public" with non-numeric characters.
- When you click the "Next" button you should get an error message,
  "Number of issues to display to staff must be a number."
- Correct the issues to display to staff field and submit again.
- You should get a different error message, "Number of issues to display
  to the public must be a number."
- Correct this field and you should be able to proceed to the next step.
Comment 19 Owen Leonard 2022-05-13 17:34:06 UTC
The atomicupdate file needs to be updated to use our current syntax, and I think it needs this addition:

If you've got existing subscriptions but you didn't specify a number for staffdisplaycount and opacdisplaycount, those columns will contain an empty string.

The database update checks to see if the columns can be updated to INT and aborts if not.  But it should be reasonable to start by updating empty entries to make them NULL isn't it?

update subscriptions set staffdisplaycount = NULL where staffdisplaycount = '';

update subscriptions set opacdisplaycount = NULL where opacdisplaycount = '';

Then the db update can proceed automatically even if there are existing blank values.

If that's correct, can someone take a crack at updating the atomicupdate file?
Comment 20 Jonathan Druart 2022-05-18 07:51:09 UTC
Created attachment 135092 [details] [review]
Bug 24010: DB Changes

Amended-patch: adjusted to new atomic update format
Comment 21 David Nind 2022-06-25 11:06:46 UTC
Created attachment 136538 [details] [review]
Bug 24010: Number of issues to display to staff accepts non-integer values

This patch modifies the subscription entry form so that it will perform
a check on the staffdisplaycount and opacdisplaycount fields before
proceding to the second step. It verifies that the values are numeric.

The changes are made in the style of the existing form validation, which
should be rewritten to either use the validation plugin or to peform
checks in a way that all checks are run before warning the user.
However, this smaller change will work in the meantime.

To test, apply the patch and go to Serials -> New subscription.

- Fill out the form with at least the required fields, but put something
  other than a number if the "Number of issues to display to staff" and
  "Number of issues to display to the public" with non-numeric characters.
- When you click the "Next" button you should get an error message,
  "Number of issues to display to staff must be a number."
- Correct the issues to display to staff field and submit again.
- You should get a different error message, "Number of issues to display
  to the public must be a number."
- Correct this field and you should be able to proceed to the next step.

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2022-06-25 11:06:51 UTC
Created attachment 136539 [details] [review]
Bug 24010: DB Changes

Amended-patch: adjusted to new atomic update format

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 Jonathan Druart 2022-07-04 13:09:09 UTC
Created attachment 137060 [details] [review]
Bug 24010: Number of issues to display to staff accepts non-integer values

This patch modifies the subscription entry form so that it will perform
a check on the staffdisplaycount and opacdisplaycount fields before
proceding to the second step. It verifies that the values are numeric.

The changes are made in the style of the existing form validation, which
should be rewritten to either use the validation plugin or to peform
checks in a way that all checks are run before warning the user.
However, this smaller change will work in the meantime.

To test, apply the patch and go to Serials -> New subscription.

- Fill out the form with at least the required fields, but put something
  other than a number if the "Number of issues to display to staff" and
  "Number of issues to display to the public" with non-numeric characters.
- When you click the "Next" button you should get an error message,
  "Number of issues to display to staff must be a number."
- Correct the issues to display to staff field and submit again.
- You should get a different error message, "Number of issues to display
  to the public must be a number."
- Correct this field and you should be able to proceed to the next step.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2022-07-04 13:09:15 UTC
Created attachment 137062 [details] [review]
Bug 24010: DB Changes

Amended-patch: adjusted to new atomic update format

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2022-07-04 13:09:35 UTC
Thanks, Owen!
Comment 26 Tomás Cohen Arazi 2022-07-05 14:09:30 UTC
Created attachment 137175 [details] [review]
Bug 24010: (QA follow-up) Correct COMMENT syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2022-07-05 14:35:44 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 28 Lucas Gass 2022-08-12 15:19:21 UTC
Backported to 22.05.x for 22.05.04
Comment 29 Arthur Suzuki 2022-08-16 09:33:29 UTC
pushed to 21.11.x for 21.11.11
Comment 30 Victor Grousset/tuxayo 2022-08-19 20:35:15 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.