http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7162
Katrin Fischer <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #36 from Katrin Fischer <[email protected]> --- Starting with some tests and a code review for these patches: 1) There are some problems with those patches pointed out by the QA script: FAIL acqui/addorder.pl OK pod FAIL forbidden patterns forbidden pattern: tab char (line 126) OK valid OK critic FAIL acqui/cancelorder.pl OK pod OK forbidden patterns FAIL valid Use of qw(...) as parentheses is deprecated OK critic 2) cancelorder.pl notes this: +# FIXME: C4::Search is needed by C4::Items::GetAnalyticsCount, which is called +# by C4::Acquisition::DelOrder. But C4::Search is not imported by C4::Items. +# Once this problem is resolved, the following line can be removed. +# See Bug 7847. +use C4::Search; 7847 is now marked resolved, can this line be removed? 3) From looking at the code it seems like you no longer have the choice to delete the bibliographic record or to keep it. 4) Some little typos: canceled -> cancelled occured -> occurred 5) Just a pet peeve of mine: a linked 'here' is not friendly if you are quickly scanning a page for the right link. It's always better to link the words that actually describe what the link will do: <p>Click <a href="[% referrer %]">here</a> to return to previous page</p> Could just be a linked: Return to previous page Or maybe just a 'OK'? 6) A bad one: Please please, don't add untranslatable strings to the database: + if($reason) { + $query .= " + , notes = IF(notes IS NULL, + CONCAT('Cancellation reason: ', ?), + CONCAT(notes, ' - Cancellation reason: ', ?) + ) + "; + } We really need to take care of that in a nicer way or at least not have any hardcoded string in there or have it in the template. It might be worth having an authorized value and a separate field to store cancellation reasons as you might want to look up the reasons in reports in a nice way later on. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
