Summary: | There is dead code in orderreceive.tt | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Acquisitions | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | 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: | ||
Circulation function: | |||
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
Created attachment 10244 [details] [review] Bug 8240: Remove dead code from orderreceive I think this code is useless and can be removed. Created attachment 10329 [details] [review] [SIGNED-OFF] Bug 8240: Remove dead code from orderreceive Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch removes a lot of code from acquisitions. I tested 2 complete acquisition workflows from ordering the item to receiving it, with AcqCreateItem set to 'on order' and to 'on receive'. Both worked without any visible changes after applying the patch. CONFLICT (content): Merge conflict in acqui/orderreceive.pl Created attachment 10479 [details] [review] [SIGNED-OFF] Bug 8240: Remove dead code from orderreceive rebased patch 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. (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. (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. Created attachment 11768 [details] [review] Bug 8240: Followup: the receive param is never used and can be deleted To search an order for receiving, the new page parcel.pl is more adapted. This patch removes the parameter 'receive' which is useless. The page parcel.pl have to be used to search an order to receive. Created attachment 12816 [details] [review] Bug 8240: Remove dead code from orderreceive Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch removes a lot of code from acquisitions. I tested 2 complete acquisition workflows from ordering the item to receiving it, with AcqCreateItem set to 'on order' and to 'on receive'. Both worked without any visible changes after applying the patch. The previous rebased and squashed patch require another SO please. Created attachment 12943 [details] [review] [SIGNED-OFF] Bug 8240: Remove dead code from orderreceive Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Patch removes a lot of code from acquisitions. I tested 2 complete acquisition workflows from ordering the item to receiving it, with AcqCreateItem set to 'on order' and to 'on receive'. Both worked without any visible changes after applying the patch. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Looking at this report now.. Tested with AcqCreateItem on cataloging. Created attachment 13904 [details] [review] [SIGNED_OFF] Patch 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.. (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.. This patch has been pushed to master. |