Bug 27082 - Problem when a basket has more of 20 orders with uncertain price
Summary: Problem when a basket has more of 20 orders with uncertain price
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Blou
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 23013
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-23 21:33 UTC by Marjorie Barry-Vila
Modified: 2022-06-06 20:25 UTC (History)
6 users (show)

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


Attachments
Bug 27082: (bug 23013 follow-up) Remove pagination from uncertain prices (1.88 KB, patch)
2020-11-24 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27082: (bug 23013 follow-up) Remove pagination from uncertain prices (1.93 KB, patch)
2020-11-24 19:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 27082: uncertainprice.pl does not handle pagination (1.48 KB, patch)
2020-11-24 22:14 UTC, Blou
Details | Diff | Splinter Review
Bug 27082: uncertainprice.pl does not handle pagination (1.53 KB, patch)
2020-11-25 12:52 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marjorie Barry-Vila 2020-11-23 21:33:54 UTC
Hi,

There is a problem when a basket has more of 20 orders with uncertain price

Test plan:

1- create a basket
2- add more of 20 orders with uncertain price
3- click on uncertain price button
4- on page 1 of list, uncheck uncertain box of an order
5- click on save button
6- orders who were on page 2 are not display anymore in page uncertainprice.pl
7- return to basket view acqui/basket.pl
8- orders who were on page 2 have "rvcd" label and quantity is null

Screen capture here https://www.screencast.com/t/00cO5f1RNJdE

Tested on 19.05 and 20.05.04

Regards

Marjorie
Comment 1 Jonathan Druart 2020-11-24 11:18:59 UTC
Coming from
  commit 247dca306407ac9568344a0a37aca3bbcffc2468
  Bug 23013: Upgrade DataTables in the staff client

What's happening:
When the form is submitted, all the orders with uncertain price are processed, but the posted values only contain the ones from the first page of the table.
The orders that are not displayed on the first page will have their prices fields and uncertainprice set to NULL.

This is a critical bug and must be fixed ASAP (data loss).
Comment 2 Jonathan Druart 2020-11-24 11:23:15 UTC
Created attachment 113940 [details] [review]
Bug 27082: (bug 23013 follow-up) Remove pagination from uncertain prices

When more than 20 orders with uncertain prices are displayed on the
"uncertain prices" view, the pagination is displayed and will cause data
loss if the form is submitted.

The pagination is there since:
  commit 247dca306407ac9568344a0a37aca3bbcffc2468
  Bug 23013: Upgrade DataTables in the staff client

What's happening:
When the form is submitted, all the orders with uncertain price are processed, but the posted values only contain the ones from the first page of the table.
The orders that are not displayed on the first page will have their prices fields and uncertainprice set to NULL.

Test plan:
1- create a basket
2- add more of 20 orders with uncertain price
3- click on uncertain price button
4- click on save button
=> There is no more pagination and the form behaves correctly
Comment 3 David Nind 2020-11-24 19:38:50 UTC
Created attachment 113965 [details] [review]
Bug 27082: (bug 23013 follow-up) Remove pagination from uncertain prices

When more than 20 orders with uncertain prices are displayed on the
"uncertain prices" view, the pagination is displayed and will cause data
loss if the form is submitted.

The pagination is there since:
  commit 247dca306407ac9568344a0a37aca3bbcffc2468
  Bug 23013: Upgrade DataTables in the staff client

What's happening:
When the form is submitted, all the orders with uncertain price are processed, but the posted values only contain the ones from the first page of the table.
The orders that are not displayed on the first page will have their prices fields and uncertainprice set to NULL.

Test plan:
1- create a basket
2- add more of 20 orders with uncertain price
3- click on uncertain price button
4- click on save button
=> There is no more pagination and the form behaves correctly

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Blou 2020-11-24 20:19:18 UTC
Please note that this bug is not originating in Bug 23013.  The issue presented by Marjorie has been fixed and rebased in their code since 17.05.

With a different solution, in the .pl.

She'd asked me to submit it, but you beat me to the punch.  Good job!
Comment 5 Jonathan Druart 2020-11-24 20:52:57 UTC
(In reply to Blou from comment #4)
> Please note that this bug is not originating in Bug 23013.  The issue
> presented by Marjorie has been fixed and rebased in their code since 17.05.
> 
> With a different solution, in the .pl.
> 
> She'd asked me to submit it, but you beat me to the punch.  Good job!

If the bug existed prior to bug 23013, I am not sure my patch will fix the problem as it just reverts the change from bug 23013.
However it seems to work when I tested it.
Comment 6 Blou 2020-11-24 21:03:16 UTC
Here's what Bouzid's fix was doing:

--- a/acqui/uncertainprice.pl
+++ b/acqui/uncertainprice.pl
@@ -88,6 +88,7 @@ if ( $op eq 'validate' ) {
     for (my $i=0; $i < $count; $i++) {
         my $order = pop(@orders);
         my $ordernumber = $order->{ordernumber};
+        next unless ($input->param('qty'.$ordernumber));
         my $order_as_from_db=GetOrder($order->{ordernumber});
         $order->{'listprice'} = $input->param('price'.$ordernumber);

But this avoids the "crash" without fixing the issue at root I fear.
Comment 7 Jonathan Druart 2020-11-24 21:35:06 UTC
Bouzid's fix is better and more robust that mine. I was simply reverting the change made by bug 23013.
Could you submit a patch?

And also confirm that the bug exists in 17.05 (23013 is in 17.11)?
Comment 8 Blou 2020-11-24 22:14:17 UTC
Created attachment 113969 [details] [review]
Bug 27082: uncertainprice.pl does not handle pagination

To reproduce:
1- create a basket
2- add more of 20 orders with uncertain price
3- click on uncertain price button
4- on page 1 of list, uncheck uncertain box of an order
5- click on save button
6- orders who were on page 2 are not display anymore in page uncertainprice.pl
7- return to basket view acqui/basket.pl
8- orders who were on page 2 have "rvcd" label and quantity is null

The data in page 2 and beyond is not transmitted, but the code doesn't
handle that.  This patch makes sure that all that (empty) data is not (wrongly) processed.

Sponsored by: CCSR
Comment 9 Blou 2020-11-24 22:19:24 UTC
I obsoleted the other patch, presuming that was your intention.

I validated our history (the ticket and the old branches) and this problem was first fixed in 16.11 in CCSR's codebase. There's even a comment about pushing it to bugzilla at the time "to be determined".  The patch has been following our rebases without much questionning for over 3 years now.

I hope it helps.
Comment 10 Jonathan Druart 2020-11-25 09:24:27 UTC
Yes, I confirmed that there was pagination in 17.05. No need to dig more into the problem, let's consider it's almost everywhere...
Thanks, Philippe!
Comment 11 Nick Clemens (kidclamp) 2020-11-25 12:52:58 UTC
Created attachment 113977 [details] [review]
Bug 27082: uncertainprice.pl does not handle pagination

To reproduce:
1- create a basket
2- add more of 20 orders with uncertain price
3- click on uncertain price button
4- on page 1 of list, uncheck uncertain box of an order
5- click on save button
6- orders who were on page 2 are not display anymore in page uncertainprice.pl
7- return to basket view acqui/basket.pl
8- orders who were on page 2 have "rvcd" label and quantity is null

The data in page 2 and beyond is not transmitted, but the code doesn't
handle that.  This patch makes sure that all that (empty) data is not (wrongly) processed.

Sponsored by: CCSR

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens (kidclamp) 2020-11-25 12:55:25 UTC
Small patch, works, passing QA - 

we should probably refactor this a bit in the future to only process submitted orders or use the api but this is a bad bug
Comment 13 Jonathan Druart 2020-11-25 15:20:12 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 14 Andrew Fuerste-Henry 2020-12-14 15:14:35 UTC
Pushed to 20.05.x for 20.05.07
Comment 15 Victor Grousset/tuxayo 2020-12-16 17:27:46 UTC
Backported to 19.11.x branch for 19.11.13