We should have a single point of patron search logic for coherency and maintainability.
Created attachment 152682 [details] [review] Bug 34092: Move patron_autocomplete to staff-global.js Remove patron-autocomplete.js file
Created attachment 152683 [details] [review] Bug 34092: Preparation: New buildPatronSearchQuery function Move patron search query logic out of patron_autocomplete into new function
Created attachment 152684 [details] [review] Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
Test plan to test different patron searches and make sure everything is working as intended, before and after applying the patch. Examples of patron-autocomplete.js: - Patrons lists (create new list, add patrons to it): /cgi-bin/koha/patron_lists/lists.pl - View logs ("Librarian:" input): /cgi-bin/koha/tools/viewlog.pl - Reserve request: /cgi-bin/koha/reserve/request.pl Examples of patron-search.inc: - ERM, "Add new user" button: /cgi-bin/koha/erm/agreements/add - New patron card batch: /cgi-bin/koha/patroncards/edit-batch.pl
Created attachment 152698 [details] [review] Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
This needs a rebase on top of bug 34094. Coming soon.
Created attachment 152830 [details] [review] Bug 34092: Move patron_autocomplete to staff-global.js Remove patron-autocomplete.js file
Created attachment 152831 [details] [review] Bug 34092: Preparation: New buildPatronSearchQuery function Move patron search query logic out of patron_autocomplete into new function
Created attachment 152832 [details] [review] Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
Test plan to is test different patron searches and make sure everything is working as intended, before and after applying the patch. Examples of patron-autocomplete.js: - Patrons lists (create new list, add patrons to it): /cgi-bin/koha/patron_lists/lists.pl - View logs ("Librarian:" input): /cgi-bin/koha/tools/viewlog.pl - Reserve request: /cgi-bin/koha/reserve/request.pl Examples of patron-search.inc: - ERM, "Add new user" button: /cgi-bin/koha/erm/agreements/add - New patron card batch: /cgi-bin/koha/patroncards/edit-batch.pl (Click "add patron" button") Bonus: - Create a patron attribute type and set it "searchable". - Edit a patron and put some unique in that new patron attribute - Search for that, or part of that. More: Play with DefaultPatronSearchFields and DefaultPatronSearchMethod system preferences and verify that the searches all return as expected.
Hi Pedro, what is the reasoning behind opening a new window for patron-search? Wouldn't a modal be more ergonomic?
(In reply to Paul Derscheid from comment #11) > Hi Pedro, > > what is the reasoning behind opening a new window for patron-search? > Wouldn't a modal be more ergonomic? Definitely would! The opening of a new window is, I believe, old Koha behavior that we aim to move away from going forward (I think?). I provided the examples above to make a point for these 2 different ways of searching for patrons, with 2 distinct search query building logic in the code, trying to do the same thing. The point of this bug though is to move the patron search query building logic in a single place in the code, so that we maintain (and fix) in just 1 place. This will also make it more consistent in the way that staff members searching for patrons will - ideally - have the same results every time they input the same query, regardless of where the search is performed. Additionally, this new consolidated patron search query logic is also able to be used by other queries that also search for, but are not limited to, patrons. See bug 34058.
Ah, I see. Was just wondering.
Created attachment 152854 [details] [review] Bug 34092: Move patron_autocomplete to staff-global.js Works as advertised. Manual tests check out. Remove patron-autocomplete.js file Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 152855 [details] [review] Bug 34092: Preparation: New buildPatronSearchQuery function Move patron search query logic out of patron_autocomplete into new function Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 152856 [details] [review] Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
QA: Looking here
Created attachment 152882 [details] [review] Bug 34092: Move patron_autocomplete to staff-global.js Remove patron-autocomplete.js file Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 152883 [details] [review] Bug 34092: Preparation: New buildPatronSearchQuery function Move patron search query logic out of patron_autocomplete into new function Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 152884 [details] [review] Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 152885 [details] [review] Bug 34092: (QA follow-up) Typo and removing unneeded spread Typo patron_searh_js. The spread is not needed; the function checks for defined later. Note for QA/RM: Ignore this qa tools warning: forbidden pattern: console.log (line 211) This is an intended console warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #21) > Created attachment 152885 [details] [review] [review] > Bug 34092: (QA follow-up) Typo and removing unneeded spread > > Typo patron_searh_js. > The spread is not needed; the function checks for defined later. > > Note for QA/RM: Ignore this qa tools warning: > forbidden pattern: console.log (line 211) > This is an intended console warning. > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Hi Marcel, thank you so much for QA! =D With your removal of spread, clicking the "add patron" button on /cgi-bin/koha/patroncards/edit-batch.pl results in a search error and errors in the console. Test this on a clean k-t-d please, without having any searchable patron attribute types in the system.
(In reply to Pedro Amorim from comment #22) > (In reply to Marcel de Rooy from comment #21) > > Created attachment 152885 [details] [review] [review] [review] > > Bug 34092: (QA follow-up) Typo and removing unneeded spread > > > > Typo patron_searh_js. > > The spread is not needed; the function checks for defined later. > > > > Note for QA/RM: Ignore this qa tools warning: > > forbidden pattern: console.log (line 211) > > This is an intended console warning. > > > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> > > Hi Marcel, thank you so much for QA! =D > > With your removal of spread, clicking the "add patron" button on > /cgi-bin/koha/patroncards/edit-batch.pl results in a search error and errors > in the console. > > Test this on a clean k-t-d please, without having any searchable patron > attribute types in the system. Will have a look soon. Not intended :)
Created attachment 152891 [details] [review] Bug 34092: (QA follow-up) Typo Typo patron_searh_js. Note for QA/RM: Ignore this qa tools warning: forbidden pattern: console.log (line 211) This is an intended console warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #23) > (In reply to Pedro Amorim from comment #22) > > Test this on a clean k-t-d please, without having any searchable patron > > attribute types in the system. Good catch. I thought I tested. But the cache must have been the cause ;) Could reproduce the vague Datatables error that removing the spread triggered..
Pushed to master for 23.11. Nice work everyone, thanks!
I don't understand why we move isolated code (patron_autocomplete.js) to a huge generic staff-global.js file. Can you explain please?
Hey! Thanks for checking this Joubu, I was hoping you would! (In reply to Jonathan Druart from comment #27) > I don't understand why we move isolated code (patron_autocomplete.js) to a > huge generic staff-global.js file. Can you explain please? Interesting! My thought process was the exact opposite: Why do we have an isolated file for this when we already have staff-global.js that holds lots of global functions used throughout all of Staff UI, and this is such a case. That said, I don't have any preference on the matter! And I'm on board with whichever. The only thing I was really invested in was to have somewhat duplicate code be consolidated into a single function. This should allow us to maintain it and enhance it more easily, and allow future work that requires searching on patrons to make use of this new buildPatronSearchQuery, auto-complete or not.
I personally think we should split staff-global into smaller pieces, but that's a discussion for another day.
<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
(In reply to Jonathan Druart from comment #30) > <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 This is not coming from this patch.
(In reply to Jonathan Druart from comment #31) > (In reply to Jonathan Druart from comment #30) > > <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 > > This is not coming from this patch. See bug 28726.
*** Bug 34067 has been marked as a duplicate of this bug. ***
Thanks for all the hard work! Pushed to 23.05.x for the next release
Nice work everyone! Pushed to oldstable for 22.11.x
*** Bug 33321 has been marked as a duplicate of this bug. ***