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.
Created attachment 135856 [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)
I'm seeing "Uncaught ReferenceError: patron_autocomplete is not defined" in the console.
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)
Sorry, I forgot to commit a new file.
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.
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.
> 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.
In case there's any confusion based on my previous comment, I'm not saying that this patch should be switched to use Select2.
Created attachment 136041 [details] [review] Bug 30578: Restore highlighting of patrons from the logged-in library
Created attachment 136042 [details] [review] Bug 30578: Fix on-select for 2 occurrences
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
Created attachment 136044 [details] [review] Bug 30578: Fix multi-terms search
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.
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>
Created attachment 136086 [details] [review] Bug 30578: Restore highlighting of patrons from the logged-in library Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 136087 [details] [review] Bug 30578: Fix on-select for 2 occurrences Signed-off-by: Owen Leonard <oleonard@myacpl.org>
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>
Created attachment 136089 [details] [review] Bug 30578: Fix multi-terms search Signed-off-by: Owen Leonard <oleonard@myacpl.org>
This is great work, Jonathan, thank you.
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>
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>
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>
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>
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>
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
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>
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>
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>
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>
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>
Conflict with bug 21978 fixed.
(In reply to Jonathan Druart from comment #31) > Conflict with bug 21978 fixed. Thanks Joubu.. it was on my list to fix that conflict.. :)
Pushed to master for 22.11. Nice work everyone, thanks!
Enhancement will not be backported to 22.05.x series
I'm unsure what needs to be documented here. Setting it to resolved, but feel free to reopen it with specifications