https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17047
Jonathan Druart <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #162 from Jonathan Druart <[email protected]> --- 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. _______________________________________________ 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/
