|
Description
Michael Hafen
2021-07-20 18:17:08 UTC
Created attachment 122991 [details] [review] Proposed fix Hey Micheal, Can you provide a test plan for testing this patch? Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Created attachment 129203 [details] [review] Bug 28726 - Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Signed-off-by: The Minh Luong <the-minh.luong@inlibro.com> Created attachment 134003 [details] [review] Bug 28726 - Add sort1 and sort2 to patron card creator - patron search. Add sort1 and sort2 filters, checkboxes and select/clear all links, and a add selected button to the patron search (for the patron card creator). Rebase after change to how the patron search works. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Hi Michaeal, could you please rebase against the master branch? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28726: Add sort1 and sort2 to patron card creator - patron search. error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc). error: could not build fake ancestor Patch failed at 0001 Bug 28726: Add sort1 and sort2 to patron card creator - patron search. hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-28726---Add-sort1-and-sort2-to-patron-card-cre-61BF84.patch Created attachment 137854 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. [ Rebased 2022-07-18 ] Please don't forget to switch back the status when you rebase :) I got this message after applying the patch (which is not displayed in the KohaGitLog) (I test using a sandbox) ------------------------------------------- Bug 28726 - Add sort1 and sort2 to patron card creator patron search 137854 - Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Apply? [(y)es, (n)o, (i)nteractive] Traceback (most recent call last): File "/usr/bin/git-bz", line 2716, in <module> applied = do_apply(bug_ref) File "/usr/bin/git-bz", line 1827, in do_apply users = bug.server.get_xmlrpc_proxy().User.get({ 'names': [patch.attacher] })['users'] File "/usr/lib/python2.7/xmlrpclib.py", line 1243, in __call__ return self.__send(self.__name, args) File "/usr/lib/python2.7/xmlrpclib.py", line 1602, in __request verbose=self.__verbose File "/usr/lib/python2.7/xmlrpclib.py", line 1283, in request return self.single_request(host, handler, request_body, verbose) File "/usr/lib/python2.7/xmlrpclib.py", line 1331, in single_request response.msg, xmlrpclib.ProtocolError: <ProtocolError for bugs.koha-community.org/xmlrpc.cgi: 404 Not Found> (In reply to Emmanuel Bétemps from comment #9) > I got this message after applying the patch (which is not displayed in the > KohaGitLog) > (I test using a sandbox) > > ------------------------------------------- > Bug 28726 - Add sort1 and sort2 to patron card creator patron search > > 137854 - Bug 28726: Add sort1 and sort2 to patron card creator - patron > search. > > Apply? [(y)es, (n)o, (i)nteractive] Traceback (most recent call last): > File "/usr/bin/git-bz", line 2716, in <module> > applied = do_apply(bug_ref) > File "/usr/bin/git-bz", line 1827, in do_apply > users = bug.server.get_xmlrpc_proxy().User.get({ 'names': > [patch.attacher] })['users'] > File "/usr/lib/python2.7/xmlrpclib.py", line 1243, in __call__ > return self.__send(self.__name, args) > File "/usr/lib/python2.7/xmlrpclib.py", line 1602, in __request > verbose=self.__verbose > File "/usr/lib/python2.7/xmlrpclib.py", line 1283, in request > return self.single_request(host, handler, request_body, verbose) > File "/usr/lib/python2.7/xmlrpclib.py", line 1331, in single_request > response.msg, > xmlrpclib.ProtocolError: <ProtocolError for > bugs.koha-community.org/xmlrpc.cgi: 404 Not Found> To be that looks like a problem with the koha-community.org git server. Hi Emmanuel, what kind of environment are you using for testing? Ok, I have the same error. Chris said it could be related to git-bz not working with ipv6. (In reply to Katrin Fischer from comment #12) > Ok, I have the same error. Chris said it could be related to git-bz not > working with ipv6. Ok, we did some more testing and git-bz works on other bugs. I am not sure what's causing the problem here, but it appears to be bug specific and so probably not network related. Michael, please check the attache file, this doesn't appear to be a correct patch file, but only the commit message. Created attachment 138552 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Re-upload (2022-08-03). Don't know how the previous one was uploaded as just the commit message. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Created attachment 138653 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> + @sort1 = sort {$a cmp $b} uniq( Koha::Patrons->search($sort_filter)->get_column('sort1') );
This is terrible if you have lot of patrons. You should at least do the unique (distinct) at the DBMS level.
What about the following suggestion: you could display sort1 and sort2 authorised values in the dropdown list, or simply display a search input.
You could also add it to the table's headers (filter at the top of the column).
(In reply to Jonathan Druart from comment #17) > + @sort1 = sort {$a cmp $b} uniq( > Koha::Patrons->search($sort_filter)->get_column('sort1') ); > > This is terrible if you have lot of patrons. You should at least do the > unique (distinct) at the DBMS level. > > What about the following suggestion: you could display sort1 and sort2 > authorised values in the dropdown list, or simply display a search input. > You could also add it to the table's headers (filter at the top of the > column). sort1 can be free text or an AV. The AV category always exists, but maybe we could check for existing values. If values exist: show a drop down with the values. If there are no entries for the AV, use a free text input field. That would match the behavior on the patron edit form. Thanks Katrin, that's exactly what I wanted to say but some parts got lost when I rephrased the comment apparently. Hi Michaeal, could you take a look at changing the form as suggested in comment#19? The patron edit form could be a good starting place for this. I realize there is another place in Koha at least where it's done like you do here - the statistics wizard for patrons - but I believe it would be worth changing and changing it there too. I can easily see a library using the field for individual notes. To explain a little more: even with just the sample data the pull down is a little hard to handle. If we get over 100 entries there, it will be much harder. I'll take a look, but it will mean a big change to how I'm handling patrons, as my use case is a little different. I'm in a school district, and all our patron info comes in from LDAP. We aren't using the Authorized Values categories for the sort fields. I wasn't even planning on trying to use them since our servers handle building those drop down pretty well as is. We also have Independant Branches on, so the list gets filtered by branchcode, so I'd also have to see if the Authorized Values categories can do that too. We have 72079 patrons with 2975 unique sort2 values, but each library has no more than 200 values in the sort2, and it's usually closer to or less than 100 values. Even as the SuperLibrarian, when I load the patron card create patron search window it doesn't take more than a second on my servers, so I've been fine with it that way. I can see that this isn't optimal, but the alternative is creating a cron script to scrape LDAP to pull all the possible values for those two attributes to sync the Authorized Values categories. Anyway, I'll have to look at the Authorized Values stuff to see if it can meet our use case. And I don't mind changing the patch to use that instead, and then changing it again in my local fork to scrape the database instead. I cannot tell about performance, but UX wise, maybe switching to a Select2 style pull down could be a compromise (like the ones used in the edit item form). Created attachment 141405 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. [ 2022-10-05 updated for recommendations: AV and select2 ] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the on ly options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patro ns' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card b atch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Created attachment 143646 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com> I don't think we should be pre-populating a select pulldown from sort1/2. These fields could contain unique information, and a library could have tens or hundreds of thousands of patrons. In that case bad things (tm) would happen. I think it would be best of those fields were free text fields. (In reply to Kyle M Hall from comment #26) > I don't think we should be pre-populating a select pulldown from sort1/2. > These fields could contain unique information, and a library could have tens > or hundreds of thousands of patrons. In that case bad things (tm) would > happen. I think it would be best of those fields were free text fields. Just to clarify, if AV values exist, would you be ok with populating from the config (authorised values) instead of the data? If the category has no entries, we do freetext, otherwise a pull down. That matches the behviour in other spots, like the patron edit/display forms and the statistical wizard for patrons. The patch right now is trying the Authorized Values first, and if those are empty, then it hits the database. Kyle, are you suggesting an auto-complete style text input? I could do that instead, though I'd have to figure out how. That would still mean a lot of database queries though. Maybe a (not unique) index on each of those two columns? (assuming we don't already have one.) I don't think we can't avoid querying the database at this point. As long as sort1 and sort2 are free entry fields on the member entry page, we can't avoid querying the database to collect those values. And we need those values in order to use them as a filter when searching members. I think maybe it will be easier to keep it simple and have a text field or a pull down from AV depending on config first. As Kyle said sort fields can have unique values, so you will have an endless pull down with one entry per patron, this is not useful. Also: pull down and free text will match how this field is used in all other places in Koha (= existing pattern). Further improvements could then be made on a follow-up bug. Making it a free text field doesn't work for me. In my opinion having a free text input doesn't work well for a filter field on a search page; especially in my case where the librarians don't control that field, it's controlled by LDAP. Katrin mentioned the Patron Statistics Wizard. Looking at that, what's done for the sort2 column is to pull distinct values limited to 200. That would work for me, that should be well under what my librarians would encounter in that field. Would doing that instead be acceptable? (In reply to Michael Hafen from comment #30) > Making it a free text field doesn't work for me. In my opinion having a > free text input doesn't work well for a filter field on a search page; > especially in my case where the librarians don't control that field, it's > controlled by LDAP. > > Katrin mentioned the Patron Statistics Wizard. Looking at that, what's done > for the sort2 column is to pull distinct values limited to 200. That would > work for me, that should be well under what my librarians would encounter in > that field. > > Would doing that instead be acceptable? It won't work for any library that stored an identifier or unique value in there that has more than 200 patrons. They would see an incomplete pull down. Also: a 200 entry pull down is not really nice to use? Actually I was not aware the wizard did that :( we should fix it as well. I'll claim the patron form then as the example and the batch patron modification templates (sorry). Why will a free text not work? If they have no idea what they are looking for, will also not be very helpful? I think it's a specific use case we are dealing with here that I'd like to understand better. The patron entry form and the batch patron mod form are both for setting the value, which is a different circumstance from using a value as a filter in a patron search. For those entry forms having either the AV drop down or a free text input makes sense. For a patron search it's easier to use a drop down, where the librarian probably want's an exact match to a value used in the database. In the case of my libraries most of our patron information is controlled by LDAP, and the librarians aren't the ones entering the information in the LDAP Directory. So they have an idea what to look for, but stuff like capitalization, or whether first name or last name comes first, isn't known. Or remembering 40 to 90 values is a burden I would spare them from. Does that explain my use case? We'll need something that works universally. The auto-complete seemed like a good idea, but I'd like Kyle and others to comment on performance concerns. Koha's tables are case insensitive in the database - Koha doesn't care about it in searches that are SQL based. Created attachment 147477 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. [rebased 2023-02-27] [changed to pass QA - use av-build-dropbox.inc] Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. I didn't even realize that the sql search would be case-insensitive. Now that you mention it, of course it is. Anyway, here's a patch that just pulls from AV Bsort1 and Bsort2, using av-build-dropbox.inc, so it's either a select or an input. I can do otherwise on my local fork if it upsets my librarians (which I expect it will). Created attachment 148489 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Looking here If I am adding borrowers to the patron card batch, I am still seeing the box "There are no patrons in this batch" and I have no possibility to go further? Could be that I am missing config data for this module still, but this form should be more helpful. Probably out of scope, but it limits testing of this change. Let me try to amend the test plan with more detail. I was just able to apply this patch to a current master and get through the test plan successfully, but there are details missing from the test plan that could cause confusion if one isn't familiar with the module. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. This will open the 'Patron search' window. Observe that Category and Library are the only options. 3. Close 'Patron search' window. 4. Apply patch. Restart Plack, etc. 5. click the Add patron(s) button. This will reopen the 'Patron search' window. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search using a sort1 or sort2 value that will have multiple results. Observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field in the main window. 9. close the 'Patron search' window. Click the Add patron(s) button again to add the patrons to the batch. (In reply to Michael Hafen from comment #39) Thanks. > 8. use the Add selected patrons button to add the selected patrons to the > card batch's Add by borrowernumber(s) text input field in the main window. This is the confusing part. Since I clicked on that button before selecting the patrons, it is quite strange that you should click it again to kind of confirm the action you did. It is outside the scope of this report, but the operation is not intuitive. Created attachment 150650 [details] [review] Bug 28726: Add sort1 and sort2 to patron card creator - patron search. Also adds a select all / clear all and checkboxes. Test plan: 1. start a new batch in the patron card creator (Tools -> Patron card creator -> New -> card batch) 2. click the Add patron(s) button. Observe that Category and Library are the only options. 3. Close search for patron window. 4. Apply patch. 5. click the Add patron(s) button. Observe that you can now search for patrons by their sort1 and sort2 values. 6. perform a search and observe the 'Select all | Clear all | Add selected patrons' links and button. 7. use the Select all link to select all the patrons found by the search. 8. use the Add selected patrons button to add the selected patrons to the card batch's Add by borrowernumber(s) text input field. 9. close the search for patron window. Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 23.11. Nice work everyone, thanks! This patch is actually adding Sort1 and Sort2 to all patrons searches. (In reply to Jonathan Druart from comment #43) > This patch is actually adding Sort1 and Sort2 to all patrons searches. This is not coming from this patch. <div class="searchheader fh-fixedHeader" id="searchheader" style=""> This empty div is visible when I search for patrons in the popup: https://snipboard.io/2zBDoA.jpg I'm guessing you are seeing that on the patron search popup from a page other then the patron card creator. I'll open another bug with a follow up patch to move the whole div into the checkbox column condition. This has wider reaching impact than the title suggests.. and I'm not sure we want to force exposure of these fields for all cases.. as such I'm opting not to backport this one right now (though I am backporting most of the other patron search improvements as they go a long way towards fixing the bugs in patron search people have been highlighting when switching to 22.11.x) Needed for simple backport of 34094 and the tree above it. Nice work everyone! Pushed to oldstable for 22.11.x |
