Bug 33568 - Use the REST API to display items on the biblio detail view
Summary: Use the REST API to display items on the biblio detail view
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL: https://gitlab.com/joubu/Koha/-/commi...
Keywords: release-notes-needed, roadmap_24_05
: 23024 29527 (view as bug list)
Depends on: 29523 33037 33497 33804 33817 34211 34226 34913
Blocks: 16122 30113 32802 33195 33746 34353 34358 34832 35191 36535 36663 33860
  Show dependency treegraph
 
Reported: 2023-04-20 06:46 UTC by Jonathan Druart
Modified: 2024-04-26 21:27 UTC (History)
24 users (show)

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


Attachments
Bug 33568: Preserve item selection across pagination and filtering (5.98 KB, patch)
2023-05-17 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Cover image browser with images attached to three items (199.33 KB, image/png)
2023-07-21 13:49 UTC, Owen Leonard
Details
Bug 33568: Remove debug_display (2.06 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Starting up (14.80 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Display string version of coded values + format dates (3.71 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: can_be_edited (2.48 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Display item type image (2.97 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore class names on th's (6.77 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Cover images (4.79 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Hide empty columns (4.10 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Temporary set searchable to false where no data (3.40 KB, patch)
2024-04-03 07:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: fix DataTables bKohaColumnsUseNames (2.25 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Add columns visibility (1.72 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Remove visibility conditions (6.33 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix search on item type (1.33 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Shelving location (2.80 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Item groups (4.53 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: enumchron/volinfo (7.31 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Status (12.88 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Add link to moredetail (1.40 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: URI (2.73 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Display stringified version of copy_number (1.47 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: public notes (1.32 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Host title (2.48 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Print label button (1.60 KB, patch)
2024-04-03 07:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore action buttons (3.00 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Separate holdings (93.86 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Course reserves (8.43 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Escape all the things (16.11 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore filters (7.69 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix action links display (6.46 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Preserve item selection across pagination and filtering (6.30 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Use as_due_date to display due dates (1.60 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Display library names instead of codes for transfers (4.12 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Bug 33568: Display library names instead of codes for first hold (3.76 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Bug 33568: Display library names instead of codes for recall (3.28 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Display the desk name for the first hold (3.53 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore bundle items (6.08 KB, patch)
2024-04-03 07:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore host records (5.28 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore analytics (4.39 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Move code to a dedicated include file (81.60 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix DT dom and remove DT info if not needed (3.63 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix select/clear all links (3.25 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Add search on home and holding library's names (5.16 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Prevent 500 when filtering by item type (FIXMEs) (1.57 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: (follow-up) Restore filters (5.82 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix local cover images (5.09 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix 'actions' links when filters are shown/hidden (2.49 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: (bug 34226 follow-up) Add type=date to date columns (1.81 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Do not embed library names (4.71 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Do not embed item types (4.44 KB, patch)
2024-04-03 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: follow-up for 29523 (4.95 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Add some missing StaffDetailItemSelection conditionals (1.80 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Hide holdings tabs if empty (SeparateHoldings) (3.21 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: JSON-encode html output of build_table (1.73 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix ordering (2.46 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Do not display 'null' when no barcode (1.67 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off (3.09 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails (2.44 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore link to patron (3.26 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Restore column filters when StaffDetailItemSelection is off (4.53 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix patron_to_html display (5.14 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: (follow-up) Fix ordering - WIP (3.34 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix columns shift when pref are off (1.90 KB, patch)
2024-04-03 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Remove LPAD ordering (3.53 KB, patch)
2024-04-03 07:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: serialitem => serial_item (4.42 KB, patch)
2024-04-03 07:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: POD fixes (3.49 KB, patch)
2024-04-03 07:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Add hint on the table settings admin page (1.98 KB, patch)
2024-04-03 07:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix false positive 'missing_filter' (1.17 KB, patch)
2024-04-05 13:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Fix translatability issues (5.71 KB, patch)
2024-04-05 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Prevent api/v1/biblios.t to fail randomly (1.60 KB, patch)
2024-04-05 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: (follow-up) Add search on home and holding library's names (1.05 KB, patch)
2024-04-05 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: Prevent api/v1/biblios.t to fail randomly (1.58 KB, patch)
2024-04-05 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33568: (follow-up) Fix duplicated key in item spec (777 bytes, patch)
2024-04-09 07:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-04-20 06:46:18 UTC
It will fix performance issues to use our REST API route /biblios/XXX/items to display the items table on the bibliographic record detail view.
Comment 1 Jonathan Druart 2023-04-20 06:46:43 UTC
*** Bug 23024 has been marked as a duplicate of this bug. ***
Comment 2 Jonathan Druart 2023-04-20 08:33:20 UTC
I've pushed a remote branch as WIP/POC, see https://gitlab.com/joubu/Koha/-/commits/bug_33568

This is really awesome, please have a look!

I let the git history to show how I made the changes, step by step. But we could decide to squash later if needed.

There is still a lot of work to be done, and I would like to make sure I won't be alone on this one before spending more time on it.

I think we really to do this move, it will help people with lot of items attached to a single biblio.

I am expecting support with:
* early code review (now, it's ready for that)
* answer the FIXMEs (in git log and code), can be done inline in gitlab?
* help with the remaining work
* testing session (it will need to be fully tested before we push it)
* keeping in mind that this is on its way and we should not work/push big changes to this script/template (!)

It's either a candidate for an early push to 23.11, and a couple of you are with me, or I drop it now (read that I am not going to rebase it if the conflicts are getting tricky).

Let me know what you think about it and if you are willing (and how) to help me here.
Comment 3 David Cook 2023-05-04 02:22:20 UTC
(In reply to Jonathan Druart from comment #2)
> I think we really to do this move, it will help people with lot of items
> attached to a single biblio.

I 100% agree. 

Since this is one of the big limitations I bump into with Koha these days, I should be able to find time to test at the very least.
Comment 4 David Cook 2023-05-04 02:23:05 UTC
Oh wait... this is for the staff interface? Hmm... it's probably a more concern for us for the OPAC. Telling staff they have to wait a while isn't so bad in comparison to expecting public users to wait.
Comment 5 David Cook 2023-05-16 05:18:38 UTC
I was looking at the REST API route /biblios/XXX/items for some discovery but realized it needed more for the "Status" column unless I wanted to re-implement all the logic that we've embedded in the template view layer...

Wonder if there's something to be done with this one and bug 16826 and https://github.com/NatLibFi/koha-plugin-rest-di
Comment 6 Jonathan Druart 2023-05-16 06:32:45 UTC
On (In reply to David Cook from comment #5)
> I was looking at the REST API route /biblios/XXX/items for some discovery
> but realized it needed more for the "Status" column unless I wanted to
> re-implement all the logic that we've embedded in the template view layer...
> 
> Wonder if there's something to be done with this one and bug 16826 and
> https://github.com/NatLibFi/koha-plugin-rest-di

That is totally outside the scope of this bug.
Comment 7 David Cook 2023-05-17 01:12:57 UTC
(In reply to Jonathan Druart from comment #6)
> That is totally outside the scope of this bug.

Sure although it'll relate in the future one hopes.
Comment 8 Jonathan Druart 2023-05-17 09:42:52 UTC
Remote branch rebased and a couple of fixes added.

Setting status to Needs Signoff, this is ready to get feedback from testers.
Comment 9 Jonathan Druart 2023-05-17 12:43:01 UTC
Checkbox selection should be preserve when a new request is made (pagination, filtering, etc.)
Comment 10 Jonathan Druart 2023-05-17 13:55:19 UTC
Created attachment 151339 [details] [review]
Bug 33568: Preserve item selection across pagination and filtering

BTW Do we really need StaffDetailItemSelection?
Comment 11 Michaela Sieber 2023-05-17 17:07:08 UTC
(In reply to Jonathan Druart from comment #8)
> Remote branch rebased and a couple of fixes added.
> 
> Setting status to Needs Signoff, this is ready to get feedback from testers.


Is there a test plan?

btw:  it is not possible to test the patch in a sandbox 
http://staff-b33568.sandboxes.ptfs-europe.co.uk/

Error:
Template process failed: undef error - C4::Koha::GetAuthorisedValues(): DBI Exception: DBD::mysql::st execute failed: 'koha_b32748.av.category' isn't in GROUP BY at /kohadevbox/koha/Koha/Template/Plugin/AuthorisedValues.pm line 39
 at /kohadevbox/koha/C4/Templates.pm line 127
Comment 12 Jonathan Druart 2023-05-22 12:46:09 UTC
(In reply to Michaela Sieber from comment #11)
> (In reply to Jonathan Druart from comment #8)
> > Remote branch rebased and a couple of fixes added.
> > 
> > Setting status to Needs Signoff, this is ready to get feedback from testers.
> 
> 
> Is there a test plan?

There is a lot to test here, so it's hard to provide an exhaustive list. The idea is to catch bugs I have missed.

You need to play with:
* datatable features (column visibility, sorting, filtering, show/hide filters)
* sysprefs SeparateHoldings and SeparateHoldingsBranch
* syspref StaffDetailItemSelection
* the different action links (when you select at least one item)
* item cover images
* item groups (EnableItemGroups)
* bundles of items (there is still a FIXME to display the 'Manage bundle' button)
* display of other values (recall, status, holds, checkout and renewal count, URIs)
* host records
* course reserves
* analytic (this is still a FIXME)
* print label button (SpineLabelShowPrintOnBibDetails)
* edit permissions will show the edit buttons
* and the other things I have missed in this list? :)

In the code there are still FIXMEs:
* display library names instead of codes
* desk name is not displayed anymore
* checkout due date is not correctly formatted
* other technical ones for QA.


> btw:  it is not possible to test the patch in a sandbox 
> http://staff-b33568.sandboxes.ptfs-europe.co.uk/
> 
> Error:
> Template process failed: undef error - C4::Koha::GetAuthorisedValues(): DBI
> Exception: DBD::mysql::st execute failed: 'koha_b32748.av.category' isn't in
> GROUP BY at /kohadevbox/koha/Koha/Template/Plugin/AuthorisedValues.pm line 39
>  at /kohadevbox/koha/C4/Templates.pm line 127

There is something wrong going on but it's not related to this patch. The same of the sandbox was 'b33568' but the error is refering to another instance 'b32748'. To test this change you need to pass the info to the remote branch (there was a patch that has been wrongly attached here):
1. Don't pass a bug number
2. Click "advanced options"
3. "Git remote & branch" should be https://gitlab.com/joubu/Koha and bug_33568

I've just tested and it is working, see https://staff-bug33568.sandboxes.biblibre.eu/cgi-bin/koha/catalogue/detail.pl?biblionumber=1
Comment 13 Jonathan Druart 2023-05-23 13:14:33 UTC
(In reply to Jonathan Druart from comment #12)
> In the code there are still FIXMEs:
> * display library names instead of codes

Done in
Bug 33568: Display library names instead of codes for recall
Bug 33568: Display library names instead of codes for first hold
Bug 33568: Display library names instead of codes for transfers

> * desk name is not displayed anymore

Done in
Bug 33568: Display the desk name for the first hold

> * checkout due date is not correctly formatted

Done in
Bug 33568: Use as_due_date to display due dates


Remaining TODOs:
* bundles of items (display the 'Manage bundle' button)
* host records
* analytic

A new one:
* Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523.
Comment 14 Jonathan Druart 2023-05-24 13:22:15 UTC
(In reply to Jonathan Druart from comment #13)
> Remaining TODOs:
> * bundles of items (display the 'Manage bundle' button)

Done in
Bug 33568: Restore bundle items

> * host records

Done in
Bug 33568: Restore host records

> * analytic

Done in
Bug 33568: Restore analytics

Remaning TODOs:
* Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523.

* Fix bug 33817

* Fix bug 33804
Comment 15 Jonathan Druart 2023-05-25 07:42:33 UTC
New branch pushed:
* Fixed a bug about SeparateHoldings
* Code moved to an include file

* New patch for discussion:
"""
Bug 33568: Fix DT dom and remove DT info if not needed
    Was there a real need to deal with a specific dom? The "no filter" view
    was not really light, only the column filters were shown/hidden.

    This patch is suggesting to remove all the DT info in case the table is
    displaying all the items of the bibliographic record: Pagination, filtering, buttons, etc.
"""
Comment 16 Owen Leonard 2023-05-25 14:41:47 UTC
A few minor things:

- use "fa fa-filter" icon for the "Show filters" link
- the bulk actions controls (delete, modify) appear if you click a checkbox but not if you click "Select all"
- The checkbox column should not be sortable (an existing bug)

I like the choice to hide all the table controls until you click "Show filters" but I am concerned that others might miss the quick filter box. I hope others will comment on that.
Comment 17 Jonathan Druart 2023-05-30 08:50:38 UTC
(In reply to Owen Leonard from comment #16)
> A few minor things:

Thanks!

> - use "fa fa-filter" icon for the "Show filters" link

Commit "Restore filters" adjusted.

> - the bulk actions controls (delete, modify) appear if you click a checkbox
> but not if you click "Select all"

Done in "Fix select/clear all links".

> - The checkbox column should not be sortable (an existing bug)

This seems trivial to fix but it's not. The problem is the StaffDetailItemSelection (I've let a comment somewhere, IMO we should remove it, I don't see how it can be useful).
As it's an existing bug I would like to deal with this specific bug on a separate bug report.
I have opened bug 33860.

> I like the choice to hide all the table controls until you click "Show
> filters" but I am concerned that others might miss the quick filter box. I
> hope others will comment on that.

There was actually a bug. I've adjusted "Fix DT dom and remove DT info if not needed".
The idea is to display the search and pagination only if there is pagination. If all the items are displayed then we show a simple table and browser search ctrl+f can then be used to search on the displayed info.
Comment 18 Nick Clemens 2023-06-01 12:23:36 UTC
* Filters are operating on codes not values, 'FPL' works in 'Home library' but 'Fairview' doesn't
* Location appears in 'Home library' column, but cannot be filtered on code or value
* Status column not filterable or sortable
* Date columns filters only work on SQL formats '2023-06-01' works '06/01/2023' doesn't
* Click 'show filters', 'hide filters' and repeat X 2 - status column disappears?

If you do choose to show all items, a sort action requires reloading the whole table - is it possible to sort/filter on the loaded data?
Comment 19 Jonathan Druart 2023-06-02 06:59:23 UTC
(In reply to Nick Clemens from comment #18)
> * Filters are operating on codes not values, 'FPL' works in 'Home library'
> but 'Fairview' doesn't

Yes, we should add dropdown lists for itemtype and libraries, like we do on the patron search.
But for instance "home library" contains the location as well, that's not that easy :-/

> * Location appears in 'Home library' column, but cannot be filtered on code
> or value

See above.

> * Status column not filterable or sortable

Yes, that's no longer possible, and won't be possible, at least in a first iteration. That's not trivial it seems.

See the following commit:
    Bug 33568: Status
    
    FIXME We need tests here!
    
    See the FIXME in the code, we are losing the ability to search on the status,
    but this is really impossible to implement if we are not storing the
    status (DB or search engine).

> * Date columns filters only work on SQL formats '2023-06-01' works
> '06/01/2023' doesn't

I thought I fixed that earlier somewhere else. And I did, on https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145096
But that's not nice. Not considering this high priority for now and considering outside the scope of this bug.

> * Click 'show filters', 'hide filters' and repeat X 2 - status column
> disappears?

See the following commit:
    Bug 33568: Restore filters
    
    FIXME - If we click 'show filters' the 'columns' list does not contain
    all the columns.
    We are destroying the DT and the hidden columns are removed from the
    dom.
    
    I think there are 2 possible solutions here:
    1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage
    to make this work correctly, because of the th filters)
    2. Move the th definition in the JS init of DT

> If you do choose to show all items, a sort action requires reloading the
> whole table - is it possible to sort/filter on the loaded data?

We could imagine a "freeze" button to remove the AJAX driven behaviour, if all data are loaded. But the behaviour will then be different. It will help to filter/sort on columns on which it's not possible server-side, but I am not sure it's a good idea...


Remaining TODOs (in addition of the FIXMEs in the code):
* Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523.

* Fix bug 33817

* Fix bug 33804

* Add dropdown lists for itemtype, libraries (current library and library) and collection in the column filter row (like we do on the main patron search). However how do we deal with "home library" which contains the location as well?

* Search by formatted date (see bug 32505)
Comment 20 Jonathan Druart 2023-06-02 09:18:48 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Nick Clemens from comment #18)
> > * Click 'show filters', 'hide filters' and repeat X 2 - status column
> > disappears?
> 
> See the following commit:
>     Bug 33568: Restore filters
>     
>     FIXME - If we click 'show filters' the 'columns' list does not contain
>     all the columns.
>     We are destroying the DT and the hidden columns are removed from the
>     dom.
>     
>     I think there are 2 possible solutions here:
>     1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage
>     to make this work correctly, because of the th filters)
>     2. Move the th definition in the JS init of DT

Should be fixed now.
Bug 33568: (folow-up) Restore filters
Comment 21 Katrin Fischer 2023-06-24 22:30:54 UTC
*** Bug 29527 has been marked as a duplicate of this bug. ***
Comment 22 Jonathan Druart 2023-07-07 13:16:59 UTC
(In reply to Jonathan Druart from comment #19)
> * Search by formatted date (see bug 32505)

Done on bug 34226.
Comment 23 Jonathan Druart 2023-07-17 08:27:27 UTC
Remote branch rebased against master.
Comment 24 Owen Leonard 2023-07-19 14:49:05 UTC
This is working really well in all the tests I could think of. I identified two issues:

1. Item cover images are not showing up.
2. The "Select all" control doesn't trigger the display of the batch actions
   controls, and likewise the "Clear all" control doesn't hide them. Clicking the 
   checkbox works as it should.
Comment 25 Jonathan Druart 2023-07-20 07:18:54 UTC
(In reply to Owen Leonard from comment #24)

Thanks a lot for your time on this one! :)

> This is working really well in all the tests I could think of. I identified
> two issues:
> 
> 1. Item cover images are not showing up.

See commit "Bug 33568: Fix local cover images"

> 2. The "Select all" control doesn't trigger the display of the batch actions
>    controls, and likewise the "Clear all" control doesn't hide them.
> Clicking the 
>    checkbox works as it should.

I cannot recreate this exact problem but I found a couple of problems with the checkbox selection and the show/hide filters, which have been fixed on "Bug 33568: Fix 'actions' links when filters are shown/hidden", so hopefully yours is fixed as well.
Comment 26 Owen Leonard 2023-07-21 13:49:51 UTC
Created attachment 153791 [details]
Cover image browser with images attached to three items

The cover image navigation seems to be getting duplicated.
Comment 27 Owen Leonard 2023-07-21 13:50:51 UTC
(In reply to Jonathan Druart from comment #25)
> See commit "Bug 33568: Fix local cover images"
> "Bug 33568: Fix 'actions' links when filters are shown/hidden"

These issues have indeed been resolved.
Comment 28 Jonathan Druart 2023-07-21 14:04:07 UTC
(In reply to Owen Leonard from comment #26)
> Created attachment 153791 [details]
> Cover image browser with images attached to three items
> 
> The cover image navigation seems to be getting duplicated.

Hum, interesting :D

Fixed (amended the patch)!

-            $(".cover-slider").each(function(){                                                                                                                                                                     
+            cover_sliders.each(function(){
Comment 29 Jonathan Druart 2023-08-31 10:59:33 UTC
2 new patches added to the remote branch, for performance purpose:

Bug 33568: Do not embed library names
Bug 33568: Do not embed item types



1000 items generated for a given bibliographic record.

1000 items displayed:
without last 2 patches: 28s
with last 2 patches:    18s
master:                 15s (But lack on the UI as you see the 1000 items not paginated)

20 items displayed:
without last 2 patches: 0.9s
with last 2 patches:    0.7s
master:                 15s (cannot display 20 items, all displayed)
Comment 30 Owen Leonard 2023-09-07 14:41:05 UTC
I'm getting this error which breaks the loading of the table:

Uncaught TypeError: can't access property "notforloan", row.item_type is undefined
Comment 31 Jonathan Druart 2023-09-07 20:05:56 UTC
(In reply to Owen Leonard from comment #30)
> I'm getting this error which breaks the loading of the table:
> 
> Uncaught TypeError: can't access property "notforloan", row.item_type is
> undefined

Should be fixed now. Looks like I forgot to include one change in my previous push :-/
Comment 32 Owen Leonard 2023-09-08 15:27:53 UTC
I noticed a couple more issues:

- With SeparateHoldings enabled (set to "home library"), the tabs show up correctly and the contents of the tabs appear to be correct but the DataTables filters are enabled by default, though the control says "Show filters"

Clicking "Show filters" makes the tables disappear.

- I happened to look at a record from my own test data having items which had invalid item types. This caused a 500 error in the console and prevented the table from loading.
Comment 33 Jonathan Druart 2023-09-18 08:49:54 UTC
(In reply to Owen Leonard from comment #32)
> I noticed a couple more issues:
> 
> - With SeparateHoldings enabled (set to "home library"), the tabs show up
> correctly and the contents of the tabs appear to be correct but the
> DataTables filters are enabled by default, though the control says "Show
> filters"

I don't see that. The filters are not there and the link is "Show filters".

> Clicking "Show filters" makes the tables disappear.

However I am seeing that. It should be fixed now (commit "Bug 33568: (folow-up) Restore filters" has been adjusted)

> - I happened to look at a record from my own test data having items which
> had invalid item types. This caused a 500 error in the console and prevented
> the table from loading.

This is actually "expected". Koha is not supposed to behave correctly if invalid item types are used. It's not directly related to this bug, see bug 33166.
For the record, the error is "Can't call method "notforloan" on an undefined value at /kohadevbox/koha/Koha/Item.pm line 1510."
Comment 34 Owen Leonard 2023-10-16 12:12:47 UTC
(In reply to Jonathan Druart from comment #33)
> I don't see that. The filters are not there and the link is "Show filters".

Well, I'm not seeing it now either. I do see another issue with the SeparateHoldings preference:

If you're looking at a record which has no items that belong to "your" library you'll see a tab for that library saying "No matching records" See http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=9 in the sample data for example (with  SeparateHoldingsBranch set to "home library").

Before this patch Koha would display only a single "Holdings" tab showing all items. I'm not sure this is necessarily an unwanted change, but it is a difference.
Comment 35 Jonathan Druart 2023-10-16 19:06:23 UTC
(In reply to Owen Leonard from comment #34)
> (In reply to Jonathan Druart from comment #33)
> > I don't see that. The filters are not there and the link is "Show filters".
> 
> Well, I'm not seeing it now either. I do see another issue with the
> SeparateHoldings preference:
> 
> If you're looking at a record which has no items that belong to "your"
> library you'll see a tab for that library saying "No matching records" See
> http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=9 in the
> sample data for example (with  SeparateHoldingsBranch set to "home library").
> 
> Before this patch Koha would display only a single "Holdings" tab showing
> all items. I'm not sure this is necessarily an unwanted change, but it is a
> difference.

Nice one, thanks!

Fixed in "Bug 33568: Hide holdings tab if empty (SeparateHoldings)"
Comment 36 Owen Leonard 2023-10-18 18:29:23 UTC
Add my signoff, Jonathan, I think I've tested everything I can think of!
Comment 37 Jonathan Druart 2023-10-19 09:51:05 UTC
(In reply to Owen Leonard from comment #36)
> Add my signoff, Jonathan, I think I've tested everything I can think of!

Thanks a lot, Owen!

I've added your signed-off-by lines to the remote branch!
Comment 38 Jonathan Druart 2023-10-19 10:09:37 UTC
And rebased on top of bug 34913.

Still looks good, no obvious errors.
Comment 39 Laurence Rault 2023-10-20 10:01:23 UTC
A bit late, tested on real data with record having more than 2500 items

What was wrong in my test : 
1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n \n \n \n display

2) no checkbox in Other holdings tab if there is the first "my library" tab
(even as superlibrarian)

3) cannot place a hold, gateway timeout after having chosen the patron from the suggested list

4) with syspref  StaffDetailItemSelection disabled : impossible to charge table (error 500)

5) EnableItemGroups : No item group column in Other holdings tab
Comment 40 Jonathan Druart 2023-10-20 10:10:26 UTC
(In reply to Laurence Rault from comment #39)
> A bit late, tested on real data with record having more than 2500 items
> 
> What was wrong in my test : 
> 1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n
> \n \n \n display
> 
> 2) no checkbox in Other holdings tab if there is the first "my library" tab
> (even as superlibrarian)
> 
> 3) cannot place a hold, gateway timeout after having chosen the patron from
> the suggested list
> 
> 4) with syspref  StaffDetailItemSelection disabled : impossible to charge
> table (error 500)
> 
> 5) EnableItemGroups : No item group column in Other holdings tab

Can you provide me with the commit id on which you have tested? Using the English interface or French?
Comment 41 Jonathan Druart 2023-10-20 11:16:34 UTC
(In reply to Laurence Rault from comment #39)

Where have you tested, on a sandbox?

> A bit late, tested on real data with record having more than 2500 items
> 
> What was wrong in my test : 
> 1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n
> \n \n \n display

Can you share a screenshot?

> 2) no checkbox in Other holdings tab if there is the first "my library" tab
> (even as superlibrarian)

Same, I don't recreate, here is what I see: https://snipboard.io/pCzOst.jpg

> 3) cannot place a hold, gateway timeout after having chosen the patron from
> the suggested list

I don't think it's caused by this patch.

> 4) with syspref  StaffDetailItemSelection disabled : impossible to charge
> table (error 500)

I do recreate this one!

> 5) EnableItemGroups : No item group column in Other holdings tab

Looks like this is totally broken with my last rebase. I don't manage to create a new item group (broken by bug 34913).
Comment 42 Jonathan Druart 2023-10-20 11:26:21 UTC
(In reply to Jonathan Druart from comment #41)
> (In reply to Laurence Rault from comment #39)
> > 4) with syspref  StaffDetailItemSelection disabled : impossible to charge
> > table (error 500)
> 
> I do recreate this one!

Fixed on "Bug 33568: Fix ordering". It should also correct the default ordering.
Comment 43 Laurence Rault 2023-10-20 12:18:24 UTC
Tested on specific vm, on branch bug_33568.
Branch updated, so now : 

1) \n : solved
2) checkbox : solved

3) cannot place a hold, gateway timeout after having chosen the patron from the suggested list

load of page request.pl : 
40 s for 1000 items
more than 60 s for 1600 items (and timeout at 60s)
The patch does not change items display for this page ?

4) solved

5) items group broken as you said
Comment 44 Jonathan Druart 2023-10-20 12:30:39 UTC
(In reply to Laurence Rault from comment #43)
> 3) cannot place a hold, gateway timeout after having chosen the patron from
> the suggested list
> 
> load of page request.pl : 
> 40 s for 1000 items
> more than 60 s for 1600 items (and timeout at 60s)
> The patch does not change items display for this page ?

Nope! Only catalogue/detail.pl
Comment 45 Jonathan Druart 2023-10-20 13:08:07 UTC
(In reply to Jonathan Druart from comment #41)
> (In reply to Laurence Rault from comment #39)
> > 5) EnableItemGroups : No item group column in Other holdings tab
> 
> Looks like this is totally broken with my last rebase. I don't manage to
> create a new item group (broken by bug 34913).

I fixed the creation of item groups on the separate bug ("Bug 34913: Move item groups table to kohaTable"). The patch is on the remote branch now.

I don't recreate the problem. I am seeing the column is at least one item of the table is in an item group.
Comment 46 Martin Renvoize 2023-10-23 16:42:54 UTC
As soon as I see 29523 move I'll come back to this.. it's right at the top of my pile but I'm struggling to juggle with that one still teetering on the edge. ;)
Comment 47 Katrin Fischer 2023-10-30 13:59:24 UTC
Testing here...
Comment 48 Katrin Fischer 2023-10-30 14:52:02 UTC
I am sorry, but my testing didn't go as well as I had hoped for :(

Problems:
1) One of the commit messages needs to be fixed
2) When there is no barcode, it appears as 'null' in the column
3) 'Host records' column should not be available, when EasyAnalytics is turned off.
4) Table configuration doesn't appear to be used as expected. I checked: Catalog > holdings_table has only holdings_issues and holdings_renewals hidden by default. But the table keeps appearing with only a minimum number of columns and a lot of them hidden. Unfortunately the hidden column also includes much neded ones like callnumber. Any changes I make don't seem to stick.
5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in small size modal. This also has the effect, that there is no way to close this window without printing.
6) The status column is no longer sortable. It's sortable in 22.11 and works well there. Alos: I can't activate a filter for this column and the normal filter doesn't find the contents of the column. There is no way to find the 'checked out' or 'ordered' items in that list.
7) When an item is checked out, there is no longer a link to the patron's account.
8) hidepatronname doesn't seem to have any effect. 
9) Using the single search box on top of the table: "checked" doesn't find the checked out items. In 22.11 that works.
10) StaffDetailItemSelection: when disabled, not only the checkboxes are gone, but I can also no longer activate the column specific filters. In 22.11 this is still possible when the select feature is disabled.

Notes/Questions:
1) Currently some column only show when there is data for them, like course reserves. this is very helpful in preserving space, but doesn't seem to be the case anymore.
2) Right now we see all items immediately in the table, one the page has loaded. Now we only see the first 20. Libraries might feel this is a too low default setting. It would be great if we could have that configurable in table settings from the start.

Works well:
1) Date sorting with dd.mm.yyyy
2) Checkboxes between pages are preserved.
3) SeparateHoldings*: works as expected
Comment 49 Jonathan Druart 2023-10-30 17:09:32 UTC
(In reply to Katrin Fischer from comment #48)
> I am sorry, but my testing didn't go as well as I had hoped for :(
> 
> Problems:
> 1) One of the commit messages needs to be fixed

Two were kept for history, I removed them from the branch.

> 2) When there is no barcode, it appears as 'null' in the column

Fixed in "Bug 33568: Do not display 'null' when no barcode"

> 3) 'Host records' column should not be available, when EasyAnalytics is
> turned off.

Fixed in "Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off"

> 4) Table configuration doesn't appear to be used as expected. I checked:
> Catalog > holdings_table has only holdings_issues and holdings_renewals
> hidden by default. But the table keeps appearing with only a minimum number
> of columns and a lot of them hidden. Unfortunately the hidden column also
> includes much neded ones like callnumber. Any changes I make don't seem to
> stick.

The table is hiding empty columns. So if the x items displayed on the page do not have callnumber, then the column will be hidden, even if other pages have items with callnumber. Is that what you are seeing?
See commit message "Bug 33568: Hide empty columns".

It is what we are doing already on master, but here we don't want to fetch all the items to know if data exist for each each column...

> 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in
> small size modal. This also has the effect, that there is no way to close
> this window without printing.

Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails"

> 6) The status column is no longer sortable. It's sortable in 22.11 and works
> well there. Alos: I can't activate a filter for this column and the normal
> filter doesn't find the contents of the column. There is no way to find the
> 'checked out' or 'ordered' items in that list.

See commit message for "Bug 33568: Status"
"""
    See the FIXME in the code, we are losing the ability to search on the status,
    but this is really impossible to implement if we are not storing the
    status (DB or search engine).
"""

> 7) When an item is checked out, there is no longer a link to the patron's
> account.

Fixed in "Bug 33568: Restore link to patron"

> 8) hidepatronname doesn't seem to have any effect. 

I think this is a bug in js-patron-format.inc, it should be reported separately.

> 9) Using the single search box on top of the table: "checked" doesn't find
> the checked out items. In 22.11 that works.

If we want that we need a filter at the top of the page. That should be a separate bug.

> 10) StaffDetailItemSelection: when disabled, not only the checkboxes are
> gone, but I can also no longer activate the column specific filters. In
> 22.11 this is still possible when the select feature is disabled.

Fixed in "Bug 33568: Restore column filters when StaffDetailItemSelection is off"

> Notes/Questions:
> 1) Currently some column only show when there is data for them, like course
> reserves. this is very helpful in preserving space, but doesn't seem to be
> the case anymore.

"Preserving space"?
See above.

> 2) Right now we see all items immediately in the table, one the page has
> loaded. Now we only see the first 20. Libraries might feel this is a too low
> default setting. It would be great if we could have that configurable in
> table settings from the start.

It should be very easy to implement. I will do it when this reaches master.
Comment 50 Katrin Fischer 2023-10-30 19:07:14 UTC
Hi Jonathan,

this was a quick return :)

There are at least 2 points that give me some headache here in that it changes behavior in a way that will hurt library workflows. (marked DISCUSS). I think they need more time/thought and eyes on them before we are ready here.

Librarians: We need you to tell us how you use this table!


> > Problems:
> > 1) One of the commit messages needs to be fixed
> Two were kept for history, I removed them from the branch.

NOT OK - QA script still complains:
* Commit title does not contain 'follow-up' correctly spelt - fd8d8f5ba2 (Bug 33568: (folow-up) Restore filters)
* We also got some perltidy complaints on top.

> > 2) When there is no barcode, it appears as 'null' in the column
> Fixed in "Bug 33568: Do not display 'null' when no barcode"

OK - Confirmed fixed.

> > 3) 'Host records' column should not be available, when EasyAnalytics is
> > turned off.
> 
> Fixed in "Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off"

OK - Confirmed fixed.


> > 4) Table configuration doesn't appear to be used as expected. I checked:
> > Catalog > holdings_table has only holdings_issues and holdings_renewals
> > hidden by default. But the table keeps appearing with only a minimum number
> > of columns and a lot of them hidden. Unfortunately the hidden column also
> > includes much neded ones like callnumber. Any changes I make don't seem to
> > stick.
> 
> The table is hiding empty columns. So if the x items displayed on the page
> do not have callnumber, then the column will be hidden, even if other pages
> have items with callnumber. Is that what you are seeing?
> See commit message "Bug 33568: Hide empty columns".
> It is what we are doing already on master, but here we don't want to fetch
> all the items to know if data exist for each each column...

DISCUSS - Yes, confirmed - the column visibility is determined by the value savailable
on the first visible page.

I think this could be problematic, I'd really like to get more opinions, favourably from daily Koha users on this.


> > 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in
> > small size modal. This also has the effect, that there is no way to close
> > this window without printing.
> 
> Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails"

NOT OK - I yarn build, restart_all... but the modal is not 'modaling' for me. I still opens 
full size in a new tab.


> > 6) The status column is no longer sortable. It's sortable in 22.11 and works
> > well there. Alos: I can't activate a filter for this column and the normal
> > filter doesn't find the contents of the column. There is no way to find the
> > 'checked out' or 'ordered' items in that list.
> 
> See commit message for "Bug 33568: Status"
> """
>     See the FIXME in the code, we are losing the ability to search on the
> status,
>     but this is really impossible to implement if we are not storing the
>     status (DB or search engine).
> """

DISCUSS - So the problem here is that we are storing different numeric values and therefore cannot sort on anything useful... is that it?

I believe that could be an issue for a lot of libraries, as it stands now for the before and after:

Before
* Can sort on status
* Can search on status using column filter
* Can search on status using the top table search field (overall search)

After
* Can't do any of that.

It's definitely something that we need to get more eyes on again as it could be seen as quite disruptive.

Crazy idea: could we collect the status appearing in the data (all of them) and provide a pull down for limiting somehow? A multi select maybe?

> > 7) When an item is checked out, there is no longer a link to the patron's
> > account.
> 
> Fixed in "Bug 33568: Restore link to patron"

OK - Confirmed fixed.

> > 8) hidepatronname doesn't seem to have any effect. 
> 
> I think this is a bug in js-patron-format.inc, it should be reported
> separately.

I double-checked in current master:
hide:  	Checked out to 42 : due 04.11.2023 
don't hide: Checked out to koha (42) : due 04.11.2023 

So it does look like it's related to this patch set somehow. Sorry.

> > 9) Using the single search box on top of the table: "checked" doesn't find
> > the checked out items. In 22.11 that works.
> 
> If we want that we need a filter at the top of the page. That should be a
> separate bug.

I was talking about the filter on top of the table - are we talking about the same thing?


> > 10) StaffDetailItemSelection: when disabled, not only the checkboxes are
> > gone, but I can also no longer activate the column specific filters. In
> > 22.11 this is still possible when the select feature is disabled.
> 
> Fixed in "Bug 33568: Restore column filters when StaffDetailItemSelection is
> off"

OK - Confirmed fixed.

11) NEW: Column filters for item type and current library don't show as a pull down. Holding branch does.


> > Notes/Questions:
> > 1) Currently some column only show when there is data for them, like course
> > reserves. this is very helpful in preserving space, but doesn't seem to be
> > the case anymore.
> 
> "Preserving space"?
> See above.

Yep, understood now. Hesitating.

> > 2) Right now we see all items immediately in the table, one the page has
> > loaded. Now we only see the first 20. Libraries might feel this is a too low
> > default setting. It would be great if we could have that configurable in
> > table settings from the start.
> 
> It should be very easy to implement. I will do it when this reaches master.

I filed it separately as: 
Bug 35191 - Make entries per page configurable for items table on staff detail page
Comment 51 Jonathan Druart 2023-10-31 07:58:24 UTC
(In reply to Katrin Fischer from comment #50)
> > > Problems:
> > > 1) One of the commit messages needs to be fixed
> > Two were kept for history, I removed them from the branch.
> 
> NOT OK - QA script still complains:
> * Commit title does not contain 'follow-up' correctly spelt - fd8d8f5ba2
> (Bug 33568: (folow-up) Restore filters)

Indeed, I missed that one, fixed now.

> * We also got some perltidy complaints on top.

It's too late now. It is too time consuming to fix inline, and I don't like the big "tidy" patches...

> > > 4) Table configuration doesn't appear to be used as expected. I checked:
> > > Catalog > holdings_table has only holdings_issues and holdings_renewals
> > > hidden by default. But the table keeps appearing with only a minimum number
> > > of columns and a lot of them hidden. Unfortunately the hidden column also
> > > includes much neded ones like callnumber. Any changes I make don't seem to
> > > stick.
> > 
> > The table is hiding empty columns. So if the x items displayed on the page
> > do not have callnumber, then the column will be hidden, even if other pages
> > have items with callnumber. Is that what you are seeing?
> > See commit message "Bug 33568: Hide empty columns".
> > It is what we are doing already on master, but here we don't want to fetch
> > all the items to know if data exist for each each column...
> 
> DISCUSS - Yes, confirmed - the column visibility is determined by the value
> savailable
> on the first visible page.
> 
> I think this could be problematic, I'd really like to get more opinions,
> favourably from daily Koha users on this.

As explained, I didn't find an alternative.

> > > 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in
> > > small size modal. This also has the effect, that there is no way to close
> > > this window without printing.
> > 
> > Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails"
> 
> NOT OK - I yarn build, restart_all... but the modal is not 'modaling' for
> me. I still opens 
> full size in a new tab.

You don't need to rebuild.
It seems ok for me however, the pop-up opens in a new window.
Could you try to add a console.log there:

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
index acda1e9edd4..e24e6deb661 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc
@@ -818,6 +818,7 @@
 
                 [% IF ( SpineLabelShowPrintOnBibDetails ) %]
                      $(".print-label").on("click", function(e){
+                        console.log("click");
                         e.preventDefault();
                         link = $(this).attr("href");
                         openWindow(link,"Print spine label",400,400);

open the console and see if you see the "click" when you click on the "Print label" button?

> > > 6) The status column is no longer sortable. It's sortable in 22.11 and works
> > > well there. Alos: I can't activate a filter for this column and the normal
> > > filter doesn't find the contents of the column. There is no way to find the
> > > 'checked out' or 'ordered' items in that list.
> > 
> > See commit message for "Bug 33568: Status"
> > """
> >     See the FIXME in the code, we are losing the ability to search on the
> > status,
> >     but this is really impossible to implement if we are not storing the
> >     status (DB or search engine).
> > """
> 
> DISCUSS - So the problem here is that we are storing different numeric
> values and therefore cannot sort on anything useful... is that it?
> 
> I believe that could be an issue for a lot of libraries, as it stands now
> for the before and after:
> 
> Before
> * Can sort on status
> * Can search on status using column filter
> * Can search on status using the top table search field (overall search)
> 
> After
> * Can't do any of that.
> 
> It's definitely something that we need to get more eyes on again as it could
> be seen as quite disruptive.

Yes, it has been advertised since the beginning.

> Crazy idea: could we collect the status appearing in the data (all of them)
> and provide a pull down for limiting somehow? A multi select maybe?

Maybe, but it's definitely not for now neither here. We need to move the "status" logic from the template to a module.
That is something we need to do, but it is outside the scope of this bug.

> > > 8) hidepatronname doesn't seem to have any effect. 
> > 
> > I think this is a bug in js-patron-format.inc, it should be reported
> > separately.
> 
> I double-checked in current master:
> hide:  	Checked out to 42 : due 04.11.2023 
> don't hide: Checked out to koha (42) : due 04.11.2023 
> So it does look like it's related to this patch set somehow. Sorry.

It is a bug in js-patron-format.inc, but we were not using it before (patron-title.inc).
I finally decided to fix in a follow-up here.
Fixed in "Bug 33568: Fix patron_to_html display". Read the commit message, there are more fixes in this one.

> > > 9) Using the single search box on top of the table: "checked" doesn't find
> > > the checked out items. In 22.11 that works.
> > 
> > If we want that we need a filter at the top of the page. That should be a
> > separate bug.
> 
> I was talking about the filter on top of the table - are we talking about
> the same thing?

Yes. This is the same as the "status" problem. "checked" is not in a DB column.

We can have this quite easily (without the other statuses however), but it's not a simple patch.

> 11) NEW: Column filters for item type and current library don't show as a
> pull down. Holding branch does.

I do see a select there. Can you retest or provide more steps to recreate?
This is what I am seeing after a reset_all on /catalogue/detail.pl?biblionumber=4, click "Show filters"

https://snipboard.io/9E0VwH.jpg


> > > 2) Right now we see all items immediately in the table, one the page has
> > > loaded. Now we only see the first 20. Libraries might feel this is a too low
> > > default setting. It would be great if we could have that configurable in
> > > table settings from the start.
> > 
> > It should be very easy to implement. I will do it when this reaches master.
> 
> I filed it separately as: 
> Bug 35191 - Make entries per page configurable for items table on staff
> detail page

I will have a look.

If I understand correctly, this is "in discussion". I need to know if people still wants it for 23.11, do I spend more time on it today or do we postpone the remaining work to next month/year?
Comment 52 Katrin Fischer 2023-10-31 10:11:44 UTC
The problem I see is, that we have lot of libraries that won't need this change, they have only a few items per record and everything works fine. They might see some of this a a regression, also it seems we still have some things to iron out and we are running out of time.

Then there is the big libraries that would be happy to have this as they experience problems with records with many items - they might be happy to accept the changes.

I think rushing a decision between 3 people (you, me, RM) here would not be good right now. We should aim to have this on the Road map early next release and maybe we can get some more reactions (I am thinking of ways to reach out)
Comment 53 Jonathan Druart 2023-10-31 10:50:26 UTC
OK.
Not sure I am ready for another round of rebase however...
Comment 54 Martin Renvoize 2023-10-31 17:22:57 UTC
I'd be keen to see this pushed pretty much immediately after release giving us a whole cycle to really work through the issues presented and making for a trivial backport for those libraries very keen.
Comment 55 Jonathan Druart 2023-12-12 12:27:18 UTC
Moving forward please?
Comment 56 David Cook 2023-12-12 22:43:51 UTC
(In reply to Katrin Fischer from comment #52)
> I think rushing a decision between 3 people (you, me, RM) here would not be
> good right now. We should aim to have this on the Road map early next
> release and maybe we can get some more reactions (I am thinking of ways to
> reach out)

I'm in favour of this change conceptually, but haven't followed it very closely. What input is needed from a wider audience? Happy to provide targeted feedback if someone can tell me what they want feedback on :)
Comment 57 Katrin Fischer 2023-12-13 10:25:20 UTC
I think we should put it on the road map if it isn't yet and I'd really like to give libraries a chance to get involved. With the holidays coming up, I mid-January might be a good time window to work on this.
Comment 58 Jonathan Druart 2023-12-13 11:08:36 UTC
How can we get feedback from libraries if it is not pushed?

This was supposed to be pushed early in the cycle.
Comment 59 Nick Clemens 2023-12-13 13:56:54 UTC
I agree with Joubu - this will become unwieldy to improve unless pushed. It will make some changes to the current display, and we can handle those individually on smaller bugs once we have this base.
Comment 60 Jonathan Druart 2023-12-13 15:26:59 UTC
Remote branch rebased.
Comment 61 Andrii Nugged 2023-12-14 10:27:09 UTC
We can take this for our libraries in Finland and maybe for Ukraine in our custom build for testing, until this "more or less complete". They asking for this feature.
Comment 62 Jonathan Druart 2024-01-09 15:18:13 UTC
Remote branch rebased against master.
Comment 63 Jonathan Druart 2024-01-09 15:19:18 UTC
(In reply to Andrii Nugged from comment #61)
> We can take this for our libraries in Finland and maybe for Ukraine in our
> custom build for testing, until this "more or less complete". They asking
> for this feature.

Is it being tested? Any feedback?
Comment 64 Emily Lamancusa 2024-01-19 21:40:31 UTC
I'll take a look at this early next week. I think I can get others from MCPL to test too - we have bibs with hundreds of items, and would definitely like to see performance improvements on that move forward!
Comment 65 Katrin Fischer 2024-01-22 21:44:07 UTC
(In reply to Emily Lamancusa from comment #64)
> I'll take a look at this early next week. I think I can get others from MCPL
> to test too - we have bibs with hundreds of items, and would definitely like
> to see performance improvements on that move forward!

Thanks Emily, that would be awesome.
Comment 66 Emily Lamancusa 2024-01-25 22:17:52 UTC
Notes from my testing:
The majority of it seems to be working as it's supposed to. I ran into a few problems with filtering, though:

1. When using the general "Search" field at the top of the table, the filter often failed with the error message: “Something went wrong when loading the table: 500 Internal Server Error”. In this case, the displayed result set remained unchanged.

    - always got the error if "Show entries" was set to a value other than "All" and I 
      searched for a value that would return 0 results

    - always got the error if "Show entries" was set to a value other than "All" and I 
      searched for a value that exists within the item set but was not showing on the
      current page

    - inconsistently got the error if "Show entries" was set to a value other than "All"
      and I searched for a value that exists within the items showing on the current page

    - the above behavior would occur even if all of the items were showing (e.g. bib has
      25 items and the table is set to show 50)

    - never got the error if "Show entries" was set to "All", no matter what I searched


2. If I typed in a value and pressed the "Enter" key, in a case that would generate the above 500 error, it generated two errors.
   First: “Something went wrong when loading the table: undefined : undefined”
   Then: “Something went wrong when loading the table: 500 Internal Server Error”
(including this mostly in case it helps with troubleshooting #1)


3. If StaffDetailItemSelection and/or LocalCoverImages is turned off, the alignment of the drop-down filters for the other columns is offset by 1 for each of the two sysprefs that is disabled
Example: StaffDetailItemSelection is enabled; LocalCoverImages is disabled.
The first 5 columns are the selection column, Item Type, Current Library, Home Library, Collection.
Item Type has a text filter instead of a drop-down.
Current Library has a drop-down, but it contains Item Type values (and is thus useless).
Home Library has a drop-down containing branch names (but this is presumably still shifted from Current Library)
Collection has a drop-down containing branch names (again, useless)
** If the cover images column is hidden, but not disabled by syspref, the offset does not occur.

4. The general "Search" bar at the top of the table can only search on AV codes, not display names (is this intentionally left for follow-up since the column-specific filters were fixed by adding drop-downs?)

A couple of other minor things that do not need to be blockers:
1. The "Clear filter" button (next to the general "Search" box at the top of the table) is always disabled

2. Missing space after the transit date: “In transit from Springfield to Fairfield since 01/25/2024There is an item level hold on this item (priority = 0).”

**Note: I didn't test course reserves, analytics, or host records, as I'm not familiar with how those work generally in Koha.
Comment 67 Emily Lamancusa 2024-01-25 22:46:36 UTC
Feedback from the MCPL staff who looked at the patch:

Inability to sort/filter on status: Front-line staff don't often do this, so it's not a big deal for them. However, acquisitions staff do need to sort/filter by item status on bib records pretty frequently. It would be great if that could be prioritized as a follow-up, even if it's a filter that's more hard-coded than we'd like with a FIXME for later.

Hiding columns based on whether the first page of results returns data: This doesn't come up often for us (most columns in our system either always have data or very rarely have data), though I see how this could potentially be an issue for other libraries.

Finally, the reload time when changing the sort/filter was also a concern - if you do multiple sorts/filters on smaller bibs, this can actually increase the overall loading time. The potential follow-up that was discussed earlier, to block the Ajax reload when all items are showing, could definitely help with this, especially on smaller bibs.
Comment 68 Katrin Fischer 2024-01-28 14:13:13 UTC
Hi Emily, thanks for the valueable testing and feedback!
I believe, as you already did in your report, we need to differentiate between what needs fixing here and what needs to be fixed later, but at least the internal server errors deserve to be looked at.
Comment 69 Jonathan Druart 2024-02-01 15:06:48 UTC
(In reply to Emily Lamancusa from comment #66)

Thanks a lot!

> Notes from my testing:
> The majority of it seems to be working as it's supposed to. I ran into a few
> problems with filtering, though:
> 
> 1. When using the general "Search" field at the top of the table, the filter
> often failed with the error message: “Something went wrong when loading the
> table: 500 Internal Server Error”. In this case, the displayed result set
> remained unchanged.

This is driving me crazy and I am totally stuck. I have pushed some info to a separate branch bug_33568_wip
https://gitlab.com/joubu/Koha/-/commit/c7969388f1a59ad83a2bf82df1d0abf9a8c065b3

> 3. If StaffDetailItemSelection and/or LocalCoverImages is turned off, the
> alignment of the drop-down filters for the other columns is offset by 1 for
> each of the two sysprefs that is disabled

Good catch. Fixed in "Fix columns shift when pref are off".

> 4. The general "Search" bar at the top of the table can only search on AV
> codes, not display names (is this intentionally left for follow-up since the
> column-specific filters were fixed by adding drop-downs?)

Yes, that's a limitation of our DT/REST API for now. We need to deal with that but that's not for here.

> A couple of other minor things that do not need to be blockers:
> 1. The "Clear filter" button (next to the general "Search" box at the top of
> the table) is always disabled

Weird, I am sure I fixed that already. This is not related directly to this table (see the patron search for instance). It should be reported separately.

> 2. Missing space after the transit date: “In transit from Springfield to
> Fairfield since 01/25/2024There is an item level hold on this item (priority
> = 0).”

Done (fixed inline)

(In reply to Emily Lamancusa from comment #67)
> Feedback from the MCPL staff who looked at the patch:
> 
> Inability to sort/filter on status: Front-line staff don't often do this, so
> it's not a big deal for them. However, acquisitions staff do need to
> sort/filter by item status on bib records pretty frequently. It would be
> great if that could be prioritized as a follow-up, even if it's a filter
> that's more hard-coded than we'd like with a FIXME for later.

It can be done, but it is not trivial. We need to move the logic to perl.

> Hiding columns based on whether the first page of results returns data: This
> doesn't come up often for us (most columns in our system either always have
> data or very rarely have data), though I see how this could potentially be
> an issue for other libraries.

As state in a previous comment this is impossible to resolve.

> Finally, the reload time when changing the sort/filter was also a concern -
> if you do multiple sorts/filters on smaller bibs, this can actually increase
> the overall loading time. The potential follow-up that was discussed
> earlier, to block the Ajax reload when all items are showing, could
> definitely help with this, especially on smaller bibs.

To be honest I have no idea how easy/hard it is to implement. But I will be happy to look into it. Don't hesitate to open new bug reports depending on this one, if you have enhancement ideas/requests.
Comment 70 Jonathan Druart 2024-02-01 15:31:30 UTC
(In reply to Jonathan Druart from comment #69)
> (In reply to Emily Lamancusa from comment #66)
> 
> Thanks a lot!
> 
> > Notes from my testing:
> > The majority of it seems to be working as it's supposed to. I ran into a few
> > problems with filtering, though:
> > 
> > 1. When using the general "Search" field at the top of the table, the filter
> > often failed with the error message: “Something went wrong when loading the
> > table: 500 Internal Server Error”. In this case, the displayed result set
> > remained unchanged.
> 
> This is driving me crazy and I am totally stuck. I have pushed some info to
> a separate branch bug_33568_wip
> https://gitlab.com/joubu/Koha/-/commit/
> c7969388f1a59ad83a2bf82df1d0abf9a8c065b3

"Fixed" on "Bug 33568: Remove LPAD ordering"
Comment 71 Jonathan Druart 2024-03-15 15:35:10 UTC
Remote branch rebased.
Comment 72 Tomás Cohen Arazi 2024-04-01 12:07:15 UTC
(In reply to Jonathan Druart from comment #49)
> > 4) Table configuration doesn't appear to be used as expected. I checked:
> > Catalog > holdings_table has only holdings_issues and holdings_renewals
> > hidden by default. But the table keeps appearing with only a minimum number
> > of columns and a lot of them hidden. Unfortunately the hidden column also
> > includes much neded ones like callnumber. Any changes I make don't seem to
> > stick.
> 
> The table is hiding empty columns. So if the x items displayed on the page
> do not have callnumber, then the column will be hidden, even if other pages
> have items with callnumber. Is that what you are seeing?
> See commit message "Bug 33568: Hide empty columns".
> 
> It is what we are doing already on master, but here we don't want to fetch
> all the items to know if data exist for each each column...

I think we should remove the feature of hiding empty columns entirely. It is a really small 'feature' that causes less harm than the benefits it brings.
Comment 73 Katrin Fischer 2024-04-01 12:18:09 UTC
(In reply to Tomás Cohen Arazi from comment #72)
> (In reply to Jonathan Druart from comment #49)
> > > 4) Table configuration doesn't appear to be used as expected. I checked:
> > > Catalog > holdings_table has only holdings_issues and holdings_renewals
> > > hidden by default. But the table keeps appearing with only a minimum number
> > > of columns and a lot of them hidden. Unfortunately the hidden column also
> > > includes much neded ones like callnumber. Any changes I make don't seem to
> > > stick.
> > 
> > The table is hiding empty columns. So if the x items displayed on the page
> > do not have callnumber, then the column will be hidden, even if other pages
> > have items with callnumber. Is that what you are seeing?
> > See commit message "Bug 33568: Hide empty columns".
> > 
> > It is what we are doing already on master, but here we don't want to fetch
> > all the items to know if data exist for each each column...
> 
> I think we should remove the feature of hiding empty columns entirely. It is
> a really small 'feature' that causes less harm than the benefits it brings.

I see what you mean, but we need to test this maybe - it can "blow up" the table quite a bit. thinking of the notes fields (materials, internal note, OPAC note, enumchron...) that you would want to see when set and not hide via column settings by default.

For libraries with only a few items per record, this could be seen as quite a regression. Would it be possible to only "activate" this behavior if we have a paged items table? Like check the count of items and then behave differently? (just brainstorming, we need to decide on something, I am aware)
Comment 74 Tomás Cohen Arazi 2024-04-01 13:54:32 UTC
(In reply to Katrin Fischer from comment #73)
> I see what you mean, but we need to test this maybe - it can "blow up" the
> table quite a bit. thinking of the notes fields (materials, internal note,
> OPAC note, enumchron...) that you would want to see when set and not hide
> via column settings by default.
>
> For libraries with only a few items per record, this could be seen as quite
> a regression. Would it be possible to only "activate" this behavior if we
> have a paged items table? Like check the count of items and then behave
> differently? (just brainstorming, we need to decide on something, I am aware)

I've been thinking about it, and maybe what we need is some sort of hint/tooltip that displays some of that info? So they can safely hide the columns entirely, but still know there's some extra info. Trying to see from another angle to unlock this.

I get it. I understand your concern, but it is too tiny compared to the benefits having this in master will bring.
Comment 75 Katrin Fischer 2024-04-01 14:08:30 UTC
(In reply to Tomás Cohen Arazi from comment #74)
> (In reply to Katrin Fischer from comment #73)
> > I see what you mean, but we need to test this maybe - it can "blow up" the
> > table quite a bit. thinking of the notes fields (materials, internal note,
> > OPAC note, enumchron...) that you would want to see when set and not hide
> > via column settings by default.
> >
> > For libraries with only a few items per record, this could be seen as quite
> > a regression. Would it be possible to only "activate" this behavior if we
> > have a paged items table? Like check the count of items and then behave
> > differently? (just brainstorming, we need to decide on something, I am aware)

 
> I've been thinking about it, and maybe what we need is some sort of
> hint/tooltip that displays some of that info? So they can safely hide the
> columns entirely, but still know there's some extra info. Trying to see from
> another angle to unlock this.
> 
> I get it. I understand your concern, but it is too tiny compared to the
> benefits having this in master will bring.

I think it's important we keep both use cases in mind. Big libraries with many items that could immensely profit from this, but also the huge number of small libraries where this might create problems.

What about the suggestion I made about making the behavior dependent on number of items? I believe that could give us best of both worlds. 

* Show all columns if the display is paginated, like you suggested.
* Keep the behavior Jonathan implemented for records with unpaginated display (number of items <= number of items per page).
Comment 76 Tomás Cohen Arazi 2024-04-01 14:12:27 UTC
(In reply to Katrin Fischer from comment #75)
> 
> I think it's important we keep both use cases in mind. Big libraries with
> many items that could immensely profit from this, but also the huge number
> of small libraries where this might create problems.

My proposal is the way I keep both cases in mind :-D I just think some sacrifices will need to be made at the beginning and maybe we can later re-add things as people express their opinions. That said,

> What about the suggestion I made about making the behavior dependent on
> number of items? I believe that could give us best of both worlds. 
> 
> * Show all columns if the display is paginated, like you suggested.
> * Keep the behavior Jonathan implemented for records with unpaginated
> display (number of items <= number of items per page).

this could work. It would be a matter of hearing from Jonathan as he's invested a lot of time and needs to tell us if he can adjust that needed bit.
Comment 77 Katrin Fischer 2024-04-01 14:20:45 UTC
(In reply to Tomás Cohen Arazi from comment #72)

> I think we should remove the feature of hiding empty columns entirely. It is
> a really small 'feature' that causes less harm than the benefits it brings.

Hm, maybe we should take a step back: 

Maybe Jonathan's solution is already good and we need a tooltip on the column configuration page instead?

My feeling is of the 3 proposed options, "always showing all columns" is the least ideal for now.
Comment 78 Tomás Cohen Arazi 2024-04-01 14:27:30 UTC
Besides the conversation about columns visibility, I found this odd behavior:

I picked `CGI programming with Perl` for my tests, and added some extra items:


```
perl -mt::lib::TestBuilder -e 'my $t = t::lib::TestBuilder->new; foreach my $i (1..100) { $t->build_sample_item({ biblionumber => 11 }); }'
```

And even though TestBuilder generated 100 random libraries for those items (checked on the DB) all the 'Home library' and 'Holding library' columns are displaying empty!
Comment 79 Tomás Cohen Arazi 2024-04-01 14:37:16 UTC
I'm reverting the FQA as I cannot reproduce anymore. Marked my comment as important as Jonathan might have some insight on what might have happened (I feel some caching got in the middle).
Comment 80 Tomás Cohen Arazi 2024-04-01 14:41:24 UTC
(In reply to Tomás Cohen Arazi from comment #79)
> I'm reverting the FQA as I cannot reproduce anymore. Marked my comment as
> important as Jonathan might have some insight on what might have happened (I
> feel some caching got in the middle).

Right, t::lib::TestBuilder totally skips Koha::<Class>->store, and thus cache invalidation doesn't take place. Sorry for the noise!

This is PQA in my opinion. Will wait until we hear from Jonathan about the columns display solution.
Comment 81 Jonathan Druart 2024-04-02 09:42:57 UTC
There are really too many columns to display them all. I think the solution proposed by this patch is a good compromise and we need to hear from end users to adjust the behaviour if needed.
Comment 82 Katrin Fischer 2024-04-02 10:31:45 UTC
(In reply to Jonathan Druart from comment #81)
> There are really too many columns to display them all. I think the solution
> proposed by this patch is a good compromise and we need to hear from end
> users to adjust the behaviour if needed.

I am OK with that. IIRC it would mean that not much changes for libraries with few items, which is good for now. What do you think about adding a little tool tip to the relevant section on the columns configuration page? Maybe something like:

"Please note that if a field is not used on the first page of items int he holdings table, it will be automatically hidden from display and can be made visible on demand. You can still use the column configuration to permanently hide columns from the holdings table."

Trying to minimize the surprise and manage expectations :)
Comment 83 Jonathan Druart 2024-04-02 11:23:33 UTC
(In reply to Katrin Fischer from comment #82)
> (In reply to Jonathan Druart from comment #81)
> > There are really too many columns to display them all. I think the solution
> > proposed by this patch is a good compromise and we need to hear from end
> > users to adjust the behaviour if needed.
> 
> I am OK with that. IIRC it would mean that not much changes for libraries
> with few items, which is good for now. What do you think about adding a
> little tool tip to the relevant section on the columns configuration page?
> Maybe something like:
> 
> "Please note that if a field is not used on the first page of items int he
> holdings table, it will be automatically hidden from display and can be made
> visible on demand. You can still use the column configuration to permanently
> hide columns from the holdings table."
> 
> Trying to minimize the surprise and manage expectations :)

Added, see commit "Add hint on the table settings admin page".
Comment 84 Katrin Fischer 2024-04-02 13:19:18 UTC
Cool. Tomas, ready to pass QA or do we miss something else?
Comment 85 Jonathan Druart 2024-04-03 07:37:20 UTC
Created attachment 164308 [details] [review]
Bug 33568: Remove debug_display

This is useless, we could move it to its own bug report if needed.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 86 Jonathan Druart 2024-04-03 07:37:25 UTC
Created attachment 164309 [details] [review]
Bug 33568: Starting up

Copying the same thead and making a new table using the REST API
endpoint

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 87 Jonathan Druart 2024-04-03 07:37:29 UTC
Created attachment 164310 [details] [review]
Bug 33568: Display string version of coded values + format dates

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 88 Jonathan Druart 2024-04-03 07:37:34 UTC
Created attachment 164311 [details] [review]
Bug 33568: can_be_edited

Not sure if this is ideal but at least the algorithm complexity is way
better than before!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 89 Jonathan Druart 2024-04-03 07:37:38 UTC
Created attachment 164312 [details] [review]
Bug 33568: Display item type image

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 90 Jonathan Druart 2024-04-03 07:37:43 UTC
Created attachment 164313 [details] [review]
Bug 33568: Restore class names on th's

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 91 Jonathan Druart 2024-04-03 07:37:48 UTC
Created attachment 164314 [details] [review]
Bug 33568: Cover images

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 92 Jonathan Druart 2024-04-03 07:37:52 UTC
Created attachment 164315 [details] [review]
Bug 33568: Hide empty columns

Instead of relying on the existence of data in at least one item we are
going to hide empty columns.
That could be confusing: it does not mean items on another page do not
have data for this column. The user can still display the column using
the "Columns" visibility button, and filter on the column.

I think this is an acceptable behaviour.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 93 Jonathan Druart 2024-04-03 07:37:56 UTC
Created attachment 164316 [details] [review]
Bug 33568: Temporary set searchable to false where no data

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 94 Jonathan Druart 2024-04-03 07:38:00 UTC
Created attachment 164317 [details] [review]
Bug 33568: fix DataTables bKohaColumnsUseNames

Looks like this bug does not exist in Koha, but we hit it here.
Could be moved to its own bug.
Compare with ../includes/columns_settings.inc (code that is used prior
to this change)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 95 Jonathan Druart 2024-04-03 07:38:05 UTC
Created attachment 164318 [details] [review]
Bug 33568: Add columns visibility

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 96 Jonathan Druart 2024-04-03 07:38:09 UTC
Created attachment 164319 [details] [review]
Bug 33568: Remove visibility conditions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 97 Jonathan Druart 2024-04-03 07:38:14 UTC
Created attachment 164320 [details] [review]
Bug 33568: Fix search on item type

Is this good? Cannot we return incorrect results here?

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 98 Jonathan Druart 2024-04-03 07:38:18 UTC
Created attachment 164321 [details] [review]
Bug 33568: Shelving location

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 99 Jonathan Druart 2024-04-03 07:38:23 UTC
Created attachment 164322 [details] [review]
Bug 33568: Item groups

FIXME - we need tests, but I'd like to make sure there is no an easier
way to do this many to many things. Tomas?
Filter on the "Item group" column needs to be tested!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 100 Jonathan Druart 2024-04-03 07:38:27 UTC
Created attachment 164323 [details] [review]
Bug 33568: enumchron/volinfo

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 101 Jonathan Druart 2024-04-03 07:38:32 UTC
Created attachment 164324 [details] [review]
Bug 33568: Status

FIXME We need tests here!

See the FIXME in the code, we are losing the ability to search on the status,
but this is really impossible to implement if we are not storing the
status (DB or search engine).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 102 Jonathan Druart 2024-04-03 07:38:36 UTC
Created attachment 164325 [details] [review]
Bug 33568: Add link to moredetail

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 103 Jonathan Druart 2024-04-03 07:38:40 UTC
Created attachment 164326 [details] [review]
Bug 33568: URI

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 104 Jonathan Druart 2024-04-03 07:38:45 UTC
Created attachment 164327 [details] [review]
Bug 33568: Display stringified version of copy_number

FIXME - should we apply the same test for other row._strings
occurrences, in case they are not linked with an AV cat?

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 105 Jonathan Druart 2024-04-03 07:38:49 UTC
Created attachment 164328 [details] [review]
Bug 33568: public notes

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 106 Jonathan Druart 2024-04-03 07:38:53 UTC
Created attachment 164329 [details] [review]
Bug 33568: Host title

See FIXMEs!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 107 Jonathan Druart 2024-04-03 07:38:58 UTC
Created attachment 164330 [details] [review]
Bug 33568: Print label button

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 108 Jonathan Druart 2024-04-03 07:39:02 UTC
Created attachment 164331 [details] [review]
Bug 33568: Restore action buttons

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 109 Jonathan Druart 2024-04-03 07:39:08 UTC
Created attachment 164332 [details] [review]
Bug 33568: Separate holdings

This needs to be well tested!
Sysprefs SeparateHoldings and SeparateHoldingsBranch
As well as the "Lost items in staff interface" that can be defined at
patron's category level.

This patch is simply restoring the previous behaviour but I think we
could do better.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 110 Jonathan Druart 2024-04-03 07:39:12 UTC
Created attachment 164333 [details] [review]
Bug 33568: Course reserves

FIXME We need tests here

FIXME/TODO - Isn't it possible to replace all this mess with the
following Koha::Item method and a new DBIC rs?
sub courses {
    my ($self) = @_;
    return Koha::Courses->search(
        { 'ci.itemnumber' => $self->itemnumber },
        { join            => { course_reserves => 'ci' } }
    );
}

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 111 Jonathan Druart 2024-04-03 07:39:17 UTC
Created attachment 164334 [details] [review]
Bug 33568: Escape all the things

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 112 Jonathan Druart 2024-04-03 07:39:21 UTC
Created attachment 164335 [details] [review]
Bug 33568: Restore filters

FIXME - If we click 'show filters' the 'columns' list does not contain
all the columns.
We are destroying the DT and the hidden columns are removed from the
dom.

I think there are 2 possible solutions here:
1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage
to make this work correctly, because of the th filters)
2. Move the th definition in the JS init of DT

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 113 Jonathan Druart 2024-04-03 07:39:26 UTC
Created attachment 164336 [details] [review]
Bug 33568: Fix action links display

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 114 Jonathan Druart 2024-04-03 07:39:30 UTC
Created attachment 164337 [details] [review]
Bug 33568: Preserve item selection across pagination and filtering

BTW Do we really need StaffDetailItemSelection?

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 115 Jonathan Druart 2024-04-03 07:39:35 UTC
Created attachment 164338 [details] [review]
Bug 33568: Use as_due_date to display due dates

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 116 Jonathan Druart 2024-04-03 07:39:40 UTC
Created attachment 164339 [details] [review]
Bug 33568: Display library names instead of codes for transfers

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 117 Jonathan Druart 2024-04-03 07:39:44 UTC
Created attachment 164340 [details] [review]
Bug 33568: Bug 33568: Display library names instead of codes for first hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 118 Jonathan Druart 2024-04-03 07:39:49 UTC
Created attachment 164341 [details] [review]
Bug 33568: Bug 33568: Display library names instead of codes for recall

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 119 Jonathan Druart 2024-04-03 07:39:53 UTC
Created attachment 164342 [details] [review]
Bug 33568: Display the desk name for the first hold

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 120 Jonathan Druart 2024-04-03 07:39:58 UTC
Created attachment 164343 [details] [review]
Bug 33568: Restore bundle items

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 121 Jonathan Druart 2024-04-03 07:40:02 UTC
Created attachment 164344 [details] [review]
Bug 33568: Restore host records

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 122 Jonathan Druart 2024-04-03 07:40:07 UTC
Created attachment 164345 [details] [review]
Bug 33568: Restore analytics

FIXME We cannot have the usual _count/+count pattern for the REST API, I
think it's acceptable

FIXME We lost the sort on this column, not a big deal IMO

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 123 Jonathan Druart 2024-04-03 07:40:12 UTC
Created attachment 164346 [details] [review]
Bug 33568: Move code to a dedicated include file

Not very nice but at least the code is isolated in a separate file and
reduce the size of detail.tt

We could also move the code related to the item bundle that is included
inside this same table.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 124 Jonathan Druart 2024-04-03 07:40:17 UTC
Created attachment 164347 [details] [review]
Bug 33568: Fix DT dom and remove DT info if not needed

Was there a real need to deal with a specific dom? The "no filter" view
was not really light, only the column filters were shown/hidden.

This patch is suggesting to remove all the DT info in case the table is
displaying all the items of the bibliographic record: Pagination, filtering, buttons, etc.

This patch is for discussion.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 125 Jonathan Druart 2024-04-03 07:40:22 UTC
Created attachment 164348 [details] [review]
Bug 33568: Fix select/clear all links

Follow-up for "Bug 33568: Preserve item selection across pagination and filtering"

FIXME We should deal with the items selection in a better way, but I
would not consider it blocker, that can be done later.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 126 Jonathan Druart 2024-04-03 07:40:26 UTC
Created attachment 164349 [details] [review]
Bug 33568: Add search on home and holding library's names

QUESTION - Do we prefer this or a dropdown list in the column filter
header?

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 127 Jonathan Druart 2024-04-03 07:40:31 UTC
Created attachment 164350 [details] [review]
Bug 33568: Prevent 500 when filtering by item type (FIXMEs)

See FIXMEs added by this patch

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 128 Jonathan Druart 2024-04-03 07:40:35 UTC
Created attachment 164351 [details] [review]
Bug 33568: (follow-up) Restore filters

This is a bit ugly but fixes the problem raised in the previous commit,
as well as others

for instance comment 18
"* Click 'show filters', 'hide filters' and repeat X 2 - status column disappears?"

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 129 Jonathan Druart 2024-04-03 07:40:40 UTC
Created attachment 164352 [details] [review]
Bug 33568: Fix local cover images

We need to reprocess the images when the table is refreshed, to do that
we teach verify_cover_images to go through a given container instead of
the whole document, and pass a call back function to build_items_table
that will be called when DataTables will have redrawn the table
(drawCallback).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 130 Jonathan Druart 2024-04-03 07:40:45 UTC
Created attachment 164353 [details] [review]
Bug 33568: Fix 'actions' links when filters are shown/hidden

It also fixes a problem when "Select all" was clicked, the checkbox
selection was not kept when the table was refreshed (was comparing
string and integer, parseInt needed)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 131 Jonathan Druart 2024-04-03 07:40:49 UTC
Created attachment 164354 [details] [review]
Bug 33568: (bug 34226 follow-up) Add type=date to date columns

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 132 Jonathan Druart 2024-04-03 07:40:54 UTC
Created attachment 164355 [details] [review]
Bug 33568: Do not embed library names

Performance for 1000 items from 30s to 22s

It replaces the input in the column header filter with a dropdown list.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 133 Jonathan Druart 2024-04-03 07:40:58 UTC
Created attachment 164356 [details] [review]
Bug 33568: Do not embed item types

Performance for 1000 items from 22 to 19s

It replaces the input in the column header filter with a dropdown list.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 134 Jonathan Druart 2024-04-03 07:41:03 UTC
Created attachment 164358 [details] [review]
Bug 33568: follow-up for 29523

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 135 Jonathan Druart 2024-04-03 07:41:08 UTC
Created attachment 164359 [details] [review]
Bug 33568: Add some missing StaffDetailItemSelection conditionals

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 136 Jonathan Druart 2024-04-03 07:41:12 UTC
Created attachment 164360 [details] [review]
Bug 33568: Hide holdings tabs if empty (SeparateHoldings)

See comment 34.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 137 Jonathan Druart 2024-04-03 07:41:17 UTC
Created attachment 164361 [details] [review]
Bug 33568: JSON-encode html output of build_table

This prevents error with quotes coming from translations

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 138 Jonathan Druart 2024-04-03 07:41:21 UTC
Created attachment 164362 [details] [review]
Bug 33568: Fix ordering

Use search_ordered for default ordering

It also fixes a crash when StaffDetailItemSelection is turned off.

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 139 Jonathan Druart 2024-04-03 07:41:26 UTC
Created attachment 164363 [details] [review]
Bug 33568: Do not display 'null' when no barcode

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 140 Jonathan Druart 2024-04-03 07:41:31 UTC
Created attachment 164364 [details] [review]
Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 141 Jonathan Druart 2024-04-03 07:41:36 UTC
Created attachment 164365 [details] [review]
Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 142 Jonathan Druart 2024-04-03 07:41:40 UTC
Created attachment 164366 [details] [review]
Bug 33568: Restore link to patron

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 143 Jonathan Druart 2024-04-03 07:41:45 UTC
Created attachment 164367 [details] [review]
Bug 33568: Restore column filters when StaffDetailItemSelection is off

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 144 Jonathan Druart 2024-04-03 07:41:49 UTC
Created attachment 164368 [details] [review]
Bug 33568: Fix patron_to_html display

Take HidePatronName into account
Display cardnumber
Remove parenthesis around cardnumber if the name is hidden

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 145 Jonathan Druart 2024-04-03 07:41:54 UTC
Created attachment 164369 [details] [review]
Bug 33568: (follow-up) Fix ordering - WIP

This is not working. We are still getting the error
Unable to programatically derive a required group_by from the supplied order_by criteria. To proceed either add an explicit group_by, or simplify your order_by to only include plain columns     (supplied order_by: 'lpad_copynumber').

To recreate the problem you can go to detail.pl and search for something
Without search the query is working.

The problem appears when the query has WHERE parameters added and "GROUP
BY" is added.
DBIC is adding to the GROUP BY what is added to the select, but does not
handle this situation (not a plain column).

You can play with:
1.
Remove from Koha::Items->search_ordered
475                     'lpad_copynumber',
=> all good

2. Remove the following block from /usr/share/perl5/DBIx/Class/Storage/DBIHacks.pm
 584     # only consider real columns (for functions the user got to do an explicit group_by)
 585     my $chunk_ci;
 586     if (
 587       @{$order_by[$o_idx]} != 1
 588         or
 589       # only declare an unknown *plain* identifier as "leftover" if we are called with
 590       # aliastypes to examine. If there are none - we are still in _resolve_attrs, and
 591       # can just assume the user knows what they want
 592       ( ! ( $chunk_ci = $colinfos->{$order_by[$o_idx][0]} ) and $attrs->{_aliastypes} )
 593     ) {
 594       push @$leftovers, $order_by[$o_idx][0];
 595     }
=> all good

I don't know if we are doing something wrong here, but I have no idea
how we can build the group_by explicitely.

Is there another way to do this?

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 146 Jonathan Druart 2024-04-03 07:41:59 UTC
Created attachment 164370 [details] [review]
Bug 33568: Fix columns shift when pref are off

From comment 66:
If StaffDetailItemSelection and/or LocalCoverImages is turned off, the alignment of the drop-down filters for the other columns is offset by 1 for each of the two sysprefs that is disabled

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 147 Jonathan Druart 2024-04-03 07:42:04 UTC
Created attachment 164371 [details] [review]
Bug 33568: Remove LPAD ordering

This is coming from bug 3521 it seems. And we are going to deal with
this later if people complains.

kidclamp> the performance issue for sites with many copies is bigger than sorting issues imho
kidclamp> drop the LPAD I say :-)

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 148 Jonathan Druart 2024-04-03 07:42:09 UTC
Created attachment 164372 [details] [review]
Bug 33568: serialitem => serial_item

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 149 Jonathan Druart 2024-04-03 07:42:13 UTC
Created attachment 164373 [details] [review]
Bug 33568: POD fixes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 150 Jonathan Druart 2024-04-03 07:42:18 UTC
Created attachment 164374 [details] [review]
Bug 33568: Add hint on the table settings admin page

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 151 Jonathan Druart 2024-04-03 07:42:55 UTC
Thanks Tomas!
I have added Laurence and Emily as well as yours.

Laurence and Emily have done extended testing and I think they deserve their signed-off-by lines on this one.

Thanks everyone!
Comment 152 Katrin Fischer 2024-04-03 07:57:19 UTC
(In reply to Jonathan Druart from comment #151)
> Thanks Tomas!
> I have added Laurence and Emily as well as yours.
> Laurence and Emily have done extended testing and I think they deserve their
> signed-off-by lines on this one.

Totally agree, thanks for thinking of it!

My goal is to push this before leaving for Marseille.
Comment 153 Jonathan Druart 2024-04-05 13:03:53 UTC
Created attachment 164472 [details] [review]
Bug 33568: Fix false positive 'missing_filter'

It was ok but the QA script complains and xt/find-missing-filters.t as
well.
Comment 154 Katrin Fischer 2024-04-05 13:24:39 UTC
I have given this priority as it's quite a big change and it will give us the chance to get a lot of eyes on it next week during the hackfest.

Some notes from my own review:

1) I notice quite a lot of remaining FIXMEs. I feel like some of those should filed as follow-up bugs, like the question of sorting serial issues here:

        my $items_rs = $biblio->items;
+        # FIXME We need to order_by serial.publisheddate if we have _order_by=+me.serial_issue_number
+        # FIXME Do we always need host_items => 1 or depending on a flag?
+        # FIXME Should we prefetch => ['issue','branchtransfer']?
+        my $items_rs = $biblio->items( { host_items => 1 } )->search_ordered( {}, { join => 'biblioitem' } );
         $items_rs = $items_rs->filter_by_bookable if $bookable_only;
+        # FIXME We need to order_by serial.publisheddate if we have _order_by=+me.serial_issue_number
         my $items = $c->objects->search($items_rs);

2) Untranslatables

node += '<a href="/cgi-bin/koha/catalogue/image.pl?itemnumber=%s&amp;imagenumber=%s" title="Local cover image">'.format(id, id);
node += '<img src="/cgi-bin/koha/catalogue/image.pl?thumbnail=1&amp;imagenumber=%s" alt="Local cover image" data-link="/cgi-bin/koha/catalogue/imageviewer.pl?itemnumber=[% item.itemnumber | uri %
return '<a href="/cgi-bin/koha/catalogue/search.pl?idx=hi&amp;q=%s">%s analytics</a>'.format(row.item_id, row.analytics_count);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
nodes += '  <li><a href="/cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=%s&amp;filetype=image"><i class="fa fa-upload"></i> Upload image</a></li>'.format(row.item_id);
nodes += '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=%s&itemnumber=%s#edititem"><i class="fa-solid fa-pencil"></i> Edit</a>'.format(row.biblio_id, row.item_id);
nodes += '<button class="btn btn-default btn-xs details-control"><i class="fa fa-folder"></i> Manage bundle (%s|%s)</button>'.format(escape_str(row.bundle_items_not_lost_count), escape_str(row.bundle_items_lost_count));

3) Unit tests

Actually you removed some :D We should add some for balance and the new methods in the various .pm.

4) Processing note

The processing note now always shows for a split second when doing an action like sorting columns. Not sure if that is an option, but could we make it only show if the action itself takeslike a little longer? It's just a fast blink now and you can hardly even read the "processing" (definitely for a separate bug of course)


IMPORTANT: When you provide follow-ups - could you please obsolete the already pushed patches? This will make it much easier to handle. (uncommenting 60+ patches for git bz apply with -i is no fun)
Comment 155 Katrin Fischer 2024-04-05 13:36:23 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 156 Jonathan Druart 2024-04-05 14:19:58 UTC
Created attachment 164480 [details] [review]
Bug 33568: Fix translatability issues
Comment 157 Jonathan Druart 2024-04-05 14:21:31 UTC
(In reply to Katrin Fischer from comment #154)
> I have given this priority as it's quite a big change and it will give us
> the chance to get a lot of eyes on it next week during the hackfest.
> 
> Some notes from my own review:
> 
> 1) I notice quite a lot of remaining FIXMEs. I feel like some of those
> should filed as follow-up bugs, like the question of sorting serial issues
> here:
> 
>         my $items_rs = $biblio->items;
> +        # FIXME We need to order_by serial.publisheddate if we have
> _order_by=+me.serial_issue_number
> +        # FIXME Do we always need host_items => 1 or depending on a flag?
> +        # FIXME Should we prefetch => ['issue','branchtransfer']?
> +        my $items_rs = $biblio->items( { host_items => 1 }
> )->search_ordered( {}, { join => 'biblioitem' } );
>          $items_rs = $items_rs->filter_by_bookable if $bookable_only;
> +        # FIXME We need to order_by serial.publisheddate if we have
> _order_by=+me.serial_issue_number
>          my $items = $c->objects->search($items_rs);

Yes, there were a lot of FIXMEs. We were supposed to discuss them.

> 2) Untranslatables
> 
> node += '<a
> href="/cgi-bin/koha/catalogue/image.pl?itemnumber=%s&amp;imagenumber=%s"
> title="Local cover image">'.format(id, id);
> node += '<img
> src="/cgi-bin/koha/catalogue/image.pl?thumbnail=1&amp;imagenumber=%s"
> alt="Local cover image"
> data-link="/cgi-bin/koha/catalogue/imageviewer.pl?itemnumber=[%
> item.itemnumber | uri %
> return '<a href="/cgi-bin/koha/catalogue/search.pl?idx=hi&amp;q=%s">%s
> analytics</a>'.format(row.item_id, row.analytics_count);
> return '<a
> href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;
> hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
> return '<a
> href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;
> hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
> return '<a
> href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&amp;
> hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
> nodes += '  <li><a
> href="/cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=%s&amp;
> filetype=image"><i class="fa fa-upload"></i> Upload
> image</a></li>'.format(row.item_id);
> nodes += '<a class="btn btn-default btn-xs"
> href="/cgi-bin/koha/cataloguing/additem.
> pl?op=edititem&biblionumber=%s&itemnumber=%s#edititem"><i class="fa-solid
> fa-pencil"></i> Edit</a>'.format(row.biblio_id, row.item_id);
> nodes += '<button class="btn btn-default btn-xs details-control"><i
> class="fa fa-folder"></i> Manage bundle
> (%s|%s)</button>'.format(escape_str(row.bundle_items_not_lost_count),
> escape_str(row.bundle_items_lost_count));

See last patch.

> 3) Unit tests
> 
> Actually you removed some :D We should add some for balance and the new
> methods in the various .pm.

Will do on bug 36535.

> 4) Processing note
> 
> The processing note now always shows for a split second when doing an action
> like sorting columns. Not sure if that is an option, but could we make it
> only show if the action itself takeslike a little longer? It's just a fast
> blink now and you can hardly even read the "processing" (definitely for a
> separate bug of course)

This is not related to this patch, if we want it that needs to be done for all DT.
Comment 158 Jonathan Druart 2024-04-05 14:34:28 UTC
Created attachment 164482 [details] [review]
Bug 33568: Prevent api/v1/biblios.t to fail randomly

koha_1       |     #   Failed test 'The items are returned'
koha_1       |     #   at t/db_dependent/api/v1/biblios.t line 196.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->[0]{external_id} = 'th151O2JtgfGdVW6aiF'
koha_1       |     #     $expected->[0]{external_id} = 'uxrMzUXrkHQy7MHOSfjK'
koha_1       |     # Looks like you failed 1 test of 11.
koha_1       |
koha_1       | #   Failed test 'get_items() tests'
koha_1       | #   at t/db_dependent/api/v1/biblios.t line 206.
Comment 159 Jonathan Druart 2024-04-05 14:41:12 UTC
Created attachment 164483 [details] [review]
Bug 33568: (follow-up) Add search on home and holding library's names

Silly typo!
Caught by a test

koha_1       |     #   Failed test 'Home branch code matches holdingbranch'
koha_1       |     #   at t/db_dependent/Items.t line 581.
koha_1       |     #          got: 'QHImNSEoNi'
koha_1       |     #     expected: 'VY5y6p'
koha_1       |     # Looks like you failed 1 test of 7.
koha_1       |
koha_1       | #   Failed test 'Koha::Item(s) tests'
koha_1       | #   at t/db_dependent/Items.t line 588.
Comment 160 Jonathan Druart 2024-04-05 14:48:08 UTC
Created attachment 164485 [details] [review]
Bug 33568: Prevent api/v1/biblios.t to fail randomly

koha_1       |     #   Failed test 'The items are returned'
koha_1       |     #   at t/db_dependent/api/v1/biblios.t line 196.
koha_1       |     #     Structures begin differing at:
koha_1       |     #          $got->[0]{external_id} = 'th151O2JtgfGdVW6aiF'
koha_1       |     #     $expected->[0]{external_id} = 'uxrMzUXrkHQy7MHOSfjK'
koha_1       |     # Looks like you failed 1 test of 11.
koha_1       |
koha_1       | #   Failed test 'get_items() tests'
koha_1       | #   at t/db_dependent/api/v1/biblios.t line 206.
Comment 161 Tomás Cohen Arazi 2024-04-09 07:56:05 UTC
Created attachment 164528 [details] [review]
Bug 33568: (follow-up) Fix duplicated key in item spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 162 Katrin Fischer 2024-04-26 21:27:45 UTC
Follow-up was pushed to main.
https://git.koha-community.org/Koha-community/Koha/commits/branch/main/search?q=33568