Has been broken by bug 36640.
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.
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>
Would've been good to have a test plan for this, but taking a look now.
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...
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...
(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...
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 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.
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...
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>
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.
Created attachment 174908 [details] [review] Bug 38484: Remove filters for 'Actions'
Created attachment 174909 [details] [review] Bug 38484: Remove unused USE and SET And tidy
Thanks for the thorough review, David!
Pushed for 24.11! Well done everyone, thank you!
24.05.x merge conflcist, rebase if needed.