https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #9 from Michael Hafen <michael.hafen@washk12.org> --- I will prepare another patch. I'll fix the name of the new method and move it to Koha/Patron.pm, I will fix the "Patron Lists" capitalization on the tab, I'll rephrase the list table headers as you suggested, I'll fix the copyright. The size of the lists could be a problem. I'll make sure that both tables on the lists tab are paged and searchable, that should help with the presentation at least. I like the convenience of having the other lists at hand to quickly add a patron to them. (In reply to Katrin Fischer from comment #8)
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.