Bug 38411 - When adding multiple items on receive, mandatory fields are not checked
Summary: When adding multiple items on receive, mandatory fields are not checked
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Adolfo Rodríguez Taboada
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-11-08 15:20 UTC by Adolfo Rodríguez Taboada
Modified: 2025-03-19 14:45 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 38411: Check mandatory item fields when creating several items ordering or receiving. (2.15 KB, patch)
2024-11-08 15:24 UTC, Adolfo Rodríguez Taboada
Details | Diff | Splinter Review
Bug 38411: Check mandatory item fields when creating several items ordering or receiving. (1.44 KB, patch)
2024-12-05 15:04 UTC, Adolfo Rodríguez Taboada
Details | Diff | Splinter Review
Bug 38411: Check mandatory item fields when creating several items ordering or receiving. (2.18 KB, patch)
2024-12-05 15:12 UTC, Adolfo Rodríguez Taboada
Details | Diff | Splinter Review
Bug 38411: Check mandatory item fields when creating several items ordering or receiving. (2.14 KB, patch)
2025-03-07 11:41 UTC, Adolfo Rodríguez Taboada
Details | Diff | Splinter Review
Bug 38411: Check mandatory item fields when creating several items ordering or receiving. (2.19 KB, patch)
2025-03-19 14:45 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adolfo Rodríguez Taboada 2024-11-08 15:20:14 UTC
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.
Comment 1 Adolfo Rodríguez Taboada 2024-11-08 15:24:15 UTC
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
Comment 2 Victor Grousset/tuxayo 2024-11-28 04:25:19 UTC
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/
Comment 3 Adolfo Rodríguez Taboada 2024-12-05 15:04:19 UTC
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
Comment 4 Adolfo Rodríguez Taboada 2024-12-05 15:12:56 UTC
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
Comment 5 Adolfo Rodríguez Taboada 2024-12-05 15:17:36 UTC
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 6 Victor Grousset/tuxayo 2024-12-18 23:21:31 UTC
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
Comment 7 Victor Grousset/tuxayo 2024-12-19 00:52:11 UTC
(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.
Comment 8 Adolfo Rodríguez Taboada 2025-03-07 11:41:11 UTC
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
Comment 9 Adolfo Rodríguez Taboada 2025-03-07 11:48:31 UTC
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.
Comment 10 PTFS Europe Sandboxes 2025-03-19 14:45:08 UTC
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>