Bug 28478 solved a long standing issue, but there's room for improvement: the controllers use a fairly big code block to handle redirection for suppressed records, with checks for things like `OpacSuppressionByIPRange`. This should be put into a sub. Probably `C4::Output::opac_suppression_redirect`
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