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.
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
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
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
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>
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.
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.
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.
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.
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.
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.
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.
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.
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
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>
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>
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.
QAing
$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?
I am bit hesitant to PQA this patch. But cannot FQA it either. Would be nice to get another QA perspective on this one.
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 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.
(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.
Unfortunatly, this patch doesn't apply anymore
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
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...
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>
Worked as expected!
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>
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>
Please tidy your patches. Specially when you introduce entirely new files.
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.
Created attachment 158210 [details] [review] Bug 32730: Add FIXME in POD to highlight return value Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x