Merge pull request #794 from folio-org/release/7.0.6

Release 7.0.6

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

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
 

service/grails-app/conf/logback-development.xml

@@ -36,8 +36,6 @@
     <level value="DEBUG" />
   </logger>
 
-
-
   <logger name="org.springframework.jdbc.support" >
     <level value="ERROR" />
   </logger>
@@ -87,6 +85,11 @@
     <level value="DEBUG" />
   </logger>
 
+  <logger name="com.k_int.web.toolkit.refdata.GrailsDomainRefdataHelpers">
+    <!-- Set to INFO to see logs like TitleInstance.setTypeFromString ( 'monograph' ) -->
+    <level value="ERROR" />
+  </logger>
+
   <if condition='isDefined("grails.util.BuildSettings.TARGET_DIR")'>
     <then>
       <appender name="FULL_STACKTRACE" class="ch.qos.logback.core.FileAppender" append="true">

service/grails-app/services/org/olf/TitleEnricherService.groovy

@@ -14,7 +14,7 @@ class TitleEnricherService {
   public static final ThreadLocal<Set> enrichedIds = new ThreadLocal<Set>()
 
   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) {
 

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
     }

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

service/src/main/groovy/org/olf/dataimport/internal/titleInstanceResolvers/IdFirstTIRSImpl.groovy

@@ -32,29 +32,37 @@ class IdFirstTIRSImpl extends BaseTIRS implements DataBinder {
     List<String> 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}"
       }
     }
 
     // If we didn't have a class one identifier AND we weren't able to match anything via
     // 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<TitleInstance> titleMatch(String title, float threshold) {
+  protected List<String> titleMatch(String title, float threshold) {
     return titleMatch(title, threshold, 'electronic');
   }
 
-  protected List<TitleInstance> titleMatch(final String title, final float threshold, final String subtype) {
+  protected List<String> titleMatch(final String title, final float threshold, final String subtype) {
     String matchTitle = StringUtils.truncate(title);
 
-    List<TitleInstance> result = new ArrayList<TitleInstance>()
+    List<String> result = new ArrayList<String>()
     TitleInstance.withSession { session ->
       try {
         result = TitleInstance.executeQuery(TEXT_MATCH_TITLE_HQL,[qrytitle: (matchTitle),threshold: (threshold), subtype:subtype], [max:20])

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<String> 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<String> 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;
       }
     }
 

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"
       ]
     },
     {

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))