[Koha-bugs] [Bug 32730] Add patron lists tab to patron details and circulation pages

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

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

--- Comment #9 from Michael Hafen <michael.hafen at 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.