[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

Katrin Fischer <katrin.fischer at bsz-bw.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA
                 CC|                            |martin.renvoize at ptfs-europe
                   |                            |.com

--- Comment #8 from Katrin Fischer <katrin.fischer at 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.