Just to add some extra context to this, one of the methods that EBSCO use to harvest data into EDS is OAI-PMH from Koha and some libraries don't want suppressed records going into EDS in the same way they don't want them in the OPAC.

Nice one.

This has been on my mind for a while. 

I think that we need a database column to store the 942$n data to make it easy to query via SQL.

OpacHiddenItems will still be an issue but that can be a separate issue...

I wondered if it was as simple as just running it through RecordProcessor.. but when I started to have a quick look at the code I couldn't wrap my head around it.. perhaps not as simple as it appears on the surface.

(In reply to Martin Renvoize from comment #4)
> I wondered if it was as simple as just running it through RecordProcessor..
> but when I started to have a quick look at the code I couldn't wrap my head
> around it.. perhaps not as simple as it appears on the surface.

I'd have to do a thorough double-check but I think we need to do the check at the SQL level.

(In reply to David Cook from comment #3)
> I think that we need a database column to store the 942$n data to make it
> easy to query via SQL.

I still think that this is the solution for this particular issue. 
 
> OpacHiddenItems will still be an issue but that can be a separate issue...

I actually just remembered that I actually have a solution for this! 

Check out the patch on bug 19704. I use SQL::Abstract to generate SQL based on OpacHiddenItems! 

(Note that the 942$n issue on that report is managed using ExtractValue() but it's not as efficient as just having a "biblio" or "biblioitems" column of "suppressed".)

(In reply to David Cook from comment #6)
> Check out the patch on bug 19704. I use SQL::Abstract to generate SQL based
> on OpacHiddenItems! 

Note that SQL::Abstract is a dependency of DBIx::Class, so it's pretty safe to rely on it. We could always add it to the cpanfile if folk thought it was necessary too.

Briefly looking at this again in the context of OpacHiddenItems... 

We'd need to either JOIN the items table to biblio_metadata OR we'd need to use a subquery. 

If the subquery were the same as the one used in the UNION, I wonder if it would just cache the 2nd run of the same query. That would be interesting to benchmark...

(In reply to Martin Renvoize from comment #4)
> I wondered if it was as simple as just running it through RecordProcessor..
> but when I started to have a quick look at the code I couldn't wrap my head
> around it.. perhaps not as simple as it appears on the surface.

Actually, looking at this again, I think running it through RecordProcessor might work. It would be *slow*, but in theory it should work.

The OAI-PMH feed is just a stream of records that we break up using resumption tokens. 

I suppose if OpacHiddenItems is set and OpacHiddenItemsHidesRecord is set to "Don't hide", then the timestamp for records could reflect changes to items that aren't visible in the OAI-PMH feed, but... that is a minor detail and doesn't look avoidable without making SQL changes.

I'm going to email Ere and see what he thinks about this, since he's been behind the latest and greatest OAI-PMH server changes.

Ah... but the $order_limit specifies a LIMIT which would be a problem if we were filtering out records using RecordProcessor...

That said, we could move the record fetch into a function and run it again if the $count hadn't hit the $max. That would make the response even slower than it already would be using RecordProcessor though...

But I can't think of a simple way to support "OpacHiddenItemsHidesRecord: Hide" except by post-processing outside the SQL.

--

Another thought I had was that maybe we do need to store "hidden_in_opac" in the "biblio" and "items" DB table records. 

If someone updates "OpacHiddenItems", then they need to do a "touch all items". 

In theory, this is a system preference that shouldn't be frequently changed anyway.

One downside of hiding records/items in the OAI-PMH is that if you turn on your hiding after you've already harvested a Koha DB, then you'll never get the update...

In theory, maybe it makes sense to mark a bib record as deleted - if it's been marked as hidden. 

That might not be compatible with the system preference "OpacSuppressionRedirect" though. If that syspref says "404" and OAI-PMH says "deleted"... that means that the record did once exist...

And since deleted records are shown first and then existent records are shown second, a savvy consumer of OAI-PMH feeds could use it to infer that a record has been hidden rather than deleted...

--

But it's an interesting idea. When I harvest records from Koha using plugins, I'll return "deleted" when a record is "hidden", as that allows newly hidden records/items to be removed from the third-party system automatically. 

--

I suppose that's only really relevant for supporting "OpacSuppression" and "OpacHiddenItemsHidesRecord: Hide"

Technically, "OpacHiddenItems" could be supported now (with the only information leaking being based on update timestamps for hidden items but technically a record could have its timestamp updated with zero bib metadata changed so that's not necessarily revealing).

I've opened bug 31725 to address the hiding of items using OpacHiddenItems.

And it's got me thinking that the "deleted" status might be the way to go for records hidden using OpacSuppression or OpacHiddenItems.

It's not a 100% perfect solution, but I think it could be a "good enough" solution.

(In reply to David Cook from comment #12)
> I've opened bug 31725 to address the hiding of items using OpacHiddenItems.
> 
> And it's got me thinking that the "deleted" status might be the way to go
> for records hidden using OpacSuppression or OpacHiddenItems.
> 
> It's not a 100% perfect solution, but I think it could be a "good enough"
> solution.

I think it's the only solution that would work without forementioned side-effects. It's important to report the records as deleted particularly since they might not have been suppressed before. And that'd be the case for practically every vanilla Koha instance harvested with OAI-PMH up until this change.

That said, there are use cases where OAI-PMH is used for non-opac functionality and suppressed records should be included just like before. For flexibility it should be possible to configure suppression per OAI-PMH set in addition to a default setting.

(In reply to Ere Maijala from comment #13)
> I think it's the only solution that would work without forementioned
> side-effects. It's important to report the records as deleted particularly
> since they might not have been suppressed before. And that'd be the case for
> practically every vanilla Koha instance harvested with OAI-PMH up until this
> change.

I'm glad to hear that you agree!
 
> That said, there are use cases where OAI-PMH is used for non-opac
> functionality and suppressed records should be included just like before.

Do you have some use cases in mind?

> For flexibility it should be possible to configure suppression per OAI-PMH
> set in addition to a default setting.

GetRecord isn't affected by sets, so are you thinking of a flag in the "oai_sets" data that would disable the suppression/hiding of records?

--

I'm planning to trial this hiding locally by using suppression/hiding by default, and then using a system preference to disable it (to cater for any use cases that come up where libraries might need the unfiltered records still).

So it turns out the OAI-PMH already hides items using OpacHiddenItems:
- Koha::OAI::Server::Repository->get_biblio_marcxml()
- C4::Biblio::GetMarcBiblio
- C4::Biblio::EmbedItemsInMarcBiblio

To avoid double-handling and performance penalties for determining record-level suppression using OpacHiddenItems, it would make sense for C4::Biblio::GetMarcBiblio to return null on a hidden record, and for C4::Biblio::EmbedItemsInMarcBiblio to return a "hide_bib" status if it's hidden all the items via OpacHiddenItems.

--

Of course, C4::Biblio::GetMarcBiblio is a very entrenched function, so I don't think we can alter default behaviour. I'm thinking that we add a parameter like "suppression".

This parameter would be an empty hash reference, which can be checked after C4::Biblio::GetMarcBiblio is called without affecting its return values. 

If OpacSuppression isn't turned on or isn't applicable, then the "suppression" gets passed to C4::Biblio::EmbedItemsInMarcBiblio as well. 

If the "suppression" returns a "hide_biblio" after that call, then C4::Biblio::GetMarcBiblio returns null.

All right... that wasn't so bad in the end.

A bit of code in C4::Biblio::GetMarcBiblio, C4::Biblio::EmbedItemsInMarcBiblio, Koha::OAI::Server::GetRecord->new, and Koha::OAI::Server::Repository->get_biblio_marcxml

--

There are no new database calls (only syspref calls to cache), so the only overhead added is CPU time and that will be negligible.

--

One could argue that there are alternatives to my hashref parameter idea, but it works.

I'll likely be implementing this locally in 21.11 tomorrow night. 

--

I've run out of time today but it shouldn't be a big drama to add unit tests (at least for the C4::Biblio code).

I ended up having to add more disparate lines of code than I would've liked, so I will be keen to upstream this one. 

The Koha::OAI::Server::GetRecord change could actually be a separate bug report, since that change isn't contingent on bug 31161.

(In reply to David Cook from comment #14)
> (In reply to Ere Maijala from comment #13)
> > I think it's the only solution that would work without forementioned
> > side-effects. It's important to report the records as deleted particularly
> > since they might not have been suppressed before. And that'd be the case for
> > practically every vanilla Koha instance harvested with OAI-PMH up until this
> > change.
> 
> I'm glad to hear that you agree!
>  
> > That said, there are use cases where OAI-PMH is used for non-opac
> > functionality and suppressed records should be included just like before.
> 
> Do you have some use cases in mind?

Anything non-OPAC could qualify, e.g. a union catalog based on OAI-PMH. Or even an OPAC instance for staff use only.

> > For flexibility it should be possible to configure suppression per OAI-PMH
> > set in addition to a default setting.
> 
> GetRecord isn't affected by sets, so are you thinking of a flag in the
> "oai_sets" data that would disable the suppression/hiding of records?

GetRecord response has header/setSpec to indicate which sets the record belongs to. It should reflect suppression by excluding a set where the record would be marked deleted. So if we have sets "all" and "opac", a suppressed record would only have <header><setSpec>all</setSpec></header>.

(In reply to Ere Maijala from comment #18)
> Anything non-OPAC could qualify, e.g. a union catalog based on OAI-PMH. Or
> even an OPAC instance for staff use only.

I was thinking about it again this morning and thinking that it could be any non-public system. 

> > > For flexibility it should be possible to configure suppression per OAI-PMH
> > > set in addition to a default setting.
> > 
> > GetRecord isn't affected by sets, so are you thinking of a flag in the
> > "oai_sets" data that would disable the suppression/hiding of records?
> 
> GetRecord response has header/setSpec to indicate which sets the record
> belongs to. It should reflect suppression by excluding a set where the
> record would be marked deleted. So if we have sets "all" and "opac", a
> suppressed record would only have <header><setSpec>all</setSpec></header>.

I was thinking that a suppressed record wouldn't be retrievable at all by GetRecord if it's suppressed/hidden by the OPAC business rules.

We have libraries who don't want records to leave Koha or be exposed in any public capacity if they're suppressed/hidden, and I think that makes sense.

--

An alternative could be to have public and private OAI-PMH endpoints but that wouldn't necessarily be backwards compatible with all harvesting systems... 

Now that I think about it, we do have non-Koha systems that have public and private OAI-PMH endpoints. It would probably be the cleanest solution, but it would involve work to transition.

Since Koha tries not to change behaviour with upgrades, I suppose the oai.pl would keep the current behaviour, and "public/oai.pl" or "public_oai.pl" would be the endpoint that doesn't send suppressed/hidden record metadata.

--

I suppose there could also be a few system preference options regarding record suppression/hiding. There could be a "full block" where records are suppressed/hidden regardless. Then there could be a "partial block" where records are retrievable by default, unless the "set" says not to block it. And then "no block" would be showing records regardless.

(Maybe "no block" should also turn off the current "OpacHiddenItem" handling too then?)

*** Bug 19064 has been marked as a duplicate of this bug. ***

Can we add 'supressed' and 'supressed_date' to the biblio table and make our high level methods take care of syncing things?

That would make it easy to build a performant query for identifying (to be marked as) deleted records in OAI.

(In reply to Tomás Cohen Arazi from comment #21)
> Can we add 'supressed' and 'supressed_date' to the biblio table and make our
> high level methods take care of syncing things?

Can you elaborate on that?

OpacSuppression could work with that, but I don't know how you'd make it work with OpacHiddenItems with OpacHiddenItemsHidesRecord. (Unless you require people to touch_all_biblios.pm after changing OpacHiddenItems and OpacHiddenItemsHidesRecord.)
 
> That would make it easy to build a performant query for identifying (to be
> marked as) deleted records in OAI.

That could be interesting. It would need to be done via a join or a subquery, but it would still probably be fairly performant.

Just realized that while my changes work well for GetRecord and ListRecords, they don't for ListIdentifiers.

For that, we'll have to fetch the metadata in order to know whether or not it needs to be suppressed/hidden.

That is unless we do the analysis at the SQL level.

(In reply to David Cook from comment #23)
> Just realized that while my changes work well for GetRecord and ListRecords,
> they don't for ListIdentifiers.
> 
> For that, we'll have to fetch the metadata in order to know whether or not
> it needs to be suppressed/hidden.
> 
> That is unless we do the analysis at the SQL level.

Ugh, and even fetching the metadata doesn't work for OpacHiddenItems unless that metadata format includes items. 

This is so frustrating.

(In reply to David Cook from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
> > Can we add 'supressed' and 'supressed_date' to the biblio table and make our
> > high level methods take care of syncing things?

I think we need to bite the bullet and store "suppressed" when "OpacSuppression" and "OpacHiddenItemsHidesRecord" are set, and just add the caveat that any changes to these settings will require a data regeneration. 

Doing the calculation 1 time and then just working off the stored state will make things so much easier across the board.

Plus, we could index that state, so that "OpacHiddenItemsHidesRecord" actually hides the record in the Zebra results as well, which would lead to better search result quality.

For now, I will proceed with my patches to solve some immediate local problems, but they're probably not worth upstreaming as they don't comprehensively solve the problem.

For that, I think we need to change how we handle OpacSuppression and OpacHiddenItems and to use SQL for the suppression/hidden lookup.

The thing is that an OAI-PMH provider should really have a publishing process that handles everything needed to publish a set of records for harvesting and keep it up to date. This would make it possible to:

1. Publish a set once in a format that makes harvesting blazing fast.
2. Update the set incrementally with changes in bibs and items.
3. Re-publish all if suppression rules change.
4. Take care of deletions when they affect the records that have been published in the set. If a record has never been a part of a set, it doesn't need to be returned even as deleted.
5. Ensure that a set doesn't change while harvesting (requires set versioning).

The obvious downside is that a publishing process is relatively heavy, and keeping sets up to date in a near-real-time manner is more difficult. Also sets published this way would require more database space.


--- 
Now to more practical stuff. The multiple endpoints idea works for me. I don't think there's any other interoperable way if you need to have different rules. You could also have several endpoints like 'internal/oai.pl', 'opac/oai.pl' and 'test/oai.pl', no problem. From the protocol point of view they'd just be separate providers. However, they should also have separate OAI identifiers. Things could go really bad if a harvester receives the same record from multiple endpoints.

(In reply to Ere Maijala from comment #27)
> Now to more practical stuff. The multiple endpoints idea works for me. I
> don't think there's any other interoperable way if you need to have
> different rules. You could also have several endpoints like
> 'internal/oai.pl', 'opac/oai.pl' and 'test/oai.pl', no problem. From the
> protocol point of view they'd just be separate providers. However, they
> should also have separate OAI identifiers. Things could go really bad if a
> harvester receives the same record from multiple endpoints.

According to the OAI spec, OAI identifiers are only unique within their repository/0provider, but I don't think a lot of OAI harvesters adhere to the OAI spec in that regard... so that's a good point.

(In reply to David Cook from comment #28)
> According to the OAI spec, OAI identifiers are only unique within their
> repository/0provider, but I don't think a lot of OAI harvesters adhere to
> the OAI spec in that regard... so that's a good point.

I'll soften my stance a bit. I suppose it boils down to whether two endpoints are considered the same repository. It might also be enough to just document that one shouldn't harvest from multiple endpoints like they were the same repository.

(In reply to Ere Maijala from comment #29)
> I'll soften my stance a bit. I suppose it boils down to whether two
> endpoints are considered the same repository. It might also be enough to
> just document that one shouldn't harvest from multiple endpoints like they
> were the same repository.

Yeah, I think documenting could be enough. We could even have separate system preferences. 

On the non-Koha system with private/public endpoints, the private one is restricted by additional security and used for internal projects. The public one is just open to the public, so very different consumers.

But we also set that up with those separate endpoints from the initial launch. 

With Koha, it'll be harder to transition for existing consumers of the OAI-PMH... 

That said, people looking for support for suppressed/hidden records, they'll probably be doing it intentionally and thus be able to coordinate a migration to a new public endpoint more easily...

FYI on October 12th I updated our OAI-PMH servers to honour OpacSuppression and OpacHiddenItems, so we've been running it in production for nearly a month. 

So far so good.

Any chance of sharing those patches David?

(In reply to Martin Renvoize from comment #32)
> Any chance of sharing those patches David?

They're a bit hacky but I could certainly share them.

Looks like Koha has changed quite a bit since I wrote those patches so I'll need to refactor them anyway...

Hoping to look at this again in August. 

I took a look at my local code in prod, and it doesn't require a ton of code changes. Mostly just a case of sitting down, porting the code, and testing it works. Writing tests of course.

Created attachment 170280 [details] [review]
Bug 31161: Suppress OAI records when all items hidden by OpacHiddenItems

This patch set adds an option to Koha::Metadata::Record to suppress a record if all items are
hidden. This option is only enabled by OAI and only when  OpacHiddenItemsHidesRecord is enabled.

This test plan assumes KTD default data
To setup:

edit/create /etc/koha/sites/kohadev/oai.conf with content below:
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

Set sysprefs:
OAI-PMH: Enable
OAI-PMH:ConfFile: /etc/koha/sites/kohadev/oai.conf
OAI-PMH:AutoUpdateSets: Enable
OAI-PMH:AutoUpdateSetsEmbedItemData: Enabled

OpacHiddenItems:
itype: [BK]
OpacHiddenItemsHidesRecord: Hide

Adminsitration->OAI set configuration->New set
Choose anything for setSpec and setName
On new set: Actrion->define mappings
Field: 942
Subfield: c
Operator: is equal to
Value: MU

To test:
1 - Set above, do not apply patch, restart_all
2 - Visit: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
3 - Confirm record 1 is visible wit no items (or anothe record that has BK itemtypes only)
4 - perl misc/migration_tools/build_oai_sets.pl -r -v -i
5 - Confirm 11 records in set created above (or correct number for your site)
6 - Apply patches, restart_all
7 - Repeat 2, note "Results fetched" is 43-50, count is off because of hidden records
8 - Confirm record 1 and many others not shown
9 - Confirm 'Show more' works
10 - Repeat 4, still 11
11 - Find a record with record type music, say 315
12 - Edit all items and change type to BK - Books
13 - Repeat 4 - now 10 records
14 - Set pref OpacHiddenItemsHidesRecord: Don't hide
15 - Repeat 2 and 4 and confirm results as before patch

We will clearly need tests here, however, the fact that we now respect OpacHiddenItems makes this a bit more urgent - in the past outside systems could use the filtering to determine if a record should be shown, essentially implementing their own OpacHiddenItems - but now the items don't come through and we cannot externally determine if a record truly has no items, or if all items have been hidden.

Marking needs signoff as I need feedback on the approach and assumptions about following 'OpacHiddenItemHidesRecord'

(In reply to Nick Clemens (kidclamp) from comment #37)
> We will clearly need tests here, however, the fact that we now respect
> OpacHiddenItems makes this a bit more urgent - in the past outside systems
> could use the filtering to determine if a record should be shown,
> essentially implementing their own OpacHiddenItems - but now the items don't
> come through and we cannot externally determine if a record truly has no
> items, or if all items have been hidden.
> 
> Marking needs signoff as I need feedback on the approach and assumptions
> about following 'OpacHiddenItemHidesRecord'

I think that I might like my approach better. If I understand this patch correctly, if a record gets harvested by a consumer, then is hidden in Koha, the record will just disappear from Koha's OAI-PMH interface, so it'll still exist in the consumer. 

I should be able to look at my local code today, so I'll look at attaching an alternate patch. (I'll also test this one, because maybe I am misunderstanding this current patch.)

I'm testing this patch using a modified test plan. 

My OpacHiddenItems:
barcode: [3999900000001]

My Set:
952$p = 3999900000001

--

http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
At the top it says "Results fetched 2-50" instead of "Results fetched 1-50". And yeah only 49 records appear on the page.

The second page says "51 - 100" which is good, and the ID for the top record is KOHA-OAI-TEST:49, so the resumption token seems to be working correctly. 

http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml&set=AAA

This shows 0 records. 

I suppose the advantage here is that the consumer doesn't know the record is hidden. 

But the disadvantage is that the consumer doesn't know the record is hidden, so it can't hide the record on the consumer end if it's already consumed that Identifier previously...

That said, the patch seems to work as described. 

I'm not too fussed about the "Results fetched 2-50" thing as that's just a human label, and a OAI-PMH consumer shouldn't care that it's only getting 49 records instead of 50.

I'm going to look at porting over my local customization and probably will add it as an alternate patch.

For now I'll sign off this one though, as I do think it's better than the status quo.

Actually, I'll mark it "Failed QA" as it's failing the QA tools for tidiness:

OK     Koha/Biblio/Metadata.pm

 WARN   Koha/OAI/Server/ListBase.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 40, now: 42)

 WARN   Koha/OAI/Server/Repository.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 35, now: 37)

 WARN   misc/migration_tools/build_oai_sets.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 33, now: 37)

(In reply to David Cook from comment #38)
> (In reply to Nick Clemens (kidclamp) from comment #37)
> > We will clearly need tests here, however, the fact that we now respect
> > OpacHiddenItems makes this a bit more urgent - in the past outside systems
> > could use the filtering to determine if a record should be shown,
> > essentially implementing their own OpacHiddenItems - but now the items don't
> > come through and we cannot externally determine if a record truly has no
> > items, or if all items have been hidden.
> > 
> > Marking needs signoff as I need feedback on the approach and assumptions
> > about following 'OpacHiddenItemHidesRecord'
> 
> I think that I might like my approach better. If I understand this patch
> correctly, if a record gets harvested by a consumer, then is hidden in Koha,
> the record will just disappear from Koha's OAI-PMH interface, so it'll still
> exist in the consumer. 
> 
> I should be able to look at my local code today, so I'll look at attaching
> an alternate patch. (I'll also test this one, because maybe I am
> misunderstanding this current patch.)

Yeah, I had assumed if you change hiding rules you would want to do a reharvest to catch any that were hidden and no aren't, or vice versa. This actually makes me think the approach of storing the suppressed status at record edit (calculated via a background job) might be better - pre-calculating for search engine and OAI - really you could have OAI fetch via a scroll from ES - though  not sure if ES supports resumption - ignore me, ranting

I am not sold on these patches, I would love to see the direction you went, this was just the smallest cut along the lines of what you mentioned - determine when fetching record - it's like what we do in the opac - fetch from search engine, then filter.

If you don't get to upload your patches I would still be interested just to hear how you approached it. I did consider here returning the hidden records as 'deleted'- it would have been even less code, but that felt like we still told them something was there - rather than full suppress the existence.

(In reply to Nick Clemens (kidclamp) from comment #42)
> If you don't get to upload your patches I would still be interested just to
> hear how you approached it. I did consider here returning the hidden records
> as 'deleted'- it would have been even less code, but that felt like we still
> told them something was there - rather than full suppress the existence.

I'll be posting a patch soon :)

And yeah I was thinking about that as well. Although someone enumerating bibs via the web UI would notice gaps in biblionumbers returning records as well.

With my patch, a discerning consumer would also notice that most deleted records are at the start of the feed and that it was a bit weird for there to be deleted records elsewhere in the feed. 

I've been running my code in prod for a while and my libraries are happy, but they're just concerned about display/visibility of the records. Not the supreme/total hiding of the existence of that record at all. (But I know there are libraries out there that do care about that.)

Btw, I need to double-check, but I think in your patch you're still showing the records with ListIdentifiers? Your change was just for ListRecord in ListBase.pm

(In reply to David Cook from comment #44)
> Btw, I need to double-check, but I think in your patch you're still showing
> the records with ListIdentifiers? Your change was just for ListRecord in
> ListBase.pm

Yeah, the record is still showing in ListIdentifiers in your patch.

(Although I've found a bug in my code for ListIdentifiers too lol)

Created attachment 170301 [details] [review]
Bug 31161: [Alternate patch] Mark hidden records as deleted

If a bib record is hidden by OpacHiddenItems,
mark it as deleted in the OAI-PMH, so that it's metadata
is not consumed in a new OAI-PMH harvest, and it's marked
for removal if the metadata was consumed in previous harvested.

Test plan:
0. Setup
vi /etc/koha/sites/kohadev/oai.conf
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

Set sysprefs:
OAI-PMH: Enable
OAI-PMH:ConfFile: /etc/koha/sites/kohadev/oai.conf
OAI-PMH:AutoUpdateSets: Enable
OAI-PMH:AutoUpdateSetsEmbedItemData: Enabled

OpacHiddenItems:
barcode: [999900000001]
OpacHiddenItemsHidesRecord: Hide

Adminsitration->OAI set configuration->New set
Choose anything for setSpec and setName
On new set: Actrion->define mappings
Field: 942
Subfield: p
Operator: is equal to
Value: 999900000001

perl misc/migration_tools/build_oai_sets.pl -r -v -i

1. koha-plack --restart kohadev
2. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
3. Confirm KOHA-OAI-TEST:1 is marked as deleted and not metadata visible
4. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListIdentifiers&metadataPrefix=marcxml
5. Confirm KOHA-OAI-TEST:1 is marked as deleted
6. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:1
7. Confirm record is marked as deleted and no metadata visible
8. View Sets and confirm the record appears in the set but as deleted record
9. Change OpacHiddenItemsHidesRecord syspref to "Don't hide" and repeat above steps
10. Confirm that the record metadata is now visible and the record is not marked as deleted

Tempted to move this to "In discussion" but then I know that's where bugs go to die...

Jonathan: do you have an opinion which you'd like better?

Once Nick fixes the ListIdentifiers and tidy, I think I'd be fine with either approach.

I do think mine requires fewer interventions. For instance, I think a lot of libraries would change OpacHiddenItems and not think to redo their OAI-PMH harvests across all consumers. It's a lot of friction. 

If we did go that route, I think we'd need to update the description for OpacHiddenItems to add a warning that they'll need to advise their OAI-PMH consumers to re-harvest all records each time it's changed.

(In reply to Nick Clemens (kidclamp) from comment #42)
> This
> actually makes me think the approach of storing the suppressed status at
> record edit (calculated via a background job) might be better -
> pre-calculating for search engine and OAI - really you could have OAI fetch
> via a scroll from ES - though  not sure if ES supports resumption - ignore
> me, ranting

I do like the idea of a suppressed status used by Search and OAI. Like you say, we could store it at record edit. (We'd also need to trigger a re-calculation if OpacHiddenItems changed. A clever way to do that would be to do a diff of the before and after, and just update the records that were affected...)

I was actually working on some changes for OAI for DSpace recently, and I was reminded how they index records into a Solr "oai" index specifically. I'd have to double-check but I don't recall if it's the full metadata or just the identifier. But I like the idea. It would be so much more performant than re-doing the database queries for every request like we do with Koha...

Jonathan: Turns out the key to getting me to post my patches is for someone else to post a different patch first ﷐[U+1F923]﷑

I am good with the base of David's patch, it's much simpler, a couple concerns:

Hidden in opac in Koha Biblio doesn't check OpacHiddenItemsHidesRecord, it should. It should also use filter_by_visible rather than looping items, just for simplicity. It should also check if record is suppressed as we're only catching hidden item hiding this way.

Neither of our patches actually triggers a record change, so partial harvests (changes since) won't pickup the difference.

(In reply to David Cook from comment #48)
> Tempted to move this to "In discussion" but then I know that's where bugs go
> to die...
> 
> Jonathan: do you have an opinion which you'd like better?
> 
> Once Nick fixes the ListIdentifiers and tidy, I think I'd be fine with
> either approach.
> 
> I do think mine requires fewer interventions. For instance, I think a lot of
> libraries would change OpacHiddenItems and not think to redo their OAI-PMH
> harvests across all consumers. It's a lot of friction. 
> 
> If we did go that route, I think we'd need to update the description for
> OpacHiddenItems to add a warning that they'll need to advise their OAI-PMH
> consumers to re-harvest all records each time it's changed.

I think I'm also fine with either approach. I agree with Nick that, if you were to change opachiddenitems, it wouldn't be unreasonable to expect to do a complete re-harvest. Certainly if EDS was going to be populated from a MARC dump, that's the approach you would take. 

Having said that, I think marking them as deleted is marginally better in that it's very clear that they are not wanted and it would be easy to spot them and also the consumer could easily act on that too. 

Happy to test and sign-off either approach.

(In reply to Nick Clemens (kidclamp) from comment #51)
> Hidden in opac in Koha Biblio doesn't check OpacHiddenItemsHidesRecord, it
> should. It should also use filter_by_visible rather than looping items, just
> for simplicity. It should also check if record is suppressed as we're only
> catching hidden item hiding this way.

Except that Koha::Biblio->hidden_in_opac does check OpacHiddenItemsHidesRecord ;). 

Good point about filter_by_visible_in_opac. It's a good idea not only because of simplicity but also performance. I think that would be an optimization though.

Funny story. In my local version, I do look for OpacSuppression as well. I didn't include it here, because I figured it was out of scope haha. I was going to do it as a follow-up.

> Neither of our patches actually triggers a record change, so partial
> harvests (changes since) won't pickup the difference.

Hmm touché. I suppose that means that the deleted record would show up in subsequent changes... but also all subsequent changes. Hmm...

(In reply to Jonathan Field from comment #52)
> Happy to test and sign-off either approach.

I suppose you could test both and then leave it up to QA?

(In reply to David Cook from comment #53)
> (In reply to Nick Clemens (kidclamp) from comment #51)
> > Neither of our patches actually triggers a record change, so partial
> > harvests (changes since) won't pickup the difference.
> 
> Hmm touché. I suppose that means that the deleted record would show up in
> subsequent changes... but also all subsequent changes. Hmm...

Just as a sidenote: I believe the proper way around this would be a publishing process that would periodically publish a new set of records. It could compare the new set against the previous one and add any records no longer available as deleted (it would of course need to carry any previously deleted records over as well, but it could keep their old timestamps). The publishing process would be more complicated, but then the actual OAI-PMH provider (replies to harvesting requests) could be really simple and fast.

This would have the additional benefit that the set of records being harvested would never change in the middle of the harvest.

Both patches look nice. I'm going to test and (hopefully) sign off both, though to follow on from what Jonathan said, I do think the reduction in friction that comes from marking the records as deleted puts that approach marginally ahead in terms of what our customers have asked for.

More discussion would be cool..!

Created attachment 170353 [details] [review]
Bug 31161: Suppress OAI records when all items hidden by OpacHiddenItems

This patch set adds an option to Koha::Metadata::Record to suppress a record if all items are
hidden. This option is only enabled by OAI and only when  OpacHiddenItemsHidesRecord is enabled.

This test plan assumes KTD default data
To setup:

edit/create /etc/koha/sites/kohadev/oai.conf with content below:
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

Set sysprefs:
OAI-PMH: Enable
OAI-PMH:ConfFile: /etc/koha/sites/kohadev/oai.conf
OAI-PMH:AutoUpdateSets: Enable
OAI-PMH:AutoUpdateSetsEmbedItemData: Enabled

OpacHiddenItems:
itype: [BK]
OpacHiddenItemsHidesRecord: Hide

Adminsitration->OAI set configuration->New set
Choose anything for setSpec and setName
On new set: Actrion->define mappings
Field: 942
Subfield: c
Operator: is equal to
Value: MU

To test:
1 - Set above, do not apply patch, restart_all
2 - Visit: http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
3 - Confirm record 1 is visible wit no items (or anothe record that has BK itemtypes only)
4 - perl misc/migration_tools/build_oai_sets.pl -r -v -i
5 - Confirm 11 records in set created above (or correct number for your site)
6 - Apply patches, restart_all
7 - Repeat 2, note "Results fetched" is 43-50, count is off because of hidden records
8 - Confirm record 1 and many others not shown
9 - Confirm 'Show more' works
10 - Repeat 4, still 11
11 - Find a record with record type music, say 315
12 - Edit all items and change type to BK - Books
13 - Repeat 4 - now 10 records
14 - Set pref OpacHiddenItemsHidesRecord: Don't hide
15 - Repeat 2 and 4 and confirm results as before patch

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>

Signed off Nick's patch — I noticed the range of records surfaced was different from in the test plan, however, I verified manually that the range was still correct. 

Just something for QA to be aware of.

Created attachment 170354 [details] [review]
Bug 31161: [Alternate patch] Mark hidden records as deleted

If a bib record is hidden by OpacHiddenItems,
mark it as deleted in the OAI-PMH, so that it's metadata
is not consumed in a new OAI-PMH harvest, and it's marked
for removal if the metadata was consumed in previous harvested.

Test plan:
0. Setup
vi /etc/koha/sites/kohadev/oai.conf
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

Set sysprefs:
OAI-PMH: Enable
OAI-PMH:ConfFile: /etc/koha/sites/kohadev/oai.conf
OAI-PMH:AutoUpdateSets: Enable
OAI-PMH:AutoUpdateSetsEmbedItemData: Enabled

OpacHiddenItems:
barcode: [3999900000001]
OpacHiddenItemsHidesRecord: Hide

Adminsitration->OAI set configuration->New set
Choose anything for setSpec and setName
On new set: Actrion->define mappings
Field: 952
Subfield: p
Operator: is equal to
Value: 3999900000001

perl misc/migration_tools/build_oai_sets.pl -r -v -i

1. koha-plack --restart kohadev
2. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
3. Confirm KOHA-OAI-TEST:1 is marked as deleted and not metadata visible
4. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListIdentifiers&metadataPrefix=marcxml
5. Confirm KOHA-OAI-TEST:1 is marked as deleted
6. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:1
7. Confirm record is marked as deleted and no metadata visible
8. View Sets and confirm the record appears in the set but as deleted record
9. Change OpacHiddenItemsHidesRecord syspref to "Don't hide" and repeat above steps
10. Confirm that the record metadata is now visible and the record is not marked as deleted

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>

Also signed off David's patch. A few observations:

1)  I had to update the barcode from 999900000001 to 3999900000001 — I think this was probably a typo, as 3999900000001 is the barcode for bib #1
2)  I also had to update the OAI set MARC field from 942 to 952, because that's where the item data is kept. 942$p didn't work.


I also note here to confirm that OpacHiddenItemsHidesRecord and OpacHiddenItems are both honoured nicely in this patch. I had a little play, and when OpacHiddenItemsHidesRecord is false, but something is still present in OpacHiddenItems, the 952 is completely omitted for matching items, but the record still surfaces. This is exactly what I'd expect, and would say the fact that is honoured makes this my preferred patch.

Signed off, with thanks to David and Nick for their hard work on this :-) 

Jake

(In reply to Jake Deery from comment #60)
> 1)  I had to update the barcode from 999900000001 to 3999900000001 — I think
> this was probably a typo, as 3999900000001 is the barcode for bib #1
> 2)  I also had to update the OAI set MARC field from 942 to 952, because
> that's where the item data is kept. 942$p didn't work.

Ooops sorry!

--

Jake, did you test ListIdentifiers with Nick's patch? I don't think it's been fixed yet. I don't want to Fail QA, but could you take a look at that one?

Well spotted, David — ListIdentifiers doesn't work in Nick's patch. I've also noticed when you go to the GetRecord verb for record #1 (e.g.), you get a 500 page:

Empty String at /kohadevbox/koha/Koha/OAI/Server/Record.pm line 63.
 at /usr/lib/aarch64-linux-gnu/perl-base/Carp.pm line 289

So I deffo think Nick's patch would fail QA. I guess the question is, do we want to mark as failed QA, or do we want to obsolete that patch and go with David's?


Discuss!!

(In reply to Jake Deery from comment #62)
> Discuss!!

I think Nick was out of town for a few days, so may as well wait for him to send an updated patch?

(In reply to David Cook from comment #63)
> (In reply to Jake Deery from comment #62)
> > Discuss!!
> 
> I think Nick was out of town for a few days, so may as well wait for him to
> send an updated patch?

I think we can move forward with David's patch - mine has a number of issues (only works when items are included, some pages not handled) 

I will obsolete mine

David, looks like you have a #FIXME and a #TODO - are those to be handled here?

(In reply to Nick Clemens (kidclamp) from comment #65)
> David, looks like you have a #FIXME and a #TODO - are those to be handled
> here?

The #TODO will be a follow-up once this is in. I can open it now and just put a "Depends on".

The "FIXME" is more a question to QAers/RM. 

Typically we try not to change existing behaviour with new changes, so it makes me wonder if we would want to syspref it. 

Locally, we have a syspref around it, which lets instances opt out of this behaviour, but I don't think I've had anyone opt out of it. Everyone has wanted it.

(In reply to David Cook from comment #66)
> The #TODO will be a follow-up once this is in. I can open it now and just
> put a "Depends on".

See Bug 37713.

> The "FIXME" is more a question to QAers/RM. 
> 
> Typically we try not to change existing behaviour with new changes, so it
> makes me wonder if we would want to syspref it. 
> 
> Locally, we have a syspref around it, which lets instances opt out of this
> behaviour, but I don't think I've had anyone opt out of it. Everyone has
> wanted it.

Personally, I don't think it needs a syspref, but the question is out there. Happy for a QA followup to just remove that FIXME.

Created attachment 170632 [details] [review]
Bug 31161: Mark hidden records as deleted

If a bib record is hidden by OpacHiddenItems,
mark it as deleted in the OAI-PMH, so that it's metadata
is not consumed in a new OAI-PMH harvest, and it's marked
for removal if the metadata was consumed in previous harvested.

Test plan:
0. Setup
vi /etc/koha/sites/kohadev/oai.conf
format:
  marcxml:
    metadataPrefix: marcxml
    metadataNamespace: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim
    schema: http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd
    include_items: 1

Set sysprefs:
OAI-PMH: Enable
OAI-PMH:ConfFile: /etc/koha/sites/kohadev/oai.conf
OAI-PMH:AutoUpdateSets: Enable
OAI-PMH:AutoUpdateSetsEmbedItemData: Enabled

OpacHiddenItems:
barcode: [3999900000001]
OpacHiddenItemsHidesRecord: Hide

Adminsitration->OAI set configuration->New set
Choose anything for setSpec and setName
On new set: Actrion->define mappings
Field: 952
Subfield: p
Operator: is equal to
Value: 3999900000001

perl misc/migration_tools/build_oai_sets.pl -r -v -i

1. koha-plack --restart kohadev
2. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListRecords&metadataPrefix=marcxml
3. Confirm KOHA-OAI-TEST:1 is marked as deleted and not metadata visible
4. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=ListIdentifiers&metadataPrefix=marcxml
5. Confirm KOHA-OAI-TEST:1 is marked as deleted
6. Go to http://localhost:8080/cgi-bin/koha/oai.pl?verb=GetRecord&metadataPrefix=marcxml&identifier=KOHA-OAI-TEST:1
7. Confirm record is marked as deleted and no metadata visible
8. View Sets and confirm the record appears in the set but as deleted record
9. Change OpacHiddenItemsHidesRecord syspref to "Don't hide" and repeat above steps
10. Confirm that the record metadata is now visible and the record is not marked as deleted

Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 170633 [details] [review]
Bug 31161: (QA follow-up) Remove FIXME

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Since there was a bit of back and forth on this one, please detail/summarize in release notes!

Please add unit tests.

Created attachment 171074 [details] [review]
Bug 31161: Unit tests

Looking at the code of this I was missing a reference to OpacHiddenItemsHidesRecord, but none the less... it works.

Pushed for 24.11!

Well done everyone, thank you!