|
Description
Nick Clemens (kidclamp)
2021-05-18 14:56:00 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 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 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 But aren't we using the information in the results? Not on the detail page, I agree. (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 (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? (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 :-) (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 :) patch = path 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 Created attachment 121611 [details] [review] Bug 28373: (QA follow-up) Fix typo in system preference description Tested OPAC, intranet and list result lists. You might want to add a unit test for QA :) Still waiting for the unit test! Created attachment 122614 [details] [review] Bug 28373: Unit test 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 Created attachment 122616 [details] [review] Bug 28373: (QA follow-up) Fix typo in system preference description 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. Created attachment 122628 [details] [review] Bug 28373: Unit test Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi> 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> 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> 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 Created attachment 124019 [details] [review] Bug 28373: (follow-up) QA fixes Use fully qualified subroutine name Don't return explicit undef 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> 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> 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> 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> 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> 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? What's actually bad here is that we call GetAuthorisedValueDesc for every subfield that needs to be replaced (and it's not cached). Shouldn't we also add a note on the xslt syspref's descriptions? Created attachment 124254 [details] [review] Bug 28373: Add note to XSLT prefs about the new pref 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 (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 (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 Pushed to master for 21.11, thanks to everybody involved! (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. 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 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 Pushed to 21.05.x for 21.05.04 Last patch needed in master. Big change, I prefer not push to 20.11.x 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. (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. Hi Kyle, it looks like you left the atomic update file / forgot moving it to updatedatabase.pl. I think that this system preference's name and description are very inaccurate... but that's for another day... (In reply to David Cook from comment #45) > I think that this system preference's name and description are very > inaccurate... but that's for another day... Be aware of Bug 31020 |