|
Description
Nick Clemens (kidclamp)
2019-06-06 11:39:15 UTC
Created attachment 90386 [details] [review] Bug 23064: Add tests This param list is scary! Created attachment 90387 [details] [review] Bug 23064: Fix mod subscription with strict sql modes 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. 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. 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 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. Switching back to NSO. 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? 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. 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 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> Reads nice and works well! Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x for 19.11.03 backported to 19.05.x for 19.05.08 Can this please be rebased for 18.11.x? 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> (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 Backported to 18.11.x for 18.11.15. |
