Bug 10723 - GetPendingOrders and SearchOrders can be merged
Summary: GetPendingOrders and SearchOrders can be merged
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10124 10724 11777
  Show dependency treegraph
 
Reported: 2013-08-14 08:53 UTC by Jonathan Druart
Modified: 2014-12-07 20:07 UTC (History)
2 users (show)

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


Attachments
Bug 10723: Merge GetPendingOrders and SearchOrders routines (23.46 KB, patch)
2013-08-14 08:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10723: Merge GetPendingOrders and SearchOrders routines (23.59 KB, patch)
2013-08-16 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10723: SearchOrders should take into account the pending parameter. (739 bytes, patch)
2013-09-17 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-08-14 08:53:02 UTC
There two routines does the same thing. They can be merge into one routine and improved.
Comment 1 Jonathan Druart 2013-08-14 08:58:22 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-08-16 15:09:35 UTC
Created attachment 20420 [details] [review]
Bug 10723: Merge GetPendingOrders and SearchOrders routines

In the C4::Acquisition module, 2 routines do the same work. This patch
merges these 2 routines.

Test plan:
test the acqui/orderreceive.pl, acqui/uncertainprice.pl
and serials/acqui-search-result.pl, acqui/parcel.pl scripts.

Note: on acqui/parcel the basket filter is a search on basket name (was
on basket id, which was not relevant).

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa.pm, no adverse bahaviors noted. All sub calls updated.
Comment 3 Jonathan Druart 2013-09-17 14:59:54 UTC
Created attachment 21140 [details] [review]
Bug 10723: SearchOrders should take into account the pending parameter.
Comment 4 Galen Charlton 2013-10-21 18:51:12 UTC
Pushed to master.  Thanks, Jonathan!
Comment 5 Mathieu Saby 2013-11-03 19:54:37 UTC
Hi Jonathan, Hi Kyle

I am trying to resolve conflict on Bug 10758, so I came to your change on this one.
Don't you think the resulting routine could be make simpler ?

I see "publishercode AS publisher"
and 
aqorders.rrp              AS unitpricesupplier,
aqorders.ecost            AS unitpricelib,
aqorders.claims_count     AS claims_count,
aqorders.claimed_date     AS claimed_date,

I don't the the meaning of 
aqorders.claims_count     AS claims_count,
aqorders.claimed_date     AS claimed_date,

For
aqorders.rrp              AS unitpricesupplier,
aqorders.ecost            AS unitpricelib,
and
publishercode AS publisher

Is this renaming made for avoiding confusion somewhere in code?

By the way, I was trying to factorize some UT in Acquisition.t (not yet done), and one of my assumptions was that the fields returned by GetOrder were the same as each orders of GetOrderS and GetCancelledOrders. It was working, but of course it is no more the case. So I'm wondering if the fields added in the hased returned by GetOrder could be usefull too in GetOrderS and GetCancelledOrders.

Mathieu
Comment 6 Mathieu Saby 2013-11-03 19:57:59 UTC
ARGH
Sorry, I'm completely wrong  ;-)
What disturbs me is the change made to GetOrder, not to GetPendingOrders.


Mathieu