https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730
--- Comment #9 from Michael Hafen <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] https://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/
