Merge branch 'main' into 1959-list-sort

cstarcher · web-flow · commit cf03238e657f · 2026-07-08T18:01:51.000-06:00
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@ -21,4 +21,4 @@
 **Additional Description:**
 <!-- Briefly describe how AI was used -->
 
-[ ] No AI Used
+- [ ] No AI Used
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -42,6 +42,8 @@ jobs:
           jq 'reduce inputs as $i (.; .source_files += $i.source_files)' target/coveralls.json coverage/coveralls.json > coveralls.json
 
       - name: "Send to Coveralls"
+        if: github.event_name != 'pull_request'
+        continue-on-error: true
         uses: MikeEdgar/github-action@raw_coverage_file
         with:
           github-token: ${{ secrets.github_token }}
diff --git a/pom.xml b/pom.xml
@@ -48,7 +48,7 @@
     <dependency>
       <groupId>org.apache.tomcat.embed</groupId>
       <artifactId>tomcat-embed-core</artifactId>
-      <version>9.0.117</version>
+      <version>9.0.118</version>
     </dependency>
     
     <dependency>
diff --git a/src/main/java/org/tdl/vireo/auth/service/VireoUserCredentialsService.java b/src/main/java/org/tdl/vireo/auth/service/VireoUserCredentialsService.java
@@ -94,7 +94,7 @@ public synchronized User updateUserByCredentials(Credentials credentials) {
         } else {
             boolean isUserUpdated = false;
 
-            if (StringUtils.isNotEmpty(shibNetid) && (user.getNetid() == null || !user.getNetid().equals(shibNetid))) {
+            if (StringUtils.isNotEmpty(shibNetid) && (user.getNetid() == null || (user.getNetid().trim().length() < 1) || !user.getNetid().equals(shibNetid))) {
                 user.setNetid(shibNetid);
                 isUserUpdated = true;
             }
diff --git a/src/main/java/org/tdl/vireo/config/AppWebSecurityConfig.java b/src/main/java/org/tdl/vireo/config/AppWebSecurityConfig.java
@@ -2,10 +2,16 @@
 
 import static org.springframework.security.config.http.SessionCreationPolicy.STATELESS;
 
+import java.nio.charset.StandardCharsets;
+import java.util.regex.Pattern;
+
+import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
 import org.springframework.security.config.annotation.web.builders.HttpSecurity;
 import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
+import org.springframework.security.web.firewall.HttpFirewall;
+import org.springframework.security.web.firewall.StrictHttpFirewall;
 import org.tdl.vireo.auth.service.VireoUserDetailsService;
 import org.tdl.vireo.model.Role;
 import org.tdl.vireo.model.User;
@@ -18,6 +24,17 @@
 @EnableGlobalMethodSecurity(prePostEnabled = true, securedEnabled = true)
 public class AppWebSecurityConfig extends AuthWebSecurityConfig<User, UserRepo, VireoUserDetailsService> {
 
+    @Bean
+    public HttpFirewall httpFirewall() {
+        StrictHttpFirewall firewall = new StrictHttpFirewall();
+        Pattern allowed = Pattern.compile("[\\p{IsAssigned}&&[^\\p{IsControl}]]*");
+        firewall.setAllowedHeaderValues(header -> {
+            String parsed = new String(header.getBytes(StandardCharsets.ISO_8859_1), StandardCharsets.UTF_8);
+            return allowed.matcher(parsed).matches();
+        });
+        return firewall;
+    }
+
     @Override
     protected void configure(HttpSecurity http) throws Exception {
         // @formatter:off
diff --git a/src/main/java/org/tdl/vireo/controller/SubmissionController.java b/src/main/java/org/tdl/vireo/controller/SubmissionController.java
@@ -193,6 +193,9 @@ public class SubmissionController {
     @Value("${app.documentType.rename:}")
     private String documentTypesToRename;
 
+    @Value("${app.secondaryDelimiter:|}")
+    private String secondaryDelimiter;
+
     @RequestMapping("/all")
     @PreAuthorize("hasRole('ADMIN')")
     public ApiResponse getAll() {
@@ -597,7 +600,7 @@ private void processBatchExport(
 
                         // Stream data rows
                         for (Submission submission : submissions) {
-                            ExportPackage exportPackage = packagerUtility.packageExport(packager, submission, columns);
+                            ExportPackage exportPackage = packagerUtility.packageExport(packager, submission, columns, secondaryDelimiter);
                             if (exportPackage.isMap()) {
                                 Map<String, String> rowData = (Map<String, String>) exportPackage.getPayload();
                                 Row row = worksheet.createRow(rowCount++);
@@ -1101,7 +1104,7 @@ public ApiResponse removeFile(@WeaverUser User user, @PathVariable Long submissi
             if (user.getRole().equals(Role.ROLE_STUDENT) && documentType.equals("_doctype_license")) {
                 apiResponse = new ApiResponse(ERROR, "You are not allowed to delete license files!");
             } else {
-                if (user.getRole().equals(Role.ROLE_ADMIN) || user.getRole().equals(Role.ROLE_MANAGER) || uri.contains(String.valueOf(hash))) {
+                if (user.getRole().equals(Role.ROLE_ADMIN) || user.getRole().equals(Role.ROLE_MANAGER) || user.getRole().equals(Role.ROLE_REVIEWER) || uri.contains(String.valueOf(hash))) {
                     String fileName = "";
                     String fileSize = "file not found";
                     if (assetService.assetFileExists(uri)) {
diff --git a/src/main/java/org/tdl/vireo/model/CustomActionDefinition.java b/src/main/java/org/tdl/vireo/model/CustomActionDefinition.java
@@ -34,6 +34,15 @@ public CustomActionDefinition(String label, Boolean isStudentVisible) {
         isStudentVisible(isStudentVisible);
     }
 
+    /**
+     * @return the position
+     */
+    @Override
+    @JsonView(Views.SubmissionList.class)
+    public Long getPosition() {
+        return super.getPosition();
+    }
+
     /**
      * @return the label
      */
diff --git a/src/main/java/org/tdl/vireo/model/export/enums/MarcXML21Key.java b/src/main/java/org/tdl/vireo/model/export/enums/MarcXML21Key.java
@@ -10,6 +10,7 @@ public enum MarcXML21Key {
     MAJOR,
     DEPOSIT_URL,
     ABSTRACT,
+    SUBJECT_FIELD_VALUES,
     KEYWORD_FIELD_VALUES,
     DEPARTMENT,
     COMMITTEE_CHAIR_FIELD_VALUES,
diff --git a/src/main/java/org/tdl/vireo/model/formatter/MarcXML21Formatter.java b/src/main/java/org/tdl/vireo/model/formatter/MarcXML21Formatter.java
@@ -66,6 +66,9 @@ public void populateContext(Context context, Submission submission) {
             case KEYWORD_FIELD_VALUES:
                 context.setVariable(key.name(), submissionHelperUtility.getKeywordFieldValues());
                 break;
+            case SUBJECT_FIELD_VALUES:
+                context.setVariable(key.name(), submissionHelperUtility.getSubjectFieldValues());
+                break;
             case DEPARTMENT:
                 context.setVariable(key.name(), submissionHelperUtility.getDepartment());
                 break;
diff --git a/src/main/java/org/tdl/vireo/model/inheritance/HeritableRepoImpl.java b/src/main/java/org/tdl/vireo/model/inheritance/HeritableRepoImpl.java
@@ -38,7 +38,7 @@ public void removeFromWorkflowStep(Organization requestingOrganization, Workflow
         // if requesting organization originates the workflow step or the workflow step is overrideable,
         if (pendingWorkflowStep.getOriginatingOrganization().getId().equals(requestingOrganization.getId()) || pendingWorkflowStep.getOverrideable()) {
             // ... and if also that workflow step originates the heritableModel or the heritableModel is overrideable,
-            if (heritableModelToRemove.getOriginatingWorkflowStep().getId().equals(heritableModelToRemove.getId()) || heritableModelToRemove.getOverrideable()) {
+            if(heritableModelToRemove.getOriginatingWorkflowStep() != null && heritableModelToRemove.getOriginatingWorkflowStep().getId().equals(pendingWorkflowStep.getId())){
                 // ...then the update is permissible.
 
                 // if requesting organization is not the workflow step's orignating organization,
diff --git a/src/main/java/org/tdl/vireo/model/packager/AbstractPackager.java b/src/main/java/org/tdl/vireo/model/packager/AbstractPackager.java
@@ -67,6 +67,10 @@ public EP packageExport(Submission submission, List<SubmissionListColumn> column
         throw new UnsupportedFormatterException("Exporter does not support submission list columns!");
     }
 
+    public EP packageExport(Submission submission, List<SubmissionListColumn> columns, String delimiter) throws UnsupportedFormatterException {
+        throw new UnsupportedFormatterException("Exporter does not support submission list columns and delimiter!");
+    }
+
     public EP packageExport(Submission submission, Map<String, String> ds_docs) throws UnsupportedFormatterException {
         throw new UnsupportedFormatterException("Exporter does not support multiple docs !");
     }
diff --git a/src/main/java/org/tdl/vireo/model/packager/ExcelPackager.java b/src/main/java/org/tdl/vireo/model/packager/ExcelPackager.java
@@ -65,18 +65,21 @@ public ExcelExportPackage packageExport(Submission submission, String manifest)
 
     @Override
     public ExcelExportPackage packageExport(Submission submission, List<SubmissionListColumn> columns) {
+        return new ExcelExportPackage(submission, "Excel", buildRow(submission, columns, ", "));
+    }
+
+    @Override
+    public ExcelExportPackage packageExport(Submission submission, List<SubmissionListColumn> columns, String delimiter) {
+        return new ExcelExportPackage(submission, "Excel", buildRow(submission, columns, delimiter));
+    }
+
+    private Map<String, String> buildRow(Submission submission, List<SubmissionListColumn> columns, String fieldValueDelimiter) {
         Map<String, String> row = new HashMap<String, String>();
         columns.forEach(column -> {
             Optional<String> predicate = Optional.ofNullable(column.getPredicate());
             if (predicate.isPresent()) {
-                List<String> fieldValues = new ArrayList<String>();
-                for (FieldValue fieldValue : submission.getFieldValues()) {
-                    if (fieldValue.getFieldPredicate().getValue().equals(predicate.get().trim())) {
-                        fieldValues.add(fieldValue.getValue());
-                        row.put(column.getTitle(), String.join(", ", fieldValues));
-                    } else {
-                        row.put(column.getTitle(), String.join(", ", fieldValues));                    }
-                }
+                List<String> fieldValues = getFieldValues(submission, predicate.get().trim());
+                row.put(column.getTitle(), String.join(fieldValueDelimiter, fieldValues));
             } else {
                 if (column.getValuePath().size() > 0) {
                     String[] valuePath = column.getValuePath().toArray(new String[column.getValuePath().size()]);
@@ -142,7 +145,22 @@ public ExcelExportPackage packageExport(Submission submission, List<SubmissionLi
                 }
             }
         });
-        return new ExcelExportPackage(submission, "Excel", row);
+        return row;
+    }
+
+    private List<String> getFieldValues(Submission submission, String predicate) {
+        List<String> fieldValues = new ArrayList<String>();
+        for (FieldValue fieldValue : submission.getFieldValues()) {
+            if (matchesPredicate(fieldValue, predicate)) {
+                fieldValues.add(fieldValue.getValue());
+            }
+        }
+        return fieldValues;
+    }
+
+    private boolean matchesPredicate(FieldValue fieldValue, String predicate) {
+        String fieldPredicate = fieldValue.getFieldPredicate().getValue();
+        return fieldPredicate.equals(predicate) || fieldPredicate.startsWith(predicate + ".");
     }
 
 }
diff --git a/src/main/java/org/tdl/vireo/model/packager/Packager.java b/src/main/java/org/tdl/vireo/model/packager/Packager.java
@@ -26,6 +26,8 @@ public interface Packager<EP extends ExportPackage> {
 
     public EP packageExport(Submission submission, List<SubmissionListColumn> columns);
 
+    public EP packageExport(Submission submission, List<SubmissionListColumn> columns, String delimiter);
+
     public EP packageExport(Submission submission, Map<String,String> dsDocs);
 
 }
diff --git a/src/main/java/org/tdl/vireo/model/repo/impl/SubmissionRepoImpl.java b/src/main/java/org/tdl/vireo/model/repo/impl/SubmissionRepoImpl.java
@@ -796,7 +796,7 @@ public int compare(SubmissionListColumn svc1, SubmissionListColumn svc2) {
                     for (String filterString : submissionListColumn.getFilters()) {
                         sqlBuilder = new StringBuilder();
 
-                        if (filterString == null) {
+                        if (filterString == null || "null".equalsIgnoreCase(filterString)) {
                             sqlBuilder.append("a.email IS NULL");
                         } else if (submissionListColumn.getExactMatch()) {
                             sqlBuilder.append("a.email = '").append(filterString).append("'");
diff --git a/src/main/java/org/tdl/vireo/utility/PackagerUtility.java b/src/main/java/org/tdl/vireo/utility/PackagerUtility.java
@@ -45,6 +45,10 @@ public ExportPackage packageExport(Packager<?> packager, Submission submission,
         return packager.packageExport(submission, columns);
     }
 
+    public ExportPackage packageExport(Packager<?> packager, Submission submission, List<SubmissionListColumn> columns, String delimiter) {
+        return packager.packageExport(submission, columns, delimiter);
+    }
+
     public ExportPackage packageExport(Packager<?> packager, Submission submission, Map<String, String> dsDocs) {
         return packager.packageExport(submission, dsDocs);
     }
diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml
@@ -137,6 +137,8 @@ app:
   # edu.tamu.weaver.auth.service.UserCredentialsService
   authority.admins: admin@tdl.org,aggieJack@tamu.edu
 
+  # ExcelExport secondary delimiter for keywords within CSV, otherwise multiple keywords appear as separate columns
+  secondaryDelimiter: '|'
 
   security:
     # edu.tamu.weaver.auth.service.CryptoService
diff --git a/src/main/resources/formats/marc21_xml.xml b/src/main/resources/formats/marc21_xml.xml
@@ -211,18 +211,19 @@
 
      <!-- 650, ETD discipline index terms -->
      <!--
-         Source: major
+         Source: subjects
          Example:
-             <marc:datafield tag="650" ind1="4" ind2=" ">
+             <marc:datafield tag="650" ind1=" " ind2=" ">
                  <marc:subfield code="a">Finance</marc:subfield>
              </marc:datafield>
              
      -->
-     <datafield th:if="${ MAJOR != null }" tag="650" ind1=" " ind2="4">
-         <subfield th:text="${ MAJOR }" code="a"></subfield>
-     </datafield>
+    <datafield th:each="subject: ${SUBJECT_FIELD_VALUES}" tag="650" ind1=" " ind2=" ">
+        <subfield th:text="${ subject.getValue() }" code="a"></subfield>
+    </datafield>
+
 
-    <!-- 653, ETD subject/keywords -->
+    <!-- 653, ETD keywords -->
     <!--
         Source: documentKeywords
         Example:
diff --git a/src/main/webapp/app/directives/fieldProfileDirective.js b/src/main/webapp/app/directives/fieldProfileDirective.js
@@ -412,7 +412,7 @@ vireo.directive("field", function ($controller, $filter, $q, $timeout, Controlle
             };
 
             $scope.controlledVocabularyTypeAhead = function (field, search) {
-                if (!field.fieldPredicate.id || !$scope.profile.controlledVocabulary.id) {
+                if (!field.fieldPredicate.id || !angular.isDefined($scope.profile.controlledVocabulary)) {
                     return [];
                 }
 
diff --git a/src/main/webapp/app/views/admin/list.html b/src/main/webapp/app/views/admin/list.html
@@ -36,7 +36,7 @@ <h1>List ETDs</h1>
           <span ng-class="{'glyphicon glyphicon-remove-circle' : $first}" ng-click="addRowFilter($parent.$index, row)"></span>
           <span ng-if="col.title() !== 'Custom Actions'">{{displaySubmissionProperty(row, col)}}</span>
           <span ng-if="col.title() === 'Custom Actions'">
-            <span ng-repeat="ca in row.customActionValues">
+            <span ng-repeat="ca in row.customActionValues | orderBy:'definition.position'">
               <input ng-checked="{{ca.value}}" type="checkbox" disabled> {{getCustomActionLabelById(ca.definition.id)}}</input>
               <br/>
             </span>
diff --git a/src/test/java/org/tdl/vireo/model/CustomActionDefinitionTest.java b/src/test/java/org/tdl/vireo/model/CustomActionDefinitionTest.java
@@ -1,8 +1,19 @@
 package org.tdl.vireo.model;
 
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.provider.Arguments;
 import org.mockito.InjectMocks;
+import org.tdl.vireo.model.response.Views;
+
+import com.fasterxml.jackson.databind.MapperFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.json.JsonMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 
 public class CustomActionDefinitionTest extends AbstractModelCustomMethodTest<CustomActionDefinition> {
 
@@ -43,4 +54,62 @@ private static Stream<Arguments> getParameterMethodStream() {
         );
     }
 
+    /**
+     * Regression test for GitHub issue #2104.
+     *
+     * Verifies that {@code CustomActionDefinition.position} is included in the
+     * JSON output when serialized under {@link Views.SubmissionList} (and its
+     * sub-views such as {@link Views.SubmissionIndividualActionLogs}).
+     *
+     * The application sets {@code jackson.mapper.default-view-inclusion: false},
+     * so any field without a matching {@code @JsonView} annotation is silently
+     * excluded when a view is active. The fix overrides {@code getPosition()} in
+     * {@code CustomActionDefinition} with {@code @JsonView(Views.SubmissionList.class)}.
+     * Removing that annotation will cause this test to fail.
+     */
+    @Test
+    public void testPositionSerializedInSubmissionListView() throws Exception {
+        // Configure ObjectMapper to match application.yml: default-view-inclusion: false
+        ObjectMapper mapper = JsonMapper.builder()
+            .disable(MapperFeature.DEFAULT_VIEW_INCLUSION)
+            .build();
+
+        CustomActionDefinition definition = new CustomActionDefinition("Test action", true);
+        definition.setPosition(3L);
+
+        // Serialize under Views.SubmissionList — the view used by /submission/query
+        String json = mapper.writerWithView(Views.SubmissionList.class).writeValueAsString(definition);
+        ObjectNode node = (ObjectNode) mapper.readTree(json);
+
+        assertNotNull(node.get("position"),
+            "position must be present in SubmissionList view JSON; " +
+            "ensure getPosition() in CustomActionDefinition is annotated with @JsonView(Views.SubmissionList.class)");
+        assertTrue(node.get("position").isNumber(),
+            "position must be a numeric value in SubmissionList view JSON");
+    }
+
+    /**
+     * Verifies that {@code position} is also included when serialized under the
+     * {@link Views.SubmissionIndividualActionLogs} view used by the admin
+     * {@code GET /submission/get-one/{id}} endpoint, since that view extends
+     * {@link Views.SubmissionList}.
+     */
+    @Test
+    public void testPositionSerializedInSubmissionIndividualActionLogsView() throws Exception {
+        ObjectMapper mapper = JsonMapper.builder()
+            .disable(MapperFeature.DEFAULT_VIEW_INCLUSION)
+            .build();
+
+        CustomActionDefinition definition = new CustomActionDefinition("Test action", true);
+        definition.setPosition(5L);
+
+        String json = mapper.writerWithView(Views.SubmissionIndividualActionLogs.class).writeValueAsString(definition);
+        ObjectNode node = (ObjectNode) mapper.readTree(json);
+
+        assertNotNull(node.get("position"),
+            "position must be present in SubmissionIndividualActionLogs view JSON");
+        assertTrue(node.get("position").isNumber(),
+            "position must be a numeric value in SubmissionIndividualActionLogs view JSON");
+    }
+
 }
diff --git a/src/test/java/org/tdl/vireo/model/export/enums/MarcXML21KeyTest.java b/src/test/java/org/tdl/vireo/model/export/enums/MarcXML21KeyTest.java
@@ -17,11 +17,12 @@ protected static Stream<Arguments> provideEnumParameters() {
             Arguments.of(MarcXML21Key.MAJOR, "MAJOR", 6),
             Arguments.of(MarcXML21Key.DEPOSIT_URL, "DEPOSIT_URL", 7),
             Arguments.of(MarcXML21Key.ABSTRACT, "ABSTRACT", 8),
-            Arguments.of(MarcXML21Key.KEYWORD_FIELD_VALUES, "KEYWORD_FIELD_VALUES", 9),
-            Arguments.of(MarcXML21Key.DEPARTMENT, "DEPARTMENT", 10),
-            Arguments.of(MarcXML21Key.COMMITTEE_CHAIR_FIELD_VALUES, "COMMITTEE_CHAIR_FIELD_VALUES", 11),
-            Arguments.of(MarcXML21Key.COMMITTEE_MEMBER_FIELD_VALUES, "COMMITTEE_MEMBER_FIELD_VALUES", 12),
-            Arguments.of(MarcXML21Key.PRIMARY_DOCUMENT_MIMETYPE, "PRIMARY_DOCUMENT_MIMETYPE", 13)
+            Arguments.of(MarcXML21Key.SUBJECT_FIELD_VALUES, "SUBJECT_FIELD_VALUES", 9),
+            Arguments.of(MarcXML21Key.KEYWORD_FIELD_VALUES, "KEYWORD_FIELD_VALUES", 10),
+            Arguments.of(MarcXML21Key.DEPARTMENT, "DEPARTMENT", 11),
+            Arguments.of(MarcXML21Key.COMMITTEE_CHAIR_FIELD_VALUES, "COMMITTEE_CHAIR_FIELD_VALUES", 12),
+            Arguments.of(MarcXML21Key.COMMITTEE_MEMBER_FIELD_VALUES, "COMMITTEE_MEMBER_FIELD_VALUES", 13),
+            Arguments.of(MarcXML21Key.PRIMARY_DOCUMENT_MIMETYPE, "PRIMARY_DOCUMENT_MIMETYPE", 14)
         );
     }
 
diff --git a/src/test/java/org/tdl/vireo/model/packager/ExcelPackagerTest.java b/src/test/java/org/tdl/vireo/model/packager/ExcelPackagerTest.java

Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ public synchronized User updateUserByCredentials(Credentials credentials) {`
`94`	`94`	`} else {`
`95`	`95`	`boolean isUserUpdated = false;`
`96`	`96`
`97`		`- if (StringUtils.isNotEmpty(shibNetid) && (user.getNetid() == null \|\| !user.getNetid().equals(shibNetid))) {`
	`97`	`+ if (StringUtils.isNotEmpty(shibNetid) && (user.getNetid() == null \|\| (user.getNetid().trim().length() < 1) \|\| !user.getNetid().equals(shibNetid))) {`
`98`	`98`	`user.setNetid(shibNetid);`
`99`	`99`	`isUserUpdated = true;`
`100`	`100`	`}`