[Bug 38411] New: When adding multiple items on receive, mandatory fields are not checked
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Bug ID: 38411 Summary: When adding multiple items on receive, mandatory fields are not checked Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: adolfo.rodriguez@xercode.es QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #1 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 174274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174274&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Assignee|koha-bugs@lists.koha-commun |adolfo.rodriguez@xercode.es |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net Status|Needs Signoff |Failed QA --- Comment #2 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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/ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174274|0 |1 is obsolete| | --- Comment #3 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 175245 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175245&action=edit Bug 38411: Check mandatory item fields when creating several items ordering or receiving. Update occurrences of _() to __() See Bug 38492 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #175245|0 |1 is obsolete| | --- Comment #4 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 175246 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175246&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #5 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #6 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Comment on attachment 175246 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=175246 Bug 38411: Check mandatory item fields when creating several items ordering or receiving. Review of attachment 175246: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=38411&attachment=175246) ----------------------------------------------------------------- ::: 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #7 from Victor Grousset/tuxayo <victor@tuxayo.net> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #175246|0 |1 is obsolete| | --- Comment #8 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- Created attachment 179067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179067&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #9 from Adolfo Rodríguez Taboada <adolfo.rodriguez@xercode.es> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 PTFS Europe Sandboxes <sandboxes@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179067|0 |1 is obsolete| | --- Comment #10 from PTFS Europe Sandboxes <sandboxes@ptfs-europe.com> --- Created attachment 179479 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179479&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Laurence Rault <laurence.rault@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |laurence.rault@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179479|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 180228 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=180228&action=edit 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> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Always glad to see bugs like this fixed! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |25.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 --- Comment #13 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable CC| |fridolin.somers@biblibre.co | |m Version(s)|25.05.00 |25.05.00,24.11.06 released in| | --- Comment #14 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 24.11.x for 24.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Jesse Maseto <jesse@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jesse@bywatersolutions.com --- Comment #15 from Jesse Maseto <jesse@bywatersolutions.com> --- Merge conflicts with 24.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to stable |Needs documenting --- Comment #16 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38411 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED --- Comment #17 from David Nind <david@davidnind.com> --- Bug fix, no changes required to the manual. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org