This was fixed Bug #34109 for the case where only one item is added (function addItem) When adding multiple items, the function addMulti is used and it doesn't check mandatory fields.
Created attachment 174274 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. This patch adds a check of mandatory item fields when adding several items Test plan: 1 We need mandatory subfields for items in the ACQ framework and an open basket configured to create items on order or a closed basket configured to create items on receiving an order 2 In both cases, in the form to create items check that there are mandatory fields. 3 Click the Add multiple items button, write a number and then add. 4 Check that the items have been added. 5 Apply patch, restart services 6 Repeat step 3 Now it doesn't add the items and shows an alert regarding the empty mandatory fields
Hola, thanks for finding out that the multiple items case wasn't covered :o And thanks for the fix :) At first I though adding an alert() would be a problem, since we are phasing them out since years: https://wiki.koha-community.org/wiki/Interface_patterns#Errors_and_messages But here it's a bit different. There is already an alert() just above in addItem(). So the actual issue is the perfect duplication of code. Almost perfect. I think it we should keep __() for translation and not switch to _(). Unless there is something warranting switching (I don't remember if _() actually works in our JS files) If you can and want to improve the UX by migrating to proper error messages as shown in the wiki, feel free to do so. Here or in a follow-up ticket depending if you feel that's too much changes. And if not it's fine to "add" an alert() because it's not really added if it's from the single item case already there with the code not duplicated. Feel free if you have questions, here or in https://chat.koha-community.org/
Created attachment 175245 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. Update occurrences of _() to __() See Bug 38492
Created attachment 175246 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. This patch adds a check of mandatory item fields when adding several items Test plan: 1 We need mandatory subfields for items in the ACQ framework and an open basket configured to create items on order or a closed basket configured to create items on receiving an order 2 In both cases, in the form to create items check that there are mandatory fields. 3 Click the Add multiple items button, write a number and then add. 4 Check that the items have been added. 5 Apply patch, restart services 6 Repeat step 3 Now it doesn't add the items and shows an alert regarding the empty mandatory fields
In Bug 38492 the function addItem was changed to replace _() with __() to make the string translatable as _() doesn't work. I have resubmitted the bug doing the same in addMulti As for the alert(), it is also used in cataloging_additem.js to check mandatory fields. I think it would be better to make or find a bug to change how Koha warns the user that there are mandatory fields empty.
Comment on attachment 175246 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. Review of attachment 175246 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/js/additem.js @@ -45,5 @@ > } > > function addMulti( count, node, unique_item_fields){ > - var index = $(node).closest("div").attr('id'); > - var countItemsBefore = $("#items_list tbody tr").length; `var countItemsBefore` seems to have been removed by mistake. As the variable is still used after. Did it somehow still work? :o
(In reply to Adolfo Rodríguez Taboada from comment #5) > In Bug 38492 the function addItem was changed to replace _() with __() to > make the string translatable as _() doesn't work. I have resubmitted the bug > doing the same in addMulti Good find. So that's how it was changed between your patch submission and my review. ------------- > As for the alert(), it is also used in cataloging_additem.js to check > mandatory fields. I think it would be better to make or find a bug to change > how Koha warns the user that there are mandatory fields empty. The alert itself isn't a blocking issue: (In reply to Victor Grousset/tuxayo from comment #2) > At first I though adding an alert() would be a problem, since we are phasing > them out since years: > https://wiki.koha-community.org/wiki/Interface_patterns#Errors_and_messages > > But here it's a bit different. There is already an alert() just above in > addItem(). So the actual issue is the [almost] perfect duplication of code. So there is still that duplication and now it's perfect with the switch to __(). And we even had an example here of how duplicating code lead to a change in only one of the duplicate instances. If the common code was extracted to a subroutine, then when I applied the patch it would have conflicted with Bug 38492 having landed in the meantime. And I would have seen the difference and applied the i18n change.
Created attachment 179067 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. This patch adds a check of mandatory item fields when adding several items Test plan: 1 We need mandatory subfields for items in the ACQ framework and an open basket configured to create items on order or a closed basket configured to create items on receiving an order 2 In both cases, in the form to create items check that there are mandatory fields. 3 Click the Add multiple items button, write a number and then add. 4 Check that the items have been added. 5 Apply patch, restart services 6 Repeat step 3 Now it doesn't add the items and shows an alert regarding the empty mandatory fields
I have sent a new version of the fix, as in the previous one I had removed the definition of the variable countItemsBefore by mistake.
Created attachment 179479 [details] [review] Bug 38411: Check mandatory item fields when creating several items ordering or receiving. This patch adds a check of mandatory item fields when adding several items Test plan: 1 We need mandatory subfields for items in the ACQ framework and an open basket configured to create items on order or a closed basket configured to create items on receiving an order 2 In both cases, in the form to create items check that there are mandatory fields. 3 Click the Add multiple items button, write a number and then add. 4 Check that the items have been added. 5 Apply patch, restart services 6 Repeat step 3 Now it doesn't add the items and shows an alert regarding the empty mandatory fields Signed-off-by: Laurence <laurence.rault@biblibre.com>