[Bug 32730] New: Add Patron Lists tab to patron details and circulation pages.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Bug ID: 32730 Summary: Add Patron Lists tab to patron details and circulation pages. Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: michael.hafen@washk12.org QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |michael.hafen@washk12.org |ity.org | Status|NEW |Needs Signoff --- Comment #1 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 145668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145668&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #2 from Owen Leonard <oleonard@myacpl.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145668|0 |1 is obsolete| | --- Comment #3 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 145710 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145710&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145710|0 |1 is obsolete| | --- Comment #4 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 145744 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145744&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #5 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 145745 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145745&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #6 from Michael Hafen <michael.hafen@washk12.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145744|0 |1 is obsolete| | Attachment #145745|0 |1 is obsolete| | --- Comment #7 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 145768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145768&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | Summary|Add Patron Lists tab to |Add Patron lists tab to |patron details and |patron details and |circulation pages. |circulation pages -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #8 from Katrin Fischer <katrin.fischer@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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Add Patron lists tab to |Add patron lists tab to |patron details and |patron details and |circulation pages |circulation pages -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #9 from Michael Hafen <michael.hafen@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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145768|0 |1 is obsolete| | --- Comment #10 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 145977 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145977&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #145977|0 |1 is obsolete| | --- Comment #11 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 146039 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=146039&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Laura.escamilla@bywatersolu | |tions.com --- Comment #12 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #13 from Michael Hafen <michael.hafen@washk12.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #14 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 147981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147981&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #147981|0 |1 is obsolete| | --- Comment #15 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 147982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=147982&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |32105 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32105 [Bug 32105] Patron lists needs to share to specific people or groups -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #146039|0 |1 is obsolete| | Attachment #147982|0 |1 is obsolete| | --- Comment #16 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 149477 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149477&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |BLOCKED QA Contact|katrin.fischer@bsz-bw.de |m.de.rooy@rijksmuseum.nl --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QAing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- $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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I am bit hesitant to PQA this patch. But cannot FQA it either. Would be nice to get another QA perspective on this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #21 from Michael Hafen <michael.hafen@washk12.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |oleonard@myacpl.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off CC| |tomascohen@gmail.com QA Contact|m.de.rooy@rijksmuseum.nl |testopia@bugs.koha-communit | |y.org --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Eric Bégin <eric.begin@inLibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply CC| |eric.begin@inLibro.com --- Comment #23 from Eric Bégin <eric.begin@inLibro.com> --- Unfortunatly, this patch doesn't apply anymore -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149477|0 |1 is obsolete| | --- Comment #24 from Michael Hafen <michael.hafen@washk12.org> --- Created attachment 154672 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154672&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Michael Hafen <michael.hafen@washk12.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Katie Bliss <kebliss@dmpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kebliss@dmpl.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Stina Hallin <stina.hallin@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |stina.hallin@ub.lu.se --- Comment #25 from Stina Hallin <stina.hallin@ub.lu.se> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154672|0 |1 is obsolete| | --- Comment #26 from Lisette Scheer <lisette.scheer@bywatersolutions.com> --- Created attachment 155575 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155575&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Lisette Scheer <lisette.scheer@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette.scheer@bywatersolut | |ions.com --- Comment #27 from Lisette Scheer <lisette.scheer@bywatersolutions.com> --- Worked as expected! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #28 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 155589 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155589&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155575|0 |1 is obsolete| | Attachment #155589|0 |1 is obsolete| | --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 157992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157992&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #30 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Please tidy your patches. Specially when you introduce entirely new files. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157992|0 |1 is obsolete| | --- Comment #31 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 158209 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158209&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #32 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 158210 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158210&action=edit Bug 32730: Add FIXME in POD to highlight return value Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #33 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 158211 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158211&action=edit Bug 32730: Add tests for Koha::Patron->get_lists_with_patron Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 --- Comment #34 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Needs documenting --- Comment #35 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 23.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38124 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38124 [Bug 38124] Don't show patron lists tab if there are no patron lists -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32730 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Documentation| |Caroline Cyr La Rose contact| | Documentation| |https://gitlab.com/koha-com submission| |munity/koha-manual/-/merge_ | |requests/978 Status|Needs documenting |RESOLVED Resolution|--- |FIXED CC| |caroline.cyr-la-rose@inlibr | |o.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org