Bug 33167 - Cleanup staff interface catalog details page
Summary: Cleanup staff interface catalog details page
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 33070
Blocks: 26802 35696 35800 33493 33497
  Show dependency treegraph
 
Reported: 2023-03-08 12:23 UTC by Laura Escamilla
Modified: 2024-01-12 22:01 UTC (History)
12 users (show)

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


Attachments
Bug 33167: Cleanup staff detail page (36.04 KB, patch)
2023-03-09 20:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Cleanup staff detail page (36.04 KB, patch)
2023-03-10 16:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Add 'host_items' param to Koha::Biblio->items (3.73 KB, patch)
2023-03-10 16:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Use 'host_items' param to fetch all items at once (2.66 KB, patch)
2023-03-10 16:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Add a filter rleationship for branchtransfer (3.22 KB, patch)
2023-03-10 16:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Cleanup staff detail page (17.76 KB, patch)
2023-03-20 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Add 'host_items' param to Koha::Biblio->items (3.80 KB, patch)
2023-03-20 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Use 'host_items' param to fetch all items at once (2.66 KB, patch)
2023-03-20 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Add a filter relationship for branchtransfers (3.22 KB, patch)
2023-03-20 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Cleanup staff detail page (17.94 KB, patch)
2023-04-14 12:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33167: Cleanup staff detail page (17.99 KB, patch)
2023-04-14 18:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 33167: Cleanup staff detail page (18.04 KB, patch)
2023-04-22 13:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33167: Fix TT comment for better translatability (2.23 KB, patch)
2023-04-22 13:18 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laura Escamilla 2023-03-08 12:23:14 UTC
Adding a new item to a bib that has 300+ items causes a slow loading screen.
Comment 1 Nick Clemens 2023-03-09 20:40:27 UTC
Created attachment 148029 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued

Template also includes addition of chomp to the template directives,
this reduces the size of the items table by several megabytes
on records with many items

WIP
Comment 2 Nick Clemens 2023-03-10 16:16:36 UTC
Created attachment 148082 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued

Template also includes addition of chomp to the template directives,
this reduces the size of the items table by several megabytes
on records with many items

WIP
Comment 3 Nick Clemens 2023-03-10 16:16:38 UTC
Created attachment 148083 [details] [review]
Bug 33167: Add 'host_items' param to Koha::Biblio->items

This patch adds an option to the $biblio->items method to allow
retrieving the items and analytic items for a record. This is intended
to allow fetching a single Items object, and related object, rather than
having to fetch the items, and the host items, and push them together

To test:
1 - prove -v t/db_dependent/Koha/Biblio.t
Comment 4 Nick Clemens 2023-03-10 16:16:40 UTC
Created attachment 148084 [details] [review]
Bug 33167: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enabl easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothign has changed
Comment 5 Nick Clemens 2023-03-10 16:16:43 UTC
Created attachment 148085 [details] [review]
Bug 33167: Add a filter rleationship for branchtransfer

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer to use this
Comment 6 David Cook 2023-03-16 04:00:27 UTC
Is this work in progress or ready for testing?
Comment 7 Nick Clemens 2023-03-20 18:44:04 UTC
Created attachment 148431 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued
Comment 8 Nick Clemens 2023-03-20 18:44:06 UTC
Created attachment 148432 [details] [review]
Bug 33167: Add 'host_items' param to Koha::Biblio->items

This patch adds an option to the $biblio->items method to allow
retrieving the items and analytic items for a record. This is intended
to allow fetching a single Items object, and related object, rather than
having to fetch the items, and the host items, and push them together

This is step towards being able to fetch items using API/DataTables directly

To test:
1 - prove -v t/db_dependent/Koha/Biblio.t
Comment 9 Nick Clemens 2023-03-20 18:44:08 UTC
Created attachment 148433 [details] [review]
Bug 33167: Use 'host_items' param to fetch all items at once

This patch adjusts the detail page to fetch items and host items
together, and prefetches transfers and checkouts

To test:
1 - Enable easy analytics
2 - Attach some items to a bib
3 - Checkout an item on the bib
4 - View the details page
5 - Apply patch
6 - View details page, confirm nothign has changed
Comment 10 Nick Clemens 2023-03-20 18:44:10 UTC
Created attachment 148434 [details] [review]
Bug 33167: Add a filter relationship for branchtransfers

Prefetching is useful, but we don't want to fetch all the inactive
transfers for an item. This patch adds a filtered relationship and
updates get_transfer to use this
Comment 11 Nick Clemens 2023-03-20 18:44:48 UTC
(In reply to David Cook from comment #6)
> Is this work in progress or ready for testing?

This can now be tested, template chomping removed in favor of bug 20657
Comment 12 Nick Clemens 2023-03-20 18:45:58 UTC
(In reply to Nick Clemens from comment #11)
> (In reply to David Cook from comment #6)
> > Is this work in progress or ready for testing?
> 
> This can now be tested, template chomping removed in favor of bug 20657

It won't completely resolve slowness, but it should offer an improvement where there are many items on a bib
Comment 13 David Nind 2023-04-12 18:15:05 UTC
I'm not sure what is required to test this one - apply the patch, and repeat the tests for bug 33493 and 33497?
Comment 14 Nick Clemens 2023-04-13 13:04:19 UTC
(In reply to David Nind from comment #13)
> I'm not sure what is required to test this one - apply the patch, and repeat
> the tests for bug 33493 and 33497?

This one shouldn't cause any visible changes to the detail page - best test plan would be to add items with varying statuses:
lost
damage
withdrawn
in transit
checked out

to a record, and confirm the page looks the same before and after
Comment 15 David Nind 2023-04-13 21:05:55 UTC
I noticed these differences from my testing (see 'How I tested' at the end), and have changed the status to Failed QA:

1. Checked out items:
   - Before the patch, a checked out item shows in the status column as "Checked out to Mary Burton (23529000651225) : due 04/18/2023"
   - After the patch, the checked out item shows as "Checked out to A patron from library : due 04/19/2023"

2. For a record that is linked to a host item (the child item - in this case http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=262) there is this text after the DataTable (after the 'Showing 1 to 2 of 2 entries'): "Show all items (-1 hidden)". The text is an active link (in this case to http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=262&showallitems=1).

Let me know if you would like me to add some screenshots.

How I tested (using KTD):

1. Setup some analytics using easy analytics:
   - Enable Easy Analytics (EasyAnalyticalRecords system preference)
   - Attach some items to a host record
   - Checkout an item on the host record
   - View the record details page

2. For an existing record:
   - Duplicate items so that there are at least 6 items
   - Mark separate items as lost, damaged, and withdrawn
   - Check out an item
   - Place a hold on an item
   - Transfer an item to another library
   - View the details page for the record

3. Apply the patch.

4. Review the details pages for the records in another browser - they should look the same before and after.

5. The tests should pass: prove -v t/db_dependent/Koha/Biblio.t
Comment 16 Jonathan Druart 2023-04-14 07:01:43 UTC
+                                [% SET item_locatoin = AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => item.location ) %]

There is a typo here.
Comment 17 Nick Clemens 2023-04-14 12:33:42 UTC
Created attachment 149677 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued
Comment 18 David Nind 2023-04-14 18:49:05 UTC
Created attachment 149704 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Jonathan Druart 2023-04-18 13:17:59 UTC
item.object.itemnotes.replace('\n','<br />') 

Is this replace relevant? How do we test it?
Comment 20 Nick Clemens 2023-04-18 13:33:17 UTC
(In reply to Jonathan Druart from comment #19)
> item.object.itemnotes.replace('\n','<br />') 
> 
> Is this replace relevant? How do we test it?

1. Enter an itemnote note longer than 100 characters
2. Save the item
3. Edit the item, note field should now be a text area
4. Enter:
a
b
c
d
5. Save the item
6. View details page
7. Confirm line breaks are preserved
Comment 21 Katrin Fischer 2023-04-22 13:18:37 UTC
Created attachment 150075 [details] [review]
Bug 33167: Cleanup staff detail page

This patch begins reduing some of the extra things we are doing in
detail.pl that could easily be handled in the templates - fetching
authorised values and branches etc

It also removes a loop to find items that should be hidden, and instead
uses a searh parameter

The template changes either use item object rather than passed
variables, or utilize plugins to fetch authorised valued

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2023-04-22 13:18:40 UTC
Created attachment 150076 [details] [review]
Bug 33167: Fix TT comment for better translatability

We just figured out that multi-line TT comments show up
weirdly in translations and fixed it on bug 33332. So
this just follows the newly established pattern.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2023-04-22 13:18:56 UTC
I am also stumbling on this change:

                    [% IF ( itemdata_itemnotes ) %]
                        <td><div class="itemnotes">[% item.object.itemnotes.replace('\n','<br />') | $raw %]</div></td>
                    [% END %]
                    [% IF itemdata_nonpublicnotes %]
                        <td class="nonpublicnote">[% item.itemnotes_nonpublic | html %]</td>
                    [% END %]

Why do this for the one note and not the other? It was also not there before and we allow HTML in this field. It seems counterintiuitive to allow both <br> and manual line breaks maybe. But: Not a blocker for me.
Comment 24 Tomás Cohen Arazi 2023-05-12 14:21:08 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 25 Matt Blenkinsop 2023-05-15 12:57:41 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 26 Mason James 2023-05-27 11:34:49 UTC
(In reply to Matt Blenkinsop from comment #25)
> Nice work everyone!
> 
> Pushed to stable for 22.11.x

hi Matt, this patchset depends on 33070, which is missing from 22.11.x

we probably need to revert this from 22.11.06, as it breaks catalogue/detail.pl
Comment 27 Chad Billman 2023-05-30 12:58:06 UTC
I can confirm this is causing issues for us on 22.11.x. Over the weekend 22.11.6 was installed and I began to get reports about 500 errors.

The have all been on catalogue/detail.pl line 411

Trace begun at /usr/share/koha/lib/Koha/Object.pm line 925
Koha::Object::AUTOLOAD('Koha::Patron=HASH(0x562a086602e8)', 'LET') called at /usr/share/koha/intranet/cgi-bin/catalogue/detail.pl line 411
Comment 28 Chad Billman 2023-05-30 13:34:58 UTC
My error went away after I upgraded the debian package to 22.11.06-3.
Comment 29 Barry Cannon 2023-05-31 08:29:26 UTC
I am still seeing this error, even after upgrading to 22.11.06-3.
Comment 30 Mason James 2023-06-02 01:53:17 UTC
(In reply to Barry Cannon from comment #29)
> I am still seeing this error, even after upgrading to 22.11.06-3.

hi Barry
can you paste your error message? it should be different from above

also try rebuilding your languages, with koha-translate tool
Comment 31 Lucas Gass 2023-06-15 19:28:32 UTC
Missing dependencies for 22.05.x, no backport
Comment 32 Matt Blenkinsop 2023-07-19 09:57:10 UTC
Backport to 22.11.x was reverted post release - updating bug to reflect this