[Bug 8240] New: There is dead code in orderreceive.tt
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Priority: P5 - low Change sponsored?: --- Bug ID: 8240 Assignee: koha-bugs@lists.koha-community.org Summary: There is dead code in orderreceive.tt Severity: enhancement Classification: Unclassified OS: All Reporter: jonathan.druart@biblibre.com Hardware: All Status: NEW Version: unspecified Component: Acquisitions Product: Koha The function C4::Acquisition::SearchOrder can't return more than 1 result with a given ordernumber. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |jonathan.druart@biblibre.co |ity.org |m -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 10244 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10244&action=edit Bug 8240: Remove dead code from orderreceive I think this code is useless and can be removed. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |8101 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10244|0 |1 is obsolete| | --- Comment #2 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 10329 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10329&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |katrin.fischer@bsz-bw.de -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #3 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- CONFLICT (content): Merge conflict in acqui/orderreceive.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10329|0 |1 is obsolete| | --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 10479 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10479&action=edit [SIGNED-OFF] Bug 8240: Remove dead code from orderreceive rebased patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master QA Contact| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #5 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #6 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 --- Comment #7 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 --- Comment #8 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 11768 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=11768&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 --- Comment #9 from Jonathan Druart <jonathan.druart@biblibre.com> --- This patch removes the parameter 'receive' which is useless. The page parcel.pl have to be used to search an order to receive. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #10479|0 |1 is obsolete| | Attachment #11768|0 |1 is obsolete| | --- Comment #10 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 12816 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12816&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #11 from Jonathan Druart <jonathan.druart@biblibre.com> --- The previous rebased and squashed patch require another SO please. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #12816|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 12943 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12943&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch --- Comment #13 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking at this report now.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #14 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Tested with AcqCreateItem on cataloging. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #12943|0 |1 is obsolete| | --- Comment #15 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 13904 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=13904&action=edit [SIGNED_OFF] Patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #16 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 --- Comment #17 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8240 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #18 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org