Bug 28375 - Inefficiencies in fetching COinS
Summary: Inefficiencies in fetching COinS
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-18 16:02 UTC by Nick Clemens
Modified: 2023-06-08 22:28 UTC (History)
5 users (show)

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


Attachments
Bug 28375: Inefficiencies in fetching COinS (6.08 KB, patch)
2021-05-18 16:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.63 KB, patch)
2021-05-18 18:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.34 KB, patch)
2021-11-12 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.33 KB, patch)
2022-09-23 16:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.38 KB, patch)
2022-09-28 11:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 28375: (follow-up) Remove guessed interface parameter - always pass (1.48 KB, patch)
2022-09-30 12:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.44 KB, patch)
2022-10-07 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28375: (follow-up) Remove guessed interface parameter - always pass (1.54 KB, patch)
2022-10-07 10:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28375: (follow-up) Use C4::Context->interface (3.61 KB, patch)
2022-10-07 12:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28375: Inefficiencies in fetching COinS (6.54 KB, patch)
2022-10-17 11:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28375: (follow-up) Remove guessed interface parameter - always pass (1.64 KB, patch)
2022-10-17 11:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28375: (follow-up) Use C4::Context->interface (3.70 KB, patch)
2022-10-17 11:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28375: (QA follow-up) Remove two remaining interface params (1.17 KB, patch)
2022-10-17 11:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-05-18 16:02:16 UTC
In XSLTParseForDisplay we fetch the coins for the OPAC even on the staff interface

On opac results we fetch the biblio object, this is already fetched in the searchResults subroutine

Additionally - we can fetch the coins in the searchResults to move logic out of the scripts
Comment 1 Nick Clemens 2021-05-18 16:06:37 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2021-05-18 18:59:04 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2021-11-11 20:30:59 UTC
The code looks great and simple. But it doesn't apply. Please resubmit.
Comment 4 Nick Clemens 2021-11-12 12:02:15 UTC
Created attachment 127582 [details] [review]
Bug 28375: Inefficiencies in fetching COinS

This patchset adds an 'interface' parameter to XSLTParseForDisplay to avoid fetching
coins when not needed

Additionally we move some logic from the scripts to searchResults to avoid an extra fetch fo the biblio object

To test:
1 - Enable  COinSinOPACResults syspref
2 - Also enable  OPACShowOpenURL  and  OPACOpenURLItemTypes  - adding an itemtype that you can find
3 - Perform a search on the OPAC and confirm coins are included and openurl shown
4 - Perform a search on staff client and confirm openurls do not show
5 - Apply patch
6 - Results should be the same
Comment 5 Owen Leonard 2022-05-06 18:30:18 UTC
I get this error:

Could not compile /kohadevbox/koha/opac/opac-search.pl: Global symbol "$oldbiblio_object" requires explicit package name (did you forget to declare "my $oldbiblio_object"?) at /kohadevbox/koha/C4/Search.pm line 2006.
Comment 6 Nick Clemens 2022-09-23 16:08:09 UTC
Created attachment 140955 [details] [review]
Bug 28375: Inefficiencies in fetching COinS

This patchset adds an 'interface' parameter to XSLTParseForDisplay to avoid fetching
coins when not needed

Additionally we move some logic from the scripts to searchResults to avoid an extra fetch fo the biblio object

To test:
1 - Enable  COinSinOPACResults syspref
2 - Also enable  OPACShowOpenURL  and  OPACOpenURLItemTypes  - adding an itemtype that you can find
3 - Perform a search on the OPAC and confirm coins are included and openurl shown
4 - Perform a search on staff client and confirm openurls do not show
5 - Apply patch
6 - Results should be the same
Comment 7 David Nind 2022-09-28 11:12:50 UTC
Created attachment 141051 [details] [review]
Bug 28375: Inefficiencies in fetching COinS

This patchset adds an 'interface' parameter to XSLTParseForDisplay to avoid fetching
coins when not needed

Additionally we move some logic from the scripts to searchResults to avoid an extra fetch fo the biblio object

To test:
1 - Enable  COinSinOPACResults syspref
2 - Also enable  OPACShowOpenURL  and  OPACOpenURLItemTypes  - adding an itemtype that you can find
3 - Perform a search on the OPAC and confirm coins are included and openurl shown
4 - Perform a search on staff client and confirm openurls do not show
5 - Apply patch
6 - Results should be the same

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-09-28 11:17:40 UTC
Testing notes (using KTD):

1. Had to add an address to the OpenURLResolverURL system preference for the OpenURL link to display under the title in the OPAC search results.

2. The actual format of the link for OpenURL was the same in the OPAC search results both before and after the patch was applied (I compared the link for a specific item in the search results).
Comment 9 Marcel de Rooy 2022-09-30 07:42:22 UTC
Lots of failures on QA tools.
Comment 10 Nick Clemens 2022-09-30 12:11:27 UTC
Created attachment 141126 [details] [review]
Bug 28375: (follow-up) Remove guessed interface parameter - always pass

Previous patch added interface parameter passed on each call. There was an
existing one that 'guessed' - this patch removes that

Fixes missing comma
Comment 11 Marcel de Rooy 2022-10-04 14:51:45 UTC
Great. No failures anymore from qa tools.

Here is another question :)

@@ -179,6 +179,7 @@ sub XSLTParse4Display {
     my $hidden_items = $params->{hidden_items} || [];
     my $variables    = $params->{xslt_variables};
     my $items_rs     = $params->{items_rs};
+    my $interface    = $params->{interface};

Why do you use a parameter? Did you consider using C4::Context->interface ?
Comment 12 Martin Renvoize 2022-10-07 10:37:54 UTC
Created attachment 141490 [details] [review]
Bug 28375: Inefficiencies in fetching COinS

This patchset adds an 'interface' parameter to XSLTParseForDisplay to avoid fetching
coins when not needed

Additionally we move some logic from the scripts to searchResults to avoid an extra fetch fo the biblio object

To test:
1 - Enable  COinSinOPACResults syspref
2 - Also enable  OPACShowOpenURL  and  OPACOpenURLItemTypes  - adding an itemtype that you can find
3 - Perform a search on the OPAC and confirm coins are included and openurl shown
4 - Perform a search on staff client and confirm openurls do not show
5 - Apply patch
6 - Results should be the same

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-10-07 10:37:59 UTC
Created attachment 141491 [details] [review]
Bug 28375: (follow-up) Remove guessed interface parameter - always pass

Previous patch added interface parameter passed on each call. There was an
existing one that 'guessed' - this patch removes that

Fixes missing comma

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-10-07 10:39:36 UTC
Adding a second signoff here as I missed that Marcel had started QAing... Leaving QA to Marcel once he's had an answer to his question.. it does bare asking.. the code would appear much simple if we just use the context.
Comment 15 Nick Clemens 2022-10-07 12:01:13 UTC
Created attachment 141497 [details] [review]
Bug 28375: (follow-up) Use C4::Context->interface
Comment 16 Marcel de Rooy 2022-10-17 11:48:27 UTC
Created attachment 141955 [details] [review]
Bug 28375: Inefficiencies in fetching COinS

This patchset adds an 'interface' parameter to XSLTParseForDisplay to avoid fetching
coins when not needed

Additionally we move some logic from the scripts to searchResults to avoid an extra fetch fo the biblio object

To test:
1 - Enable  COinSinOPACResults syspref
2 - Also enable  OPACShowOpenURL  and  OPACOpenURLItemTypes  - adding an itemtype that you can find
3 - Perform a search on the OPAC and confirm coins are included and openurl shown
4 - Perform a search on staff client and confirm openurls do not show
5 - Apply patch
6 - Results should be the same

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2022-10-17 11:48:32 UTC
Created attachment 141956 [details] [review]
Bug 28375: (follow-up) Remove guessed interface parameter - always pass

Previous patch added interface parameter passed on each call. There was an
existing one that 'guessed' - this patch removes that

Fixes missing comma

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2022-10-17 11:48:36 UTC
Created attachment 141957 [details] [review]
Bug 28375: (follow-up) Use C4::Context->interface

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2022-10-17 11:48:41 UTC
Created attachment 141958 [details] [review]
Bug 28375: (QA follow-up) Remove two remaining interface params

No test plan: parameter is already obsolete.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Tomás Cohen Arazi 2022-10-21 12:58:34 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!