Bug 14752 - Add multiple copies to a basket at once
Summary: Add multiple copies to a basket at once
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 15877 15878
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-28 16:25 UTC by Jesse Weaver
Modified: 2018-06-04 20:10 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch add an 'Add multiple copies' button on the new order page in acquisitions. While processing the multiple copies a modal is displayed.
Version(s) released in:


Attachments
Bug 14752 - Add multiple copies to a basket at once (8.43 KB, patch)
2015-12-30 16:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - (QA Followup) Use input type="number" and small update to error modal (3.27 KB, patch)
2016-01-08 15:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - Add multiple copies to a basket at once (8.48 KB, patch)
2016-01-18 23:23 UTC, Briana Greally
Details | Diff | Splinter Review
Bug 14752 - (QA Followup) Use input type="number" and small update to error modal (3.33 KB, patch)
2016-01-18 23:23 UTC, Briana Greally
Details | Diff | Splinter Review
Bug 14752 [QA followup] (3.70 KB, patch)
2016-01-22 21:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - [QA Followup] Correct error message (1.05 KB, patch)
2016-01-25 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - increment barcodes when adding multiples if present and uniqueness enforced (7.38 KB, patch)
2016-01-29 19:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - [QA followup] Auto-Generate barcode if left blank and autoBarcode is set (3.90 KB, patch)
2016-02-19 21:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - Add multiple copies to a basket at once (12.09 KB, patch)
2016-05-13 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead (6.90 KB, patch)
2016-06-06 14:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - Add multiple copies to a basket at once (12.09 KB, patch)
2016-06-20 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead (6.93 KB, patch)
2016-06-20 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - Add multiple copies to a basket at once (12.15 KB, patch)
2016-06-20 15:35 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead (6.98 KB, patch)
2016-06-20 15:35 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 14752 - Add multiple copies to a basket at once (12.22 KB, patch)
2016-09-13 11:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14752 - (QA followup) Remove annoying modal, use dialog box instead (7.05 KB, patch)
2016-09-13 11:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14752 - Followup to fix error if barcode fields undefined (1008 bytes, patch)
2016-10-19 10:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14752 - Followup to fix error if barcode fields undefined (1.06 KB, patch)
2016-10-20 07:20 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2015-08-28 16:25:07 UTC

    
Comment 1 Nick Clemens 2015-12-30 16:07:25 UTC Comment hidden (obsolete)
Comment 2 Jesse Weaver 2016-01-06 22:45:16 UTC
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.
Comment 3 Nick Clemens 2016-01-08 15:53:21 UTC Comment hidden (obsolete)
Comment 4 Briana Greally 2016-01-18 23:23:13 UTC Comment hidden (obsolete)
Comment 5 Briana Greally 2016-01-18 23:23:44 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-01-19 11:11:55 UTC
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.
Comment 7 Nick Clemens 2016-01-22 21:35:10 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2016-01-22 21:44:07 UTC
(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?
Comment 9 Jonathan Druart 2016-01-25 12:17:49 UTC
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.
Comment 10 Nick Clemens 2016-01-25 13:43:50 UTC Comment hidden (obsolete)
Comment 11 Nick Clemens 2016-01-25 13:45:02 UTC
(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
Comment 12 Jonathan Druart 2016-01-29 12:08:09 UTC
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.
Comment 13 Nick Clemens 2016-01-29 19:29:58 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-02-16 11:37:11 UTC
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     }
Comment 15 Jonathan Druart 2016-02-16 11:40:37 UTC
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?
Comment 16 Katrin Fischer 2016-02-16 12:11:03 UTC
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.
Comment 17 Nick Clemens 2016-02-19 21:02:49 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2016-02-19 21:06:26 UTC
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?
Comment 19 Nick Clemens 2016-05-13 19:25:55 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2016-05-13 19:27:04 UTC
Patches rebased and squashed, added dependency
Comment 21 Koha Team University Lyon 3 2016-06-03 12:57:33 UTC
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 :)
Comment 22 Nick Clemens 2016-06-03 17:53:21 UTC
(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.
Comment 23 Koha Team University Lyon 3 2016-06-04 08:52:16 UTC
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.
Comment 24 Nick Clemens 2016-06-06 14:15:49 UTC Comment hidden (obsolete)
Comment 25 Nick Clemens 2016-06-06 14:17:10 UTC
(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!
Comment 26 Koha Team University Lyon 3 2016-06-20 13:21:06 UTC
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
Comment 27 Nick Clemens 2016-06-20 13:24:12 UTC Comment hidden (obsolete)
Comment 28 Nick Clemens 2016-06-20 13:24:16 UTC Comment hidden (obsolete)
Comment 29 Koha Team University Lyon 3 2016-06-20 15:33:53 UTC
> 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.
Comment 30 Biblibre Sandboxes 2016-06-20 15:35:18 UTC
Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 31 Biblibre Sandboxes 2016-06-20 15:35:50 UTC Comment hidden (obsolete)
Comment 32 Biblibre Sandboxes 2016-06-20 15:35:53 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2016-09-13 11:12:08 UTC
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>
Comment 34 Jonathan Druart 2016-09-13 11:12:13 UTC
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>
Comment 35 Kyle M Hall 2016-09-13 18:20:24 UTC
Pushed to master for 16.11, thanks Nick!
Comment 36 Jonathan Druart 2016-10-18 15:32:28 UTC
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.
Comment 37 Nick Clemens 2016-10-19 10:10:53 UTC
Created attachment 56649 [details] [review]
Bug 14752 - Followup to fix error if barcode fields undefined
Comment 38 Nick Clemens 2016-10-19 10:11:57 UTC
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
Comment 39 Jonathan Druart 2016-10-20 07:20:32 UTC
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>
Comment 40 Jonathan Druart 2016-10-20 07:21:34 UTC
(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!