Bug 38484 - Filters on the "Holds to pull" page is broken
Summary: Filters on the "Holds to pull" page is broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: David Cook
URL:
Keywords:
Depends on: 36640
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-19 12:37 UTC by Jonathan Druart
Modified: 2024-12-05 23:19 UTC (History)
4 users (show)

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


Attachments
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull" (6.01 KB, patch)
2024-11-19 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull" (6.08 KB, patch)
2024-11-19 18:49 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull" (6.13 KB, patch)
2024-11-22 03:05 UTC, David Cook
Details | Diff | Splinter Review
Bug 38484: Remove filters for 'Actions' (1.35 KB, patch)
2024-11-22 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38484: Remove unused USE and SET (2.43 KB, patch)
2024-11-22 07:37 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 Jonathan Druart 2024-11-19 12:37:40 UTC
Has been broken by bug 36640.
Comment 1 Jonathan Druart 2024-11-19 12:42:15 UTC
Created attachment 174788 [details] [review]
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull"

On "Bug 36640: Remove fnAddFilters" I assume that it was only used for
debounce. But actually it was used on this table to add filtering.

This patch adjusts how we deal with the filters on this table, reusing
the standard "add_filters" and "filters_options" parameters of
KohaTable.

It also passes the DT object allowing to use DT's api. Here it is
useful to retrieve the data in the column.
Comment 2 Emily Lamancusa (emlam) 2024-11-19 18:49:53 UTC
Created attachment 174806 [details] [review]
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull"

On "Bug 36640: Remove fnAddFilters" I assume that it was only used for
debounce. But actually it was used on this table to add filtering.

This patch adjusts how we deal with the filters on this table, reusing
the standard "add_filters" and "filters_options" parameters of
KohaTable.

It also passes the DT object allowing to use DT's api. Here it is
useful to retrieve the data in the column.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 3 David Cook 2024-11-22 02:50:06 UTC
Would've been good to have a test plan for this, but taking a look now.
Comment 4 David Cook 2024-11-22 02:53:39 UTC
With this patch, I'm now getting "Incorrect use of <label for=FORM_ELEMENT>" warnings in the console. Looks like it's probably because the FORM_ELEMENT are converted by Flatpickr into hidden inputs. 

Not really a blocker... but worth noting...
Comment 5 David Cook 2024-11-22 02:56:02 UTC
The filter boxes are too narrow as well. With this patch, they're smaller and don't have equal distance whitespace between the filter box and the edge of the table cell. 

Also not a blocker but noting it...
Comment 6 David Cook 2024-11-22 02:57:39 UTC
(In reply to David Cook from comment #4)
> With this patch, I'm now getting "Incorrect use of <label for=FORM_ELEMENT>"
> warnings in the console. Looks like it's probably because the FORM_ELEMENT
> are converted by Flatpickr into hidden inputs. 
> 
> Not really a blocker... but worth noting...

Actually, that looks pre-existing. Not sure why it didn't come up before the patch, but I unapplied the patch, and it's still there.

So you can disregard here...
Comment 7 David Cook 2024-11-22 02:59:25 UTC
Noting that the patch has added more filter boxes.

Before patch, there were no boxes for Available collections, Earliest hold date, Hold notes, and Action. 

Those first three are OK, but we probably don't want one on Action.

Still don't think that's a blocker though...
Comment 8 David Cook 2024-11-22 03:04:23 UTC
Comment on attachment 174806 [details] [review]
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull"

Review of attachment 174806 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt
@@ +1,2 @@
>  [% USE raw %]
> +[% USE To %]

Does this line get used? I can't see it used anywhere.

@@ +321,5 @@
>                  });
>          }
>  
>          $(document).ready(function() {
> +                [% SET libraries = Branches.all %]

This line also seems unused?

::: koha-tmpl/intranet-tmpl/prog/js/datatables.js
@@ +808,4 @@
>                  if ( !filters_options.hasOwnProperty(i) ) {
>                      filters_options[i] = eval(filter_type)
>                  } else if ( typeof filters_options[i] === "function" ) {
> +                    filters_options[i] = filters_options[i](table_dt)

I don't know what this line does, but I'm going to assume it's OK.
Comment 9 David Cook 2024-11-22 03:05:20 UTC
I'm going to Pass QA since this fixes an important functional problem, and we're close to the release, so I think we should move forward.

But a few things to look at there still...
Comment 10 David Cook 2024-11-22 03:05:49 UTC
Created attachment 174907 [details] [review]
Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull"

On "Bug 36640: Remove fnAddFilters" I assume that it was only used for
debounce. But actually it was used on this table to add filtering.

This patch adjusts how we deal with the filters on this table, reusing
the standard "add_filters" and "filters_options" parameters of
KohaTable.

It also passes the DT object allowing to use DT's api. Here it is
useful to retrieve the data in the column.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 Jonathan Druart 2024-11-22 07:30:57 UTC
This patch makes the filters look like other table's filters. So yes there is certainly a change in the style, but it's expected.

(In reply to David Cook from comment #7)
> Noting that the patch has added more filter boxes.
> 
> Before patch, there were no boxes for Available collections, Earliest hold
> date, Hold notes, and Action. 
>
> Those first three are OK, but we probably don't want one on Action.


Filter on action is not expected, patch coming.

(In reply to David Cook from comment #8)
> Comment on attachment 174806 [details] [review] [review]
> Bug 38484: (bug 36640 follow-up) Fix filters on "Holds to pull"
> 
> Review of attachment 174806 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt
> @@ +1,2 @@
> >  [% USE raw %]
> > +[% USE To %]
> 
> Does this line get used? I can't see it used anywhere.

Unused, will be removed.

> @@ +321,5 @@
> >                  });
> >          }
> >  
> >          $(document).ready(function() {
> > +                [% SET libraries = Branches.all %]
> 
> This line also seems unused?

Unused, will be removed.

> ::: koha-tmpl/intranet-tmpl/prog/js/datatables.js
> @@ +808,4 @@
> >                  if ( !filters_options.hasOwnProperty(i) ) {
> >                      filters_options[i] = eval(filter_type)
> >                  } else if ( typeof filters_options[i] === "function" ) {
> > +                    filters_options[i] = filters_options[i](table_dt)
> 
> I don't know what this line does, but I'm going to assume it's OK.

This page is different, we build the dropdown filters with the content of the table, not all the data in DB (what we need to do when using the REST API/ajax pagination).
Here we pass the DataTable object so _dt_add_filters needs to loop over the columns of the table and retrieve its data.
Comment 12 Jonathan Druart 2024-11-22 07:37:02 UTC
Created attachment 174908 [details] [review]
Bug 38484: Remove filters for 'Actions'
Comment 13 Jonathan Druart 2024-11-22 07:37:05 UTC
Created attachment 174909 [details] [review]
Bug 38484: Remove unused USE and SET

And tidy
Comment 14 Jonathan Druart 2024-11-22 07:37:38 UTC
Thanks for the thorough review, David!
Comment 15 Katrin Fischer 2024-11-22 09:23:48 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 16 Lucas Gass (lukeg) 2024-12-05 23:19:23 UTC
24.05.x merge conflcist, rebase if needed.