Description
Caroline Cyr La Rose
2025-01-31 20:03:09 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 There are no itemcallnumbers in KTD by default, but I found that those work fine, like the barcodes. (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. 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. Settings to "Needs signoff" so Caroline can test. But I would like feedback from other people close to this code. 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. 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. (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. 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> (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! 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> 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 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? (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. Ignoring the QA complaining about a console.log, as I assume that is wanted. 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> (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. Created attachment 177725 [details] [review] Bug 39011: Apply the pre-process to column filters 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. $ 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 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> Created attachment 178034 [details] [review] Bug 39011: Apply the pre-process to column filters 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> Created attachment 178119 [details] [review] Bug 39011: Apply the pre-process to column filters Signed-off-by: David Nind <david@davidnind.com> 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. (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. 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 (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. 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. s/DataTables.js/datatables.js Tests for those changes will be on bug 39315. 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. 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> 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> 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> 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> Pushed for 25.05! Well done everyone, thank you! 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> Follow-up for selenium was pushed to main. Pushed to 24.11.x for 24.11.08 Very useful ;) |