|
Description
Pedro Amorim
2024-10-04 14:47:39 UTC
Created attachment 172466 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Created attachment 172467 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Created attachment 172468 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Created attachment 172469 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (skip if sandboxes): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. Created attachment 172470 [details] [review] [DONT PUSH] Bug 38096: Add test data for fields 856 and 857 Created attachment 172471 [details] [review] Bug 38096: [DONT PUSH] Add test data for fields 856 and 857 Created attachment 172532 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 172533 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 172534 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 172535 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (skip if sandboxes): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 172536 [details] [review] Bug 38096: [DONT PUSH] Add test data for fields 856 and 857 Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Thanks Pedro! In the test, I'd add to refresh the page in step 8 after changing the pref. I hope I attached everything correctly... I don't often test bugs with many patches like this... (In reply to Caroline Cyr La Rose from comment #12) > Thanks Pedro! > > In the test, I'd add to refresh the page in step 8 after changing the pref. That's great Caroline, thank you very much. (In reply to Caroline Cyr La Rose from comment #12) > I hope I attached everything correctly... I don't often test bugs with many > patches like this... All patches seem to have been attached correctly. Thank you. Patch no longer applies, updating here to come back to this later. Created attachment 192376 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Created attachment 192377 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Created attachment 192378 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Created attachment 192379 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (skip if sandboxes): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. Created attachment 192380 [details] [review] Bug 38096: [DONT PUSH] Add test data for fields 856 and 857 Created attachment 192381 [details] [review] Bug 38096: (QA follow-up): Fix passing string param to template Upon following original test plan after rebase, noticed that OPACDisplay856uAsImage wasn't working for 'Details' or 'Results'. The culprit was how each screen is invoking the template, passing the string param in a wrong XSL way. Marcel, are you willing to QA this one? :) My hands start shaking when I see xslt... (In reply to Jonathan Druart from comment #21) > Marcel, are you willing to QA this one? :) > > My hands start shaking when I see xslt... Sure. I've identified an issue with the first patch Bug 38096: OPAC: Preparation. The rebase is not correct. Looking again. False alarm, all appears well. The first patch appears to be "removing" functionality but it's not. It's DRYing the code as it was effectively duplicated for with and without OPACURLOpenInNewWindow. This DRY work puts MARC21slim2OPACResults in line with MARC21slim2OPACDetail ensuring that the following patches converging the two do not introduce new regressions. (In reply to Pedro Amorim from comment #14) > Patch no longer applies, updating here to come back to this later. You removed all previous signoff lines.. (In reply to Pedro Amorim from comment #18) > 1) If testing k-t-d, add the test data for test plan (skip if sandboxes): > updatedatabase Hmm. Dont like such test plans as a member of the non-ktd minority.. Created attachment 192589 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192590 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192591 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192592 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (otherwise prep instructions below): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. If not on k-t-d and updatedatabase not available: perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl or koha-shell -c "perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl" <instance_name> If CLI not available (preparation on UI): A) Add field 857 to the 'Default' framework B) Edit the field: <staff_url>/cgi-bin/koha/admin/marc_subfields_structure.pl?op=add_form&tagfield=857&frameworkcode= C) Check the 'Editor' checkbox for subfields q, u and z (Add these if not there already) D) Edit the record in question, visit: <staff_url>/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=76 E) Click 'Settings' dropdown and pick 'Default' framework. F) Add MARC data that corresponds to the following: 3 instances of 856 as below. 3 instances of 857 as below. <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://koha-community.org/files/2013/09/cropped-kohabanner3.jpg</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">KohaCon 2024 logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://2024.kohacon.org/wp-content/uploads/2024/01/kohacon24_fr_verticale_vert_noir.png</subfield> </datafield> G) At this point you're ready to start the original test plan from step 4) Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192593 [details] [review] Bug 38096: [DONT PUSH] Add test data for fields 856 and 857 Created attachment 192594 [details] [review] Bug 38096: (QA follow-up): Fix passing string param to template Upon following original test plan after rebase, noticed that OPACDisplay856uAsImage wasn't working for 'Details' or 'Results'. The culprit was how each screen is invoking the template, passing the string param in a wrong XSL way. Please note that I am still working on this. So we are now on parallel tracks :) (In reply to Marcel de Rooy from comment #33) > Please note that I am still working on this. So we are now on parallel > tracks :) Hi Marcel, appreciate your time here. My new patches are the same, just with signed-off lines added and an updated test plan in case not using k-t-d. Great work. I like the improvement to consolidate code in the Utils. Looks good to me. Compare opac and staff Utils addition diff koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slimUtils.xsl koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl Too much differences. At some point we have let go here and left the idea of keeping those two more or less in sync. This patch set adds changes too. No blocker. Lets continue. koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl Indentation of those new 856 and 857 blocks is a bit messy. Please correct. Could we perhaps squash some patches? Changing status for minor correction. Doesn't apply. This time because of bug 41588 (maybe more). Working on it. Created attachment 192813 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192814 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192815 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192816 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (otherwise prep instructions below): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. If not on k-t-d and updatedatabase not available: perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl or koha-shell -c "perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl" <instance_name> If CLI not available (preparation on UI): A) Add field 857 to the 'Default' framework B) Edit the field: <staff_url>/cgi-bin/koha/admin/marc_subfields_structure.pl?op=add_form&tagfield=857&frameworkcode= C) Check the 'Editor' checkbox for subfields q, u and z (Add these if not there already) D) Edit the record in question, visit: <staff_url>/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=76 E) Click 'Settings' dropdown and pick 'Default' framework. F) Add MARC data that corresponds to the following: 3 instances of 856 as below. 3 instances of 857 as below. <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://koha-community.org/files/2013/09/cropped-kohabanner3.jpg</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">KohaCon 2024 logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://2024.kohacon.org/wp-content/uploads/2024/01/kohacon24_fr_verticale_vert_noir.png</subfield> </datafield> G) At this point you're ready to start the original test plan from step 4) Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Created attachment 192817 [details] [review] Bug 38096: [DONT PUSH] Add test data for fields 856 and 857 Created attachment 192818 [details] [review] Bug 38096: (QA follow-up): Fix passing string param to template Upon following original test plan after rebase, noticed that OPACDisplay856uAsImage wasn't working for 'Details' or 'Results'. The culprit was how each screen is invoking the template, passing the string param in a wrong XSL way. Created attachment 192819 [details] [review] Bug 38096: (QA follow-up): Fix indentation Rebased and retested with test plan to ensure all works as before and recent changes from bug 41588 have moved over. (In reply to Marcel de Rooy from comment #35) > koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetDetail.xsl > koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2intranetResults.xsl > koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl > koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl > Indentation of those new 856 and 857 blocks is a bit messy. Please correct. > Could we perhaps squash some patches? > > Changing status for minor correction. Marcel, thank you, submitted follow-up to fix indentation. I'm on board with squashing the patches, I organized them that way initially to ease the review (i.e. make the 'change path' more clear). If you agree, I'll leave to your best judgement in regards to squashing (I'm on board with whatever you decide). Thank you once again. Created attachment 192906 [details] [review] Bug 38096: OPAC: Preparation The OPAC logic for 856 in results page and detail page has diverged. This commit ensures the 856 in the OPAC detail and search results pages are consistent, in preparation to become DRY in the following commits. With this DRY refactor, the following divergences are now aligned: - 856 block now calls AddMissingProtocol regardless of value OPACURLOpenInNewWindow (previously was only being called if OPACURLOpenInNewWindow was 0). Now it's in line with what exists in MARC21slim2OPACDetail.xsl - substring(,1,6)='image/' is also now a consideration when OPACURLOpenInNewWindow is 1. (previously was only being called if OPACURLOpenInNewWindow was 0). This is now in line with that exists in MARC21slim2OPACDetail.xsl Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 192907 [details] [review] Bug 38096: OPAC: Move 856 logic to renderMARCOnlineResourceField utils Now that OPAC detail and search results are consitent, we can DRY this logic to its own function to be also utilized by 857 in the following commit Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 192908 [details] [review] Bug 38096: OPAC: Display 857 on detail and results pages Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 192909 [details] [review] Bug 38096: Staff: Display 857 to detail and results pages This work DRYs the 856 display logic to renderMARCOnlineResourceField. This was previously written in detail and results pages. Once this single function exists, 857 can also make use of it to be displayed using the same logic/features already present for 856. 1) If testing k-t-d, add the test data for test plan (otherwise prep instructions below): updatedatabase 2) Force reindex of this biblio by saving it, visit the biblio: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 3) Click "Edit" -> "Edit record" -> "Save" 4) Open the following URL's, preferably on 4 different browser tabs (we're using biblio 76 for this test): <staff_url>/cgi-bin/koha/catalogue/search.pl?q=music (first record [76] is the one we're testing) <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 <opac_url>/cgi-bin/koha/opac-search.pl?idx=&q=music (first record [76] is the one we're testing) <opac_url>/cgi-bin/koha/opac-detail.pl?biblionumber=76 5) Verify that on all pages there are 3 instances of 856 (Online resources) and 3 instances of 857 (Online archive resources) with their respective link labels and URLs. 6) Click on the "Koha community no protocol" link for 856 and 857, notice the 'http://' protocol is added. 7) Change the values of OPACDisplay856uAsImage + Display856uAsImage and refresh the respective page. Verify it works as expected (respects the system preference). Note: This only applies for 856, not 857 (out of scope here). 8) Change the value of OPACURLOpenInNewWindow and click on the links before and after. Verify they respect the system preference. If not on k-t-d and updatedatabase not available: perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl or koha-shell -c "perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl" <instance_name> If CLI not available (preparation on UI): A) Add field 857 to the 'Default' framework B) Edit the field: <staff_url>/cgi-bin/koha/admin/marc_subfields_structure.pl?op=add_form&tagfield=857&frameworkcode= C) Check the 'Editor' checkbox for subfields q, u and z (Add these if not there already) D) Edit the record in question, visit: <staff_url>/cgi-bin/koha/cataloguing/addbiblio.pl?biblionumber=76 E) Click 'Settings' dropdown and pick 'Default' framework. F) Add MARC data that corresponds to the following: 3 instances of 856 as below. 3 instances of 857 as below. <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="856" ind1="4" ind2="2"> <subfield code="z">Koha logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://koha-community.org/files/2013/09/cropped-kohabanner3.jpg</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community no protocol</subfield> <subfield code="u">koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">Koha community with protocol</subfield> <subfield code="u">https://koha-community.org/</subfield> </datafield> <datafield tag="857" ind1="4" ind2="2"> <subfield code="z">KohaCon 2024 logo image</subfield> <subfield code="q">jpeg</subfield> <subfield code="u">https://2024.kohacon.org/wp-content/uploads/2024/01/kohacon24_fr_verticale_vert_noir.png</subfield> </datafield> G) At this point you're ready to start the original test plan from step 4) Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 192910 [details] [review] Bug 38096: (QA follow-up): Fix passing string param to template Upon following original test plan after rebase, noticed that OPACDisplay856uAsImage wasn't working for 'Details' or 'Results'. The culprit was how each screen is invoking the template, passing the string param in a wrong XSL way. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 192911 [details] [review] Bug 38096: (QA follow-up): Fix indentation Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Nice work everyone! Pushed to main for 26.05 Created attachment 192919 [details] [review] Bug 38096: (QA follow-up): Fix URLLinkText Created attachment 192920 [details] [review] Bug 38096: (follow-up) Fix missing biblionumber parameter in renderMARCOnlineResourceField The renderMARCOnlineResourceField template uses $biblionumber for the TrackClicks feature but this parameter was never declared in the template definition nor passed by any of the call sites. This caused XSLT runtime errors when TrackClicks system preference is enabled ('track' or 'anonymous') and records contain MARC fields 856 or 857 with subfield $u (URLs): Variable 'biblionumber' has not been declared. Undefined variable XPath evaluation returned no result. This patch: 1. Adds biblionumber parameter to renderMARCOnlineResourceField template 2. Passes biblionumber value in all 4 call sites (2 in Detail, 2 in Results) Test plan: 1. Enable TrackClicks: System Preferences > OPAC > TrackClicks = "track" 2. Find a bibliographic record with field 856 or 857 containing subfield $u 3. Run: yarn build (to regenerate XSLT for all languages) 4. View the record in OPAC detail and search results pages 5. Verify no XSLT errors in plack-opac-error.log 6. Verify online resource links display correctly 7. Verify links include biblionumber parameter in tracklinks.pl URLs Lucas these 2 follow-ups are very important and are fixing 2 major oversights from the original patch. Things will blow up as described in Martin's commit. The short of it is, the DRY work missed some variables that are only utilized in specific conditions. Created attachment 192921 [details] [review] Bug 38096: (QA follow-up): Fix URLLinkText Created attachment 192922 [details] [review] Bug 38096: (follow-up) Fix missing biblionumber parameter in renderMARCOnlineResourceField The renderMARCOnlineResourceField template uses $biblionumber for the TrackClicks feature but this parameter was never declared in the template definition nor passed by any of the call sites. This caused XSLT runtime errors when TrackClicks system preference is enabled ('track' or 'anonymous') and records contain MARC fields 856 or 857 with subfield $u (URLs): Variable 'biblionumber' has not been declared. Undefined variable XPath evaluation returned no result. This patch: 1. Adds biblionumber parameter to renderMARCOnlineResourceField template 2. Passes biblionumber value in all 4 call sites (2 in Detail, 2 in Results) Test plan: 1. Enable TrackClicks: System Preferences > OPAC > TrackClicks = "track" 2. Find a bibliographic record with field 856 or 857 containing subfield $u 3. Run: yarn build (to regenerate XSLT for all languages) 4. View the record in OPAC detail and search results pages 5. Verify no XSLT errors in plack-opac-error.log 6. Verify online resource links display correctly 7. Verify links include biblionumber parameter in tracklinks.pl URLs Rebased to apply nicely due to tidy. follow-ups pushed to main t/db_dependent/Search.t is failing, I guess it's coming from here. 02:30:02 koha-1 | # XSLT::Base: runtime error: file /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl line 730 element choose 02:30:02 koha-1 | # Variable 'URLLinkText' has not been declared. 02:30:02 koha-1 | # Undefined variable (In reply to Jonathan Druart from comment #59) > t/db_dependent/Search.t is failing, I guess it's coming from here. > > 02:30:02 koha-1 | # XSLT::Base: runtime error: file > /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > line 730 element choose > 02:30:02 koha-1 | # Variable 'URLLinkText' has not been declared. > 02:30:02 koha-1 | # Undefined variable The last 2 patches should've fixed that. Is this after the last 2 follow-ups? Looking into it, the URLLinkText intra fix is also required on the opac MARC21slimUtils.xsl Created attachment 192966 [details] [review] Bug 38096: (QA follow-up): Add URLLinkText to opac xsl also Fix error: [2026/02/11 09:18:00] [WARN] XSLT::Base: runtime error: file /usr/share/koha/opac/htdocs/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl line 726 element choose Variable 'URLLinkText' has not been declared. Undefined variable at /usr/share/koha/lib/Koha/XSLT/Base.pm line 231. Run tests to prove the patch fixes the issue (this fails before the patch): prove t/db_dependent/Search.t (In reply to Jonathan Druart from comment #59) > t/db_dependent/Search.t is failing, I guess it's coming from here. > > 02:30:02 koha-1 | # XSLT::Base: runtime error: file > /kohadevbox/koha/koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl > line 730 element choose > 02:30:02 koha-1 | # Variable 'URLLinkText' has not been declared. > 02:30:02 koha-1 | # Undefined variable Last patch fixes it please verify. Adding 25.11.x release candidate here for the following reasons: - Not showing 857 on the search results + detail pages may arguably be considered a bug by some as that field has been included on the DEFAULT framework since 23.11 (bug 34667). - Back porting fixes on top of this rewrite will become increasingly difficult if not back ported. Created attachment 193000 [details] [review] Bug 38096: (QA follow-up): Add URLLinkText to opac xsl also Fix error: [2026/02/11 09:18:00] [WARN] XSLT::Base: runtime error: file /usr/share/koha/opac/htdocs/opac-tmpl/bootstrap/en/xslt/MARC21slimUtils.xsl line 726 element choose Variable 'URLLinkText' has not been declared. Undefined variable at /usr/share/koha/lib/Koha/XSLT/Base.pm line 231. Run tests to prove the patch fixes the issue (this fails before the patch): prove t/db_dependent/Search.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> follow-up pushed to main |