Bug 39011 - Unable to search the holdings table (except home/holding libraries and barcode)
Summary: Unable to search the holdings table (except home/holding libraries and barcode)
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 33568
Blocks: 39315
  Show dependency treegraph
 
Reported: 2025-01-31 20:03 UTC by Caroline Cyr La Rose
Modified: 2025-05-12 16:01 UTC (History)
11 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes and improves searching the holdings table for columns that use an authorized or coded value. You can now use either the codes or the description when searching for item type, current library, home library, and collection columns. For example, searching for BK or Books now works as expected.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39011: Pre-process coded values when building the query (8.96 KB, patch)
2025-02-03 11:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (9.00 KB, patch)
2025-02-05 20:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (9.05 KB, patch)
2025-02-05 20:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (9.11 KB, patch)
2025-02-07 13:04 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (9.16 KB, patch)
2025-02-10 16:22 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39011: Apply the pre-process to column filters (1.57 KB, patch)
2025-02-11 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (10.22 KB, patch)
2025-02-13 16:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39011: Apply the pre-process to column filters (1.42 KB, patch)
2025-02-13 16:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (10.26 KB, patch)
2025-02-14 20:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 39011: Apply the pre-process to column filters (1.47 KB, patch)
2025-02-14 20:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 39011: Pre-process coded values when building the query (10.33 KB, patch)
2025-04-02 08:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39011: Apply the pre-process to column filters (1.53 KB, patch)
2025-04-02 08:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39011: (follow-up) JSDoc (2.05 KB, patch)
2025-04-02 08:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39011: (follow-up) JSDoc (2.05 KB, patch)
2025-04-02 08:58 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 39011: (follow-up) Update selenium tests (1.28 KB, patch)
2025-05-09 13:50 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2025-01-31 20:03:09 UTC
When using the search box in the holdings tab on a record details page in the staff interface, you can only find holdings with the home or holding library. You can't search for the item type or anything else.

To recreate:

1. Go to a record details page in the staff interface (e.g. http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=285)

2. Add an item with a different itemtype
   2.1. Click New > New item
   2.2. Change the itemtype to Music (or another itemtype)
   2.3. Click Add item
   2.4. Click Normal to go back to the record details page

3. Try to search for the itemtype in the holdings
   3.1. Using the small search box right above the holdings table, search for the new itemtype (Music)
   --> No results

4. Optionally, search using other text in the holdings table
   - "Book" --> NOT OK does not find the items with the itemtype "Books"
   - "Midway" --> OK finds the items with the home or holding library "Midway"
   - "General" --> NOT OK does not find the items with the location "General stacks"
   - "Reference" --> NOT OK does not find the items with the collection "Reference"
   -  "2477" --> OK finds the item with the barcode 39999000012477

With git bisect, I was able to retrace that it was ok before bug 33568 and not after, but I don't know which commit exactly causes the problem since the holdings table doesn't load if I'm somewhere in the middle of all the 33568 commits.
Comment 1 Caroline Cyr La Rose 2025-01-31 20:45:44 UTC
I noticed that when searching for BK or MU, it finds the items with those itemtypes. 

I'm guessing it's a similar problem to bug 37334 (??) of the table using the codes instead of the labels.

However, home/holding libraries are searchable both by code and by name. I don't see why it couldn't be the same for item types.

- "CPL" --> OK finds items with Centerville home or holding library
- "Center" --> OK finds items with Centerville home or holding library
- "FPL" --> OK finds items with Fairview home or holding library
- "FAIR" --> OK finds items with Fairview home or holding library
Comment 2 Caroline Cyr La Rose 2025-01-31 20:59:34 UTC
There are no itemcallnumbers in KTD by default, but I found that those work fine, like the barcodes.
Comment 3 Jonathan Druart 2025-02-03 11:20:45 UTC
(In reply to Caroline Cyr La Rose from comment #1)
> I'm guessing it's a similar problem to bug 37334 (??) of the table using the
> codes instead of the labels.

No, it's similar to bug 38130.
Comment 4 Jonathan Druart 2025-02-03 11:32:18 UTC
Created attachment 177465 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.
Comment 5 Jonathan Druart 2025-02-03 11:33:46 UTC
Settings to "Needs signoff" so Caroline can test.

But I would like feedback from other people close to this code.
Comment 6 David Nind 2025-02-03 21:12:15 UTC
I tested, and this is working for me (both the code and the full name) for:
- Current library (holding library)
- Home library (home library)
- Collection (collection code)

It is not working for me for item type, for example BK or Books shows no results.
Comment 7 Jonathan Druart 2025-02-05 20:13:49 UTC
Created attachment 177553 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.
Comment 8 Jonathan Druart 2025-02-05 20:14:05 UTC
(In reply to David Nind from comment #6)
> I tested, and this is working for me (both the code and the full name) for:
> - Current library (holding library)
> - Home library (home library)
> - Collection (collection code)
> 
> It is not working for me for item type, for example BK or Books shows no
> results.

Indeed, code and description were swapped. I also fixed the search on holding branch.
Comment 9 David Nind 2025-02-05 20:36:11 UTC
Created attachment 177556 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-02-05 20:44:28 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to David Nind from comment #6)
> > I tested, and this is working for me (both the code and the full name) for:
> > - Current library (holding library)
> > - Home library (home library)
> > - Collection (collection code)
> > 
> > It is not working for me for item type, for example BK or Books shows no
> > results.
> 
> Indeed, code and description were swapped. I also fixed the search on
> holding branch.

Thanks Jonathan!
Comment 11 Nick Clemens (kidclamp) 2025-02-07 13:04:37 UTC
Created attachment 177620 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens (kidclamp) 2025-02-07 13:06:05 UTC
Added 150 branches and 900 items, this performed as well as or better than the previous method for searching libraries and the new searches all worked in my testing.

Asking Lucas to give a final stamp here as he has been working on these issues
Comment 13 Lucas Gass (lukeg) 2025-02-10 16:15:54 UTC
Now we have a situation where there are two "filters". The dataTables default one ( #dt-search-0 ) above the table and one for each column after clicking 'Show filters'.

With collection code these two now behave differently with this patch. The dataTable filter can filter by collection code or description. The one directly above the column only searches on code. That behavior should probably be consistent OR the filter above the column should become a dropdown like item type/home branch/holding branch?
Comment 14 Lucas Gass (lukeg) 2025-02-10 16:18:50 UTC
(In reply to Lucas Gass (lukeg) from comment #13)
> Now we have a situation where there are two "filters". The dataTables
> default one ( #dt-search-0 ) above the table and one for each column after
> clicking 'Show filters'.
> 
> With collection code these two now behave differently with this patch. The
> dataTable filter can filter by collection code or description. The one
> directly above the column only searches on code. That behavior should
> probably be consistent OR the filter above the column should become a
> dropdown like item type/home branch/holding branch?

However, this is probably a seperate bug/issue so I'm willing to PQA here.
Comment 15 Lucas Gass (lukeg) 2025-02-10 16:22:04 UTC
Ignoring the QA complaining about a console.log, as I assume that is wanted.
Comment 16 Lucas Gass (lukeg) 2025-02-10 16:22:59 UTC
Created attachment 177702 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 17 Jonathan Druart 2025-02-10 19:57:31 UTC
(In reply to Lucas Gass (lukeg) from comment #14)
> (In reply to Lucas Gass (lukeg) from comment #13)
> > Now we have a situation where there are two "filters". The dataTables
> > default one ( #dt-search-0 ) above the table and one for each column after
> > clicking 'Show filters'.
> > 
> > With collection code these two now behave differently with this patch. The
> > dataTable filter can filter by collection code or description. The one
> > directly above the column only searches on code. That behavior should
> > probably be consistent OR the filter above the column should become a
> > dropdown like item type/home branch/holding branch?
> 
> However, this is probably a seperate bug/issue so I'm willing to PQA here.

I will have a look.
Comment 18 Jonathan Druart 2025-02-11 09:07:56 UTC
Created attachment 177725 [details] [review]
Bug 39011: Apply the pre-process to column filters
Comment 19 Jonathan Druart 2025-02-11 09:11:40 UTC
Please have a look at the second patch. It is supposed to apply the pre-process step to the values of the column filters.

I have not tested it a lot, I am a bit hesitant to change the core of our datatable code without providing Cypress tests. But unfortunately I don't have time right now to provide them.

Feel free to have a look and test it.
Comment 20 Magnus Enger 2025-02-13 15:44:06 UTC
$ git bz apply 39011

Bug 39011 - Unable to search the holdings table (except home/holding libraries and barcode)

177702 - Bug 39011: Pre-process coded values when building the query
177725 - Bug 39011: Apply the pre-process to column filters

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 39011: Pre-process coded values when building the query
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
M	koha-tmpl/intranet-tmpl/prog/js/datatables.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/datatables.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/datatables.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
error: Failed to merge in the changes.
Patch failed at 0001 Bug 39011: Pre-process coded values when building the query
Comment 21 Jonathan Druart 2025-02-13 16:00:18 UTC
Created attachment 178033 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 22 Jonathan Druart 2025-02-13 16:00:21 UTC
Created attachment 178034 [details] [review]
Bug 39011: Apply the pre-process to column filters
Comment 23 David Nind 2025-02-14 20:31:30 UTC
Created attachment 178118 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2025-02-14 20:31:34 UTC
Created attachment 178119 [details] [review]
Bug 39011: Apply the pre-process to column filters

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 Marcel de Rooy 2025-02-21 08:19:31 UTC
Not sure about this change. Seems like we want something from DataTables that it simply does not offer. Resulting code looks complexer than needed? Harder to maintain?
We create an inconsistency here too. I can look for the coded values with homebranch, searching for CPL. But I can not look for an item type with BKS. If we  would go for the descriptions that are shown, we should remove searching on the branch codes imo. This is a result of a join somewhere iiuc (not checked)?

Needs feedback.
Comment 26 Lucas Gass (lukeg) 2025-02-27 17:01:59 UTC
(In reply to Marcel de Rooy from comment #25)
> Not sure about this change. Seems like we want something from DataTables
> that it simply does not offer. Resulting code looks complexer than needed?
> Harder to maintain?
> We create an inconsistency here too. I can look for the coded values with
> homebranch, searching for CPL. But I can not look for an item type with BKS.
> If we  would go for the descriptions that are shown, we should remove
> searching on the branch codes imo. This is a result of a join somewhere iiuc
> (not checked)?
> 
> Needs feedback.

The inconsistency you mentioned should be fixed. Will removing the ability to also search by a code really be a problem? If we can we should leave it. Searching by descriptions is very important to librarians but I know some still like to search on code. 

Why do you think it is so complex? It doesn't seem so bad to me. Do you have a simpler solution in mind? If so, great! But if not I don't think we should block this because it is too complex. This is a bad bug for many libraries and a fix is needed ASAP.
Comment 27 Jonathan Druart 2025-03-11 16:02:00 UTC
Searching by item type works for me, using BK for Books.

I've tried to make this generic enough to have it applied to other tables easily.

The main idea is to make the job client-side and avoid unnecessary joins on the server. Here you will need to have 1 extra join per column with a dropdown filter which will result in monstrous SQL queries.

I personally think it's quite elegant :D
Comment 28 Lucas Gass (lukeg) 2025-03-11 16:37:47 UTC
(In reply to Jonathan Druart from comment #27)
> Searching by item type works for me, using BK for Books.
> 
> I've tried to make this generic enough to have it applied to other tables
> easily.
> 
> The main idea is to make the job client-side and avoid unnecessary joins on
> the server. Here you will need to have 1 extra join per column with a
> dropdown filter which will result in monstrous SQL queries.
> 
> I personally think it's quite elegant :D

I re-tested, itemtype works for me also. This is a pretty big blocker for many of our libraries. In my opinion this cannot be held up any longer so I am going to set it back to PQA.
Comment 29 Jonathan Druart 2025-03-12 08:31:43 UTC
Sorry, Lucas, but we have a QA member with doubts about the approach. I’d like to request another QA review, either Marcel needs to be convinced, or another member should approve it or suggest an alternative approach.

I’ll ask Tomas since he’s familiar with the DataTables.js code.
Comment 30 Jonathan Druart 2025-03-12 08:33:26 UTC
s/DataTables.js/datatables.js
Comment 31 Jonathan Druart 2025-03-12 12:18:03 UTC
Tests for those changes will be on bug 39315.
Comment 32 Martin Renvoize (ashimema) 2025-04-02 08:24:16 UTC
I think this is actually pretty clear and, importantly, performant, code.

My one caveat is that I'd like to see an update to the JSDoc to highlight the usage option this introduces so other developers can make use of it in the future.

I'm currently working on that as a followup, but I generally feel this is a pass once we have that.
Comment 33 Martin Renvoize (ashimema) 2025-04-02 08:56:25 UTC
Created attachment 180264 [details] [review]
Bug 39011: Pre-process coded values when building the query

When a column contains authorised/coded values, it is not possible to
search on the description. It is confusing for the user as the
description is the value displayed in the table.

We have discussed this already on bug 38130 and decided to fix the
problem by doing a new SQL join. But this can lead to performance
issues, especially for tables like the items table where a lot of coded
values are displayed.

This patch suggests to do a pre-processing step, before the query is
sent, to let the client build the query it needs.

An alternative approach would be to do the same job server-side, and
allow this kind of processing for all the attributes with a object using
columns_to_str. But we will certainly reach performance problems.

This is still not perfect, but it's fixing a regression quite easily,
and will open the door to more fixes for tables using coded values.

This patch revert one we have done on bug 38130, and use this new
mecanism for home library, holding library, item type and collection
code.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 34 Martin Renvoize (ashimema) 2025-04-02 08:56:30 UTC
Created attachment 180265 [details] [review]
Bug 39011: Apply the pre-process to column filters

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 35 Martin Renvoize (ashimema) 2025-04-02 08:56:35 UTC
Created attachment 180266 [details] [review]
Bug 39011: (follow-up) JSDoc

Add some initial JSDoc for the enhancments to datatables we introduce
here.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 36 Martin Renvoize (ashimema) 2025-04-02 08:58:39 UTC
Created attachment 180267 [details] [review]
Bug 39011: (follow-up) JSDoc

Add some initial JSDoc for the enhancments to datatables we introduce
here.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 37 Katrin Fischer 2025-04-14 06:52:46 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 38 Martin Renvoize (ashimema) 2025-05-09 13:50:19 UTC
Created attachment 182175 [details] [review]
Bug 39011: (follow-up) Update selenium tests

We alter how the options are added to the patrons_search datatables
search bar, but we didn't update the selenium test to match.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 39 Katrin Fischer 2025-05-12 16:01:48 UTC
Follow-up for selenium was pushed to main.