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
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
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 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.
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
Created attachment 28192 [details] Bug 12206: fix HTML in other holdings list in staff search results page To test: [1] View, in the staff interface, a record with alternate holdings (as controlled by the AlternateHoldingsField system preference). [2] Verify that the HTML list is closed correctly. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
The content of attachment 28192 [details] has been deleted for the following reason: wrong bug