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.
Same as bug 22812, we must use Koha::Subscription here.
Created attachment 95610 [details] [review] Bug 24010: 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.
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.
Sorry, I read the description too quickly!
Created attachment 95717 [details] [review] Bug 24010: DB Changes
Created attachment 95718 [details] [review] Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount
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.
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?
(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.
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
Created attachment 95847 [details] [review] Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount (second try)
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.
Does it block the form submission?
Created attachment 95935 [details] [review] Bug 24010: Enforce integer values for staffdisplaycount and opacdisplaycount (second try)
(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?
No, it does not work.
Created attachment 134985 [details] [review] Bug 24010: DB Changes
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.
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?
Created attachment 135092 [details] [review] Bug 24010: DB Changes Amended-patch: adjusted to new atomic update format
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>
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>
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>
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>
Thanks, Owen!
Created attachment 137175 [details] [review] Bug 24010: (QA follow-up) Correct COMMENT syntax Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.04
pushed to 21.11.x for 21.11.11
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document, marking resolved.