Bug 12975 - Refactor VAT and price calculation - basketgroup page
Summary: Refactor VAT and price calculation - basketgroup page
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12969
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-22 14:08 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
3 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 12975: Use the centralize VAT and prices calculation - basketgroup.pl (4.11 KB, patch)
2014-09-22 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl (4.11 KB, patch)
2014-09-23 10:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl (19.57 KB, patch)
2014-09-25 14:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl (4.26 KB, patch)
2014-10-02 15:07 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl (4.28 KB, patch)
2015-01-28 21:54 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-09-22 14:08:57 UTC
Bug 12969 introduces a subroutine to centralize the VAT and price calculation.
The basketgroup page should use it.
Comment 1 Jonathan Druart 2014-09-22 14:11:22 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-09-23 10:26:25 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-09-25 14:20:50 UTC Comment hidden (obsolete)
Comment 4 Paola Rossi 2014-10-02 15:07:53 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-10-02 15:13:11 UTC
(In reply to Paola Rossi from comment #4)
> Created attachment 31981 [details] [review] [review]

Paola, you forgot to attach your signoff patch :)
Comment 6 Jonathan Druart 2014-10-02 15:16:00 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Paola Rossi from comment #4)
> > Created attachment 31981 [details] [review] [review] [review]
> 
> Paola, you forgot to attach your signoff patch :)

Sorry, end of the day...
Comment 7 Katrin Fischer 2014-10-28 22:28:53 UTC
Blocked by bug 12896 currently not applying. :(
Comment 8 Katrin Fischer 2014-12-27 19:33:16 UTC
Comment on attachment 31981 [details] [review]
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl

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

::: acqui/basketgroup.pl
@@ -212,5 @@
>                  }
>              }
>  
> -            my $ba_order = {
> -                isbn => ($ord->{isbn} ? $ord->{isbn} : undef),

Hi Jonathan, bit irritated by this line - was removing it intentional? I see that that PDF should be printing the ISBN, but have a bit of trouble making it work in my installation currently.
Comment 9 Katrin Fischer 2014-12-27 19:57:32 UTC
Changing to 'failed qa' for now, waiting for Jonathan to take a look at the last comment.
Comment 10 Jonathan Druart 2014-12-29 10:47:55 UTC
(In reply to Katrin Fischer from comment #8)
> Comment on attachment 31981 [details] [review] [review]
> Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl
> 
> Review of attachment 31981 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: acqui/basketgroup.pl
> @@ -212,5 @@
> >                  }
> >              }
> >  
> > -            my $ba_order = {
> > -                isbn => ($ord->{isbn} ? $ord->{isbn} : undef),
> 
> Hi Jonathan, bit irritated by this line - was removing it intentional? I see
> that that PDF should be printing the ISBN, but have a bit of trouble making
> it work in my installation currently.

Hi Katrin,
Yes it is intentional.
Actually before the patch, the $ba_order was pushed to @ba_orders.
Now, the order $ord is pushed.
We could add:
  $ord->{isbn} = $ord->{isbn} ? $order->{isbn} : undef;

But it does not make sense, except if we need to manage the empty string or 0, but it would not be a valid ISBN.
Comment 11 Katrin Fischer 2014-12-29 11:52:57 UTC
Hi Jonathan, I think you lost me there, but I believe you :) 

It looks like the printed PDF should include the ISBN, which also seems crucial for an order to a vendor. Does it print for you? It didn't for me, so maybe that's a separate bug to file.
Comment 12 Jonathan Druart 2014-12-29 13:38:26 UTC
(In reply to Katrin Fischer from comment #11)
> Hi Jonathan, I think you lost me there, but I believe you :) 
> 
> It looks like the printed PDF should include the ISBN, which also seems
> crucial for an order to a vendor. Does it print for you? It didn't for me,
> so maybe that's a separate bug to file.

Yes, I see it with English 3-pages with and without the patch.
Comment 13 Katrin Fischer 2015-01-28 21:54:24 UTC
Created attachment 35594 [details] [review]
[PASSED QA] Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl

Bug 12969 introduces a subroutine to centralize VAT and prices
calculation.
It should be use in the acqui/basketgroup.pl script.

Test plan:
0/ Don't apply the patch
1/ Create 4 suppliers with the different configurations
2/ Create a basket and create several orders
3/ Close the basket and create the corresponding basket groups.
4/ Print the basket group
5/ Verify you don't see any difference before and after applying the
patch on the pdf file.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Works as described, passes tests and QA script.
Comment 14 Tomás Cohen Arazi 2015-02-05 17:57:27 UTC
Patch pushed to master.

Thanks Jonathan!