Bug 32447 - In items table barcode column can not be filtered
Summary: In items table barcode column can not be filtered
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Andreas Roussos
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-12 06:28 UTC by Fridolin Somers
Modified: 2023-12-28 20:43 UTC (History)
3 users (show)

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


Attachments
Screenshot of the problem (86.92 KB, image/png)
2022-12-12 06:28 UTC, Fridolin Somers
Details
Bug 32447: Clean up table configuraton settings for holdings table (2.49 KB, patch)
2023-02-11 00:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32447: Fix DataTable filtering when hidden columns are in place (10.45 KB, patch)
2023-02-21 11:32 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 32447: Clean up table configuraton settings for holdings table (2.54 KB, patch)
2023-03-03 10:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32447: Fix DataTable filtering when hidden columns are in place (10.51 KB, patch)
2023-03-03 10:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 32447: Clean up table configuraton settings for holdings table (2.60 KB, patch)
2023-03-03 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32447: Fix DataTable filtering when hidden columns are in place (10.56 KB, patch)
2023-03-03 19:25 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2022-12-12 06:28:50 UTC
Created attachment 144533 [details]
Screenshot of the problem

In record details page when activating filters in items table, no input appears for barcode column, it is only a text.
Comment 1 Katrin Fischer 2022-12-28 22:07:11 UTC
Also confirmed on current master.
Comment 2 Katrin Fischer 2023-02-10 23:55:44 UTC
It's not specific to barcode. If you add an URL to the item that makes an additional column after barcode appear. Now the URL field and actions are not filterable. It appears to always be the last 2 columns?
Comment 3 Katrin Fischer 2023-02-11 00:49:11 UTC
Created attachment 146528 [details] [review]
Bug 32447: Clean up table configuraton settings for holdings table
Comment 4 Katrin Fischer 2023-02-11 00:49:48 UTC
I was not able to fix the issue, but cleaned up the table configuration some in the process. Maybe it will help.
Comment 5 Katrin Fischer 2023-02-11 08:07:54 UTC
(In reply to Katrin Fischer from comment #2)
> It's not specific to barcode. If you add an URL to the item that makes an
> additional column after barcode appear. Now the URL field and actions are
> not filterable. It appears to always be the last 2 columns?

And then... it is specific to barcode. I think maybe some change I made in between moved it, but now we are back to barcode not being filterable, even with other new columns appearing in between? 

I don't understand the datatables well enough to see what's going on here.
Comment 6 Katrin Fischer 2023-02-15 14:06:29 UTC
Andreas, are you working on a patch? I stared at this forever... I really want to see this fixed (and see what the solution was!)
Comment 7 Andreas Roussos 2023-02-15 15:02:41 UTC
(In reply to Katrin Fischer from comment #6)
> Andreas, are you working on a patch? I stared at this forever... I really
> want to see this fixed (and see what the solution was!)
Indeed I am, Katrin. I actually stared at this forever too, and
almost broke my F11 key while debugging the code in Firefox's
Developer Tools over the last few days ;-)

I realised that if you undo the changes from commit 018a981b9b
the barcode search box appears again and searching for barcodes
is working, but there's still the issue of how the DataTable
behaves when you start hiding colums.

Anyway, I have a patch ready which fixes the issue described
here, and also fixes the issue from Bug 32448. As a matter of
fact, I think that both bugs are a bit similar since it's the
presence of hidden columns that causes the erratic DataTable.

I'm now in the process of preparing a Selenium unit test that
will help us detect problems with this particular DataTable
in the future.

Since you're eager to see the solution ;-) I uploaded the
patch here: https://diffy.org/diff/ceabcff747159
(toggling the mode to side-by-side makes it easier to read)

BTW, I didn't know which of the two Bugs I should assign myself
to, so I went for this one as it was the first one reported.
Comment 8 Andreas Roussos 2023-02-16 02:48:49 UTC
Apparently, there's just one more case where a DataTable (DT)
can be filtered with search <input>s and that takes place in
reports/itemslost.pl (Reports > Lost items > Results):

$ git grep --name-only table_filters.js -- ':!*.po'
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
koha-tmpl/intranet-tmpl/prog/en/modules/reports/itemslost.tt

Because the lost items results page is structured differently
compared to the bibliographic record details page (for example
the header cells don't have ids or data-colname attributes),
the patch mentioned earlier inadvertently breaks the 'Activate
filters' link with the following message logged in the console:

Uncaught Error: Syntax error, unrecognized expression: tr.columnFilter # input

I actually managed to fix that and got the link to work either
by manually adding unique ids to the <th> elements _or_ by
changing the related jQuery selector in my patch to not depend
on the cell id (the latter is probably a better idea until we
sort out the template file in question on a separate bug).

Then, clicking on 'Activate filters' worked but I run into
a different issue: the <input> elements were not being created
by the columnFilter DT plugin. To fix that, I temporarily
modified the function bound to the 'Activate filters' button's
click() event so that it always passes bRedrawFilters == true
when calling activate_filters(), effectively redrawing the DT
filters every time their visibility (not to be confused with
column visibility!) is toggled.

Now the <input>s appeared and search was working properly
save for the erratic behaviour whenever you toggled a column
(outlined in Bug 32448).

To fix that I added the necessary columnsInit() function to
koha-tmpl/intranet-tmpl/prog/en/modules/reports/itemslost.tt
and reverted the temporary change I made above so that it
would now pass bRedrawFilters == false (which had to be the
optimal solution as you should't have to redraw when you just
want to display, right?)

There was a glitch, though: unless I toggled the visibility
of at least one column *before* activating the DT filters,
the DT filters would not render when I clicked on the
'Activate filters' button. And here's why:

In my patch I take advantage of the already existing code
in koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc
that attaches an event handler function to the DataTables
column-visibility.dt event, which in turn calls columnsInit()
(should that function be defined elsewhere):

213     table.dataTable(new_parameters);
214     table.DataTable().on("column-visibility.dt", function(){
215         if( typeof columnsInit == 'function' ){
216             // This function can be created separately and used to trigger
217             // an event after the DataTable has loaded AND column visibility
218             // has been updated according to the table's configuration
219             columnsInit();
220         }
221     }).columns( hidden_ids ).visible( false );

The only modification I made to the above code was to change
the columnsInit() call to also pass on the DT table object:

219             columnsInit(this);

To make this work I also added the following code in
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt:

1969         function columnsInit(table) {
1970             activate_filters(table.id, false, true);
1971         }

That way, whenever a table column is added to/removed from the
table via the "⚙ Columns" button, the DT filters are redrawn
and their search boxes work just fine.

There's a crucial point to be made, however, which is this:

Because the 'Lost items' results DT currently has no hidden
columns (per the admin/columns_settings.yml column settings),
the column-visibility.dt event does _not_ fire upon page load
to trigger the initial render of the table filters.

To avoid this, the simple-yet-not-optimal solution is to
simply call activate_filters() with bRedrawFilters == true
all the time!
Comment 9 Andreas Roussos 2023-02-21 11:14:10 UTC
Unfortunately, I can't get my Selenium unit test to work consistently
as it fails randomly while testing the 'Holdings' and 'Other holdings'
DataTables. Most of the time it passes, but sometimes it fails with:

Unable to locate element: //*[@id="otherholdings_table_activate_filters"]

Also, sometimes the 'Holdings' DataTable will not include both items
from the bibliographic record I'm creating within the unit test, and
this causes my tests to fail. I've added a 10-second sleep to ensure
Zebra has indexed the records by the time I'm drawing the table, but
it doesn't seem to make a difference.

I have some difficulty in designing the subtest for the Lost items
report too, as the 'Activate filters' link element does not react
when clicked and does not expand the table filters. Clicking on the
same link in the first two tests works fine, so I'm at loss as to
what's causing this...

For these reasons, I'm just going to attach only my patch for the
time being and leave the Selenium unit test for later. I've manually
tested the proposed changes extensively and also wrote a detailed
test plan -- hopefully that should be enough to move this forward.
Comment 10 Andreas Roussos 2023-02-21 11:32:51 UTC
Created attachment 147024 [details] [review]
Bug 32447: Fix DataTable filtering when hidden columns are in place

The bibliographic record's details page in the Staff interface
includes a 'Holdings' table at the bottom with information for
each item attached to the record. When activating the filters
in this table, there is no input field for the barcode column
but just bold text.

This broke in v22.11.00, the related commit being 018a981b9b
from Bug 29282 where two new hidden columns were added to that
table.

We can fix this by taking advantage of the existing code in
koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc
(introduced by commit dfb7af91af6 from Bug 23307) which allows
us to create and hook our own custom columnsInit() function in
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
to redraw the DataTable filters upon page load if a column is
marked as 'is_hidden: 1' in admin/columns_settings.yml, or if
a column is added/removed via the "⚙ Columns" button (both
are handled by the DataTables column-visibility.dt event).

Redrawing the filters via the above method also fixes the
issue described in Bug 32448.

Test plan:

1) Confirm the erratic DataTable behaviour outlined above
2) Apply this patch and reload all JS assets (hit CTRL-F5)
3) Confirm that you now see the correct input text field
   for the 'Barcode' column
4) Confirm that you can search for barcodes or in any other
   column successfully
5) Try toggling the visibility of the columns and making as
   many search variations as possible -- it should all now
   work without any glitches!

For extra credit ;-) you can also test the 'Other holdings'
table by setting the SeparateHoldings SysPref to 'Separate'.
Comment 11 Andreas Roussos 2023-02-21 11:35:45 UTC
I've uploaded my proposed Selenium unit test here:

https://gist.github.com/a-roussos/438e69fc9d52bf9feb69760baf1efae6
Comment 12 Julian Maurice 2023-03-03 10:17:33 UTC
Created attachment 147684 [details] [review]
Bug 32447: Clean up table configuraton settings for holdings table

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 13 Julian Maurice 2023-03-03 10:17:36 UTC
Created attachment 147685 [details] [review]
Bug 32447: Fix DataTable filtering when hidden columns are in place

The bibliographic record's details page in the Staff interface
includes a 'Holdings' table at the bottom with information for
each item attached to the record. When activating the filters
in this table, there is no input field for the barcode column
but just bold text.

This broke in v22.11.00, the related commit being 018a981b9b
from Bug 29282 where two new hidden columns were added to that
table.

We can fix this by taking advantage of the existing code in
koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc
(introduced by commit dfb7af91af6 from Bug 23307) which allows
us to create and hook our own custom columnsInit() function in
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
to redraw the DataTable filters upon page load if a column is
marked as 'is_hidden: 1' in admin/columns_settings.yml, or if
a column is added/removed via the "⚙ Columns" button (both
are handled by the DataTables column-visibility.dt event).

Redrawing the filters via the above method also fixes the
issue described in Bug 32448.

Test plan:

1) Confirm the erratic DataTable behaviour outlined above
2) Apply this patch and reload all JS assets (hit CTRL-F5)
3) Confirm that you now see the correct input text field
   for the 'Barcode' column
4) Confirm that you can search for barcodes or in any other
   column successfully
5) Try toggling the visibility of the columns and making as
   many search variations as possible -- it should all now
   work without any glitches!

For extra credit ;-) you can also test the 'Other holdings'
table by setting the SeparateHoldings SysPref to 'Separate'.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Julian Maurice 2023-03-03 10:18:51 UTC
Followed the test plan (with and without SeparateHoldings). Works perfectly!
Signed off.
Comment 15 Nick Clemens 2023-03-03 19:25:02 UTC
Created attachment 147729 [details] [review]
Bug 32447: Clean up table configuraton settings for holdings table

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2023-03-03 19:25:04 UTC
Created attachment 147730 [details] [review]
Bug 32447: Fix DataTable filtering when hidden columns are in place

The bibliographic record's details page in the Staff interface
includes a 'Holdings' table at the bottom with information for
each item attached to the record. When activating the filters
in this table, there is no input field for the barcode column
but just bold text.

This broke in v22.11.00, the related commit being 018a981b9b
from Bug 29282 where two new hidden columns were added to that
table.

We can fix this by taking advantage of the existing code in
koha-tmpl/intranet-tmpl/prog/en/includes/columns_settings.inc
(introduced by commit dfb7af91af6 from Bug 23307) which allows
us to create and hook our own custom columnsInit() function in
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
to redraw the DataTable filters upon page load if a column is
marked as 'is_hidden: 1' in admin/columns_settings.yml, or if
a column is added/removed via the "⚙ Columns" button (both
are handled by the DataTables column-visibility.dt event).

Redrawing the filters via the above method also fixes the
issue described in Bug 32448.

Test plan:

1) Confirm the erratic DataTable behaviour outlined above
2) Apply this patch and reload all JS assets (hit CTRL-F5)
3) Confirm that you now see the correct input text field
   for the 'Barcode' column
4) Confirm that you can search for barcodes or in any other
   column successfully
5) Try toggling the visibility of the columns and making as
   many search variations as possible -- it should all now
   work without any glitches!

For extra credit ;-) you can also test the 'Other holdings'
table by setting the SeparateHoldings SysPref to 'Separate'.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Tomás Cohen Arazi 2023-04-03 10:30:15 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 18 Jacob O'Mara 2023-04-16 17:24:23 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 19 Lucas Gass 2023-04-24 17:44:20 UTC
Doesn't apply cleanly to 22.05.x, if needed please rebase.