Bug 8240

Summary: There is dead code in orderreceive.tt
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8101    
Attachments: Bug 8240: Remove dead code from orderreceive
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive
Bug 8240: Followup: the receive param is never used and can be deleted
Bug 8240: Remove dead code from orderreceive
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive
[SIGNED_OFF] Patch

Description Jonathan Druart 2012-06-12 07:49:01 UTC
The function C4::Acquisition::SearchOrder can't return more than 1 result with a given ordernumber.
Comment 1 Jonathan Druart 2012-06-12 07:50:49 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2012-06-17 08:50:12 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2012-06-25 13:14:46 UTC
CONFLICT (content): Merge conflict in acqui/orderreceive.pl
Comment 4 Jonathan Druart 2012-06-25 13:46:54 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2012-06-27 13:56:40 UTC
Jonathan,
You are probably right about this code in practice. But theoretically, if ordernumber is empty, you may have more than one result.
In one of the templates below the ordernumber comes from parsing some text. What if this fails somehow?
And how about a user removing the ordernumber from the URL in the browser?

Was the original idea of this script: If we have one, receive it rightaway. If we have more, let the user choose (one more click to receive)? And just thinking: why not receive more items at once if the date is the same and the quantity is most often 1 ? Also keep in mind that not everybody creates the items at the same time (AcqCreateItem pref).

koha-tmpl/intranet-tmpl/prog/en/modules/acqui/spent.tt
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt

If we follow your approach here, I think we must add some check around ordernumber and results, and change a few lines of documentation in the code conveying the original idea of multiple results.

Changing status for need of clarification.
Comment 6 Jonathan Druart 2012-08-10 11:04:05 UTC
(In reply to comment #5)
> Changing status for need of clarification.

Hi Marcel,
For me, this kind of page needs an id (here ordernumber).
2 cases : 
- The id exists and the script can provide consistent results.
- The id is not defined (or is a bad id, ie. there is no result) and the template displays an error (here "This ordernumber does not exist.").

I think in the latter case the error message is necessary to inform the user (or developper) that the page should not exist.
Comment 7 Marcel de Rooy 2012-08-23 07:45:56 UTC
(In reply to comment #6)
> - The id is not defined (or is a bad id, ie. there is no result) and the
> template displays an error (here "This ordernumber does not exist.").
> 
> I think in the latter case the error message is necessary to inform the user
> (or developper) that the page should not exist.

I tested this with ordernumber= (making it blank)
In that case there are many results for the line:
my $results = SearchOrder($ordernumber,$search);

The script presented the first result (order 1).

I still think that you should add some checks for this situation.
Comment 8 Jonathan Druart 2012-08-23 09:03:49 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-08-23 09:07:58 UTC
This patch removes the parameter 'receive' which is useless. The page parcel.pl have to be used to search an order to receive.
Comment 10 Jonathan Druart 2012-10-15 14:18:34 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-10-15 14:19:20 UTC
The previous rebased and squashed patch require another SO please.
Comment 12 Kyle M Hall 2012-10-19 13:20:53 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2012-12-06 12:41:09 UTC
Looking at this report now..
Comment 14 Marcel de Rooy 2012-12-06 13:10:39 UTC
Tested with AcqCreateItem on cataloging.
Comment 15 Marcel de Rooy 2012-12-06 13:11:18 UTC
Created attachment 13904 [details] [review]
[SIGNED_OFF] Patch
Comment 16 Marcel de Rooy 2012-12-06 13:14:28 UTC
Code looks good to me: Passed QA

Small point:
my $freight      = $invoice->{shipmentcost};
my $datereceived = $invoice->{shipmentdate};
As far as I can see, these parameters are not passed any more to the script via URL. (Not in spent.tt and parcel.tt)
So these lines are dead code too..
Comment 17 Marcel de Rooy 2012-12-06 13:15:56 UTC
(In reply to comment #16)
> Small point:
> my $freight      = $invoice->{shipmentcost};
> my $datereceived = $invoice->{shipmentdate};
> As far as I can see, these parameters are not passed any more to the script
> via URL. (Not in spent.tt and parcel.tt)
> So these lines are dead code too..

Oops, freight is not used. Date received is..
Comment 18 Jared Camins-Esakov 2012-12-06 13:52:26 UTC
This patch has been pushed to master.