Bug 11206

Summary: Factorize code for getting orders in C4::Acquisitions
Product: Koha Reporter: Mathieu Saby <mathsabypro>
Component: AcquisitionsAssignee: Bugs List <koha-bugs>
Status: CLOSED WONTFIX QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: [PATCH] Bug 11206 - Factorize code for getting orders in C4::Acquisitions
Bug 12206: fix HTML in other holdings list in staff search results page

Description Mathieu Saby 2013-11-06 09:28:28 UTC
A lot of subs are used in C4::Acquisitions for doing basically the same thing : searching orders.
Code can be factorized, to improve maintainability, build stronger unit tests, and make further improvements easier (like using DBIx::Class, or Bug 10758 ( 	Show bibliographic information of deleted records in acquisitions).

First draft patch attached here.
Fully testable, but can maybe be improved, and UT are not written yet.

To test : make a full acquisition workflow, search orders, display uncertain prices, export baskets, basketgroups and late orders in CSV or PDF when possible.

M. Saby
Comment 1 Mathieu Saby 2013-11-06 09:32:20 UTC
Created attachment 22747 [details] [review]
[PATCH] Bug 11206 - Factorize code for getting orders in C4::Acquisitions

This patch adds a new sub C4::Acquisition::_get_orders_with_params
This sub is now called by
- GetOrder
- GetOrders
- GetCancelledOrders
- GetOrdersByBiblionumber
- GetOrderFromItemnumber
- SearchOrder

It also makes changes to  acqui/lateorders-export.pl, .../prog/en/modules/acqui/csv/lateorders.tt and .../prog/en/modules/acqui/lateorders.tt to take into account some variable renaming

Next step : use it in GetLateOrders
Comment 2 Jonathan Druart 2013-11-06 10:10:46 UTC
Mathieu,
Not sure it is a good idea to factorize like that.
Now, some routines did 1, 2 or 3 join, with your patch, they will always do 8 join.
That will increase the load in acquisition pages.
Comment 3 Mathieu Saby 2013-11-06 10:26:16 UTC
(In reply to Jonathan Druart from comment #2)
> Mathieu,
> Not sure it is a good idea to factorize like that.
> Now, some routines did 1, 2 or 3 join, with your patch, they will always do
> 8 join.
> That will increase the load in acquisition pages.

In fact, what first disturbed me was that the same fields were not named in the same way in GetOrder and GetCancelledOrders and GetOrders (publishercode, rrp...). And working on that, I tried to factorize all the code.
But you are right, in terms of performance it is not a good solution.

Maybe the 2 subs which could be safely factorized are GetCancelledOrders and GetOrders, as they differ only by the exclusion of an order status.
Comment 4 Mathieu Saby 2013-11-09 13:52:16 UTC
In fact, the 1st step is adding UT to current master. Then, I will maybe work again on that bug, and more surely on bug 10758.
Setting this one in discussion

See Bug 11224 for new UTs

Mathieu
Comment 5 Chris Cormack 2014-05-13 03:48:13 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2014-05-13 03:49:04 UTC
The content of attachment 28192 [details] has been deleted for the following reason:

wrong bug