https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Michael, some notes: 1) Naming of the new method Your patch adds a GetListsWithPatron to Koha/List/Patron.pm. This module is not really written to our coding standards, but the method name should be snake case instead of camel case. PERL9: Subroutine naming conventions Koha namespace [current] subroutines in the Koha namespace should be snake case. 2) Location of the new method I also wonder if Koha/List/Patron is the right spot (some other devs might want to weigh in), but I would have expected this in Koha/Patron. There we also have other methods like get_club_enrollments or get_routing_lists that serve similar purposes. 3) Missing unit tests PERL17: Unit tests are required (updated Apr 26, 2017) Unit tests must be provided for *ALL* new routines in modules, as well as for changes to existing routines 4) Capitalization + <a id="pat_lists-tab-link" href="#pat_lists-tab" aria-controls="pat_lists-tab" role="tab" data-toggle="tab"> + Patron Lists ([% patron_lists_count | html %]) + </a> This should be "Patron lists" (we have a rule to only capitalize the first word) 5) Templates This is a little hard to understand/translate, wonder if we could rephrase a little (but that might be on me, I needed a few takes to get it) + <h4>Patron lists currently in</h4> Patron lists without/with this patron Lists this patron is/is not on ? 6) Copyright Is this correct for the new file? +++ b/patron_lists/patron-lists-tab.pl @@ -0,0 +1,91 @@ +#!/usr/bin/perl + +# Copyright 2013 ByWater Solutions +# 7) I wonder about the use case of showing the lists the patron is not in and if that could not get a little much in a big system. Not a blocker, just wondering and wondering if a different presentation, like a 'patron list search' could be interesting. -- You are receiving this mail because: You are watching all bug changes.