Bug 8240 - There is dead code in orderreceive.tt
Summary: There is dead code in orderreceive.tt
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: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 8101
  Show dependency treegraph
 
Reported: 2012-06-12 07:49 UTC by Jonathan Druart
Modified: 2013-05-23 06:24 UTC (History)
3 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 8240: Remove dead code from orderreceive (10.84 KB, patch)
2012-06-12 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive (11.15 KB, patch)
2012-06-17 08:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive (11.21 KB, patch)
2012-06-25 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8240: Followup: the receive param is never used and can be deleted (1.86 KB, patch)
2012-08-23 09:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8240: Remove dead code from orderreceive (13.84 KB, patch)
2012-10-15 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8240: Remove dead code from orderreceive (13.93 KB, patch)
2012-10-19 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED_OFF] Patch (13.99 KB, patch)
2012-12-06 13:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review

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