Bug 40118 - Regression - 'Holds to pull' library filters don't work
Summary: Regression - 'Holds to pull' library filters don't work
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Pedro Amorim
QA Contact: Jonathan Druart
URL:
Keywords: rel_24_11_candidate, rel_25_05_candidate
Depends on: 39473
Blocks: 40122 40180
  Show dependency treegraph
 
Reported: 2025-06-11 13:20 UTC by Pedro Amorim
Modified: 2025-06-18 21:24 UTC (History)
4 users (show)

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


Attachments
Bug 40118: Trim again after splitting (1018 bytes, patch)
2025-06-11 13:28 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40118: Only concat ^ and $ if table is ajax (2.47 KB, patch)
2025-06-11 13:28 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40118: Prevent adding another empty value option (3.32 KB, patch)
2025-06-11 14:17 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40118: Add cypress (3.08 KB, patch)
2025-06-11 15:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40118: [24.11] Squashed patch for 24.11 (3.75 KB, patch)
2025-06-11 15:38 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 40118: Trim again after splitting (1.04 KB, patch)
2025-06-16 12:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 40118: Only concat ^ and $ if table is ajax (2.54 KB, patch)
2025-06-16 12:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 40118: Prevent adding another empty value option (3.37 KB, patch)
2025-06-16 12:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 40118: Trim again after splitting (1.10 KB, patch)
2025-06-18 07:43 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40118: Only concat ^ and $ if table is ajax (2.59 KB, patch)
2025-06-18 07:43 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 40118: Prevent adding another empty value option (3.42 KB, patch)
2025-06-18 07:43 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2025-06-11 13:20:49 UTC

    
Comment 1 Pedro Amorim 2025-06-11 13:28:41 UTC
Created attachment 183161 [details] [review]
Bug 40118: Trim again after splitting
Comment 2 Pedro Amorim 2025-06-11 13:28:43 UTC
Created attachment 183162 [details] [review]
Bug 40118: Only concat ^ and $ if table is ajax

Test plan, k-t-d, before applying patches:
1) Place a hold (next available) to biblio 76 and 437:
http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=76
http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=437
2) Visit the 'holds to pull' page:
http://localhost:8081/cgi-bin/koha/circ/pendingreserves.pl
3) Notice the 'Libraries' filter contains 'Centervile' twice. Notice the filtering never works regardless of option picked.
4) Apply patches. Repeat test plan. Notice 'Centerville' only shows once. Notice filtering works.
Comment 3 Jonathan Druart 2025-06-11 14:13:54 UTC
-                    let value = this.value.length ? "^" + this.value + "$" : "";

Can you explain this?

Now we should have tests for changes to this file (See bug 40002 that introduced this change).
Comment 4 Pedro Amorim 2025-06-11 14:15:58 UTC
(In reply to Jonathan Druart from comment #3)
> -                    let value = this.value.length ? "^" + this.value + "$"
> : "";
> 
> Can you explain this?
> 
> Now we should have tests for changes to this file (See bug 40002 that
> introduced this change).

It's unused code. The 'value' variable is never used.
Comment 5 Pedro Amorim 2025-06-11 14:17:37 UTC
Created attachment 183164 [details] [review]
Bug 40118: Prevent adding another empty value option

This is already done for all select dropdowns by default. If a particular table provides an empty value it should be skipped or 2 empty options may show
Comment 6 Jonathan Druart 2025-06-11 14:48:08 UTC
(In reply to Pedro Amorim from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > -                    let value = this.value.length ? "^" + this.value + "$"
> > : "";
> > 
> > Can you explain this?
> > 
> > Now we should have tests for changes to this file (See bug 40002 that
> > introduced this change).
> 
> It's unused code. The 'value' variable is never used.

It's caused by 3fce8f83dc9696936342c6c3289389cc0eb38c2b


-                        table_dt
-                            .column(i)
-                            .search( this.value.length ? '^'+this.value+'$' : '', true, false )
-                            .draw();


+        $(this).find("select")
+            .unbind()
+            .bind("keyup change", function(){
+                let value = this.value.length ? '^'+this.value+'$' : '';
+                col_select_search(i, this.value)
+            });

Should be `col_select_search(i, value)` then I assume. But now the values have the ^$ in them.

I am then wondering what's better... Maybe we need to deal with that on a separate bug.
Comment 7 Pedro Amorim 2025-06-11 15:19:51 UTC
Created attachment 183169 [details] [review]
Bug 40118: Add cypress

cypress run --spec t/cypress/integration/KohaTable/Holdings_spec.ts
Comment 8 Pedro Amorim 2025-06-11 15:38:04 UTC
Created attachment 183170 [details] [review]
Bug 40118: [24.11] Squashed patch for 24.11
Comment 9 Jonathan Druart 2025-06-12 08:27:55 UTC
The cypress tests are wrong:
1. Holdings_spec.ts if for the holdings table on catalogue/detail.pl)
2. NEVER delete everything from a table, you have to create the data you need for the tests
3. The test will fail on Jenkins (it does not have the misc4dev sample data)
4. You should have a look at the mockData (buildSampleObject)
Comment 10 Jonathan Druart 2025-06-12 10:52:20 UTC
I am working on the Cypress tests, it's for a separate bug report (not trivial).

The other patches are correct.
Comment 11 Owen Leonard 2025-06-13 15:09:29 UTC
We have a library named 'Wells (Albany)' and filtering doesn't work for that value. A problem with the parentheses?
Comment 12 Pedro Amorim 2025-06-13 15:21:15 UTC
(In reply to Owen Leonard from comment #11)
> We have a library named 'Wells (Albany)' and filtering doesn't work for that
> value. A problem with the parentheses?

Yes, different bug. Please bug 40122.
Comment 13 Owen Leonard 2025-06-16 12:59:06 UTC
Created attachment 183280 [details] [review]
Bug 40118: Trim again after splitting

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2025-06-16 12:59:09 UTC
Created attachment 183281 [details] [review]
Bug 40118: Only concat ^ and $ if table is ajax

Test plan, k-t-d, before applying patches:
1) Place a hold (next available) to biblio 76 and 437:
   http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=76
   http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=437
2) Visit the 'holds to pull' page:
   http://localhost:8081/cgi-bin/koha/circ/pendingreserves.pl
3) Notice the 'Libraries' filter contains 'Centervile' twice. Notice the
   filtering never works regardless of option picked.
4) Apply patches. Repeat test plan. Notice 'Centerville' only shows
   once. Notice filtering works.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Owen Leonard 2025-06-16 12:59:13 UTC
Created attachment 183282 [details] [review]
Bug 40118: Prevent adding another empty value option

This is already done for all select dropdowns by default. If a
particular table provides an empty value it should be skipped or 2
empty options may show

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 16 Paul Derscheid 2025-06-18 07:43:04 UTC
Created attachment 183316 [details] [review]
Bug 40118: Trim again after splitting

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 17 Paul Derscheid 2025-06-18 07:43:06 UTC
Created attachment 183317 [details] [review]
Bug 40118: Only concat ^ and $ if table is ajax

Test plan, k-t-d, before applying patches:
1) Place a hold (next available) to biblio 76 and 437:
   http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=76
   http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=437
2) Visit the 'holds to pull' page:
   http://localhost:8081/cgi-bin/koha/circ/pendingreserves.pl
3) Notice the 'Libraries' filter contains 'Centervile' twice. Notice the
   filtering never works regardless of option picked.
4) Apply patches. Repeat test plan. Notice 'Centerville' only shows
   once. Notice filtering works.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 18 Paul Derscheid 2025-06-18 07:43:08 UTC
Created attachment 183318 [details] [review]
Bug 40118: Prevent adding another empty value option

This is already done for all select dropdowns by default. If a
particular table provides an empty value it should be skipped or 2
empty options may show

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>