Bug 11471 - Basket groups display 0 0
Summary: Basket groups display 0 0
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-02 19:02 UTC by Mark Tompsett
Modified: 2015-06-04 23:30 UTC (History)
5 users (show)

See Also:
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 (2.84 KB, patch)
2014-01-02 22:23 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11471 - Basket groups display 0 0 (2.91 KB, patch)
2014-01-02 22:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
[Signed-off] Bug 11471 - Basket groups display 0 0 (3.11 KB, patch)
2014-02-17 19:50 UTC, Marc Véron
Details | Diff | Splinter Review
[ALT] Bug 11471: Display the currency for baskets in a basketgroup (2.08 KB, patch)
2014-02-18 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11471: Display the currency for baskets in a basketgroup (3.24 KB, patch)
2014-02-18 17:04 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11471: Display the currency for baskets in a basketgroup (3.28 KB, patch)
2014-02-21 13:45 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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