Bug 32730 - Add patron lists tab to patron details and circulation pages
Summary: Add patron lists tab to patron details and circulation pages
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Michael Hafen
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 32105
  Show dependency treegraph
 
Reported: 2023-01-25 20:08 UTC by Michael Hafen
Modified: 2023-11-08 08:38 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 32730 - Add Patron Lists tab to patron details and circulation pages (23.33 KB, patch)
2023-01-25 20:15 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730 - Add Patron Lists tab to patron details and circulation pages (23.42 KB, patch)
2023-01-26 23:13 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (23.42 KB, patch)
2023-01-27 13:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32730: (follow-up) Minor corrections (17.71 KB, patch)
2023-01-27 13:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.06 KB, patch)
2023-01-27 18:15 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.79 KB, patch)
2023-02-01 19:32 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.74 KB, patch)
2023-02-02 20:49 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.77 KB, patch)
2023-03-08 19:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.77 KB, patch)
2023-03-08 19:43 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (24.74 KB, patch)
2023-04-11 20:45 UTC, Michael Hafen
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (18.03 KB, patch)
2023-08-18 23:40 UTC, Michael Hafen
Details | Diff | Splinter Review
Attachment to Bug 32730 - Add patron lists tab to patron details and circulation pages (18.03 KB, patch)
2023-09-12 20:31 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (18.08 KB, patch)
2023-09-13 11:05 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (18.11 KB, patch)
2023-10-27 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32730: Add Patron Lists tab to patron details and circulation pages (18.25 KB, patch)
2023-11-01 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32730: Add FIXME in POD to highlight return value (774 bytes, patch)
2023-11-01 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32730: Add tests for Koha::Patron->get_lists_with_patron (2.15 KB, patch)
2023-11-01 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Hafen 2023-01-25 20:08:19 UTC
Add a tab showing Patron Lists to the patron details and circulation pages.  The tab shows patron lists the patron is in and lists available to the logged in user to enable quickly adding a patron to a list.
Comment 1 Michael Hafen 2023-01-25 20:15:58 UTC
Created attachment 145668 [details] [review]
Bug 32730 - Add Patron Lists tab to patron details and circulation pages

Bug 32730 - Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the Patron Lists tab showing the list that the patron is not in 
5. try adding the patron to the list and removing them from the list to be
   sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation page
   for this patron
7. observe the Patron Lists tab, and verify it operates as it did on the
   patron details page
Comment 2 Owen Leonard 2023-01-26 12:54:15 UTC
I get this error when I try to add a patron to a list from the new tab:

Can't use string ("0") as a HASH ref while "strict refs" in use at /kohadevbox/koha/patron_lists/patron-lists-tab.pl line 54
Comment 3 Michael Hafen 2023-01-26 23:13:01 UTC
Created attachment 145710 [details] [review]
Bug 32730 - Add Patron Lists tab to patron details and  circulation pages

Here is a fresh patch with a couple bugs fixed

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the Patron Lists tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to be
   sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation page
   for this patron
7. observe the Patron Lists tab, and verify it operates as it did on the
   patron details page
Comment 4 Owen Leonard 2023-01-27 13:50:42 UTC
Created attachment 145744 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the Patron Lists tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the Patron Lists tab, and verify it operates as it did on the
   patron details page

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Owen Leonard 2023-01-27 13:50:45 UTC
Created attachment 145745 [details] [review]
Bug 32730: (follow-up) Minor corrections

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.
Comment 6 Michael Hafen 2023-01-27 18:03:19 UTC
Sorry, I found another bug with this patch.  In the GetListsWithPatron() the early return causes problems with how the function is used, and is unnecessary.  New patch forthcoming.
Comment 7 Michael Hafen 2023-01-27 18:15:40 UTC
Created attachment 145768 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and  circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the Patron Lists tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the Patron Lists tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.
Comment 8 Katrin Fischer 2023-01-30 21:34:35 UTC
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.
Comment 9 Michael Hafen 2023-02-01 18:52:09 UTC
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.
Comment 10 Michael Hafen 2023-02-01 19:32:15 UTC
Created attachment 145977 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and  circulation pages

Fresh patch with updates as suggested by QA  (this is my first time writing a unit test).

Test plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.
Comment 11 Michael Hafen 2023-02-02 20:49:15 UTC
Created attachment 146039 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and  circulation pages

(rebased 2023-02-02)

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.
Comment 12 Laura Escamilla 2023-03-06 14:11:55 UTC
Followed steps 1-3, on step 4 the Patron List tab is showing under the details page but the tab shows Patron lists () instead of Patron lists (1). Whenever I click the Patrons list () tab, it just shows a message of "Loading" but it does not actually load anything.
Comment 13 Michael Hafen 2023-03-07 19:28:16 UTC
Odd, I was able to follow the test plan all the way through on an up-to-date branch of master.
I'm surprised that both the tab load and the list count on the tab aren't working.  I expected they were distinct enough that one or the other would work.  Unless there's an error in the get_lists_with_patron function, both the tab and the tab contents use that.

Are you running with Plack?  It's possible that Plack didn't pick up some changes, that could explain why this isn't working.

Are you able to see the error logs?  Should be either /var/log/koha/[instance]/intranet-error.log or /var/log/koha/[instance]/plack-intranet-error.log
Comment 14 ByWater Sandboxes 2023-03-08 19:43:40 UTC
Created attachment 147981 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 15 ByWater Sandboxes 2023-03-08 19:43:50 UTC
Created attachment 147982 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
Comment 16 Michael Hafen 2023-04-11 20:45:25 UTC
Created attachment 149477 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and  circulation pages

[Rebased 2023-04-10]

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.
Comment 17 Marcel de Rooy 2023-05-09 13:07:35 UTC
QAing
Comment 18 Marcel de Rooy 2023-05-09 14:27:30 UTC
$paton->get_lists_with_patron
Looking at package Koha::List::Patron, it feels to me that it better should live there. No absolute blocker.
That package is no Koha::Object btw. Explaining probably too why you add DBIx code
 Seeing some calls that only need the count of this result btw.

+<div class="patroninfo-section">
+    <h4>Patron lists without this patron</h4>
Looks like this section contains a lot of duplicated code? Could probably be merged in some named block.
Not sure why we need two tables, but that seems just a matter of taste.

    @available_lists = GetPatronLists();
    @available_lists = grep { ! $list_id_lookup{$_->patron_list_id} } @available_lists;
[...]
    in_lists => \@in_lists,
    list_id_lookup => \%list_id_lookup,
    available_lists => \@available_lists,
This makes me even more wonder why we are using get_lists_with_patron. You are selecting the reverse here?
Comment 19 Marcel de Rooy 2023-05-09 14:28:46 UTC
I am bit hesitant to PQA this patch. But cannot FQA it either. Would be nice to get another QA perspective on this one.
Comment 20 Marcel de Rooy 2023-05-09 14:51:26 UTC
Asked for a second perspective, tcohen replied on IRC:

[16:40] <tcohen> I don't like those method names
[16:40] <tcohen> well, name
[16:41] <tcohen> or plain DBIC
[16:41] <tcohen> it needs more work
[16:41] <tcohen> FQA
[16:41] <tcohen> it would be good to give a list of changes
[16:41] <tcohen> to encourage them to keep working on it
Comment 21 Michael Hafen 2023-05-15 16:54:38 UTC
Comment 8 on this bug mentioned the method name, but only that it needed to be snake case, which it now is.

Same comment also mentioned the get_lists_with_patron() method and suggested putting it in Koha/Patron.  I'm fine either way; as a Patron method it would need a Patron object, as a List method it would need the borrowernumber or patron object passed as a parameter.  Either way works for me.

There is a lot of apparent duplication in the patron lists tables, but there is a purpose.  It's mostly for convenience.  One table show the lists the patron is in, and needs the get_lists_with_patron() call.  The second table shows the available lists the patron is not in; this is for the convenience of quickly adding the patron to one of those lists.  So it calls get_lists() and then culls out the lists the patron is already in.  If this convenience is deemed to system heavy to be feasible it can be pulled out.  Or the second table could be reduced to a drop down with list names.

This package isn't a Koha Object, as you noted, so there's a lot of DBIC.  The specialized nature of some of those methods, in that they have to dig into the patronlistpatrons table, makes me think that's the best way to handle it either way.  I don't think get_lists_with_patron() could be done efficiently without going to DBIC.

I'd like to see some suggestions for a better method name, I think it's got a pretty good name myself.  It's very descriptive.
Comment 22 Marcel de Rooy 2023-06-02 07:52:17 UTC
(In reply to Michael Hafen from comment #21)
> Comment 8 on this bug mentioned the method name, but only that it needed to
> be snake case, which it now is.
> 
> Same comment also mentioned the get_lists_with_patron() method and suggested
> putting it in Koha/Patron.  I'm fine either way; as a Patron method it would
> need a Patron object, as a List method it would need the borrowernumber or
> patron object passed as a parameter.  Either way works for me.
> 
> There is a lot of apparent duplication in the patron lists tables, but there
> is a purpose.  It's mostly for convenience.  One table show the lists the
> patron is in, and needs the get_lists_with_patron() call.  The second table
> shows the available lists the patron is not in; this is for the convenience
> of quickly adding the patron to one of those lists.  So it calls get_lists()
> and then culls out the lists the patron is already in.  If this convenience
> is deemed to system heavy to be feasible it can be pulled out.  Or the
> second table could be reduced to a drop down with list names.
> 
> This package isn't a Koha Object, as you noted, so there's a lot of DBIC. 
> The specialized nature of some of those methods, in that they have to dig
> into the patronlistpatrons table, makes me think that's the best way to
> handle it either way.  I don't think get_lists_with_patron() could be done
> efficiently without going to DBIC.
> 
> I'd like to see some suggestions for a better method name, I think it's got
> a pretty good name myself.  It's very descriptive.

Thanks for your feedback.
Tomas may be in a better position to provide further clarification of his commments.
I am moving this one back in the SO queue to get another QA perspective.
Comment 23 Eric Bégin 2023-08-18 09:13:19 UTC
Unfortunatly, this patch doesn't apply anymore
Comment 24 Michael Hafen 2023-08-18 23:40:56 UTC
Created attachment 154672 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

[rebased 2023-08-18]
[ changed the second table, lists patron is not in, to a select for adding the patron to a list ]

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the Patron Lists tab showing the list that the patron is not in 
5. try adding the patron to the list and removing them from the list to be
   sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation page
   for this patron
7. observe the Patron Lists tab, and verify it operates as it did on the
   patron details page
Comment 25 Stina Hallin 2023-09-11 09:43:36 UTC
I tried all the steps in a sandbox, but at step 7, when clicking on Patron List (1) it just shows message Loading... and nothing happens. When going to a patron that is not in a list, it's the same behaviour. Patron List (0) and the message Loading...
Comment 26 Lisette Scheer 2023-09-12 20:31:51 UTC
Created attachment 155575 [details] [review]
Attachment to Bug 32730 - Add patron lists tab to patron details and circulation pages

Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: Lisette Scheer <lisette.scheer@bywatersolutions.com>
Comment 27 Lisette Scheer 2023-09-12 20:32:18 UTC
Worked as expected!
Comment 28 ByWater Sandboxes 2023-09-13 11:05:56 UTC
Created attachment 155589 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: John Doe <you@example.com>
Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 29 Kyle M Hall 2023-10-27 16:19:54 UTC
Created attachment 157992 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Tomás Cohen Arazi 2023-11-01 19:11:49 UTC
Please tidy your patches. Specially when you introduce entirely new files.
Comment 31 Tomás Cohen Arazi 2023-11-01 19:15:37 UTC
Created attachment 158209 [details] [review]
Bug 32730: Add Patron Lists tab to patron details and circulation pages

Test Plan:
1. Apply patch
2. create a patron list if there aren't any
3. search for a patron
4. observe the "Patron lists" tab showing the list that the patron is not in
5. try adding the patron to the list and removing them from the list to
   be sure the feature has full operation
6. click "Check Out" on the side bar menu to navigate to the circulation
   page for this patron
7. observe the "Patron lists" tab, and verify it operates as it did on the
   patron details page

Bug 32730: (follow-up) Minor corrections
   by Owen Leonard

This patch corrects an instance of an incorrect capital letter
("Patron Lists" -> "Patron lists") and makes minor tweaks to
indentation.

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Edit: (tcohen) new files should be run through perltidy ALWAYS. Did it
and squashed it here.
Comment 32 Tomás Cohen Arazi 2023-11-01 19:15:44 UTC
Created attachment 158210 [details] [review]
Bug 32730: Add FIXME in POD to highlight return value

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2023-11-01 19:57:08 UTC
Created attachment 158211 [details] [review]
Bug 32730: Add tests for Koha::Patron->get_lists_with_patron

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2023-11-01 20:26:23 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 35 Fridolin Somers 2023-11-08 08:38:51 UTC
Enhancement not pushed to 23.05.x