Bug 11206 - Factorize code for getting orders in C4::Acquisitions
Summary: Factorize code for getting orders in C4::Acquisitions
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-06 09:28 UTC by Mathieu Saby
Modified: 2023-06-08 22:26 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
[PATCH] Bug 11206 - Factorize code for getting orders in C4::Acquisitions (29.47 KB, patch)
2013-11-06 09:32 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 12206: fix HTML in other holdings list in staff search results page (deleted)
2014-05-13 03:48 UTC, Chris Cormack
Details

Note You need to log in before you can comment on or make changes to this bug.
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