Published datasets should contain files #10994

stevenwinship · 2024-10-31T18:35:18Z

What this PR does / why we need it: The existing publishing model in HDV allows the publishing of datasets without files. This lends to the curation team contacting depositors about uploading files via support and often to deleting "empty" datasets.

To decrease the support contact and dataset deletions, change the publishing model to require all deposits contain at least one file before publishing can happen.

Which issue(s) this PR closes:#10981

Closes Publishing: Published datasets should contain files #10981

Special notes for your reviewer:

Suggestions on how to test this: See the added IT test

Does this PR introduce a user interface change? If mockups are available, please link/include them here: No

Is there a release notes update needed for this change?: Included

Additional documentation:

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/PublishDatasetCommand.java

…iles

coveralls · 2024-11-06T21:25:43Z

coverage: 22.451% (+0.004%) from 22.447%
when pulling d12d9f6 on 10981-published-datasets-should-contain-files
into f95c1a0 on develop.

qqmyers

Looks good. I've suggested that the Harvard team verify the UI is acceptable and made a note questioning whether we need a feature flag for this, but assuming no other feedback, the only change I'd suggest is just moving/renaming one method.

qqmyers · 2024-11-21T12:46:03Z

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/PublishDatasetCommand.java

        }
    }
-
+    private boolean requiresFilesToPublishDataset() {


I think in other cases we've used the patter getEffectiveRequireFilesToPublishDataset which might be useful for consistency. I also think this should probably be in Dataverse.java with the get/set - when there's a UI, we need to know the effective setting at other times than just publishing.

renamed and added to Dataverse.Java

qqmyers · 2024-11-21T12:49:00Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

@@ -292,6 +292,9 @@ public static JsonObjectBuilder json(Dataverse dv, Boolean hideEmail, Boolean re
        if (dv.getFilePIDsEnabled() != null) {
            bld.add("filePIDsEnabled", dv.getFilePIDsEnabled());
        }
+        if (dv.getRequireFilesToPublishDataset() != null) {
+            bld.add("requireFilesToPublishDataset", dv.getRequireFilesToPublishDataset());
+        }


FWIW: My guess is that the SPA will want the effective settings for this and other things (file pids, storage driver, etc.). Not sure if that would go here or not. I guess it can wait until we address the whole set.

swapped the requireFilesToPublishDataset for effectiveRequiresFilesToPublishDataset

qqmyers · 2024-11-21T12:54:18Z

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/PublishDatasetCommand.java

+
+            if (getDataset().getFiles().isEmpty() && requiresFilesToPublishDataset()) {
+                throw new IllegalCommandException(BundleUtil.getStringFromBundle("dataset.mayNotPublish.FilesRequired"), this);
+            }


This gives a warning in the current UI, right? It would probably be better to just disable the publish button, or have a persistent message that files must be added before this dataset can be published, but if just failing with a warning is OK with @sbarbosadataverse, et. al., the current code avoids changes to the current UI code.

@stevenwinship - can you post a UI image for what this looks like?

@stevenwinship images like this are so helpful. Can you please also add it under "Does this PR introduce a user interface change?" above?

doc/release-notes/10981-published-datasets-should-contain-files.md

qqmyers · 2024-11-21T12:59:34Z

src/main/resources/db/migration/V6.5.0.1.sql

@@ -0,0 +1,2 @@
+-- files are required to publish datasets


Guessing this will end up being V6.4.0.3

I'll rename it but I still think anything going into 6.5 should be v6.5...

FWIW: I think historically we did this because no one knew if the next version would be 6.4.1, 6.5, or 7.0. We could revisit this convention.

Exactly. The next version is unknown.

qqmyers · 2024-11-21T13:02:53Z

src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java

+        createUserResponse.then().assertThat().statusCode(OK.getStatusCode());
+        String apiToken = UtilIT.getApiTokenFromResponse(createUserResponse);
+
+        // Create and publish a top level Dataverse (under root) with a requireFilesToPublishDataset set to true


Kudos for test comments!

…iles

sbarbosadataverse · 2024-11-21T16:38:51Z

I have one comment @stevenwinship can we say "published datasets should contain at least one 'data' file? To help users understand we want data and not just a pdf of am article?

stevenwinship · 2024-11-21T18:40:41Z

Changed the message

ofahimIQSS · 2024-11-22T22:15:11Z

@stevenwinship Looks like the branch has conflicts which must be resolved.

…iles

…://github.com/IQSS/dataverse into 10981-published-datasets-should-contain-files

github-actions · 2024-11-25T15:27:23Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10981-published-datasets-should-contain-files

ghcr.io/gdcc/configbaker:10981-published-datasets-should-contain-files

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

stevenwinship self-assigned this Oct 31, 2024

stevenwinship added Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) labels Oct 31, 2024

qqmyers reviewed Oct 31, 2024

View reviewed changes

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/PublishDatasetCommand.java Outdated Show resolved Hide resolved

stevenwinship added 4 commits November 1, 2024 13:03

Published datasets should contain files

a8f09e6

cosmetic fixes

1159134

change Admin to superuser in docs

c0c4c89

change how hierarchy works

85af55f

stevenwinship force-pushed the 10981-published-datasets-should-contain-files branch from d5bb6c2 to 85af55f Compare November 1, 2024 17:04