Bug 30578 - We should drop circ/ysearch.pl in preference to using the REST API's
Summary: We should drop circ/ysearch.pl in preference to using the REST API's
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 30980 25498 30965 31782
  Show dependency treegraph
 
Reported: 2022-04-21 12:13 UTC by Martin Renvoize
Modified: 2023-12-28 20:42 UTC (History)
6 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:
22.11.00


Attachments
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route (26.46 KB, patch)
2022-06-09 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route (31.08 KB, patch)
2022-06-10 08:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Restore highlighting of patrons from the logged-in library (1.05 KB, patch)
2022-06-13 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Fix on-select for 2 occurrences (1.26 KB, patch)
2022-06-13 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Remove ajaxSetup from checkouts.js (903 bytes, patch)
2022-06-13 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Fix multi-terms search (3.28 KB, patch)
2022-06-13 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route (31.10 KB, patch)
2022-06-15 11:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30578: Restore highlighting of patrons from the logged-in library (1.09 KB, patch)
2022-06-15 11:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30578: Fix on-select for 2 occurrences (1.30 KB, patch)
2022-06-15 11:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30578: Remove ajaxSetup from checkouts.js (952 bytes, patch)
2022-06-15 11:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30578: Fix multi-terms search (3.33 KB, patch)
2022-06-15 11:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route (31.16 KB, patch)
2022-06-16 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30578: Restore highlighting of patrons from the logged-in library (1.16 KB, patch)
2022-06-16 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30578: Fix on-select for 2 occurrences (1.37 KB, patch)
2022-06-16 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30578: Remove ajaxSetup from checkouts.js (1017 bytes, patch)
2022-06-16 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30578: Fix multi-terms search (3.39 KB, patch)
2022-06-16 08:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route (31.71 KB, patch)
2022-06-27 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Restore highlighting of patrons from the logged-in library (1.16 KB, patch)
2022-06-27 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Fix on-select for 2 occurrences (1.37 KB, patch)
2022-06-27 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Remove ajaxSetup from checkouts.js (1019 bytes, patch)
2022-06-27 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30578: Fix multi-terms search (3.39 KB, patch)
2022-06-27 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-04-21 12:13:34 UTC
As the title says, we should really stop using ysearch.pl for autocomplete.. it doesn't fully respect the various restrictions and the REST API is well developed now.
Comment 1 Jonathan Druart 2022-06-09 09:17:20 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2022-06-09 12:50:08 UTC
I'm seeing "Uncaught ReferenceError: patron_autocomplete is not defined" in the console.
Comment 3 Jonathan Druart 2022-06-10 08:16:26 UTC
Created attachment 135920 [details] [review]
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route

This patch removes the circ/ysearch.pl script used by the jQuery autocomplete widget.
We can now use the /api/v1/patrons endpoint to retrieve the patrons and
generate the patron result list.

Prior to this patch the different occurrences were defining the style
and the list of patron's attributes to display for each option (name,
date of birth, age, address, etc.). Now they are all displaying the same
information.

To acchieve this we had to:
* Make js-date-format.inc and js-patron-get-age.inc available from js_includes.inc
and so available from everywhere, which is certainly a good move. We
could discuss why this code is in include file instead of JS files
however.
* Remove the .ajaxSetup call in tags-review.js to reduce its scope: an
underscore parameter was added to the REST API query (?)

A better solution would have been to extend the existing widget
(https://learn.jquery.com/jquery-ui/widget-factory/extending-widgets/)
but I didn't manage to do it, and I feel like there is a bug in jQuery
autocomplete. The "source" was not taken into account.
We could think about replacing the jQuery autocomplete with something
else, but that's outside the scope of this bug.

Test plan:
Search for patrons and confirm the autocomplete works and that the
"select" action works as before (either a redirect or select the
patrons) on the different views:
* Place a hold
* Search for tags (form on the left)
* In the header, "Check out" and "Search patrons"
* Add instructors to course reserves
* View logs (the "librarian" input)
Comment 4 Jonathan Druart 2022-06-10 08:16:47 UTC
Sorry, I forgot to commit a new file.
Comment 5 Martin Renvoize 2022-06-13 15:25:19 UTC
Interesting; I will have to take a deeper look at what you've done here.

When I first submitted this bug I was working on the bookings submission and I opted to use Select2 for patron selection. I basically wondered if Select2 was the better option for at least some of the occurrences where we were using this customer ysearch service.

I felt there was a clear mix of two things.. actual autocomplete in a search box and what was really just a select in a form.
Comment 6 Owen Leonard 2022-06-13 18:02:34 UTC
I found a couple of issues:

-  The colored highlighting of patrons from the user's logged-in branch isn't working anymore
- The circulation and patron header searches don't do anything when you click one of the autocomplete results
- The search doesn't work with more than one term. With the old ysearch I could type "leonard owen" or "owen leonard" and get the correct result.
Comment 7 Owen Leonard 2022-06-13 18:05:00 UTC
> I basically wondered if Select2
> was the better option for at least some of the occurrences where we were
> using this customer ysearch service.

As I said to Martin in IRC, if Select2 can do everything we need I'm in favor of switching. That would mean we wouldn't have to add another JS library when we're ready to get rid of jQueryUI altogether.
Comment 8 Owen Leonard 2022-06-13 18:06:37 UTC
In case there's any confusion based on my previous comment, I'm not saying that this patch should be switched to use Select2.
Comment 9 Jonathan Druart 2022-06-13 18:59:25 UTC
Created attachment 136041 [details] [review]
Bug 30578: Restore highlighting of patrons from the logged-in library
Comment 10 Jonathan Druart 2022-06-13 18:59:29 UTC
Created attachment 136042 [details] [review]
Bug 30578: Fix on-select for 2 occurrences
Comment 11 Jonathan Druart 2022-06-13 18:59:34 UTC
Created attachment 136043 [details] [review]
Bug 30578: Remove ajaxSetup from checkouts.js

It was a IE fix apparently
  commit f021b52e71deaac0b46992fa33ef02f2b001e56d
  Bug 11703 [QA Followup] - Stop IE from caching ajax request
Comment 12 Jonathan Druart 2022-06-13 18:59:39 UTC
Created attachment 136044 [details] [review]
Bug 30578: Fix multi-terms search
Comment 13 Jonathan Druart 2022-06-13 19:00:57 UTC
All fixed, thanks a lot, Owen!

This patch is simply doing what the bug title is describing, nothing else. I found it when I was opening a new one. We could still move to select2 later, it will be easier with this move anyway.
Comment 14 Owen Leonard 2022-06-15 11:59:38 UTC
Created attachment 136085 [details] [review]
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route

This patch removes the circ/ysearch.pl script used by the jQuery autocomplete widget.
We can now use the /api/v1/patrons endpoint to retrieve the patrons and
generate the patron result list.

Prior to this patch the different occurrences were defining the style
and the list of patron's attributes to display for each option (name,
date of birth, age, address, etc.). Now they are all displaying the same
information.

To acchieve this we had to:
* Make js-date-format.inc and js-patron-get-age.inc available from js_includes.inc
and so available from everywhere, which is certainly a good move. We
could discuss why this code is in include file instead of JS files
however.
* Remove the .ajaxSetup call in tags-review.js to reduce its scope: an
underscore parameter was added to the REST API query (?)

A better solution would have been to extend the existing widget
(https://learn.jquery.com/jquery-ui/widget-factory/extending-widgets/)
but I didn't manage to do it, and I feel like there is a bug in jQuery
autocomplete. The "source" was not taken into account.
We could think about replacing the jQuery autocomplete with something
else, but that's outside the scope of this bug.

Test plan:
Search for patrons and confirm the autocomplete works and that the
"select" action works as before (either a redirect or select the
patrons) on the different views:
* Place a hold
* Search for tags (form on the left)
* In the header, "Check out" and "Search patrons"
* Add instructors to course reserves
* View logs (the "librarian" input)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Owen Leonard 2022-06-15 11:59:42 UTC
Created attachment 136086 [details] [review]
Bug 30578: Restore highlighting of patrons from the logged-in library

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Owen Leonard 2022-06-15 11:59:46 UTC
Created attachment 136087 [details] [review]
Bug 30578: Fix on-select for 2 occurrences

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 17 Owen Leonard 2022-06-15 11:59:50 UTC
Created attachment 136088 [details] [review]
Bug 30578: Remove ajaxSetup from checkouts.js

It was a IE fix apparently
  commit f021b52e71deaac0b46992fa33ef02f2b001e56d
  Bug 11703 [QA Followup] - Stop IE from caching ajax request

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2022-06-15 11:59:54 UTC
Created attachment 136089 [details] [review]
Bug 30578: Fix multi-terms search

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Owen Leonard 2022-06-15 12:40:49 UTC
This is great work, Jonathan, thank you.
Comment 20 Martin Renvoize 2022-06-16 08:51:01 UTC
Created attachment 136130 [details] [review]
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route

This patch removes the circ/ysearch.pl script used by the jQuery autocomplete widget.
We can now use the /api/v1/patrons endpoint to retrieve the patrons and
generate the patron result list.

Prior to this patch the different occurrences were defining the style
and the list of patron's attributes to display for each option (name,
date of birth, age, address, etc.). Now they are all displaying the same
information.

To acchieve this we had to:
* Make js-date-format.inc and js-patron-get-age.inc available from js_includes.inc
and so available from everywhere, which is certainly a good move. We
could discuss why this code is in include file instead of JS files
however.
* Remove the .ajaxSetup call in tags-review.js to reduce its scope: an
underscore parameter was added to the REST API query (?)

A better solution would have been to extend the existing widget
(https://learn.jquery.com/jquery-ui/widget-factory/extending-widgets/)
but I didn't manage to do it, and I feel like there is a bug in jQuery
autocomplete. The "source" was not taken into account.
We could think about replacing the jQuery autocomplete with something
else, but that's outside the scope of this bug.

Test plan:
Search for patrons and confirm the autocomplete works and that the
"select" action works as before (either a redirect or select the
patrons) on the different views:
* Place a hold
* Search for tags (form on the left)
* In the header, "Check out" and "Search patrons"
* Add instructors to course reserves
* View logs (the "librarian" input)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-06-16 08:51:05 UTC
Created attachment 136131 [details] [review]
Bug 30578: Restore highlighting of patrons from the logged-in library

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-06-16 08:51:09 UTC
Created attachment 136132 [details] [review]
Bug 30578: Fix on-select for 2 occurrences

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-06-16 08:51:13 UTC
Created attachment 136133 [details] [review]
Bug 30578: Remove ajaxSetup from checkouts.js

It was a IE fix apparently
  commit f021b52e71deaac0b46992fa33ef02f2b001e56d
  Bug 11703 [QA Followup] - Stop IE from caching ajax request

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2022-06-16 08:51:17 UTC
Created attachment 136134 [details] [review]
Bug 30578: Fix multi-terms search

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2022-06-16 08:52:59 UTC
Great work, similarly I didn't mean we had to do a select2 migration now.. more just pointing out where my brain was headed.. I can create a bug to record the idea for the future.

So, on with QA.. it's all working really nicely and is a nice consistency improvement. Owens uncovered and had fixed all the issues I spotted (and some more).

QA scripts are happy..

Passing QA
Comment 26 Jonathan Druart 2022-06-27 12:36:07 UTC
Created attachment 136575 [details] [review]
Bug 30578: Remove circ/ysearch.pl in favor of the /patrons REST API route

This patch removes the circ/ysearch.pl script used by the jQuery autocomplete widget.
We can now use the /api/v1/patrons endpoint to retrieve the patrons and
generate the patron result list.

Prior to this patch the different occurrences were defining the style
and the list of patron's attributes to display for each option (name,
date of birth, age, address, etc.). Now they are all displaying the same
information.

To acchieve this we had to:
* Make js-date-format.inc and js-patron-get-age.inc available from js_includes.inc
and so available from everywhere, which is certainly a good move. We
could discuss why this code is in include file instead of JS files
however.
* Remove the .ajaxSetup call in tags-review.js to reduce its scope: an
underscore parameter was added to the REST API query (?)

A better solution would have been to extend the existing widget
(https://learn.jquery.com/jquery-ui/widget-factory/extending-widgets/)
but I didn't manage to do it, and I feel like there is a bug in jQuery
autocomplete. The "source" was not taken into account.
We could think about replacing the jQuery autocomplete with something
else, but that's outside the scope of this bug.

Test plan:
Search for patrons and confirm the autocomplete works and that the
"select" action works as before (either a redirect or select the
patrons) on the different views:
* Place a hold
* Search for tags (form on the left)
* In the header, "Check out" and "Search patrons"
* Add instructors to course reserves
* View logs (the "librarian" input)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Jonathan Druart 2022-06-27 12:36:12 UTC
Created attachment 136576 [details] [review]
Bug 30578: Restore highlighting of patrons from the logged-in library

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Jonathan Druart 2022-06-27 12:36:16 UTC
Created attachment 136577 [details] [review]
Bug 30578: Fix on-select for 2 occurrences

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Jonathan Druart 2022-06-27 12:36:20 UTC
Created attachment 136578 [details] [review]
Bug 30578: Remove ajaxSetup from checkouts.js

It was a IE fix apparently
  commit f021b52e71deaac0b46992fa33ef02f2b001e56d
  Bug 11703 [QA Followup] - Stop IE from caching ajax request

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Jonathan Druart 2022-06-27 12:36:25 UTC
Created attachment 136579 [details] [review]
Bug 30578: Fix multi-terms search

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Jonathan Druart 2022-06-27 12:36:45 UTC
Conflict with bug 21978 fixed.
Comment 32 Martin Renvoize 2022-06-29 15:51:57 UTC
(In reply to Jonathan Druart from comment #31)
> Conflict with bug 21978 fixed.

Thanks Joubu.. it was on my list to fix that conflict.. :)
Comment 33 Tomás Cohen Arazi 2022-07-18 14:53:39 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 34 Lucas Gass 2022-08-23 17:22:19 UTC
Enhancement will not be backported to 22.05.x series
Comment 35 Caroline Cyr La Rose 2023-04-14 18:05:21 UTC
I'm unsure what needs to be documented here. Setting it to resolved, but feel free to reopen it with specifications