diff --git a/NEWS.md b/NEWS.md index b1043c60..fea94c93 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,10 @@ +## 7.0.6 2024-07-05 + * ERM-3291 Fix permissions on /erm/validate/remoteKB in mod-agreements + * ERM-3289 Fix permission on /erm/validate/subscriptionAgreement in mod-agreements + * ERM-3288 Fix permission on /erm/files/{id}/raw in mod-agreements + * ERM-3274 If a citation gets passed to BaseTIRS without an instancePublicationMedia field it will crash + * ERM-3254 With WorkSourceIDTIRS for GOKb we see "Field error in object" errors + ## 7.0.5 2024-05-28 * ERM-3246 Improve performance of entitlementOptions endpoint * ERM-3187 Re-write query to show "list of resources in an agreement" for improved performance diff --git a/service/gradle.properties b/service/gradle.properties index c4bf905d..f42cf6d8 100644 --- a/service/gradle.properties +++ b/service/gradle.properties @@ -4,7 +4,7 @@ gormVersion=8.0.3 # Application appName=mod-agreements -appVersion=7.0.5 +appVersion=7.0.6 dockerTagSuffix= dockerRepo=folioci diff --git a/service/grails-app/conf/logback-development.xml b/service/grails-app/conf/logback-development.xml index 1c410be0..75a6cc8c 100644 --- a/service/grails-app/conf/logback-development.xml +++ b/service/grails-app/conf/logback-development.xml @@ -36,8 +36,6 @@ - - @@ -87,6 +85,11 @@ + + + + + diff --git a/service/grails-app/services/org/olf/TitleEnricherService.groovy b/service/grails-app/services/org/olf/TitleEnricherService.groovy index dc559d96..51163584 100644 --- a/service/grails-app/services/org/olf/TitleEnricherService.groovy +++ b/service/grails-app/services/org/olf/TitleEnricherService.groovy @@ -14,7 +14,7 @@ class TitleEnricherService { public static final ThreadLocal enrichedIds = new ThreadLocal() public void secondaryEnrichment(RemoteKB kb, String sourceIdentifier, String ermIdentifier) { - log.debug("TitleEnricherService::secondaryEnrichment called for title with source identifier: ${sourceIdentifier}, erm identifier: ${ermIdentifier} and RemoteKb: ${kb.name}") + //log.debug("TitleEnricherService::secondaryEnrichment called for title with source identifier: ${sourceIdentifier}, erm identifier: ${ermIdentifier} and RemoteKb: ${kb.name}") // Check for existence of sourceIdentifier. LOCAL source imports will pass null here. if (sourceIdentifier) { diff --git a/service/grails-app/services/org/olf/TitleIngestService.groovy b/service/grails-app/services/org/olf/TitleIngestService.groovy index dc20b30d..5f7841b0 100644 --- a/service/grails-app/services/org/olf/TitleIngestService.groovy +++ b/service/grails-app/services/org/olf/TitleIngestService.groovy @@ -78,6 +78,7 @@ class TitleIngestService implements DataBinder { titleId = titleInstanceResolverService.resolve(pc, trustedSourceTI) } catch (Exception e){ log.error("Error resolving title (${pc.title}) with identifiers ${pc.instanceIdentifiers}, skipping. ${e.message}") + // TODO consider turning these on by default for developers, perhaps in TRACE or likewise? //e.printStackTrace(); return result } diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy index e3250f67..bf407485 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/BaseTIRS.groovy @@ -117,14 +117,19 @@ abstract class BaseTIRS implements TitleInstanceResolverService { /* * For some reason whenever a title is updated with just refdata fields it fails to properly mark as dirty. * The below solution of '.markDirty()' is not ideal, but it does solve the problem for now. - * TODO: Ian to Review with Ethan - this makes no sense to me at the moment * - * If the "Authoritative" publication type is not equal to whatever mad value a remote site has sent then - * replace the authortiative value with the one sent? + * "publication instance media" refers to the remote site's own instructions about what an instance actually is, + * as opposed to monograph/serial which are our internal controlled ones. Since we are only in this method if we + * have marked this source as "trusted" for updating title medata (ie they are the 'owners' of this title) we allow + * the updating of this field with whatever the institution wishes to call these things. + * + * + * TL;DR we don't care what's in this field since we do our logic based on a controlled "instanceMedia" field instead. */ - if (title.publicationType?.value != RefdataValue.normValue(citation.instancePublicationMedia)) { - if (citation.instancePublicationMedia) { - title.publicationTypeFromString = citation.instancePublicationMedia + String citationIPM = citation.instancePublicationMedia ? RefdataValue.normValue(citation.instancePublicationMedia) : null; + if (title.publicationType?.value != citationIPM) { + if (citationIPM) { + title.publicationTypeFromString = citationIPM } else { title.publicationType = null; } @@ -191,7 +196,7 @@ abstract class BaseTIRS implements TitleInstanceResolverService { // Different TIRS implementations will have different workflows with identifiers, but the vast majority of the creation will be the same // We assume that the incoming citation already has split ids and siblingIds protected TitleInstance createNewTitleInstanceWithoutIdentifiers(final ContentItemSchema citation, String workId = null) { - Work work = workId ? Work.get(workId) : null; + Work work = workId ? Work.get(workId) as Work : null; TitleInstance result = null // Ian: adding this - Attempt to make sense of the instanceMedia value we have been passed diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy index 119ea92e..fc8bd45f 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy @@ -32,18 +32,24 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { List candidate_list = classOneMatch(citation.instanceIdentifiers); int num_matches = candidate_list.size() int num_class_one_identifiers = countClassOneIDs(citation.instanceIdentifiers); + + // Ensure logging messages accurately reflect _which_ matching attempt + // caused the multiple title match problem or which match led to the exact match + String multiple_match_message; + String match_vector = "classOneMatch"; + if ( num_matches > 1 ) { - log.debug("Class one match found multiple titles:: ${candidate_list}"); + multiple_match_message="Class one match found ${num_matches} records::${candidate_list}"; } // We weren't able to match directly on an identifier for this instance - see if we have an identifier // for a sibling instance we can use to narrow down the list. if ( num_matches == 0 ) { + match_vector = "siblingMatch" candidate_list = siblingMatch(citation) num_matches = candidate_list.size() - log.debug("siblingMatch for ${citation.title} found ${num_matches} titles"); if ( num_matches > 1 ) { - log.debug("Sibling match found multiple titles:: ${candidate_list}"); + multiple_match_message="Sibling match found ${num_matches} records::${candidate_list}" } } @@ -51,10 +57,12 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { // a sibling match, try to do a fuzzy match as a last resort // DO NOT ATTEMPT if there is no title on the citation if ( ( num_matches == 0 ) && ( num_class_one_identifiers == 0 ) && citation.title ) { - log.debug("No matches on identifier - try a fuzzy text match on title(${citation.title})"); - // No matches - try a simple title match + match_vector = "fuzzy title match" candidate_list = titleMatch(citation.title,MATCH_THRESHOLD); num_matches = candidate_list.size() + if ( num_matches > 1 ) { + multiple_match_message="Title fuzzy matched ${num_matches} records with a threshold >= ${MATCH_THRESHOLD}::${candidate_list}" + } } if ( candidate_list != null ) { @@ -64,13 +72,18 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { result = createNewTitleInstanceWithSiblings(citation).id break; case(1): - log.debug("Exact match. Enrich title.") + log.debug("Exact match via ${match_vector}.${trustedSourceTI ? ' Enrich title.' : ''}") result = candidate_list.get(0); - checkForEnrichment(candidate_list.get(0), citation, trustedSourceTI); + + // This isn't logically necessary, but will cut down on a call to checkForEnrichment + // for WorkSourceIdTIRS per title. + if (trustedSourceTI) { + checkForEnrichment(candidate_list.get(0), citation, trustedSourceTI); + } break; default: throw new TIRSException( - "title matched ${num_matches} records with a threshold >= ${MATCH_THRESHOLD} . Unable to continue. Matching IDs: ${candidate_list.collect { it.id }}. class one identifier count: ${num_class_one_identifiers}", + "${multiple_match_message}. Class one identifier count: ${num_class_one_identifiers}", TIRSException.MULTIPLE_TITLE_MATCHES, ); break; @@ -157,7 +170,7 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { protected static final float MATCH_THRESHOLD = 0.775f protected static final String TEXT_MATCH_TITLE_HQL = ''' - SELECT ti from TitleInstance as ti + SELECT ti.id from TitleInstance as ti WHERE trgm_match(ti.name, :qrytitle) = true AND similarity(ti.name, :qrytitle) > :threshold @@ -280,16 +293,16 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder { } /** - * Attempt a fuzzy match on the title + * Attempt a fuzzy match on the title -- returns IDs now */ - protected List titleMatch(String title, float threshold) { + protected List titleMatch(String title, float threshold) { return titleMatch(title, threshold, 'electronic'); } - protected List titleMatch(final String title, final float threshold, final String subtype) { + protected List titleMatch(final String title, final float threshold, final String subtype) { String matchTitle = StringUtils.truncate(title); - List result = new ArrayList() + List result = new ArrayList() TitleInstance.withSession { session -> try { result = TitleInstance.executeQuery(TEXT_MATCH_TITLE_HQL,[qrytitle: (matchTitle),threshold: (threshold), subtype:subtype], [max:20]) diff --git a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy index ce9a5ab2..f97b120c 100644 --- a/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy +++ b/service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/WorkSourceIdentifierTIRSImpl.groovy @@ -142,10 +142,11 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder tiId = super.resolve(citation, false); } catch (TIRSException tirsException) { // We treat a multiple title match here as NBD and move onto creation - // Any other TIRSExceptions are legitimate concerns and we should rethrow - if ( - tirsException.code != TIRSException.MULTIPLE_TITLE_MATCHES - ) { + if (tirsException.code == TIRSException.MULTIPLE_TITLE_MATCHES) { + // Logging out where we hit this so we can potentially make decisions from the logs + log.debug("MULTIPLE_TITLE_MATCHES in IdFirstTIRS. Creating new TitleInstance (${tirsException.message})") + } else { + // Any other TIRSExceptions are legitimate concerns and we should rethrow throw new TIRSException(tirsException.message, tirsException.code); } } //Dont catch any other exception, those are legitimate reasons to stop @@ -191,7 +192,7 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder status: IdentifierOccurrence.lookupOrCreateStatus('approved') ]) work.setSourceIdentifier(sourceIdentifier); - work.save(failOnError: true); + work.save(failOnError: true); // We removed the withNewSession around sibling creation so that unflushed changes here are available to each // At this point we are assuming this TI is the right one, allow metadata updates checkForEnrichment(tiId, citation, true); @@ -369,50 +370,44 @@ class WorkSourceIdentifierTIRSImpl extends IdFirstTIRSImpl implements DataBinder // We need previous sibling work to have _taken_ // in the DB by the time we hit the next sibling citation - // Force withNewSession, but NOT withNewTransaction - // Not really sure on the nuance here, but prevents HibernateAccessException whilst - // Still ensuring that each directMatch has the DB changes from the previous citation - // in place? - TitleInstance.withNewSession { - // Match sibling citation to siblings already on the work (ONLY looking at approved identifiers) - List matchedSiblings = directMatch(sibling_citation.instanceIdentifiers, workId, 'print'); - - switch(matchedSiblings.size()) { - case 0: - // No sibling found, add it. - createNewTitleInstance(sibling_citation, workId); - break; - case 1: - // Found single sibling citation, update identifiers and check for enrichment - String siblingId = matchedSiblings.get(0); - updateTIIdentifiers(siblingId, sibling_citation.instanceIdentifiers); - checkForEnrichment(siblingId , sibling_citation, true); - // We've matched this sibling, remove it from the unmatchedSiblings list. - unmatchedSiblings.removeIf { it == siblingId } - - // Force save + flush -- necessary - saveTitleInstance(TitleInstance.get(siblingId)); - break; - default: - // Found multiple siblings which would match citation. - // Remove each from the work and progress - log.warn("Matched multiple siblings from single citation. Removing from work: ${matchedSiblings}") - matchedSiblings.each { matchedSibling -> - TitleInstance matchedSiblingDomainObject = TitleInstance.get(matchedSibling) - // Mark all identifier occurrences as error - matchedSiblingDomainObject.identifiers.each {io -> - io.setStatusFromString(ERROR) - io.save(failOnError: true); - } - // Remove Work - matchedSiblingDomainObject.work = null; - saveTitleInstance(matchedSiblingDomainObject); - - // We've matched this sibling, remove it from the unmatchedSiblings list. - unmatchedSiblings.removeIf { it == matchedSibling } + // Match sibling citation to siblings already on the work (ONLY looking at approved identifiers) + List matchedSiblings = directMatch(sibling_citation.instanceIdentifiers, workId, 'print'); + + switch(matchedSiblings.size()) { + case 0: + // No sibling found, add it. + createNewTitleInstance(sibling_citation, workId); + break; + case 1: + // Found single sibling citation, update identifiers and check for enrichment + String siblingId = matchedSiblings.get(0); + updateTIIdentifiers(siblingId, sibling_citation.instanceIdentifiers); + checkForEnrichment(siblingId , sibling_citation, true); + // We've matched this sibling, remove it from the unmatchedSiblings list. + unmatchedSiblings.removeIf { it == siblingId } + + // Force save + flush -- necessary + saveTitleInstance(TitleInstance.get(siblingId)); + break; + default: + // Found multiple siblings which would match citation. + // Remove each from the work and progress + log.warn("Matched multiple siblings from single citation. Removing from work: ${matchedSiblings}") + matchedSiblings.each { matchedSibling -> + TitleInstance matchedSiblingDomainObject = TitleInstance.get(matchedSibling) + // Mark all identifier occurrences as error + matchedSiblingDomainObject.identifiers.each {io -> + io.setStatusFromString(ERROR) + io.save(failOnError: true); } - break; - } + // Remove Work + matchedSiblingDomainObject.work = null; + saveTitleInstance(matchedSiblingDomainObject); + + // We've matched this sibling, remove it from the unmatchedSiblings list. + unmatchedSiblings.removeIf { it == matchedSibling } + } + break; } } diff --git a/service/src/main/okapi/ModuleDescriptor-template.json b/service/src/main/okapi/ModuleDescriptor-template.json index 196c59ab..cf92c6a5 100644 --- a/service/src/main/okapi/ModuleDescriptor-template.json +++ b/service/src/main/okapi/ModuleDescriptor-template.json @@ -405,22 +405,22 @@ { "methods": ["POST"], "pathPattern": "/erm/validate/subscriptionAgreement", - "permissionsRequired": [ "erm.agreements.item.post", "erm.agreements.item.put" ] + "permissionsRequired": [ "erm.agreements.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/subscriptionAgreement/*", - "permissionsRequired": [ "erm.agreements.item.post", "erm.agreements.item.put" ] + "permissionsRequired": [ "erm.agreements.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/remoteKB", - "permissionsRequired": [ "erm.kbs.item.post", "erm.kbs.item.put" ] + "permissionsRequired": [ "erm.kbs.validate" ] }, { "methods": ["POST"], "pathPattern": "/erm/validate/remoteKB/*", - "permissionsRequired": [ "erm.kbs.item.post", "erm.kbs.item.put" ] + "permissionsRequired": [ "erm.kbs.validate" ] }, { "methods": ["GET"], @@ -564,6 +564,11 @@ "displayName": "Agreements usage data providers get", "description": "Get the usage data providers for an agreement" }, + { + "permissionName": "erm.agreements.validate", + "displayName": "Validate subscription agreement post", + "description": "Validate subscription agreement information" + }, { "permissionName": "erm.agreements.view", "subPermissions": [ @@ -597,7 +602,8 @@ "erm.agreements.view", "erm.agreements.item.post", "erm.agreements.item.put", - "erm.agreements.item.clone" + "erm.agreements.item.clone", + "erm.agreements.validate" ] }, { @@ -632,10 +638,7 @@ { "permissionName": "erm.files.item.download", "displayName": "Files item download", - "description": "Download a raw files record", - "subPermissions": [ - "erm.files.view" - ] + "description": "Download a raw files record" }, { "permissionName": "erm.files.item.post", @@ -821,6 +824,11 @@ "displayName": "Knowledge base item get", "description": "Get knowledge base record" }, + { + "permissionName": "erm.kbs.validate", + "displayName": "Validate knowledge base post", + "description": "Validate knowledge base information" + }, { "permissionName": "erm.kbs.view", "subPermissions": [ @@ -843,7 +851,8 @@ "subPermissions": [ "erm.kbs.view", "erm.kbs.item.post", - "erm.kbs.item.put" + "erm.kbs.item.put", + "erm.kbs.validate" ] }, { diff --git a/service/src/main/okapi/tenant/sample_data/_data.groovy b/service/src/main/okapi/tenant/sample_data/_data.groovy index 9ede3c14..f6219637 100644 --- a/service/src/main/okapi/tenant/sample_data/_data.groovy +++ b/service/src/main/okapi/tenant/sample_data/_data.groovy @@ -21,7 +21,7 @@ RemoteKB.findByName('GOKb') ?: (new RemoteKB( uri:'https://gokb.org/gokb/oai/index', fullPrefix:'gokb', rectype: RemoteKB.RECTYPE_PACKAGE, - active:Boolean.FALSE, + active:Boolean.TRUE, supportsHarvesting:true, activationEnabled:false ).save(failOnError:true))