Description
Tomás Cohen Arazi (tcohen)
2025-01-21 11:05:51 UTC
Moving to the security component as the diff would expose unpublished code from a `Koha security` bug. Created attachment 176855 [details] [review] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 176856 [details] [review] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 176857 [details] [review] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Looking good to me :) Created attachment 178576 [details] [review] [24.05.x] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 178577 [details] [review] [24.05.x] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 178578 [details] [review] [24.05.x] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> From a separation of concerns perspective, I don't like the $biblio object being in this function, but having this code in a function is a great step forward. I am ready to QA this as soon as it's signed off. Patch doesn't apply Created attachment 185249 [details] [review] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185250 [details] [review] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185251 [details] [review] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Rebased. Also: * Removed Jonathan from QA as he's on vacation. * Removed the 24.05.x patches. Can regenerate if required later. The patch no longer applies: git bz apply 28478 Bug 28478 - MARC detail and ISBD pages still show suppressed records 176898 - Bug 28478: Add get_opac_suppression extractor 176899 - Bug 28478: Add Koha::Biblio->opac_suppressed() 176900 - Bug 28478: Make opac-*detail.pl scripts use Koha::Biblio->opac_suppressed() Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28478: Add get_opac_suppression extractor Using index info to reconstruct a base tree... M Koha/Biblio/Metadata/Extractor/MARC.pm M t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t CONFLICT (content): Merge conflict in t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t error: Failed to merge in the changes. Patch failed at 0001 Bug 28478: Add get_opac_suppression extractor They apply cleanly to me <.<: ``` ❯ git bz apply 38936 Bug 38936 - Move suppressed record redirection into a sub 185249 - Bug 38936: Unit tests 185250 - Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed 185251 - Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Apply? [(y)es, (n)o, (i)nteractive] y Aplicando: Bug 38936: Unit tests Aplicando: Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Aplicando: Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages ``` Resubmitting just in case my git version is using newer tricks to solve the apply. Created attachment 185937 [details] [review] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185938 [details] [review] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185939 [details] [review] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I guess it would help if I applied the correct bug! 8-) Created attachment 185967 [details] [review] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 185968 [details] [review] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 185969 [details] [review] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. The new tests should pass after the patch is applied: 1.1 prove t/db_dependent/Output.t 2. The other tests in bug 28478 should also continue to pass: 2.1 prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t 2.2 prove t/db_dependent/Koha/Biblio.t 3. OPAC suppression works as expected: 3.1 Set OpacSuppression system preference to "Hide". 3.2 Edit a record (I used Programming Perl, 262) and changed 942$n to "Yes". 3.3 Attempt to access the record in the OPAC and you should get "Record blocked You are not authorized to view this record.". 3.4 Checked for all OPAC views: - Normal view: /cgi-bin/koha/opac-detail.pl?biblionumber=262 - MARC view: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=262 - ISBD view: /cgi-bin/koha/opac-ISBDdetail.pl?biblionumber=262 Created attachment 186767 [details] [review] Bug 38936: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186768 [details] [review] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186769 [details] [review] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186780 [details] [review] Bug 38936: Remove local WARN We now have Test::NoWarnings Nice work everyone! Pushed to main for 25.11 We ended up with another http link in the license of Output.t causing xt/find-license-problems.t to fail Created attachment 187139 [details] [review] Bug 38936: Use https in license Follow up pushed to main |