Bug 38936 - Move suppressed record redirection into a sub
Summary: Move suppressed record redirection into a sub
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 28478
Blocks:
  Show dependency treegraph
 
Reported: 2025-01-21 11:05 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-08-29 20:04 UTC (History)
15 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 38936: Unit tests (5.23 KB, patch)
2025-01-21 12:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed (2.08 KB, patch)
2025-01-21 12:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages (5.45 KB, patch)
2025-01-21 12:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 38936: Unit tests (5.24 KB, patch)
2025-02-24 13:59 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed (2.09 KB, patch)
2025-02-24 13:59 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[24.05.x] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages (5.47 KB, patch)
2025-02-24 13:59 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Unit tests (5.25 KB, patch)
2025-08-07 20:54 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed (2.19 KB, patch)
2025-08-07 20:54 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages (5.42 KB, patch)
2025-08-07 20:54 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Unit tests (5.25 KB, patch)
2025-08-29 15:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed (2.19 KB, patch)
2025-08-29 15:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages (5.42 KB, patch)
2025-08-29 15:33 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38936: Unit tests (5.30 KB, patch)
2025-08-29 19:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed (2.23 KB, patch)
2025-08-29 19:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages (5.46 KB, patch)
2025-08-29 19:41 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-01-21 11:05:51 UTC
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`
Comment 1 Tomás Cohen Arazi (tcohen) 2025-01-21 11:28:16 UTC
Moving to the security component as the diff would expose unpublished code from a `Koha security` bug.
Comment 2 Tomás Cohen Arazi (tcohen) 2025-01-21 12:43:37 UTC
Created attachment 176855 [details] [review]
Bug 38936: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi (tcohen) 2025-01-21 12:43:40 UTC
Created attachment 176856 [details] [review]
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi (tcohen) 2025-01-21 12:43:43 UTC
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>
Comment 5 Martin Renvoize (ashimema) 2025-01-21 13:23:20 UTC
Looking good to me :)
Comment 6 Tomás Cohen Arazi (tcohen) 2025-02-24 13:59:01 UTC
Created attachment 178576 [details] [review]
[24.05.x] Bug 38936: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi (tcohen) 2025-02-24 13:59:04 UTC
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>
Comment 8 Tomás Cohen Arazi (tcohen) 2025-02-24 13:59:07 UTC
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>
Comment 9 David Cook 2025-02-24 22:26:33 UTC
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.
Comment 10 Jonathan Druart 2025-03-11 14:39:03 UTC
I am ready to QA this as soon as it's signed off.
Comment 11 Jesse Maseto 2025-04-03 09:31:14 UTC
Patch doesn't apply
Comment 12 Tomás Cohen Arazi (tcohen) 2025-08-07 20:54:23 UTC
Created attachment 185249 [details] [review]
Bug 38936: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi (tcohen) 2025-08-07 20:54:27 UTC
Created attachment 185250 [details] [review]
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi (tcohen) 2025-08-07 20:54:31 UTC
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>
Comment 15 Tomás Cohen Arazi (tcohen) 2025-08-07 20:56:12 UTC
Rebased. Also:

* Removed Jonathan from QA as he's on vacation.
* Removed the 24.05.x patches. Can regenerate if required later.
Comment 16 David Nind 2025-08-14 22:14:58 UTC
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
Comment 17 Tomás Cohen Arazi (tcohen) 2025-08-29 15:30:52 UTC
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.
Comment 18 Tomás Cohen Arazi (tcohen) 2025-08-29 15:33:30 UTC
Created attachment 185937 [details] [review]
Bug 38936: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi (tcohen) 2025-08-29 15:33:33 UTC
Created attachment 185938 [details] [review]
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi (tcohen) 2025-08-29 15:33:37 UTC
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>
Comment 21 David Nind 2025-08-29 19:40:52 UTC
I guess it would help if I applied the correct bug! 8-)
Comment 22 David Nind 2025-08-29 19:41:34 UTC
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>
Comment 23 David Nind 2025-08-29 19:41:38 UTC
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>
Comment 24 David Nind 2025-08-29 19:41:42 UTC
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>
Comment 25 David Nind 2025-08-29 20:04:08 UTC
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