Bug 30093 - Rewrite the patron search when placing a hold with the REST API route
Summary: Rewrite the patron search when placing a hold with the REST API route
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 29136 30055 30074
Blocks: 30094
  Show dependency treegraph
 
Reported: 2022-02-14 14:29 UTC by Jonathan Druart
Modified: 2022-04-08 12:26 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.21 KB, patch)
2022-02-15 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (63.09 KB, patch)
2022-02-15 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.21 KB, patch)
2022-02-15 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (63.09 KB, patch)
2022-02-15 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.08 KB, patch)
2022-02-17 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.21 KB, patch)
2022-02-21 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.06 KB, patch)
2022-02-21 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.44 KB, patch)
2022-02-21 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.21 KB, patch)
2022-03-17 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.15 KB, patch)
2022-03-17 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.44 KB, patch)
2022-03-17 09:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.10 KB, patch)
2022-03-30 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.44 KB, patch)
2022-03-30 08:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.21 KB, patch)
2022-03-30 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.13 KB, patch)
2022-03-30 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.44 KB, patch)
2022-03-30 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.08 KB, patch)
2022-03-30 15:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.55 KB, patch)
2022-03-30 15:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.30 KB, patch)
2022-03-30 16:06 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.21 KB, patch)
2022-03-30 16:06 UTC, Séverine Queune
Details | Diff | Splinter Review
bug 30093: Restore the autocomplete feature (3.69 KB, patch)
2022-03-30 16:06 UTC, Séverine Queune
Details | Diff | Splinter Review
Search results view (136.01 KB, image/png)
2022-03-31 11:43 UTC, Martin Renvoize
Details
Bug 30093: Fix QA failures (5.93 KB, patch)
2022-03-31 11:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Restore the autocomplete feature (3.69 KB, patch)
2022-03-31 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Fix QA failures (5.93 KB, patch)
2022-03-31 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30093: Rename patron-search.inc to patron-search-header.inc (18.37 KB, patch)
2022-03-31 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30093: Make patron search from request.pl use the REST API (64.27 KB, patch)
2022-03-31 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30093: Restore the autocomplete feature (3.69 KB, patch)
2022-03-31 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30093: Fix QA failures (5.99 KB, patch)
2022-03-31 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30093: (QA follow-up) Final QA Script Fix (1.02 KB, patch)
2022-03-31 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-02-14 14:29:00 UTC

    
Comment 1 Jonathan Druart 2022-02-15 10:19:32 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2022-02-15 10:19:37 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2022-02-15 16:20:12 UTC
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
Comment 4 Jonathan Druart 2022-02-15 16:20:18 UTC
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.
Comment 5 David Nind 2022-02-16 18:18:53 UTC
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).
Comment 6 Jonathan Druart 2022-02-16 19:53:00 UTC
(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?
Comment 7 Jonathan Druart 2022-02-17 10:09:21 UTC
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.
Comment 8 Katrin Fischer 2022-02-19 13:32:33 UTC
(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.
Comment 9 Jonathan Druart 2022-02-21 10:40:01 UTC
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
Comment 10 Jonathan Druart 2022-02-21 10:40:06 UTC
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.
Comment 11 Jonathan Druart 2022-02-21 10:56:24 UTC
Created attachment 130946 [details] [review]
bug 30093: Restore the autocomplete feature
Comment 12 Jonathan Druart 2022-03-17 09:43:32 UTC
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
Comment 13 Jonathan Druart 2022-03-17 09:43:36 UTC
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.
Comment 14 Jonathan Druart 2022-03-17 09:43:40 UTC
Created attachment 131832 [details] [review]
bug 30093: Restore the autocomplete feature
Comment 15 Jonathan Druart 2022-03-30 08:01:46 UTC
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.
Comment 16 Jonathan Druart 2022-03-30 08:01:51 UTC
Created attachment 132538 [details] [review]
bug 30093: Restore the autocomplete feature
Comment 17 Jonathan Druart 2022-03-30 08:03:15 UTC
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("");
Comment 18 Jonathan Druart 2022-03-30 08:24:07 UTC
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
Comment 19 Jonathan Druart 2022-03-30 08:24:12 UTC
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.
Comment 20 Jonathan Druart 2022-03-30 08:24:18 UTC
Created attachment 132542 [details] [review]
bug 30093: Restore the autocomplete feature
Comment 21 Séverine Queune 2022-03-30 14:43:46 UTC
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.
Comment 22 Jonathan Druart 2022-03-30 15:22:13 UTC
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.
Comment 23 Jonathan Druart 2022-03-30 15:22:18 UTC
Created attachment 132648 [details] [review]
bug 30093: Restore the autocomplete feature
Comment 24 Jonathan Druart 2022-03-30 15:23:31 UTC
(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.
Comment 25 Séverine Queune 2022-03-30 16:06:09 UTC
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>
Comment 26 Séverine Queune 2022-03-30 16:06:17 UTC
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>
Comment 27 Séverine Queune 2022-03-30 16:06:34 UTC
Created attachment 132655 [details] [review]
bug 30093: Restore the autocomplete feature

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Comment 28 Séverine Queune 2022-03-30 16:08:50 UTC
(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 :)
Comment 29 Martin Renvoize 2022-03-31 11:42:21 UTC
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.
Comment 30 Martin Renvoize 2022-03-31 11:43:38 UTC
Created attachment 132699 [details]
Search results view
Comment 31 Jonathan Druart 2022-03-31 11:44:40 UTC
Created attachment 132700 [details] [review]
Bug 30093: Fix QA failures
Comment 32 Jonathan Druart 2022-03-31 11:45:00 UTC
The console.log is expected.
Comment 33 Jonathan Druart 2022-03-31 11:53:07 UTC
Created attachment 132701 [details] [review]
Bug 30093: Restore the autocomplete feature

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Comment 34 Jonathan Druart 2022-03-31 11:53:16 UTC
Created attachment 132702 [details] [review]
Bug 30093: Fix QA failures
Comment 35 Martin Renvoize 2022-03-31 12:14:02 UTC
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>
Comment 36 Martin Renvoize 2022-03-31 12:14:08 UTC
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>
Comment 37 Martin Renvoize 2022-03-31 12:14:16 UTC
Created attachment 132726 [details] [review]
Bug 30093: Restore the autocomplete feature

Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Comment 38 Martin Renvoize 2022-03-31 12:14:23 UTC
Created attachment 132727 [details] [review]
Bug 30093: Fix QA failures

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2022-03-31 12:14:28 UTC
Created attachment 132728 [details] [review]
Bug 30093: (QA follow-up) Final QA Script Fix

Just remove a console log that crept in somewhere
Comment 40 Martin Renvoize 2022-03-31 12:16:47 UTC
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
Comment 41 Fridolin Somers 2022-04-04 07:49:10 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄