Skip to content

Conversation

@aelkiss
Copy link
Member

@aelkiss aelkiss commented Dec 11, 2025

Because the deposit holdings analysis is primarily for resource sharing which is (for now) a US-based service, we wanted to include icus material in that analysis.

This also exposes functionality for the mock solr response to include different records or queries, which could be useful for certain kinds of testing.

Arguably we shouldn't include nobody or pd-pvt for the purposes of this analysis since resource sharing people can't get access ot them, but the rights codes are at least now included in the report output so we can filter them out if desired.

This definitely exposes a bit of a question about how we consider what's "in copyright" for different purposes; it probably doesn't make sense to have a unilateral definition of that for holdings; future needs around overlap reporting focused around particular services might help clarify this.

@aelkiss
Copy link
Member Author

aelkiss commented Dec 11, 2025

I pushed something with a different approach to mocking the solr response. This is definitely exposing limitations both in the way we're testing the output of things and the assumption about the rights overall. Annoyingly, it's a pretty easy change to make, but messy to test. It was a bit messy to test to begin with too -- this at least starts to expose the possibility of mocking different responses from solr.

@aelkiss aelkiss force-pushed the deposit-analysis-icus branch 2 times, most recently from 3bdc5a0 to 45fdec2 Compare December 11, 2025 21:05
@coveralls
Copy link

coveralls commented Dec 11, 2025

Coverage Status

coverage: 77.25% (-0.02%) from 77.271%
when pulling 84553d4 on deposit-analysis-icus
into a85e34f on main.

Copy link
Contributor

@moseshll moseshll left a comment

Choose a reason for hiding this comment

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

I had to stare at mock_solr_search_filtered a bit to get it crowbarred into my head. The non-testing logic is simple and reasonable. APPROVE

Because the deposit holdings analysis is primarily for resource sharing
which is (for now) a US-based service, we wanted to include icus
material in that analysis.

This also exposes functionality for the mock solr response to include
different records or queries, which could be useful for certain kinds of
testing.
@aelkiss aelkiss force-pushed the deposit-analysis-icus branch from 45fdec2 to 84553d4 Compare December 11, 2025 21:36
@aelkiss aelkiss merged commit f9eb7bc into main Dec 11, 2025
1 check passed
@aelkiss aelkiss deleted the deposit-analysis-icus branch December 11, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants