Bug 11755

Summary: Argument 'booksellerid' not properly handled in orderreceive.pl
Product: Koha Reporter: Jacek Ablewicz <abl>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: brendan, fridolin.somers, gmcharlt, jonathan.druart, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10613
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 10613    
Attachments: Bug 11755: SearchOrders does not return booksellerid
Bug 11755: SearchOrders does not return booksellerid
[Signed-off] Bug 11755: SearchOrders does not return booksellerid
Bug 11755: SearchOrders does not return booksellerid
Bug 11755: SearchOrders does not return booksellerid

Description Jacek Ablewicz 2014-02-13 12:40:11 UTC 
In orderreceive.pl, argument 'booksellerid' passed to the template is always empty (= because we don't have booksellerid field in order records); subsequently, finishreceive.pl script is also not getting this parameter (and it relies on it to do some possibly important work). This has the following consequences:

1) link to vendor in the breadcrumbs on the receiving page is not working
2) this code block (introduced by Bug 5335 - More granular VAT)

 if ( $bookseller->{listincgst} ) {
        if ( not $bookseller->{invoiceincgst} ) {
            $order->{rrp} = $order->{rrp} * ( 1 + $order->{gstrate} );
            $order->{ecost} = $order->{ecost} * ( 1 + $order->{gstrate} );
            $order->{unitprice} = $order->{unitprice} * ( 1 + $order->{gstrate} );
        }
    } else {
        if ( $bookseller->{invoiceincgst} ) {
            $order->{rrp} = $order->{rrp} / ( 1 + $order->{gstrate} );
            $order->{ecost} = $order->{ecost} / ( 1 + $order->{gstrate} );
            $order->{unitprice} = $order->{unitprice} / ( 1 + $order->{gstrate} );
        }
    }
 }

in finishreceive.pl never runs

3) in the received item records, booksellerid (952 $e) field is not updated with the correct value by ModItem(), it always becomes '' after receiving.

Regarding 2), I'm not quite sure it is neceserilly the bad thing ;). But it (partially) explains to me why tax calculations in Koha are occasionally working seemingly weird (at least for some less usual invoiceincgst & listincgst settings in vendor record).
Comment 1 Jonathan Druart 2014-03-05 11:10:59 UTC Comment hidden (obsolete)
Comment 2 Jacek Ablewicz 2014-03-05 14:04:19 UTC 
(In reply to Jonathan Druart from comment #1)
> Created attachment 25834 [details] [review] [review]
> Bug 11755: SearchOrders does not return booksellerid
> 
> The SearchOrders routine should return the booksellerid.
> 
> Booksellerid was returned before bug 10723.
> 

Patch does resolve all the issues mentioned; I'm a little worried about an impact it will have on issue 2), though.. Situation regarding tax recalculation code in finishreceive.pl seems to be somehow like that:

- in 3.10 & 3.12 branches, this code block was, and still is working like intended,
- it got broken in 3.14 and up (since bug 10723) resulting in price values in all newly received orders being inconsistently saved in the database (at least for those cases where listincgst xor invoiceincgst != 1) ?

As a side effect, some bug reports currently sitting in the queue which regard various problems with tax calculations in Koha will probably need to be retested / reevaluated. Still, IMO pushing this patch first (and ASAP) will be a huge change in the right direction.

I wonder if there may be some way to retroactively correct wrongly calculated price values which may already got stored in aqorders table, for those people using 3.14 right now in production environment?
Comment 3 Jonathan Druart 2014-03-06 08:43:02 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2014-03-06 13:53:54 UTC Comment hidden (obsolete)
Comment 5 Brendan Gallagher 2014-03-11 10:19:11 UTC 
Jonathan - 

tests failed.  There are a lot of new additions to the Acquisitions.t 

Besides that - the patch does do what it advertize - so once tests past :)
Comment 6 Jonathan Druart 2014-03-11 11:12:16 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-03-11 11:12:44 UTC 
Last patch rebased.
Comment 8 Brendan Gallagher 2014-03-11 15:35:07 UTC 
Created attachment 26121 [details] [review]
Bug 11755: SearchOrders does not return booksellerid

The SearchOrders routine should return the booksellerid.

Booksellerid was returned before bug 10723.

Quick test plan:
Go on orderreceive.pl and verify that the vendor link is correct.

Followed test plan. Vendor link is now correct.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 9 Galen Charlton 2014-03-11 16:21:58 UTC 
Pushed to master.  Thanks, Jonathan!
Comment 10 Fridolin Somers 2014-05-27 15:53:32 UTC 
Pushed to 3.14.x, will be in 3.14.07