Bug 38330

Summary: Make bib-level suppression a biblio table field instead of part of a marc tag
Product: Koha Reporter: Dani Elder <danielle.elder>
Component: CatalogingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Failed QA --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: angela.berrett, dcook, hebah, kebliss, m.de.rooy, martin.renvoize, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28478
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 39336, 39337    
Attachments: Bug 38330: DB changes
Bug 38330: DBIC updates
Bug 38330: Map to biblio.opac_suppressed on saving record
Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save
Bug 38330: DB changes
Bug 38330: DBIC updates
Bug 38330: Map to biblio.opac_suppressed on saving record
Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save
Bug 38330: DB changes
Bug 38330: DBIC updates
Bug 38330: Map to biblio.opac_suppressed on saving record
Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save
Bug 38330: (QA follow-up) Remove use of metadata_extractor
Bug 38330: (QA follow-up) Remove use of metadata_extractor
Bug 38330: (QA follow-up) Add population of opac_suppressed from biblio_metadata

Description Dani Elder 2024-11-01 20:08:14 UTC
Suppression should be a biblio table field instead of in a marc field. Running reports with it to extract that field each time is not optimal.
Comment 1 Angela Berrett 2024-11-07 17:45:46 UTC
+1 I like this idea.
Comment 2 Tomás Cohen Arazi (tcohen) 2024-12-18 13:14:14 UTC
Created attachment 175648 [details] [review]
Bug 38330: DB changes
Comment 3 Tomás Cohen Arazi (tcohen) 2024-12-18 13:14:17 UTC
Created attachment 175649 [details] [review]
Bug 38330: DBIC updates
Comment 4 Tomás Cohen Arazi (tcohen) 2024-12-18 13:14:19 UTC
Created attachment 175650 [details] [review]
Bug 38330: Map to biblio.opac_suppressed on saving record
Comment 5 Tomás Cohen Arazi (tcohen) 2024-12-18 13:14:22 UTC
Created attachment 175651 [details] [review]
Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save

This patchset adds a new column to biblio/deletedbiblio for storing the
information about OPAC suppression for the record.

It works as any other framework-mapped attribute. It defaults to 0 (not
suppressed) and some sanitization is done on storing, because the DB
should only contain 0 or 1.

To test:
0. Have a couple records with 942$n set to suppress them.
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: All good
3. Check the `opac_suppressed` column was added to both tables
4. Notice it tells you need to run `touch_all_biblios.pl`
5. Run:
  k$ koha-mysql kohadev
   > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed;
=> FAIL: There are no records with the opac_suppressed flag set to 1
6. Run:
  k$ perl misc/maintenance/touch_all_biblios.pl -v
=> SUCCESS: No failures
7. Repeat 5
=> SUCCESS: Your couple records have the flag set. This means ModBiblio
is doing the right thing
8. Play with adding new records (suppressed and not)
=> SUCCESS: Suppression status is set correctly
9. Change the status to select records
=> SUCCESS: The DB column and the MARC are in sync!
10. Sign off :-D
Comment 6 Tomás Cohen Arazi (tcohen) 2024-12-18 13:14:50 UTC
My two cents towards all this.
Comment 7 Andrew Fuerste-Henry 2024-12-19 14:49:40 UTC Comment hidden (obsolete)
Comment 8 Andrew Fuerste-Henry 2024-12-19 14:49:42 UTC Comment hidden (obsolete)
Comment 9 Andrew Fuerste-Henry 2024-12-19 14:49:45 UTC Comment hidden (obsolete)
Comment 10 Andrew Fuerste-Henry 2024-12-19 14:49:47 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize (ashimema) 2025-03-14 15:54:30 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize (ashimema) 2025-03-14 15:54:33 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-03-14 15:54:36 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize (ashimema) 2025-03-14 15:54:39 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize (ashimema) 2025-03-14 15:55:21 UTC
This is a great move in a positive direction.. it highlights a few API oversights though.. which I think should be corrected in a follow-up bug.

Passing QA as it is.
Comment 16 Martin Renvoize (ashimema) 2025-03-14 16:11:25 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize (ashimema) 2025-03-14 16:16:05 UTC
Created attachment 179340 [details] [review]
Bug 38330: (QA follow-up) Remove use of metadata_extractor

We had a local method for getting opac_suppressed in Koha::Biblio which
relied on extracting from the MARC record.  Now that we have an
opac_suppressed field in the Biblio table, we no longer need this
wrapper method.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize (ashimema) 2025-03-14 16:31:32 UTC
Created attachment 179341 [details] [review]
Bug 38330: (QA follow-up) Add population of opac_suppressed from biblio_metadata

This patch updates the database update to populate the new
opac_suppressed field in the bilios table using the data in the
biblio_metadata table.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize (ashimema) 2025-03-14 16:32:12 UTC
Wouldn't be a bad idea for someone to have a final look at those follow-ups.
Comment 20 David Cook 2025-03-16 22:50:18 UTC
This is super exciting to see, but we've got a little problem in the QA follow-up, as it's using 952$n instead of 942$n. Marking Failed QA but hopefully will be in this status only for a short time.

I'll try to take a look at this again shortly (once I've gotten through the rest of my emails...)
Comment 21 David Cook 2025-03-16 22:53:39 UTC
(In reply to Martin Renvoize (ashimema) from comment #19)
> Wouldn't be a bad idea for someone to have a final look at those follow-ups.

I'll fix up the 952$n to 942$n... and I might do a different UPDATE since historically any true 942$n value was valid to trigger suppression. I'll have a think about that one.
Comment 22 David Cook 2025-03-16 23:29:09 UTC
(In reply to David Cook from comment #21)
> (In reply to Martin Renvoize (ashimema) from comment #19)
> > Wouldn't be a bad idea for someone to have a final look at those follow-ups.
> 
> I'll fix up the 952$n to 942$n... and I might do a different UPDATE since
> historically any true 942$n value was valid to trigger suppression. I'll
> have a think about that one.

I'm thinking about using boolean() in the Xpath.

The following returns 1 for any true value and should return 0 for any false value, so it'll return 1 for new value of 1 or old values like "YES" or "yes" or whatever. 0 will be returned for 0 or empty or non-existent.

IF(ExtractValue(metadata,'//datafield[@tag="942"]/subfield[@code="n" and boolean(text())]/text()'),1,0)
Comment 23 David Cook 2025-03-16 23:45:59 UTC
Keeping in mind bug 39337 it might be worthwhile adding an index on the opac_suppressed column.

That said, it might not be used by the query planner. I suppose that's something that we could explore later.