Bug 34092 - patron-autocomplete.js and patron-search.inc search logic should match
Summary: patron-autocomplete.js and patron-search.inc search logic should match
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal with 10 votes (vote)
Assignee: Pedro Amorim
QA Contact: Marcel de Rooy
URL:
Keywords:
: 33321 34067 (view as bug list)
Depends on: 34094
Blocks: 34058 34256
  Show dependency treegraph
 
Reported: 2023-06-22 09:44 UTC by Pedro Amorim
Modified: 2023-12-28 20:47 UTC (History)
10 users (show)

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


Attachments
Bug 34092: Move patron_autocomplete to staff-global.js (12.11 KB, patch)
2023-06-26 13:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Preparation: New buildPatronSearchQuery function (3.64 KB, patch)
2023-06-26 13:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery (8.59 KB, patch)
2023-06-26 13:00 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery (9.08 KB, patch)
2023-06-26 15:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Move patron_autocomplete to staff-global.js (12.11 KB, patch)
2023-06-28 16:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Preparation: New buildPatronSearchQuery function (3.60 KB, patch)
2023-06-28 16:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery (9.17 KB, patch)
2023-06-28 16:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34092: Move patron_autocomplete to staff-global.js (12.17 KB, patch)
2023-06-29 09:13 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 34092: Preparation: New buildPatronSearchQuery function (3.66 KB, patch)
2023-06-29 09:13 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery (9.23 KB, patch)
2023-06-29 09:13 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 34092: Move patron_autocomplete to staff-global.js (12.26 KB, patch)
2023-06-30 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34092: Preparation: New buildPatronSearchQuery function (3.76 KB, patch)
2023-06-30 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery (9.32 KB, patch)
2023-06-30 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34092: (QA follow-up) Typo and removing unneeded spread (1.72 KB, patch)
2023-06-30 07:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34092: (QA follow-up) Typo (1.20 KB, patch)
2023-06-30 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-06-22 09:44:59 UTC
We should have a single point of patron search logic for coherency and maintainability.
Comment 1 Pedro Amorim 2023-06-26 13:00:20 UTC
Created attachment 152682 [details] [review]
Bug 34092: Move patron_autocomplete to staff-global.js

Remove patron-autocomplete.js file
Comment 2 Pedro Amorim 2023-06-26 13:00:23 UTC
Created attachment 152683 [details] [review]
Bug 34092: Preparation: New buildPatronSearchQuery function

Move patron search query logic out of patron_autocomplete into new function
Comment 3 Pedro Amorim 2023-06-26 13:00:26 UTC
Created attachment 152684 [details] [review]
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
Comment 4 Pedro Amorim 2023-06-26 13:17:58 UTC
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
Comment 5 Pedro Amorim 2023-06-26 15:45:00 UTC
Created attachment 152698 [details] [review]
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
Comment 6 Pedro Amorim 2023-06-27 11:01:44 UTC
This needs a rebase on top of bug 34094. Coming soon.
Comment 7 Pedro Amorim 2023-06-28 16:31:54 UTC
Created attachment 152830 [details] [review]
Bug 34092: Move patron_autocomplete to staff-global.js

Remove patron-autocomplete.js file
Comment 8 Pedro Amorim 2023-06-28 16:31:57 UTC
Created attachment 152831 [details] [review]
Bug 34092: Preparation: New buildPatronSearchQuery function

Move patron search query logic out of patron_autocomplete into new function
Comment 9 Pedro Amorim 2023-06-28 16:32:01 UTC
Created attachment 152832 [details] [review]
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery
Comment 10 Pedro Amorim 2023-06-28 16:34:33 UTC
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.
Comment 11 Paul Derscheid 2023-06-29 08:53:04 UTC
Hi Pedro,

what is the reasoning behind opening a new window for patron-search? Wouldn't a modal be more ergonomic?
Comment 12 Pedro Amorim 2023-06-29 09:02:53 UTC
(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.
Comment 13 Paul Derscheid 2023-06-29 09:03:33 UTC
Ah, I see. Was just wondering.
Comment 14 Paul Derscheid 2023-06-29 09:13:22 UTC
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>
Comment 15 Paul Derscheid 2023-06-29 09:13:25 UTC
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>
Comment 16 Paul Derscheid 2023-06-29 09:13:28 UTC
Created attachment 152856 [details] [review]
Bug 34092: Move search-patron.inc logic into buildPatronSearchQuery

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 17 Marcel de Rooy 2023-06-30 06:28:48 UTC
QA: Looking here
Comment 18 Marcel de Rooy 2023-06-30 07:13:54 UTC
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>
Comment 19 Marcel de Rooy 2023-06-30 07:13:57 UTC
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>
Comment 20 Marcel de Rooy 2023-06-30 07:13:59 UTC
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>
Comment 21 Marcel de Rooy 2023-06-30 07:14:02 UTC
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>
Comment 22 Pedro Amorim 2023-06-30 08:33:27 UTC
(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.
Comment 23 Marcel de Rooy 2023-06-30 08:36:56 UTC
(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 :)
Comment 24 Marcel de Rooy 2023-06-30 08:56:12 UTC
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>
Comment 25 Marcel de Rooy 2023-06-30 08:58:02 UTC
(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..
Comment 26 Tomás Cohen Arazi 2023-07-03 17:51:08 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 27 Jonathan Druart 2023-07-04 06:41:01 UTC
I don't understand why we move isolated code (patron_autocomplete.js) to a huge generic staff-global.js file. Can you explain please?
Comment 28 Pedro Amorim 2023-07-04 07:39:48 UTC
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.
Comment 29 Jonathan Druart 2023-07-04 08:06:44 UTC
I personally think we should split staff-global into smaller pieces, but that's a discussion for another day.
Comment 30 Jonathan Druart 2023-07-04 08:07:56 UTC
<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
Comment 31 Jonathan Druart 2023-07-04 08:20:14 UTC
(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.
Comment 32 Jonathan Druart 2023-07-04 08:21:42 UTC
(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.
Comment 33 Nick Clemens 2023-07-10 17:35:09 UTC
*** Bug 34067 has been marked as a duplicate of this bug. ***
Comment 34 Martin Renvoize 2023-07-17 15:05:26 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 35 Matt Blenkinsop 2023-07-18 13:12:05 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 36 Laura Escamilla 2023-07-27 18:37:00 UTC
*** Bug 33321 has been marked as a duplicate of this bug. ***