Bug 38936

Summary: Move suppressed record redirection into a sub
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Patch doesn't apply --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27734
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:
Bug Depends on: 28478    
Bug Blocks:    
Attachments: Bug 38936: Unit tests
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages
[24.05.x] Bug 38936: Unit tests
[24.05.x] Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed
[24.05.x] Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages
Bug 38936: Unit tests
Bug 38936: Introduce C4::Output::redirect_if_opac_suppressed
Bug 38936: Use redirect_if_opac_suppressed() in opac-*detail.pl pages

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