Bug 28373 - Items fields not used in default XSLT
Summary: Items fields not used in default XSLT
Status: Pushed to stable
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: Testopia
URL:
Keywords: Manual
Depends on:
Blocks: 15262
  Show dependency treegraph
 
Reported: 2021-05-18 14:56 UTC by Nick Clemens
Modified: 2021-10-02 21:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
When processing records for display we loop through each field in the record and translate authorized values into descriptions. Item fields in the record contain many authorised values, and the lookups can cause a delay in displaying the record. If using the default XSLT these fields are not displayed as they exist in the record, so parsing them is not necessary and can save time. This bug adds a system preference that disables sending these fields for processing and thus saving time. Enabling the system preference will allow users to pass the items to custom style sheets if needed.
Version(s) released in:
21.11.00,21.05.04


Attachments
Bug 28373: Add PassItemMarcToXSLT system preference (4.99 KB, patch)
2021-05-18 15:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: Add PassItemMarcToXSLT system preference (4.99 KB, patch)
2021-06-03 21:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: (QA follow-up) Fix typo in system preference description (1.18 KB, patch)
2021-06-03 21:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: Unit test (2.10 KB, patch)
2021-07-06 12:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: Add PassItemMarcToXSLT system preference (4.96 KB, patch)
2021-07-06 12:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: (QA follow-up) Fix typo in system preference description (1.18 KB, patch)
2021-07-06 12:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: Unit test (2.16 KB, patch)
2021-07-07 07:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 28373: Add PassItemMarcToXSLT system preference (5.02 KB, patch)
2021-07-07 07:50 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 28373: (QA follow-up) Fix typo in system preference description (1.24 KB, patch)
2021-07-07 07:50 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 28373: (follow-up) QA fixes (1.79 KB, patch)
2021-08-23 14:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: Unit test (2.21 KB, patch)
2021-08-29 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: Add PassItemMarcToXSLT system preference (5.10 KB, patch)
2021-08-29 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: (QA follow-up) Fix typo in system preference description (1.30 KB, patch)
2021-08-29 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: (follow-up) QA fixes (1.85 KB, patch)
2021-08-29 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: (QA follow-up) Add . to end of system preference description (1.25 KB, patch)
2021-08-29 11:49 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28373: Add note to XSLT prefs about the new pref (7.91 KB, patch)
2021-08-30 19:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: Add warning if custom stylesheets are defined (1.67 KB, patch)
2021-08-30 19:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: [21.05.x] Add PassItemMarcToXSLT system preference (16.74 KB, patch)
2021-09-03 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: (follow-up) Fix copy error (8.48 KB, patch)
2021-09-03 15:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28373: [21.05] Fix transformMARCXML4XSLT export in C4::XSLT (951 bytes, patch)
2021-09-09 08:33 UTC, Jonathan Druart
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 14:56:00 UTC

    
Comment 1 Nick Clemens 2021-05-18 14:57:29 UTC
searchResults passes the item/952 fields to XSLT for parsing, and we spend time in transformMARC4XSLT translating authorised values in these fields, but our default XSLT never uses them

We should make passing them optional to support anyone who has added 952 to their XSLT, but saving time for anyone who has not
Comment 2 Nick Clemens 2021-05-18 15:09:59 UTC
Created attachment 121115 [details] [review]
Bug 28373: Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster
Comment 3 Nick Clemens 2021-05-18 15:24:28 UTC
Note: I say "slightly", however, that is a function of how many items are on your records.

The perf increase is small on the sample database, but very large on a consortium site with many items per record
Comment 4 Katrin Fischer 2021-05-24 15:11:23 UTC
But aren't we using the information in the results? Not on the detail page, I agree.
Comment 5 Nick Clemens 2021-05-24 16:06:58 UTC
(In reply to Katrin Fischer from comment #4)
> But aren't we using the information in the results? Not on the detail page,
> I agree.

Not in the XSLT - we are either using the items array we build in searchResults or the items info we build in BuildKohaItemsNameSpace - the item fields in the record are not used in the XSLT
Comment 6 Katrin Fischer 2021-05-24 16:14:14 UTC
(In reply to Nick Clemens from comment #5)
> (In reply to Katrin Fischer from comment #4)
> > But aren't we using the information in the results? Not on the detail page,
> > I agree.
> 
> Not in the XSLT - we are either using the items array we build in
> searchResults or the items info we build in BuildKohaItemsNameSpace - the
> item fields in the record are not used in the XSLT

Ah ok, I just remembered hat there was some complicated XSLT there that I never wanted to touch:

https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl#L1250

:) That's using output of BuildKohaItemsNameSpace then?
Comment 7 Nick Clemens 2021-05-24 16:36:07 UTC
(In reply to Katrin Fischer from comment #6)
> (In reply to Nick Clemens from comment #5)
> > (In reply to Katrin Fischer from comment #4)
> > > But aren't we using the information in the results? Not on the detail page,
> > > I agree.
> > 
> > Not in the XSLT - we are either using the items array we build in
> > searchResults or the items info we build in BuildKohaItemsNameSpace - the
> > item fields in the record are not used in the XSLT
> 
> Ah ok, I just remembered hat there was some complicated XSLT there that I
> never wanted to touch:
> 
> https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-
> tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl#L1250
> 
> :) That's using output of BuildKohaItemsNameSpace then?

Correct, you can verify too that none of that disappears with this patch :-)
Comment 8 Katrin Fischer 2021-05-24 16:37:13 UTC
(In reply to Nick Clemens from comment #7)
> (In reply to Katrin Fischer from comment #6)
> > (In reply to Nick Clemens from comment #5)
> > > (In reply to Katrin Fischer from comment #4)
> > > > But aren't we using the information in the results? Not on the detail page,
> > > > I agree.
> > > 
> > > Not in the XSLT - we are either using the items array we build in
> > > searchResults or the items info we build in BuildKohaItemsNameSpace - the
> > > item fields in the record are not used in the XSLT
> > 
> > Ah ok, I just remembered hat there was some complicated XSLT there that I
> > never wanted to touch:
> > 
> > https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-
> > tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl#L1250
> > 
> > :) That's using output of BuildKohaItemsNameSpace then?
> 
> Correct, you can verify too that none of that disappears with this patch :-)

All good - just trying to get the picture. Wonder why this patch was chosen? But that question is clearly out of scope here :)
Comment 9 Katrin Fischer 2021-05-24 16:57:05 UTC
patch = path
Comment 10 Katrin Fischer 2021-06-03 21:48:52 UTC
Created attachment 121610 [details] [review]
Bug 28373: Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster
Comment 11 Katrin Fischer 2021-06-03 21:48:58 UTC
Created attachment 121611 [details] [review]
Bug 28373: (QA follow-up) Fix typo in system preference description
Comment 12 Katrin Fischer 2021-06-03 21:49:34 UTC
Tested OPAC, intranet and list result lists. 

You might want to add a unit test for QA :)
Comment 13 Katrin Fischer 2021-07-05 21:00:33 UTC
Still waiting for the unit test!
Comment 14 Nick Clemens 2021-07-06 12:35:55 UTC
Created attachment 122614 [details] [review]
Bug 28373: Unit test
Comment 15 Nick Clemens 2021-07-06 12:36:01 UTC
Created attachment 122615 [details] [review]
Bug 28373: Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster
Comment 16 Nick Clemens 2021-07-06 12:36:07 UTC
Created attachment 122616 [details] [review]
Bug 28373: (QA follow-up) Fix typo in system preference description
Comment 17 Katrin Fischer 2021-07-06 12:47:00 UTC
Hi Nick, if it's only the unit tests I am happy to leave my sign-off - but I can also come back for QA.
Comment 18 Emmi Takkinen 2021-07-07 07:49:40 UTC
Created attachment 122628 [details] [review]
Bug 28373: Unit test

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 19 Emmi Takkinen 2021-07-07 07:50:12 UTC
Created attachment 122629 [details] [review]
Bug 28373: Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 20 Emmi Takkinen 2021-07-07 07:50:47 UTC
Created attachment 122630 [details] [review]
Bug 28373: (QA follow-up) Fix typo in system preference description

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 21 Katrin Fischer 2021-08-10 23:23:44 UTC
Can you please check the QA tools? I am having this failure:

 FAIL	t/Search.t
   FAIL	  critic
		# Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s).  
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid


Also the tests are failing:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/Search.t
t/Search.t .. 1/4     # No tests run!

#   Failed test 'No tests run for subtest "searchResults PassItemMarcToXSLT test"'
#   at t/Search.t line 207.
Undefined subroutine &main::searchResults called at t/Search.t line 197.
# Looks like your test exited with 255 just after 4.
t/Search.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/4 subtests
Comment 22 Nick Clemens 2021-08-23 14:13:51 UTC
Created attachment 124019 [details] [review]
Bug 28373: (follow-up) QA fixes

Use fully qualified subroutine name
Don't return explicit undef
Comment 23 Katrin Fischer 2021-08-29 11:49:41 UTC
Created attachment 124222 [details] [review]
Bug 28373: Unit test

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2021-08-29 11:49:46 UTC
Created attachment 124223 [details] [review]
Bug 28373: Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2021-08-29 11:49:50 UTC
Created attachment 124224 [details] [review]
Bug 28373: (QA follow-up) Fix typo in system preference description

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2021-08-29 11:49:54 UTC
Created attachment 124225 [details] [review]
Bug 28373: (follow-up) QA fixes

Use fully qualified subroutine name
Don't return explicit undef

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2021-08-29 11:49:58 UTC
Created attachment 124226 [details] [review]
Bug 28373: (QA follow-up) Add . to end of system preference description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Jonathan Druart 2021-08-30 14:43:09 UTC
So people using this processing will have their displays broken. What do we do?

Cannot we do this replacement in the xslt itself?

(In reply to Nick Clemens from comment #3)
> Note: I say "slightly", however, that is a function of how many items are on
> your records.
> 
> The perf increase is small on the sample database, but very large on a
> consortium site with many items per record

Did you benchmark it?
Comment 29 Jonathan Druart 2021-08-30 14:47:11 UTC
What's actually bad here is that we call GetAuthorisedValueDesc for every subfield that needs to be replaced (and it's not cached).
Comment 30 Jonathan Druart 2021-08-30 14:57:49 UTC
Shouldn't we also add a note on the xslt syspref's descriptions?
Comment 31 Nick Clemens 2021-08-30 19:55:13 UTC
Created attachment 124254 [details] [review]
Bug 28373: Add note to XSLT prefs about the new pref
Comment 32 Nick Clemens 2021-08-30 19:55:17 UTC
Created attachment 124255 [details] [review]
Bug 28373: Add warning if custom stylesheets are defined

This patch adds a check of the current XSLT prefs and warns to check the new pref
if any are not set to default
Comment 33 Nick Clemens 2021-08-30 19:55:31 UTC
(In reply to Jonathan Druart from comment #29)
> What's actually bad here is that we call GetAuthorisedValueDesc for every
> subfield that needs to be replaced (and it's not cached).

See bug 28371
Comment 34 Nick Clemens 2021-08-30 19:56:13 UTC
(In reply to Jonathan Druart from comment #30)
> Shouldn't we also add a note on the xslt syspref's descriptions?

Added, as well as a warning for users with custom sheets.

If preferred the update can be made to default to ON
Comment 35 Jonathan Druart 2021-08-31 08:28:34 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 36 Jonathan Druart 2021-08-31 08:30:24 UTC
(In reply to Nick Clemens from comment #32)
> Created attachment 124255 [details] [review] [review]
> Bug 28373: Add warning if custom stylesheets are defined
> 
> This patch adds a check of the current XSLT prefs and warns to check the new
> pref
> if any are not set to default

I've removed the 'last' and display the syspref's name to provide more information.
Comment 37 Nick Clemens 2021-09-03 15:22:11 UTC
Created attachment 124495 [details] [review]
Bug 28373: [21.05.x] Add PassItemMarcToXSLT system preference

Default stylesheets do not reference item fields for XSLT display, however, we
spend time translating the values in the item fields.

This patch adds a system preference, PassItemMarcToXSLT. and unless enabled we remove
item fields before processing

To test:
1 - Perform some search on the staff client and opac
2 - Use the console (F12) to view the time spent on the network tab
3 - Note performance
4 - Apply patch, updatedatabase, restart_all
5 - Repeat searches
6 - Note that display has not changed
7 - Note performance, results should display slightly faster

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 28373: Unit test

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 28373: (QA follow-up) Fix typo in system preference description

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 28373: (follow-up) QA fixes

Use fully qualified subroutine name
Don't return explicit undef

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 28373: (QA follow-up) Add . to end of system preference description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Bug 28373: Add note to XSLT prefs about the new pref

Bug 28373: Add warning if custom stylesheets are defined

This patch adds a check of the current XSLT prefs and warns to check the new pref
if any are not set to default
Comment 38 Nick Clemens 2021-09-03 15:26:49 UTC
Created attachment 124496 [details] [review]
Bug 28373: (follow-up) Fix copy error

Previous commit linked all 'default' buttons for sysprefs to OPACXSLTListsDisplay

This adjusts the links

To test:
Click "default" for all 6 XSLT preferences
Confirm the syspref relating to the link is marked 'modified' when clicked
Comment 39 Kyle M Hall 2021-09-03 17:05:28 UTC
Pushed to 21.05.x for 21.05.04
Comment 40 Jonathan Druart 2021-09-06 09:23:02 UTC
Last patch needed in master.
Comment 41 Fridolin Somers 2021-09-08 20:54:17 UTC
Big change, I prefer not push to 20.11.x
Comment 42 Jonathan Druart 2021-09-09 08:33:53 UTC
Created attachment 124686 [details] [review]
Bug 28373: [21.05] Fix transformMARCXML4XSLT export in C4::XSLT

#   Failed test 'use Koha::OAI::Server::Repository;'
 #   at t/00-load.t line 62.
 #     Tried to use 'Koha::OAI::Server::Repository'.
 #     Error:  "transformMARCXML4XSLT" is not exported by the C4::XSLT module
 # Can't continue after import errors at /kohadevbox/koha/Koha/OAI/Server/Repository.pm line 39.
Comment 43 Jonathan Druart 2021-09-09 09:01:39 UTC
(In reply to Nick Clemens from comment #38)
> Created attachment 124496 [details] [review] [review]
> Bug 28373: (follow-up) Fix copy error
> 
> Previous commit linked all 'default' buttons for sysprefs to
> OPACXSLTListsDisplay
> 
> This adjusts the links
> 
> To test:
> Click "default" for all 6 XSLT preferences
> Confirm the syspref relating to the link is marked 'modified' when clicked

Pushed to master.
Comment 44 Katrin Fischer 2021-10-02 21:10:08 UTC
Hi Kyle, it looks like you left the atomic update file / forgot moving it to updatedatabase.pl.