Summary: | Add validation for replacement price in item input form | ||
---|---|---|---|
Product: | Koha | Reporter: | Kati Sillgren <kati.sillgren> |
Component: | Cataloging | Assignee: | Leo Stoyanov <leo.stoyanov> |
Status: | Failed QA --- | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | minor | ||
Priority: | P5 - low | CC: | anneli.osterman, david, leo.stoyanov, lucas, m.de.rooy, oleonard, william.lavoie |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This fixes the item input form (and the batch item modification form) so that the "v - Cost, replacement price" field is now validated - only use numbers with a period should be entered, for example, 10.99. Previously, if a comma was entered, it would silently fail and not add any value when submitted.
|
Version(s) released in: | |
Circulation function: | |||
Attachments: |
Bug 38839: Added validation for replacementprice in items form and batch mod form.
Bug 38839: Add validation for replacement price in item and batch modification forms Bug 38839: (follow-up) Use "for example" instead of "e.g." Bug 38839: Add validation for replacement price in item and batch modification forms Bug 38839: Added "decimal" validation for replacement price in item and batch modification forms. Bug 38839: (follow-up) Removed maxLength check for replacementPrice field. |
Description
Kati Sillgren
2025-01-07 11:14:46 UTC
Hi Kati, you are right that this should give a warning instead of failing silently. I have adjusted the bug description a little. Created attachment 176279 [details] [review] Bug 38839: Added validation for replacementprice in items form and batch mod form. Validation has been added for the batch mod form because it also takes replacement price as input and lacked validation. To recreate: 1. On the dashboard, make sure to select "Search catalog". 2. Search in the catalog, select anything, click "Edit", and select "Manage items". 3. Click "Actions" on the item in the table, select "Edit", and input an invalid "Cost, replacement price" (e.g., 1,00). 4. Click "Save changes" and the form will submit but with no price shown in the table on the next page. 5. Apply the patch and repeat steps 1-4. A red error message below the input will appear, notifying the user that only numbers and periods are allowed. The user will not be able to submit the form. 6. The same bug and fix should apply to the batch mod form under "Cataloging" and "Batch item modification". Created attachment 176285 [details] [review] Bug 38839: Add validation for replacement price in item and batch modification forms Adds validation for the replacement price on the item input and batch modification forms. Test plan: 1. Search for a record in the staff interface, select any record with an item, click "Edit", and select "Manage items". 2. Click "Actions" on the item in the table, select "Edit", and input an invalid "v - Cost, replacement price" (e.g., 1,00). 3. Click "Save changes" and the form will submit but with no price shown in the table on the next page. 4. Apply the patch, update the CSS (yarn build) and restart everything (restart_all). 5. Repeat steps 1-3. A red error message will now appear below the input field, notifying the user that only numbers and periods are allowed. The user will not be able to submit the form. 6. The same change was also made to the batch modification form under Cataloging > Batch item modification. Signed-off-by: David Nind <david@davidnind.com> Created attachment 176286 [details] [review] Bug 38839: (follow-up) Use "for example" instead of "e.g." Signed-off-by: David Nind <david@davidnind.com> Hi Leo. I amended the commit message slightly - see the commit message guidelines https://wiki.koha-community.org/wiki/Commit_messages - needed to run yarn build for the CSS changes to take effect - "Test plan" instead of "To recreate" - some wording changes to make a little clearer (I hope) I also added a follow-up to change "e.g." to "for example" (see the proposed change in the Terminology guidelines https://wiki.koha-community.org/wiki/Terminology#E). And I added a draft release note. David Hi David, Thank you for taking the time to correct my mistakes and improve the clarity of the commit message. I appreciate your thoroughness and the helpful suggestions regarding the guidelines. Best regards, Leo (In reply to Leo Stoyanov from comment #6) > Hi David, > > Thank you for taking the time to correct my mistakes and improve the clarity > of the commit message. I appreciate your thoroughness and the helpful > suggestions regarding the guidelines. > > Best regards, > Leo Thanks Leo for the patch! There is always a lot to learn with Koha! David Hi Leo, Not sure if I am going to give you a first FQA. But just view it as a learning experience :) Two points of attention: [1] Invalid Replacement Price => Seems not in harmony with our current 'case policy'. Only first word needs uc? [2] Validation + if (!/^[0-9.]+$/.test(replacementprice)) { + var isValid = /^[0-9.]*$/.test(value); This allows for a price like ... Shouldnt we have some common way to check a monetary value ? How does Koha do that elsewhere? Too bad that HTML(5) does not seem to have a uniform way for doing this.. Similar question for $("input[name='items.replacementprice']").on("input", function () { Should we check on input or on change btw? Actaully... reading Marcel's comment: If the validator works on this form, we do have something like a standard solution: See Bug 34169 Created attachment 176403 [details] [review] Bug 38839: Add validation for replacement price in item and batch modification forms Adds validation for the replacement price on the item input and batch modification forms. This new patch utilizes the standard jQuery "decimal" validator to check if monetary input is appropriatly formatted. Test plan: 1. Search for a record in the staff interface, select any record with an item, click "Edit", and select "Manage items". 2. Click "Actions" on the item in the table, select "Edit", and input an invalid "v - Cost, replacement price" (for example, 1,00). 3. Click "Save changes" and the form will submit but with no price shown in the table on the next page. 4. Apply the patch, update the CSS (yarn build) and restart everything (restart_all). 5. Repeat steps 1-3. A red error message will now appear to the right of the input field, notifying the user that only numbers and periods are allowed. The user will not be able to submit the form. 6. The same change was also made to the batch modification form under Cataloging > Batch item modification. Hi Leo, please check the attached patches! I think we might be missing one and the sequence is not quite right. Maybe good to know: when you have the exact same commit line, one patch will overwrite the other when using git bz. As you are looking for a sign-off, maybe squashing to avoid add/remove of lines would be good. Thanks for taking this one on! Created attachment 176407 [details] [review] Bug 38839: Added "decimal" validation for replacement price in item and batch modification forms. Adds validation for the replacement price on the item input and batch modification forms. This new patch utilizes the standard jQuery "decimal" validator to check if monetary input is appropriatly formatted. Test plan: 1. Search for a record in the staff interface, select any record with an item, click "Edit", and select "Manage items". 2. Click "Actions" on the item in the table, select "Edit", and input an invalid "v - Cost, replacement price" (for example, 1,00). 3. Click "Save changes" and the form will submit but with no price shown in the table on the next page. 4. Apply the patch, update the CSS (yarn build) and restart everything (restart_all). 5. Repeat steps 1-3. A red error message will now appear to the right of the input field, notifying the user that only numbers and periods are allowed. The user will not be able to submit the form. 6. The same change was also made to the batch modification form under Cataloging > Batch item modification. sha1 error Only the appropriate patch should apply now. I tested this in PTFS Europe's sandbox and replacement price does ask to use the valid format if you enter price like 20,00: Please add amount in valid format: 0.00 But if I change the price for example to 20.00 it says Please enter no more than 0 characters. which is a bit troublesome if the price is more than 0 characters, like in most cases it is. :) Created attachment 176839 [details] [review] Bug 38839: (follow-up) Removed maxLength check for replacementPrice field. I tested this again and it does not complain about the maximum length anymore. You can add amounts that are 999999.99 but if you add amount of 1 000 000 (one milloin or more) the save process crashes. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Out of range value for column 'replacementprice' at row 1 at /kohadevbox/koha/Koha/Object.pm line 172 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 I checked from the Koha schema that replacementprice field's type is DECIMAL and size is 8,2. I had to google what that means and if I understood correctly, the field accepts 6 digits before the . and two after it. So, it kind of works (milloin has 7 digits before the .) but should it give more descriptive error message? Of course, it is highly unlikely that someone adds that big of a replacement price but I think it should not just "crash". :) I am not sure if the last patch is correct removing the maxlength. This value is supposed to be pulled from the frameworks and probably we should fix the value for the fields there instead. As right now it's removing the feature to restrict the length? (haven't tested) Katrin, yes, you can add as big number as you please (for example 900 000 000) and you can try to save it. But I guess the save crashes because the database does not accept the big/long number. (In reply to Anneli Österman from comment #19) > Katrin, yes, you can add as big number as you please (for example 900 000 > 000) and you can try to save it. But I guess the save crashes because the > database does not accept the big/long number. I believe without the last fix it should be able to set it to 10 in frameworks (8+2 decimal?) and that should create a better error message. |