Created attachment 130605 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch
Created attachment 130606 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 130627 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch
Created attachment 130628 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
I like it. However, the autocomplete when you start typing a name is no longer working (it was previously the same as when searching from the patron screens or the header box with search patrons selected).
(In reply to David Nind from comment #5) > I like it. > > However, the autocomplete when you start typing a name is no longer working > (it was previously the same as when searching from the patron screens or the > header box with search patrons selected). As the search is more effective now, with all the filters available, I removed it on purpose. Do you think it's really needed?
Created attachment 130705 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
(In reply to Jonathan Druart from comment #6) > (In reply to David Nind from comment #5) > > I like it. > > > > However, the autocomplete when you start typing a name is no longer working > > (it was previously the same as when searching from the patron screens or the > > header box with search patrons selected). > > As the search is more effective now, with all the filters available, I > removed it on purpose. Do you think it's really needed? It's usually a feature people quite like independent of the other search options, but hard to tell without testing the patch more closely for comparison.
Created attachment 130944 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch
Created attachment 130945 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 130946 [details] [review] bug 30093: Restore the autocomplete feature
Created attachment 131830 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch
Created attachment 131831 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 131832 [details] [review] bug 30093: Restore the autocomplete feature
Created attachment 132537 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 132538 [details] [review] bug 30093: Restore the autocomplete feature
Fixing bug 30055 comment 85 point 4. Changes: [% CASE 'branch' %] library_id = $("#branchcode_filter").val() || ""; patrons_table.find('thead tr:eq(1) th[data-filter="libraries"] select').val(library_id); - if (library_id) - table_dt.column([% loop.count - 1 %]).search('^'+library_id+'$'); + table_dt.column([% loop.count - 1 %]).search(library_id ? '^'+library_id+'$' : ''); [% CASE 'category' %] let category_id = $("#categorycode_filter").val() || ""; patrons_table.find('thead tr:eq(1) th[data-filter="categories"] select').val(category_id); - if (category_id) - table_dt.column([% loop.count - 1 %]).search('^'+category_id+'$'); + table_dt.column([% loop.count - 1 %]).search(category_id ? '^'+category_id+'$' : ''); [% END %] [% END %] table_dt.search("");
Created attachment 132540 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch
Created attachment 132541 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 132542 [details] [review] bug 30093: Restore the autocomplete feature
I failed the patch because the launching the search isn't taken into account anymore, and all users are display. To reproduce : - search for keychain that brings several users ('hen' for example) - in master, only the 2 results matching with 'hen' are displayed - with the patch, all users are displayed I don't think it's the expected behaviour. All the other points are ok for me.
Created attachment 132647 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters.
Created attachment 132648 [details] [review] bug 30093: Restore the autocomplete feature
(In reply to Séverine Queune from comment #21) > I failed the patch because the launching the search isn't taken into account > anymore, and all users are display. > To reproduce : > - search for keychain that brings several users ('hen' for example) > - in master, only the 2 results matching with 'hen' are displayed > - with the patch, all users are displayed > > I don't think it's the expected behaviour. > All the other points are ok for me. It was fixed in a follow-up bugs, but I fixed the patches here.
Created attachment 132653 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132654 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters. Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132655 [details] [review] bug 30093: Restore the autocomplete feature Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
(In reply to Jonathan Druart from comment #24) > (In reply to Séverine Queune from comment #21) > > I failed the patch because the launching the search isn't taken into account > > anymore, and all users are display. > > To reproduce : > > - search for keychain that brings several users ('hen' for example) > > - in master, only the 2 results matching with 'hen' are displayed > > - with the patch, all users are displayed > > > > I don't think it's the expected behaviour. > > All the other points are ok for me. > > It was fixed in a follow-up bugs, but I fixed the patches here. Nice, I works fine now :)
One for a later follow-up rather than hold up this series.. but I did a quick poll and so far everyone I aske doesn't like having two search boxes appearing.. especially as they're not synced. We should either introduce an option to remove the search box from the datatable or after initial search submission we should hide the main search box and populate the datatable one with the query we just performed.
Created attachment 132699 [details] Search results view
Created attachment 132700 [details] [review] Bug 30093: Fix QA failures
The console.log is expected.
Created attachment 132701 [details] [review] Bug 30093: Restore the autocomplete feature Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132702 [details] [review] Bug 30093: Fix QA failures
Created attachment 132724 [details] [review] Bug 30093: Rename patron-search.inc to patron-search-header.inc We will use 'patron-search.inc' in the next patch Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132725 [details] [review] Bug 30093: Make patron search from request.pl use the REST API In this patch we want to reuse what has been done in the previous bug report to search patrons using the REST API route. The code is mainly in members/search.tt, for all the patron searches that "add" or "select" a patron (popup windows). The patron search for holds is a bit different, we don't want to open a popup window. We are moving to code to an include file (patron-search.tt) to make it reusable easily. Note that we are improving how the patron's addresses are displayed, and provide a JS equivalent to the TT includes files. Test plan: Search for patrons from the "Place a hold on" view. You should see the same view as behaviour, with more filters. Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132726 [details] [review] Bug 30093: Restore the autocomplete feature Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132727 [details] [review] Bug 30093: Fix QA failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132728 [details] [review] Bug 30093: (QA follow-up) Final QA Script Fix Just remove a console log that crept in somewhere
All looking good again here.. the search box stuff can certainly happen on a later bug report, especially after a little further feedback. QA scripts now happy, though I did have to add another minor follow-up to drop a console.log that was introduced.. feel free to squash or drop that follow-up as required. Great work, Passing QA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]