Bug 22390 - When duplicating existing order lines new items are not created
Summary: When duplicating existing order lines new items are not created
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15184
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-21 15:35 UTC by Nick Clemens (kidclamp)
Modified: 2020-06-04 20:32 UTC (History)
5 users (show)

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


Attachments
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial (4.37 KB, patch)
2019-02-21 17:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22390: Never copy items if original order has been created from a subscription (2.44 KB, patch)
2019-03-04 19:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial (4.42 KB, patch)
2019-03-12 12:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22390: Never copy items if original order has been created from a subscription (2.50 KB, patch)
2019-03-12 12:45 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial (4.49 KB, patch)
2019-04-08 21:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22390: Never copy items if original order has been created from a subscription (2.63 KB, patch)
2019-04-08 21:52 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2019-02-21 15:35:06 UTC
To recreate:
1 - Create a basket set to create items on ordering
2 - Add to this basket, confirm items are created
3 - Add to this basket from the prevous order
4 - Note no new items are created
Comment 1 Andrew Fuerste-Henry 2019-02-21 16:13:16 UTC
In our testing, a new item was created if biblio.serial was null but not if biblio.serial was 0.
Comment 2 Andrew Fuerste-Henry 2019-02-21 17:55:02 UTC
(In reply to Andrew from comment #1)
> In our testing, a new item was created if biblio.serial was null but not if
> biblio.serial was 0.

Oops, sorry, disregard this. Mis-remembered our testing. No items were created for any value of biblio.serial.
Comment 3 Jonathan Druart 2019-02-21 17:56:21 UTC
Created attachment 85473 [details] [review]
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial

It seems that we made a wrong assumption on bug 15184, see
  commit d658cb6f7ecb18845a78d4708ee63ad1126f220f
  Bug 15184: Do copy items for not a serial OR if items are created on ordering

To know if an order has been created from a subscription we should check
$order->subscriptionid instead of the $biblio->serial flag
Comment 4 Jonathan Druart 2019-02-21 17:58:35 UTC
(In reply to Andrew from comment #2)
> (In reply to Andrew from comment #1)
> > In our testing, a new item was created if biblio.serial was null but not if
> > biblio.serial was 0.
> 
> Oops, sorry, disregard this. Mis-remembered our testing. No items were
> created for any value of biblio.serial.

Should have been copied for biblio.serial set to 1, but it seems that this behavior was wrong, please test the patch.
Comment 5 Nick Clemens (kidclamp) 2019-02-22 17:38:47 UTC
This seems to have improved things - If I copy an order from a basket where effective create items is 'ordering' then a new item is created.

However

If I copy from a basket that did create items to a new basket that where effective create items is 'cataloging' an item is created, though it should not be.

I think the correct condition might be
if ( (!$self->subscriptionid) && ($self->basket->effective_create_items eq 'ordering') ) { 
# Do copy items if not a serial AND if items are created on ordering in this basket
Comment 6 Jonathan Druart 2019-03-04 19:07:50 UTC
Created attachment 86027 [details] [review]
Bug 22390: Never copy items if original order has been created from a subscription
Comment 7 Martin Renvoize (ashimema) 2019-03-12 12:45:45 UTC
Created attachment 86523 [details] [review]
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial

It seems that we made a wrong assumption on bug 15184, see
  commit d658cb6f7ecb18845a78d4708ee63ad1126f220f
  Bug 15184: Do copy items for not a serial OR if items are created on ordering

To know if an order has been created from a subscription we should check
$order->subscriptionid instead of the $biblio->serial flag

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2019-03-12 12:45:48 UTC
Created attachment 86524 [details] [review]
Bug 22390: Never copy items if original order has been created from a subscription

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2019-03-12 12:46:39 UTC
Nice catch and followup. Signing Off.
Comment 10 Katrin Fischer 2019-04-08 21:52:31 UTC
Created attachment 87560 [details] [review]
Bug 22390: (bug 15184 follow-up) Use aqorders.subscriptionid instead of biblio.serial

It seems that we made a wrong assumption on bug 15184, see
  commit d658cb6f7ecb18845a78d4708ee63ad1126f220f
  Bug 15184: Do copy items for not a serial OR if items are created on ordering

To know if an order has been created from a subscription we should check
$order->subscriptionid instead of the $biblio->serial flag

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2019-04-08 21:52:35 UTC
Created attachment 87561 [details] [review]
Bug 22390: Never copy items if original order has been created from a subscription

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Tiny change to the code comment: serial => subscription order
Comment 12 Nick Clemens (kidclamp) 2019-04-10 20:13:33 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize (ashimema) 2019-04-15 11:25:52 UTC
Pushed to 18.11.x for 18.11.05
Comment 14 Lucas Gass (lukeg) 2019-04-16 20:22:52 UTC
missing dependency, not backporting to 18.05.x
Comment 15 Sandre Cunha 2019-05-15 17:20:35 UTC
This would be a great enhancement if only it would allow you to change the quantity. Typically we only order 1 copy initially, but if the item is popular, when we re-order we would want to add 2 or 3 (or more) copies. I don't see much advantage of having to replicate this process multiple times, versus just creating a new order with the number of items needed.
Comment 16 Andrew Fuerste-Henry 2019-05-15 17:45:42 UTC
Sandre, the use case laid out in the original bug 15184 was focused on cases where you would be copying the whole order exactly -- like serials or databases, where generally you're only going to have a quantity of 1 anyway and it will stay consistent order-to-order. I agree that this is not going to be the easiest way to do what you're doing. For that I'd probably just add to basket from the existing bib.