Description
Jonathan Druart
2013-11-13 13:59:46 UTC
Created attachment 22906 [details] [review] Bug 11243: Supplier list should distinct canceled items On the supplier result list, the "Item count" columns contain the sum of all items ordered for a basket. But if an order is canceled, the item count is not really meaningful. This patch just adds, in parenthesis, the number of items canceled. Test plan: - create a basket and 3 orders with different number of items - cancel 1 order - verify on the supplier list that the number of items is correct and the number of canceled items is correct too. Created attachment 22946 [details] [review] [SIGNED-OFF] Bug 11243: Supplier list should distinct canceled items On the supplier result list, the "Item count" columns contain the sum of all items ordered for a basket. But if an order is canceled, the item count is not really meaningful. This patch just adds, in parenthesis, the number of items canceled. Test plan: - create a basket and 3 orders with different number of items - cancel 1 order - verify on the supplier list that the number of items is correct and the number of canceled items is correct too. Signed-off-by: Owen Leonard <oleonard@myacpl.org> This includes a string change, so it can't go in immediately. And I think I also found a problem: With the patch, the items column will display a total, followed by the number of items cancelled. Example: Item count: 3 (2 cancelled) So 2 out of 3 are cancelled. In this case, it is a basket with 2 order lines. Of those 2 order lines, one with 2 items ordered has been cancelled. The biblio count is: 1 This makes it a bit confusing. I see 2 options: Don't count the cancelled items and order lines, then it would be: Item count: 1 Biblio count: 1 Or we do count the cancelled items and order lines in both columns: Item count: 3 (2 cancelled) Bibio count: 1 (1 cancelled) What do you think? Created attachment 23050 [details] [review] Bug 11243: Display count of items and biblios cancelled Same as previous patch but for the biblios count. Created attachment 23051 [details] [review] Bug 11243: UT: unit tests for GetBasketsInfosByBookseller Verify prove t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t returns green. Katrin, I did not want to add a new sql query for the count of cancelled biblios. But I agree it is the more consistent to do. Back to needs signoff Thx Jonathan - I will wait until the follow-ups are signed off Created attachment 23844 [details] [review] Bug 11243: Supplier list should distinct canceled items On the supplier result list, the "Item count" columns contain the sum of all items ordered for a basket. But if an order is canceled, the item count is not really meaningful. This patch just adds, in parenthesis, the number of items canceled. Test plan: - create a basket and 3 orders with different number of items - cancel 1 order - verify on the supplier list that the number of items is correct and the number of canceled items is correct too. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 23845 [details] [review] Bug 11243: Display count of items and biblios cancelled Same as previous patch but for the biblios count. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 23846 [details] [review] Bug 11243: UT: unit tests for GetBasketsInfosByBookseller Verify prove t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t returns green. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Hi Jonathan, I retested this and think I spotted a problem: Order: 2 orders Cancel one order, delete bibliographic record Item cound is correct, but biblio count doesn't show the cancelled biblio. (In reply to Katrin Fischer from comment #11) > Hi Jonathan, I retested this and think I spotted a problem: > > Order: 2 orders > Cancel one order, delete bibliographic record > Item cound is correct, but biblio count doesn't show the cancelled biblio. Since the aqorders.biblionumber is set to null on deleting a biblio I don't know how to get the correct count. I could do something like: COUNT(DISTINCT(COALESCE(aqorders.biblionumber, timestamp))) AS total_biblios in order to get the total number of biblio for a basket. But if a same biblio is ordered in several orders, the value will be wrong. Switch back to Signed off, I need help from QA. I think maybe the problem here is that the biblionumber should not be nulled - I think Mathieu was working on something in that area. I think we can move this to another bug. Bug 10758 looks like a likely candidate to improve the situation here in the future. Created attachment 27318 [details] [review] 0001-PASSED-QA-Bug-11243-Supplier-list-should-distinct-ca.patch Created attachment 27319 [details] [review] 0002-PASSED-QA-Bug-11243-Display-count-of-items-and-bibli.patch Created attachment 27320 [details] [review] 0003-PASSED-QA-Bug-11243-UT-unit-tests-for-GetBasketsInfo.patch Pushed to master. Thanks, Jonathan! Pushed to 3.14.x, will be in 3.14.10 |