Deaccessioned dataset file edit fix. #10901

jp-tosca · 2024-10-01T16:28:17Z

What this PR does / why we need it:
It was reported on #9351 that trying to update any of the files for a deaccessioned dataset would result in an error message.

My test showed that this would only happen when the only previous version available is deaccessioned, for example if you create a dataset and have version 1, then make version 2 and deaccession version 2 it will not show this error.

While reviewing this issue we found that even if the UI would fail this was possible on the API due a parameter sent that would cause to bypass a check, this caused that even the new test would pass with the old code.

You can test this by:

Deploy develop branch
Create a dataset with file
Publish the dataset
Deaccession the dataset
Try to delete a file and you will see an error
Get the id of the file (lmk if you need help with this step)
Try to delete the file with http://localhost:8080/api/files/ and you will succeed (you shouldn't)
Undeploy and make the changes to the file Files.java on your develop branch, deploy and repeat steps 1-7, you should get an error message on both.
This is where the fix comes in, once that the fix from this branch from UpdateDatasetVersionCommand are deployed the functionality should be restored.

Which issue(s) this PR closes:

Closes #9351

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

qqmyers · 2024-10-01T16:39:36Z

src/main/java/edu/harvard/iq/dataverse/Dataset.java

@@ -335,7 +335,7 @@ public DatasetVersion getLatestVersion() {

    public DatasetVersion getLatestVersionForCopy() {
        for (DatasetVersion testDsv : getVersions()) {
-            if (testDsv.isReleased() || testDsv.isArchived()) {
+            if (testDsv.isReleased() || testDsv.isArchived() || testDsv.isDeaccessioned()) {


This looks like it will fix the npe problem, but this method is used in many places and I'm not sure that all of them should include a deaccessioned version. For example -

dataverse/src/main/java/edu/harvard/iq/dataverse/export/ExportService.java

Line 152 in 4c1e069

for (FileMetadata fm : dataset.getLatestVersionForCopy().getFileMetadatas()) {

would export info about a deaccessioned version. We might need a separate getLatestPublicVersion method for such cases unless there's some other way to fix.

qqmyers · 2024-10-01T20:07:06Z

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

@@ -115,7 +115,7 @@ public Dataset execute(CommandContext ctxt) throws CommandException {
         */
        if(persistedVersion==null) {
            Long id = getDataset().getLatestVersion().getId();
-            persistedVersion = ctxt.datasetVersion().find(id!=null ? id: getDataset().getLatestVersionForCopy().getId());
+            persistedVersion = ctxt.datasetVersion().find(id!=null ? id : getDataset().getLatestVersionForCopy(true).getId());


Adding the true parameter so that only this call is affected resolved my earlier comment. Thinking further on this particular use - we're getting the persistedVersion so we can see if there are changes to a system metadatablock (line 122) in which case you need a key to make changes. It's an interesting question whether that comparison should be versus the last public/published version if there is one or against a later deaccessioned one (if it exists). Probably rare enough that we should just ignore my comment and accept the fix as is.

…mand.

jp-tosca · 2024-10-02T22:17:40Z

Changes should be good to review @qqmyers, the copy still needs to be created on the api but not sent as a parameter, after making these changes both UI and the API should fail before the changes and the change should resolve this issue for both.

qqmyers

This looks fine (I made minor comment changes).
For posterity - this does change the currently inconsistent behavior between the UI and API for file deletion - the latter was triggering edit-draft logging while the former did not. It sounds like this isn't used much now, and, in #10890, I'm proposing further changes to this - basically making edit-draft logging an optional feature and then working to enable it for any/all operation that update the dataset version if enabled (versus it only happening for methods that send in a clone datasetversion now). So - in talking with @jp-tosca, we thought making the api consistent with the UI, which also allows an IT test for this bug, is a reasonable approach in this PR.

qqmyers · 2024-10-03T14:07:49Z

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

@@ -667,6 +667,60 @@ public void testCreatePublishDestroyDataset() {
        deleteDatasetResponse.prettyPrint();
        assertEquals(200, deleteDatasetResponse.getStatusCode());

+        // Start of deaccession test.


Wow - the testCreatePublishDestroyDataset method is doing quite a bit more than testing destroy! +1 for splitting it up some day.

github-actions · 2024-10-03T14:22:25Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:9351-edit-optionds-deaccessioned-dataset-fix

ghcr.io/gdcc/configbaker:9351-edit-optionds-deaccessioned-dataset-fix

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

ofahimIQSS · 2024-10-03T18:13:32Z

I tested this ticket a little bit differently. I pushed the ticket to internal to test the fix. Fix looks good and no error message was displayed after deleting a file from a deaccessioned dataset. I was able to reproduce the defect in demo (without the fix) so I could see what the issue was. I attached screenshots of the testing below.
10901 Testing.docx

Fix Deaccessioned edition.

97a38ea

jp-tosca self-assigned this Oct 1, 2024

jp-tosca added Type: Bug a defect Feature: Deaccession Size: 10 A percentage of a sprint. 7 hours. labels Oct 1, 2024

jp-tosca changed the title ~~Fix Deaccessioned edition.~~ Deaccessioned dataset file edit fix. Oct 1, 2024

qqmyers reviewed Oct 1, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Patch notes

2aad633

jp-tosca marked this pull request as ready for review October 1, 2024 18:36

jp-tosca removed their assignment Oct 1, 2024

jp-tosca added 2 commits October 1, 2024 14:55

Added a signature of the method to receive includeDeaccessioned param

db1a066

Missing call to the new method

c2e1e15

This comment has been minimized.

Sign in to view

qqmyers reviewed Oct 1, 2024

View reviewed changes

qqmyers assigned jp-tosca Oct 1, 2024

cmbz added the FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) label Oct 2, 2024

jp-tosca added 2 commits October 2, 2024 13:57

Test added

c46e0ce

Typo

2f4a84c

This comment has been minimized.

Sign in to view

Remove of clone from the API file deletion.

e57f834

This comment has been minimized.

Sign in to view

jp-tosca added 2 commits October 2, 2024 18:13

Copy still needs to be created but not sent as a parameter to the com…

8accad7

…mand.

File upload assertion

70a2651

This comment has been minimized.

Sign in to view

jp-tosca removed the Size: 10 A percentage of a sprint. 7 hours. label Oct 2, 2024

jp-tosca added the Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) label Oct 2, 2024

This comment has been minimized.

Sign in to view

Update DatasetsIT.java

4e2bc1a

qqmyers approved these changes Oct 3, 2024

View reviewed changes

jp-tosca removed their assignment Oct 3, 2024

ofahimIQSS self-assigned this Oct 3, 2024

ofahimIQSS merged commit 7be6c1b into develop Oct 3, 2024
19 checks passed

ofahimIQSS deleted the 9351-edit-optionds-deaccessioned-dataset-fix branch October 3, 2024 18:13

ofahimIQSS removed their assignment Oct 3, 2024

jp-tosca mentioned this pull request Oct 3, 2024

Editing a deaccessioned dataset: the file edit options should allow the "delete" function to work #9351

Closed

pdurbin added this to the 6.5 milestone Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deaccessioned dataset file edit fix. #10901

Deaccessioned dataset file edit fix. #10901

jp-tosca commented Oct 1, 2024 •

edited

Loading

qqmyers Oct 1, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

qqmyers Oct 1, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jp-tosca commented Oct 2, 2024

This comment has been minimized.

This comment has been minimized.

qqmyers left a comment

qqmyers Oct 3, 2024

github-actions bot commented Oct 3, 2024

ofahimIQSS commented Oct 3, 2024

Deaccessioned dataset file edit fix. #10901

Deaccessioned dataset file edit fix. #10901

Conversation

jp-tosca commented Oct 1, 2024 • edited Loading

qqmyers Oct 1, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

qqmyers Oct 1, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jp-tosca commented Oct 2, 2024

This comment has been minimized.

This comment has been minimized.

qqmyers left a comment

Choose a reason for hiding this comment

qqmyers Oct 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024

ofahimIQSS commented Oct 3, 2024

jp-tosca commented Oct 1, 2024 •

edited

Loading