Created attachment 46093 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields Sponsored by: Middletown Township Public Library (http://www.mtpl.org)
I'd suggest using <input type="number"> (like http://www.wufoo.com/html5/types/7-number.html) for the number-of-copies control. It'll add useful UI and make validation easier. I'd also suggest using the humanMsg API for error messages (like Rancor does), though that's more a matter of opinion.
Created attachment 46439 [details] [review] Bug 14752 - (QA Followup) Use input type="number" and small update to error modal
Created attachment 46832 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields Sponsored by: Middletown Township Public Library (http://www.mtpl.org) Signed-off-by: Briana <brianagreally@gmail.com>
Created attachment 46833 [details] [review] Bug 14752 - (QA Followup) Use input type="number" and small update to error modal Signed-off-by: Briana <brianagreally@gmail.com>
QA comments: 1/ (window.MSG_ADDITEM_JS_UPDATEITEM || "Update item ") If the string is not defined, there is something wrong in the code, we should not provide a hardcoded string 2/ When I click on the "add multiple" button, I get a JS error "TypeError: document.f is undefined" And the behavior if weird: I get the "Invalid number of copies" even if I enter "2", it may come from the JS error. 3/ If I am not wrong, the barcode could be sent, the next one will be generated.
Created attachment 47223 [details] [review] Bug 14752 [QA followup] Remove reference to document.f Remove backup hardcoded string for update item button
(In reply to Jonathan Druart from comment #6) > QA comments: > 1/ (window.MSG_ADDITEM_JS_UPDATEITEM || "Update item ") > If the string is not defined, there is something wrong in the code, we > should not provide a hardcoded string Fixed! I copied that from existing code and fixed there too > 2/ When I click on the "add multiple" button, I get a JS error "TypeError: > document.f is undefined" > And the behavior if weird: I get the "Invalid number of copies" even if I > enter "2", it may come from the JS error. This was a copy paste error, removed the code. I can't replicate the 'Invalid number' though, hoping fixing the error clears that for you. I checked firefox and chromium on Ubuntu > 3/ If I am not wrong, the barcode could be sent, the next one will be > generated. I am not sure what you mean. If barcode is listed in the UniqueItemFields (default) I don't pass barcode into generated records but I do leave it in the form. If it isn't in UniqueItemFields it is passed as is (copied multiple times) and left in the form. It seems tricky because the items are not created until you hit save at the bottom, so incrementing could cause conflicts with autoincrement if others are cataloging at the same time. Thoughts?
Nick, could you double check, I don't get the modal. + <p>Please enter a <b>number</b>, greater than 1</p> I am able to enter 1, without a warning message.
Created attachment 47252 [details] [review] Bug 14752 - [QA Followup] Correct error message Message changed from 'greater than 1' to 'greater than or equal to 1'
(In reply to Jonathan Druart from comment #9) > I am able to enter 1, without a warning message. I think that is the correct behaviour, I altered the error acordingly
Nick, I am sorry but I don't understand why 1/ the barcode is not kept (and auto generated) 2/ you are not using the same method as in additem.tt Also: <p>Please enter a <b>number</b>, greater than or equal to 1</p> Don't add tagin the middle of a sentence, it makes translation harder.
Created attachment 47477 [details] [review] Bug 14752 - increment barcodes when adding multiples if present and uniqueness enforced To test: 1 - Enter a barcode value that ends in a number 2 - Attempt to add multiple items 3 - Items should eb added with increasing barcodes 4 - Enter a barcode not ending in a number 5 - Attempt to add multiple items 6 - You should get an error modal
Hi Nick, I am sorry but I don't understand your last patch. I was referring to this code from additem.pl: 455 # if autoBarcode is set to 'incremental', calculate barcode... 456 if ( C4::Context->preference('autoBarcode') eq 'incremental' ) { 457 $record = _increment_barcode($record, $frameworkcode); 458 }
Ha yes ok, got it! You the previous item is not inserted, then the barcode cannot be generated. Maybe we could display a value in the barcode field to tell the user it will be auto-generated on saving. I would like to avoid a JS function to calculate the next barcode, we already have perl subroutines to do this job. Katrin, could I get your opinion please?
Auto-generating on saving sounds good to me - showing a note if AutoBarcode is active? The other alternative would probably be to add the item immediately in the background, but not sure that's worth the trouble right now.
Created attachment 48254 [details] [review] Bug 14752 - [QA followup] Auto-Generate barcode if left blank and autoBarcode is set Adds a warning/notice to item form and modal that barcodes will be auto-filled using syspref if autoBarcode is turned on Fills in barcodes if passed in blank
Okay, I attached a patch that 'works' I used C4::Barcodes for generating the next barcode, however, this is apparently broken for annual and hbyymmdd and the latter indicates it is deprecated Do we patch C4::Barcodes, or use ValueBuilder.pm basically copying code from value_builder.pl? Opinions? (In reply to Jonathan Druart from comment #15) > Ha yes ok, got it! > You the previous item is not inserted, then the barcode cannot be generated. > Maybe we could display a value in the barcode field to tell the user it will > be auto-generated on saving. > I would like to avoid a JS function to calculate the next barcode, we > already have perl subroutines to do this job. > > Katrin, could I get your opinion please?
Created attachment 51481 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields 7 - Enable autoBarcode for various formats, ensure you are warned that barcodes will be generated, and ensure they are generated correctly Sponsored by: Middletown Township Public Library (http://www.mtpl.org)
Patches rebased and squashed, added dependency
When you empty the field "UniqueItemFields" but that you have AcqCreateItem="Create an item when placing an order", you should have the items part present when you're placing an order ... but this is not the case. You have only "Catalog details" and "Accounting details" on neworderempty.pl and you can't change manualy the quantity in Quantity field. So it's a real problem !! In fact, in my libreary, we're creating items when placing an order but without barcode. Barcode are sticked on physical items at the reception. But, we have UniqueItemFields=barcode, because, I thought that there's a check at any moment of the process of the acquisition (even on reception). It's not really clear. If this is not the case, we can left UniqueItemFields empty. (but we need to be able to add items informations in the order ;-) ) If UniqueItemFields is checked not only when you place an order but also after, i think that it's a pity to add a message that blocks the user when barcode is empty when you place an order. Last comment, as an user, I would prefer to be abble to add the number of items, without to click on a button before. For exemple,could we have a button "Place item(s)" and then a box where you can type the number with 1 as a default value ? It will be really much easier for people that enter orders all the day :)
(In reply to Koha Team Lyon 3 from comment #21) > When you empty the field "UniqueItemFields" but that you have > AcqCreateItem="Create an item when placing an order", you should have the > items part present when you're placing an order ... but this is not the case. > You have only "Catalog details" and "Accounting details" on neworderempty.pl > and you can't change manualy the quantity in Quantity field. > So it's a real problem !! I don't see this in my testing - I matched my settings and still have the form to add items, can you recheck? > But, we have UniqueItemFields=barcode, because, I thought that there's a > check at any moment of the process of the acquisition (even on reception). > It's not really clear. You should still be able to add the items without barcodes when barcode is listed in UniqueitemFields, it should only trigger if you try to add two of the same barcode and should only be a block when saving the order > If UniqueItemFields is checked not only when you place an order but also > after, i think that it's a pity to add a message that blocks the user when > barcode is empty when you place an order. The messages I add shuold not block a user from ordering multiples with no barcode, it should only warn when creating multiple that, if inetered, a barcode will not be copied to each item. I can easily convert the message to be a static alert on the page instead of a popup when creating multiple, I just wanted to be super clear about why some fields wouldn't be copied > Last comment, as an user, I would prefer to be abble to add the number of > items, without to click on a button before. > For exemple,could we have a button "Place item(s)" and then a box where you > can type the number with 1 as a default value ? > It will be really much easier for people that enter orders all the day :) T his is more to prevent accidentally adding multiples and mimics the setup on adding items while cataloguing. I would like to hear a QA opinion on this one Moving back to needs sign-off to get more comments, but let me know your thoughts on the above.
I have tested it again today. It's ok, I haven't the same issue when emptying "UniqueItemFields". There's problem on sandboxes yesterday, so it can come from that. >The messages I add shuold not block a user from ordering multiples with no barcode, it should only warn when creating multiple that, if inetered, a barcode will not be copied to each item. Yeah, sorry, I don't want to mean block, but this add a warn and an action for users who don't add barcode when placing an order. It's 2 different ways to act but If you want to add items when placing orders without the barcode because you stick it after when you have the physical document, this warning can be a bit annoying. Perhaps, it's really important to warn when you have autoBarcode. It's just a thought... This enhancement will be very usefull :) It's just that I'm wondering if there's not a easier way that can suit every users.
Created attachment 52083 [details] [review] Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead - Uses a dialog box to warn of unique fields not copying, dialog was in place for barcode generation so removed the extar modal completey - Fixes a problem when barcode was undefined and autobarcode on - deleted an extra space in Barcodes/hbymmincr.pm
(In reply to Koha Team Lyon 3 from comment #23) > This enhancement will be very usefull :) > It's just that I'm wondering if there's not a easier way that can suit every > users. Agreed, I even found the modal annoying so have removed it in favor of a dialog when adding multiple. Please test and let me know if this seems better. Thank you for testing!
I have tried to install it on a sandbox : Some problems occurred applying patches from bug 14752: <h1>Something went wrong !</h1>Applying: Bug 14752 - Add multiple copies to a basket at once Applying: Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Barcodes/hbyymmincr.pm CONFLICT (content): Merge conflict in C4/Barcodes/hbyymmincr.pm Failed to merge in the changes. Patch failed at 0001 Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 14752 - Add multiple copies to a basket at once 51481 - Bug 14752 - Add multiple copies to a basket at once 52083 - Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead Sonia
Created attachment 52587 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields 7 - Enable autoBarcode for various formats, ensure you are warned that barcodes will be generated, and ensure they are generated correctly Sponsored by: Middletown Township Public Library (http://www.mtpl.org)
Created attachment 52588 [details] [review] Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead - Uses a dialog box to warn of unique fields not copying, dialog was in place for barcode generation so removed the extar modal completey - Fixes a problem when barcode was undefined and autobarcode on - deleted an extra space in Barcodes/hbymmincr.pm
> Agreed, I even found the modal annoying so have removed it in favor of a dialog when adding multiple. Please test and let me know if this seems better. Thank you for the change : yes, I find it much better ! I have tested several modes for adding multiple items (empty record, suggestion, ...) : everything is Ok. I'm still thinking that it would be better to be abble to add the number of items, without to click on a button before. But, as you say, QA team will decide.
Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 52605 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields 7 - Enable autoBarcode for various formats, ensure you are warned that barcodes will be generated, and ensure they are generated correctly Sponsored by: Middletown Township Public Library (http://www.mtpl.org) Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 52606 [details] [review] Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead - Uses a dialog box to warn of unique fields not copying, dialog was in place for barcode generation so removed the extar modal completey - Fixes a problem when barcode was undefined and autobarcode on - deleted an extra space in Barcodes/hbymmincr.pm Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Created attachment 55527 [details] [review] Bug 14752 - Add multiple copies to a basket at once This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed. To test: 1 - Add an order to an acquisitions basket 2 - Choose to add multiple items 3 - A modal shouold warn about ignoring UniqueItemFields from syspref 4 - When submitting the modal should popup until all items are processed. 5 - The modal should disappear after items are added. 6 - Items should be cloned, minus unique fields 7 - Enable autoBarcode for various formats, ensure you are warned that barcodes will be generated, and ensure they are generated correctly Sponsored by: Middletown Township Public Library (http://www.mtpl.org) Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 55528 [details] [review] Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead - Uses a dialog box to warn of unique fields not copying, dialog was in place for barcode generation so removed the extar modal completey - Fixes a problem when barcode was undefined and autobarcode on - deleted an extra space in Barcodes/hbymmincr.pm Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 16.11, thanks Nick!
Nick, if the barcode is not mandatory and you don't fill it, you will get: Can't call method "delete_subfield" on an undefined value at /home/koha/src/acqui/addorder.pl line 331. Please fix.
Created attachment 56649 [details] [review] Bug 14752 - Followup to fix error if barcode fields undefined
Jonathan, can you try the added patch to see if that fixes your error? It works for me and was the only way I triggered the error
Created attachment 56684 [details] [review] Bug 14752 - Followup to fix error if barcode fields undefined Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Nick Clemens from comment #37) > Created attachment 56649 [details] [review] [review] > Bug 14752 - Followup to fix error if barcode fields undefined Yes indeed I was wrong. Thanks for the patch!