[Koha-bugs] [Bug 7162] Factorize code for order cancellation

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7162

Katrin Fischer <katrin.fischer at bsz-bw.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #36 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
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.