Bug 12975

Summary: Refactor VAT and price calculation - basketgroup page
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, paola.rossi, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12969    
Bug Blocks:    
Attachments: Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl
Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl
[PASSED QA] Bug 12975: Use the centralize VAT and prices calculation - basketgroup.pl

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!