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
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).
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
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>
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!
(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.
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.
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)?
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
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.
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!
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>
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
Pushed to master for 20.11, thanks to everybody involved!
Pushed to 20.05.x for 20.05.07
Backported to 19.11.x branch for 19.11.13