Bug 23397 - Order lines can be duplicated in acqui scripts spent.pl and ordered.pl
Summary: Order lines can be duplicated in acqui scripts spent.pl and ordered.pl
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-29 18:11 UTC by Nick Clemens
Modified: 2019-09-09 07:03 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.04


Attachments
Bug 23397: Fix grouping of orders in acqui scripts (2.16 KB, patch)
2019-07-29 18:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23397: Fix grouping of orders in acqui scripts (2.21 KB, patch)
2019-08-18 10:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23397: (follow-up) Split and display itemtypes (4.16 KB, patch)
2019-08-23 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23397: Fix grouping of orders in acqui scripts (2.31 KB, patch)
2019-08-23 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23397: (follow-up) Split and display itemtypes (4.25 KB, patch)
2019-08-23 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23397: (QA follow-up) Replace mod 2 by last (1.81 KB, patch)
2019-08-23 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-07-29 18:11:06 UTC
Bug 21622 attempted to fix some grouping for strict mode, however, it failed to take into account that single order can have multiple item types.

To recreate:
1 - Place an order for 4 copies of a title in acq
2 - Make sure you are creating orders when ordering (AcqCreateItems syspref and/or when creating basket)
3 - Set three items to one type, and one to another
4 - Go to Acqui-Home and click 'ordered' for the used budget
5 - Note the line is duplicated for each itemtype in the order
6 - Recieve the items and note the same issue on 'Spent'
7 - Place another order as before so you have the problem in both ordered.pl and spent/pl
Comment 1 Nick Clemens 2019-07-29 18:13:36 UTC
Created attachment 91855 [details] [review]
Bug 23397: Fix grouping of orders in acqui scripts

Bug 21622 attempted to fix some grouping for strict mode, however, it failed to take into account that single order can have multiple item types.

To recreate:
1 - Place an order for 4 copies of a title in acq
2 - Make sure you are creating orders when ordering (AcqCreateItems syspref and/or when creating basket)
3 - Set three items to one type, and one to another
4 - Go to Acqui-Home and click 'ordered' for the used budget
5 - Note the line is duplicated for each itemtype in the order
6 - Recieve the items and note the same issue on 'Spent'
7 - Place another order as before so you have the problem in both ordered.pl and spent/pl
8 - Apply patch
9 - Note orders are no longer duplicated
Comment 2 Katrin Fischer 2019-08-18 10:21:06 UTC
Created attachment 92344 [details] [review]
Bug 23397: Fix grouping of orders in acqui scripts

Bug 21622 attempted to fix some grouping for strict mode, however, it failed to take into account that single order can have multiple item types.

To recreate:
1 - Place an order for 4 copies of a title in acq
2 - Make sure you are creating orders when ordering (AcqCreateItems syspref and/or when creating basket)
3 - Set three items to one type, and one to another
4 - Go to Acqui-Home and click 'ordered' for the used budget
5 - Note the line is duplicated for each itemtype in the order
6 - Recieve the items and note the same issue on 'Spent'
7 - Place another order as before so you have the problem in both ordered.pl and spent/pl
8 - Apply patch
9 - Note orders are no longer duplicated

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Marcel de Rooy 2019-08-23 06:50:00 UTC
Form shows itemtype, which is empty now.
Template contains: ItemTypes.GetDescription( order.itype )
If you would add AS itype to the GROUP_CONCAT you added, it still does not work, since the concatenated string is no itype anymore..
Remove it from the form ?
Comment 4 Marcel de Rooy 2019-08-23 07:01:31 UTC
BTW While testing:
DBD::mysql::st execute failed: Incorrect double value: '' for column `koha_master`.`aqorders`.`discount` at row 1 [for Statement "UPDATE aqorders SET basketno=?, biblionumber=?, quantity=?, budget_id=?, currency=?, listprice=?, discount=?, rrp=?, replacementprice=?, ecost=?, unitprice=?, order_internalnote=?, order_vendornote=?, sort1=?, sort2=?, subscriptionid=?, uncertainprice=?, tax_rate_on_ordering=?, rrp_tax_excluded=?, rrp_tax_included=?, ecost_tax_excluded=?, ecost_tax_included=?, tax_value_on_ordering=?, timestamp=NOW()  WHERE  ordernumber=?" with ParamValues: 0="67", 1="2578", 2="2", 3="123", 4="USD", 5="10", 6="", 7="10.00", 8="10.00", 9="10.00", 10="0.00", 11="", 12="", 13="", 14="", 15=undef, 16="0", 17="0", 18="10.00", 19="10", 20="10", 21="10", 22="0", 23="15"] at /usr/share/koha/C4/Acquisition.pm line 1358.

With strict SQL saving the order failed invisibly..
Comment 5 Katrin Fischer 2019-08-23 07:26:30 UTC
Could we deal with that in the template, splitting up the string and displaying all itemtypes of an order?

Worried about this one as it makes the amounts not add up... and libraries don't like that.
Comment 6 Marcel de Rooy 2019-08-23 07:50:54 UTC
(In reply to Katrin Fischer from comment #5)
> Could we deal with that in the template, splitting up the string and
> displaying all itemtypes of an order?
> 
> Worried about this one as it makes the amounts not add up... and libraries
> don't like that.

We could show the concatenated itypes if GetDescription does not come up with a value?
Comment 7 Katrin Fischer 2019-08-23 07:56:18 UTC
I think if we can't fix showing the description, nothing is better right now and matches the behaviour before as far as I can tell from our installations where we have a mix of empty and itemtype description.

What I was thinking was:

if I understood GROUP_CONCAT right it gives you a comma separated string of values? Could we not split by , and loop through it template side?
Comment 8 Nick Clemens 2019-08-23 13:15:58 UTC
Created attachment 92446 [details] [review]
Bug 23397: (follow-up) Split and display itemtypes
Comment 9 Marcel de Rooy 2019-08-23 14:22:52 UTC
(In reply to Nick Clemens from comment #8)
> Created attachment 92446 [details] [review] [review]
> Bug 23397: (follow-up) Split and display itemtypes

Looking at it again
Comment 10 Marcel de Rooy 2019-08-23 14:38:39 UTC
Created attachment 92448 [details] [review]
Bug 23397: Fix grouping of orders in acqui scripts

Bug 21622 attempted to fix some grouping for strict mode, however, it failed to take into account that single order can have multiple item types.

To recreate:
1 - Place an order for 4 copies of a title in acq
2 - Make sure you are creating orders when ordering (AcqCreateItems syspref and/or when creating basket)
3 - Set three items to one type, and one to another
4 - Go to Acqui-Home and click 'ordered' for the used budget
5 - Note the line is duplicated for each itemtype in the order
6 - Recieve the items and note the same issue on 'Spent'
7 - Place another order as before so you have the problem in both ordered.pl and spent/pl
8 - Apply patch
9 - Note orders are no longer duplicated

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2019-08-23 14:38:44 UTC
Created attachment 92449 [details] [review]
Bug 23397: (follow-up) Split and display itemtypes

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2019-08-23 14:38:48 UTC
Created attachment 92450 [details] [review]
Bug 23397: (QA follow-up) Replace mod 2 by last

Looks better: Maps | Books | Continuing Resources

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Jonathan Druart 2019-09-01 18:49:06 UTC
I was completely stuck with circ/pendingreserves.pl because of the use of GROUP_CONCAT. It makes things super hard to rewrite. Do not we have another option?
Comment 14 Jonathan Druart 2019-09-01 18:56:30 UTC
I do not have much to suggest apart from rewriting the scripts from scratch, which seems quite out of the scope ;)
Comment 15 Martin Renvoize 2019-09-06 12:12:34 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin SOMERS 2019-09-09 07:03:18 UTC
Pushed to 19.05.x for 19.05.04