Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip all records with only suppressed holdings/items/portfolios. #2077

Closed
wants to merge 1 commit into from

Conversation

blackwinter
Copy link
Member

Mimics similar behaviour in DigiBib IntrOX (Limetrans; relevant Jira issues: DOX-1354, DOX-1568, DOX-1622).

If you don't want to exclude these records altogether, we'd have to apply this change to the inCollection statement for "DigiBib hbz Verbundkatalog" instead.

Mimics similar behaviour in DigiBib IntrOX (Limetrans; relevant Jira issues: DOX-1354, DOX-1568, DOX-1622).
@blackwinter
Copy link
Member Author

Do we maybe need more test cases?

@TobiasNx
Copy link
Contributor

Concerning the suppression I will get feedback. to you what we prefer.

I would have preferred two commits because one cannot relate the changes in the tests to the changes in the code so easily. I was confused that the reject mechanism resulted in the deletion of a single holding object. But then I realized that this was due to the new "suppression" mechanism for POR that was also commited.

@blackwinter
Copy link
Member Author

Concerning the suppression I will get feedback. to you what we prefer.

Okay, thanks.

I would have preferred two commits because one cannot relate the changes in the tests to the changes in the code so easily.

Which changes exactly would you have separated?

copy_field("$i.d", "$i.@sublocation")
replace_all("$i.@sublocation","https://hbz-network.userservices.exlibrisgroup.com/view/uresolver/49HBZ_NETWORK","")
do list(path:"$i.A", "var": "$j")
if none_equal("$i.b", "Not Available")
Copy link
Contributor

@TobiasNx TobiasNx Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional introduces something new. A suppression mechanism that was not there before.
Introduce this in a separate commit. In the second commit add the helper elements @suppressed and @unsuppressed and the rejection mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither were the other two. But you would prefer one commit with the holding/item changes (no test coverage) and one with the portfolio changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I understand. Sorry for being dense so early in the morning ;) You mean the suppression of the portfolio, not the suppression of the record; holdings and items were suppressed before, but the portfolio wasn't.

@blackwinter
Copy link
Member Author

So there are two decisions to be made:

  1. Should unavailable portfolios be suppressed?
  2. Should records with only suppressed holdings/items/portfolios be suppressed altogether, or only the "DigiBib hbz Verbundkatalog" collection?

I'll adjust the commits once you've made your decision.

@acka47
Copy link
Contributor

acka47 commented Sep 26, 2024

My first thoughts on this (after not much reflection): It makes sense to skip those records but we should make sure to not exclude records that we actually want in lobid. E.g. in NWBib, there are 52k resources that are not articles and do not have any holding information: inCollection.id:"http://lobid.org/resources/HT014176012#!" AND NOT _exists_:hasItem AND NOT type:Article

@blackwinter
Copy link
Member Author

do not have any holding information

Just to be clear: The proposed change only applies to records that only have suppressed holdings; records without any holdings aren't skipped (neither are records with both suppressed and unsuppressed holdings).

@TobiasNx
Copy link
Contributor

TobiasNx commented Sep 26, 2024

How do we ensure that these are not suppressed:
https://lobid.org/resources/search?q=hasItem.type%3A%22DigitalDocument%22+AND+%28hasItem.type%3A%22NurTitel%22+OR+hasItem.type%3A%22PhysikalischerTitel%22%29

And

https://lobid.org/resources/search?q=hasItem.type%3A%22PhysicalObject%22+AND+%28hasItem.type%3A%22NurTitel%22+OR+hasItem.type%3A%22PhysikalischerTitel%22%29

Because the supression marker is only set for ITM, H52 (PhysicalObject) and POR (DigitalDocument) , but not for the other types of holdings (NurTitel and PhysikalischerTitel)

@blackwinter
Copy link
Member Author

Because the supression marker is only set for ITM, H52 (PhysicalObject) and POR (DigitalDocument) , but not for the other types of holdings (NurTitel and PhysikalischerTitel)

You mean, e.g., what if a record has a suppressed H52 and a different HOL without an (unsuppressed) H52? That certainly complicates things.

I guess the safest option would be to limit the change to the "DigiBib hbz Verbundkatalog" collection (question 2 above). This way you don't have to suffer any unforeseen consequences.

blackwinter added a commit that referenced this pull request Sep 26, 2024
…DigiBib hbz Verbundkatalog" collection.

Mimics similar behaviour in DigiBib IntrOX (Limetrans; relevant Jira issues: DOX-1354, DOX-1568, DOX-1622).

See #2077 for alternate proposal (skip these records altogether).
@blackwinter
Copy link
Member Author

See #2078 for alternate proposal.

@acka47
Copy link
Contributor

acka47 commented Sep 26, 2024

Just to be clear: The proposed change only applies to records that only have suppressed holdings; records without any holdings aren't skipped (neither are records with both suppressed and unsuppressed holdings).

I understand. But NWBib titles might be affected by this, anyway, which we should check. However, it looks like we are going in the direction of removing the DigiBib collection statement so that we won't have to dig deeper into this.

@blackwinter
Copy link
Member Author

The discussion might not have been totally in vain, but we've decided not to go forward with this change (for the time being). Sorry for the noise, everyone.

(The examples which @eschackmann identified didn't match the use case we previously saw in IntrOX and weren't covered by the conditions implemented here.)

@blackwinter blackwinter deleted the skipSuppressedRecords branch September 26, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants