12367 GitHub actions workflow for jenkins redundancy#90
12367 GitHub actions workflow for jenkins redundancy#90srmanda-cs wants to merge 135 commits intodevelopfrom
Conversation
# Conflicts: # src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java
…led vocabulary fields
…values are selected, preventing invalid N/A placeholders.
IQSS/7956-show_original_file_format
…-files Fix Publish and Submit for Review when no files are present in Dataset
feat: let APIs return more info about role assignments
…otOutdated(). IQSS#12354 A slightly cleaner fix IQSS#12354 Refined (and, maybe even improved?) the language in the guides documenting the sourceLastUpdateTime parameter. IQSS#12354 a typo IQSS#12354
…response Fix timing issue when posting guestbook response and requesting the d…
11912 edit template api
Timezone fix in validateInternalTimestampIsNotOutdated()
…ormat API: File Get Citation In Other Formats
… integration tests
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Updated concurrency group name to include branch reference and removed .txt files from paths-ignore.
Removed .txt files from paths-ignore in workflow.
Temporarily disable unreliable integration test for retrieving data collections.
Removed force_run input from workflow dispatch.
Replace curl command with docker cp for SUSHI config file.
There was a problem hiding this comment.
Pull request overview
This PR adds a new GitHub Actions workflow intended to run container-based integration tests, while also bundling a broad set of application changes (new/updated APIs, UI/indexing adjustments, validation, and test/resource updates) across Dataverse.
Changes:
- Add a new GitHub Actions workflow to build/start the container stack and run integration tests with report/log artifact upload.
- Add/extend APIs around templates (update metadata/instructions, update license/terms, update access terms), plus enrich role-assignment JSON output and add an Access API endpoint for formatted datafile citations.
- Update “requires files to publish” behavior to apply to submit-for-review, and update UI/indexing to display original filenames/sizes (notably for tabular/ingested files); refresh Croissant export fixtures and related docs.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/resources/croissant/validate.sh |
Adds a helper script to validate croissant JSON exports. |
src/test/resources/croissant/requirements.txt |
Pins mlcroissant version for validation tooling. |
src/main/java/edu/harvard/iq/dataverse/export/croissant/CroissantExportUtil.java |
Updates schema.org context URLs to https://… and notes validation script. |
src/test/resources/croissant/restricted/expected/restricted-croissantSlim.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/restricted/expected/restricted-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/minimal/expected/minimal-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/max/expected/max-croissantSlim.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/max/expected/max-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/junk/expected/junk-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/draft/expected/draft-croissantSlim.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/draft/expected/draft-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/cars/expected/cars-croissantSlim.json |
Updates expected croissant context URLs to https://…. |
src/test/resources/croissant/cars/expected/cars-croissant.json |
Updates expected croissant context URLs to https://…. |
src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java |
Updates tests for new JsonPrinter.injectSettingsService signature and role-assignee name behavior. |
src/test/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateTermsOfAccessCommandTest.java |
Adds unit tests for template terms-of-access update command. |
src/test/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateLicenseCommandTest.java |
Adds unit tests for template license/custom-terms update command. |
src/test/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateFieldsCommandTest.java |
Adds unit tests for template field/instruction update behavior. |
src/test/java/edu/harvard/iq/dataverse/api/UtilIT.java |
Adds REST-assured helpers for new endpoints and improves optional auth header handling. |
src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java |
Adjusts search query and expected file name indexing behavior. |
src/test/java/edu/harvard/iq/dataverse/api/InReviewWorkflowIT.java |
Adds an integration test for “files required to submit for review”. |
src/test/java/edu/harvard/iq/dataverse/api/FilesIT.java |
Adds integration tests for formatted file citation endpoint and guest access after publish. |
src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java |
Expands template API integration tests (create/update license/terms/access). |
src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java |
Extends publish checks to ensure draft version must contain files. |
src/test/java/edu/harvard/iq/dataverse/api/DataRetrieverApiIT.java |
Disables a flaky integration test in CI. |
src/main/webapp/file.xhtml |
Updates file page title/label/size display to show “original” file details. |
src/main/webapp/file-info-fragment.xhtml |
Updates file listing display to use original labels/sizes and tweaks tabular display hints. |
src/main/webapp/editFilesFragment.xhtml |
Adjusts file-name editing UI to edit base name + show original extension separately. |
src/main/webapp/dataset.xhtml |
Tightens submit-for-review button visibility and sanitizes publish disclaimer HTML. |
src/main/java/propertyFiles/Bundle.properties |
Adds new i18n strings (submit-for-review files required, template update messages), plus a new template parse-error key. |
src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinterHelper.java |
Injects RoleAssigneeServiceBean into JsonPrinter. |
src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java |
Adds more role-assignment JSON fields and includes datasetType id in JSON output. |
src/main/java/edu/harvard/iq/dataverse/util/json/JsonParser.java |
Adds instruction parsing helper and allows missing fields array in template/dataset parsing path. |
src/main/java/edu/harvard/iq/dataverse/search/IndexServiceBean.java |
Switches filename indexing to use label-for-original. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateTermsOfAccessCommand.java |
Introduces a new command to update template access terms. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateLicenseCommand.java |
Introduces a new command to update template license/custom terms of use. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateTemplateFieldsCommand.java |
Introduces a new command to update template fields + instructions (merge/replace). |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/SubmitDatasetForReviewCommand.java |
Adds enforcement for “requires files to publish” when submitting for review. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/PublishDatasetCommand.java |
Updates publish enforcement to check latest version file metadata list; delegates requires-files logic to base class. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractDatasetCommand.java |
Centralizes getEffectiveRequiresFilesToPublishDataset() for publish + submit-for-review. |
src/main/java/edu/harvard/iq/dataverse/api/dto/TemplateDTO.java |
Renames/repurposes DTO and moves instruction parsing to JsonParser. |
src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java |
Adds template update endpoints and adjusts default-template lookup behavior. |
src/main/java/edu/harvard/iq/dataverse/api/Access.java |
Adds formatted datafile citation endpoint; adjusts guestbook signed URL timing window. |
src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java |
Adds template lookup helper and tightens timestamp parsing rules. |
src/main/java/edu/harvard/iq/dataverse/ValidateDataFileLabel.java |
Adds a bean-validation annotation for file label validation. |
src/main/java/edu/harvard/iq/dataverse/FileLabelValidator.java |
Adds validator implementing illegal-character checks for file names. |
src/main/java/edu/harvard/iq/dataverse/PermissionsWrapper.java |
Adds UI-side gating for publish/submit-for-review when files are required. |
src/main/java/edu/harvard/iq/dataverse/FileMetadata.java |
Adds transient “label without extension” field + original label getter for tabular data. |
src/main/java/edu/harvard/iq/dataverse/DvObject.java |
Exposes dtype via getter for API serialization. |
src/main/java/edu/harvard/iq/dataverse/DatasetServiceBean.java |
Updates reminder-string logic for file-required publish/submit cases. |
src/main/java/edu/harvard/iq/dataverse/DataFile.java |
Adds friendly original file size and exposes derived original file name. |
src/main/java/edu/harvard/iq/dataverse/DataCitation.java |
Adds format lookup helper and uses original label for file citations. |
doc/sphinx-guides/source/api/native-api.rst |
Documents new template update endpoints and clarifies timestamp requirements. |
doc/sphinx-guides/source/api/dataaccess.rst |
Documents new Access API endpoint for formatted datafile citations. |
doc/sphinx-guides/source/api/changelog.rst |
Notes API behavioral change for submit-for-review when files are required. |
doc/sphinx-guides/source/_static/api/template-update-metadata.json |
Adds example payload for template metadata update. |
doc/sphinx-guides/source/_static/api/template-update-license.json |
Adds example payload for template license update. |
doc/sphinx-guides/source/_static/api/template-update-terms.json |
Adds example payload for template custom terms update. |
doc/sphinx-guides/source/_static/api/template-update-access.json |
Adds example payload for template access terms update. |
doc/release-notes/7956-show_original_file_format.md |
Release note for showing/editing original file name details. |
doc/release-notes/12340-timing-issue-guestbook-response.md |
Release note for guestbook signed URL timing fix. |
doc/release-notes/12258-publish-submit-contains-files.md |
Release note for requiring files for publish/submit-for-review. |
doc/release-notes/11920-more-ra-info.md |
Release note for enriched role assignment JSON output. |
doc/release-notes/11912-edit-template-apis |
Release note for new template update endpoints. |
doc/release-notes/11733-api-get-file-citation-format.md |
Release note for formatted datafile citation endpoint. |
.github/workflows/container_integration_tests.yml |
Adds a new container-based integration test workflow (build, start, test, upload artifacts/logs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (customTermsOfUseAndAccess == null) { | ||
| throw new InvalidCommandArgumentsException(BundleUtil.getStringFromBundle("updateDatasetLicenseCommand.errors.customTermsOfUseNotProvided"), this); | ||
| } |
There was a problem hiding this comment.
The exception message key updateDatasetLicenseCommand.errors.customTermsOfUseNotProvided is about license/terms of use, but this command updates terms of access. Using the license error key will produce a misleading API error. Please add/use an access-specific bundle key/message here.
| JsonObjectBuilder job = jsonObjectBuilder() | ||
| .add("id", ra.getId()) | ||
| .add("assignee", ra.getAssigneeIdentifier()) | ||
| .add("assigneeName", roleAssigneeService.getRoleAssignee(ra.getAssigneeIdentifier()).getDisplayInfo().getTitle()) | ||
| .add("roleId", ra.getRole().getId()) | ||
| .add("roleName", ra.getRole().getName()) |
There was a problem hiding this comment.
JsonPrinter.json(RoleAssignment) unconditionally calls roleAssigneeService.getRoleAssignee(...).getDisplayInfo().getTitle(). RoleAssigneeServiceBean.getRoleAssignee can return null (e.g., unknown built-in identifier or missing user/group), which would cause a 500/NPE when serializing role assignments. Please handle a null RoleAssignee (and/or missing roleAssigneeService) by falling back to the raw identifier or an empty string for assigneeName instead of dereferencing blindly.
| @PUT | ||
| @AuthRequired | ||
| @Path("{templateId}/licenseTerms") | ||
| public Response updateTemplateLicenseTerms(@Context ContainerRequestContext crc, LicenseUpdateRequest requestBody, @PathParam("templateId") Long templateId, @QueryParam("replace") boolean replaceData) { |
There was a problem hiding this comment.
updateTemplateLicenseTerms declares a @QueryParam("replace") boolean replaceData parameter but never uses it. This is confusing for API consumers and makes the signature look like it supports replace semantics when it doesn't. Either implement the behavior or remove the unused query param.
| public Response updateTemplateLicenseTerms(@Context ContainerRequestContext crc, LicenseUpdateRequest requestBody, @PathParam("templateId") Long templateId, @QueryParam("replace") boolean replaceData) { | |
| public Response updateTemplateLicenseTerms(@Context ContainerRequestContext crc, LicenseUpdateRequest requestBody, @PathParam("templateId") Long templateId) { |
| dataverses.api.update.template.license.success=Template license updated | ||
| dataverses.api.update.template.terms.success=Template Terms of Use updated | ||
| dataverses.api.update.template.access.success=Template Terms of Access updated | ||
| dataverses.api.editMetadata.template.error.parseUpdate==Error parsing template update: {0} |
There was a problem hiding this comment.
Property key dataverses.api.editMetadata.template.error.parseUpdate==... has an extra = in the key/value separator, which will make the value start with a literal = (and the intended key may not be found). Change it to a single = (and verify the key name matches what callers use).
| private void applyCustomTerms(TermsOfUseAndAccess target, TermsOfUseAndAccess source) { | ||
|
|
||
| target.setFileAccessRequest(source.isFileAccessRequest()); | ||
| target.setTermsOfAccess(source.getTermsOfAccess()); | ||
| target.setDataAccessPlace(source.getDataAccessPlace()); | ||
| target.setOriginalArchive(source.getOriginalArchive()); | ||
| target.setAvailabilityStatus(source.getAvailabilityStatus()); | ||
| target.setContactForAccess(source.getContactForAccess()); | ||
| target.setSizeOfCollection(source.getSizeOfCollection()); | ||
| target.setStudyCompletion(source.getStudyCompletion()); | ||
| } |
There was a problem hiding this comment.
applyCustomTerms does not copy confidentialityDeclaration from the parsed TermsOfUseAndAccess source, even though JsonParser.parseTermsOfAccess populates it. As a result, /api/dataverses/{templateId}/access updates will silently drop/ignore this field. Include target.setConfidentialityDeclaration(source.getConfidentialityDeclaration()) (and any other Terms of Access fields you intend to support) to keep behavior consistent with parsing and docs.
| } else { | ||
| Map <String, String> currentInstructionsMap = template.getInstructionsMap(); | ||
| Map <String, String> merged = new HashMap<>(currentInstructionsMap); | ||
| if (instructions != null) { | ||
| merged.putAll(instructions); | ||
| template.setInstructionsMap(merged); | ||
|
|
||
| } else { | ||
| template.getInstructionsMap().clear(); | ||
| } | ||
|
|
||
| template.updateInstructions(); | ||
|
|
||
| } |
There was a problem hiding this comment.
updateInstructions clears all existing template instructions whenever instructions is null and replaceData is false (template.getInstructionsMap().clear()). In Dataverses.updateTemplateMetadata, instructions will be null whenever the request body omits the instructions key, so a metadata-only update (without replace=true) would unexpectedly wipe instructions. Consider treating instructions == null as "no change" (keep current instructions) and only clearing when the client explicitly sends an empty instructions: [] (or add a separate flag to clear).
| } else { | |
| Map <String, String> currentInstructionsMap = template.getInstructionsMap(); | |
| Map <String, String> merged = new HashMap<>(currentInstructionsMap); | |
| if (instructions != null) { | |
| merged.putAll(instructions); | |
| template.setInstructionsMap(merged); | |
| } else { | |
| template.getInstructionsMap().clear(); | |
| } | |
| template.updateInstructions(); | |
| } | |
| } else if (instructions != null) { | |
| Map <String, String> currentInstructionsMap = template.getInstructionsMap(); | |
| Map <String, String> merged = new HashMap<>(currentInstructionsMap); | |
| merged.putAll(instructions); | |
| template.setInstructionsMap(merged); | |
| template.updateInstructions(); | |
| } |
| List<DatasetField> fields = new ArrayList(); | ||
| if (fieldsJson != null) { | ||
| fields = parseFieldsFromArray(fieldsJson, false, replaceData); |
There was a problem hiding this comment.
parseMultipleFields uses a raw type (new ArrayList()) which will generate unchecked warnings. Please use generics (new ArrayList<>()) to match the method return type and keep compilation clean.
| name: Container Integration Tests Workflow | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
|
|
||
| push: | ||
| branches: | ||
| - develop |
There was a problem hiding this comment.
The PR title suggests this change is primarily about adding a GitHub Actions workflow for CI redundancy, but the diff also includes multiple functional/API/UI changes (template update APIs, role assignment JSON output, file citation endpoint, file label/original filename handling, publish/submit file checks, etc.). Please update the PR title/description to reflect the full scope or split into smaller PRs to make review and risk assessment manageable.
|
|
||
| TermsOfUseAndAccess toua = jsonParser().parseTermsOfAccess(json); | ||
|
|
||
| if (publicInstall && (toua.isFileAccessRequest() || !toua.getTermsOfAccess().isEmpty())){ |
There was a problem hiding this comment.
updateTemplateTermsOfAccess can throw NPEs for malformed/partial JSON: jsonParser().parseTermsOfAccess(json) will dereference customTermsOfAccess without checking it exists, and toua.getTermsOfAccess().isEmpty() will NPE when termsOfAccess is null (which parseTermsOfAccess allows). Add explicit validation for presence of customTermsOfAccess and treat termsOfAccess as nullable (e.g., null/blank check) before calling .isEmpty().
| TermsOfUseAndAccess toua = jsonParser().parseTermsOfAccess(json); | |
| if (publicInstall && (toua.isFileAccessRequest() || !toua.getTermsOfAccess().isEmpty())){ | |
| if (!json.containsKey("customTermsOfAccess") | |
| || json.isNull("customTermsOfAccess") | |
| || json.get("customTermsOfAccess").getValueType() != ValueType.OBJECT) { | |
| return error(BAD_REQUEST, "Required object 'customTermsOfAccess' is missing."); | |
| } | |
| TermsOfUseAndAccess toua = jsonParser().parseTermsOfAccess(json); | |
| String termsOfAccess = toua.getTermsOfAccess(); | |
| boolean hasTermsOfAccess = termsOfAccess != null && !termsOfAccess.isBlank(); | |
| if (publicInstall && (toua.isFileAccessRequest() || hasTermsOfAccess)){ |
Test Results2 497 tests 2 443 ✅ 30m 14s ⏱️ Results for commit 588ff79. ♻️ This comment has been updated with latest results. |
Replaced local file copy with curl command to fetch SUSHI config.
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: