[Koha-bugs] [Bug 17047] Mana Knowledge Base : share data

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17047

Jonathan Druart <jonathan.druart at bugs.koha-community.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Passed QA                   |Failed QA

--- Comment #162 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
Quick code review:
1/ Please squash patches when possible to ease readability
2/ Some strings are not translatable (at least "Subscription found on Mana
Knowledge Base")
3/ Please fix indentation
4/ Remove use of onclick attribute
5/ 
-       [% INCLUDE 'serials-toolbar.inc' %]
+    [% INCLUDE 'serials-toolbar.inc' mana_id = mana_id %]
Not sure it is needed
6/ +        'mana_success' => $input->param('mana_success'),
CGI->param raises warning called in list context (there are other occurrences)
7/ subroutines added in serials/subscription-add.pl smell
8/ serials/subscription-add.pl
+    my $mana_id;
+    if ( $query->param('mana_id') ne "" ) {
+        $mana_id = $query->param('mana_id');
+        Koha::SharedContent::manaIncrementRequest("subscription",$mana_id,
"nbofusers");
+    }
+    else {
+        $mana_id = undef;
+    }

=> the else block is useless
9/ Too many "use" statements in serials/subscription-detail.pl
10/ script names in svc/mana are not consistent
11/ 
+my $templatename;
+$templatename = "mana/mana-".$input->param( "resource" )."-search-result.tt";
That smells too. There are only 2 files, list them (same for other
occurrences).
12/ Test coverage for Koha::SharedContent is nonexistent
13/ Why tests have been removed from
t/db_dependent/Serials/GetFictiveIssueNumber.t
14/ No reference of mana_config from debian/templates/koha-conf-site.xml.in

-- 
You are receiving this mail because:
You are watching all bug changes.