Bug 11471

Summary: Basket groups display 0 0
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: AcquisitionsAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9654
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11820
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 11471 - Basket groups display 0 0
Bug 11471 - Basket groups display 0 0
[Signed-off] Bug 11471 - Basket groups display 0 0
[ALT] Bug 11471: Display the currency for baskets in a basketgroup
[SIGNED OFF] Bug 11471: Display the currency for baskets in a basketgroup
Bug 11471: Display the currency for baskets in a basketgroup

Description Mark Tompsett 2014-01-02 19:02:42 UTC
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9654#c9
I was looking into the function GetBasketsByBasketgroup, and realized that invoiceprice, from aqbooksellers, is not included in the SQL query. Also, invoiceprice should really be invoicingcurrency. But let's deal with the functional problems, before other problems. :)
Comment 1 Mark Tompsett 2014-01-02 22:23:11 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-01-02 22:47:50 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2014-02-17 19:50:13 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-02-18 14:10:55 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-02-18 14:13:06 UTC
Hello Marc,
Your patch modifies a core routine without providing UT, so it can't be pushed as it.
However I think my patch fixes the issue by correcting a bug in the code: The second parameter for BasketTotal is $bookseller, not a budget.
Comment 6 Mark Tompsett 2014-02-18 16:10:26 UTC
(In reply to Jonathan Druart from comment #5)
> Your patch modifies a core routine without providing UT, so it can't be
> pushed as [is].

You are correct. I suppose I should fix up a t/db_dependent test for it.
Comment 7 Mark Tompsett 2014-02-18 16:20:49 UTC
(In reply to Jonathan Druart from comment #5)
> However I think my patch fixes the issue by correcting a bug in the code:
> The second parameter for BasketTotal is $bookseller, not a budget.

Having read through the code further, you are correct. My fix fixes it as a side effect. Yours fixes it properly.
Comment 8 Mark Tompsett 2014-02-18 17:04:26 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-02-21 13:45:17 UTC
Created attachment 25532 [details] [review]
Bug 11471: Display the currency for baskets in a basketgroup

On editing a basketgroup, the currency for baskets in a basketgroup is
always '0'.

With this patch, the currency is correctly displayed.

TEST PLAN
=========
 1) Log into staff client
 2) Acquisitions
 3) Click 'Search' in the 'Manage orders' box
 4) Click '+ New basket' because a vendor name
 5) Type 'Test Basket' into basket name
 6) Click 'Save'
 7) Click 'Add to basket'
 8) Click 'From an external source'
 9) Type 'Green Eggs and Ham' into the Title text box
10) Click 'Search'
11) Click 'Order' on any one of the results
12) Click 'Add Item' in the 'Item' box
13) Select a Fund from the dropdown in the
     'Accounting details' box
14) Click 'Save'
15) Click 'Close the basket'
16) Click 'Yes, close (Y)' without checking attach to a
     basket group
17) Click the 'Basket groups' tab
18) Click '+ New basket group'
19) Notice the listing in the 'Ungrouped baskets'.
20) Drag and drop the entry into the 'Baskets in this group'
     text area
21) Click 'Save'
22) Click 'Edit'
23) Notice it displays incorrectly. (e.g. Total: 0 0)
24) Apply the patch (git bz apply 11471)
25) Refresh the page
26) Notice it displays the currency correctly. (e.g. Total: 0 USD)
    NOTE: If there is a space issue, see Bug 9654.
          This can be applied separately from that bug.
27) Run the Koha QA Tool: (~/qa-test-tools/koha-qa.pl -v 2 -c 1)

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Galen Charlton 2014-02-21 19:32:04 UTC
Comment on attachment 25532 [details] [review]
Bug 11471: Display the currency for baskets in a basketgroup

Review of attachment 25532 [details] [review]:
-----------------------------------------------------------------

::: acqui/basketgroup.pl
@@ +254,1 @@
>      if(! $booksellerid){

Ordinarily, fetching the vendor record before seeing if a booksellerid was passed would be a mistake.  In this case, however, I don't see any evidence that this script is every called with op eq 'add' but no booksellerid.

Can that whole conditional branch be removed, perhaps?
Comment 11 Galen Charlton 2014-02-21 19:46:04 UTC
Pushed to master.  Thanks, Jonathan!
Comment 12 Galen Charlton 2014-02-21 20:12:20 UTC
(In reply to Galen Charlton from comment #10)
> Ordinarily, fetching the vendor record before seeing if a booksellerid was
> passed would be a mistake.  In this case, however, I don't see any evidence
> that this script is every called with op eq 'add' but no booksellerid.
> 
> Can that whole conditional branch be removed, perhaps?

See bug 11820.
Comment 13 Fridolin Somers 2014-05-21 09:03:52 UTC
Pushed to 3.14.x, will be in 3.14.07
Comment 14 Kyle M Hall 2014-06-06 13:55:49 UTC
Pushed to 3.12.x will be in 3.12.13