Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deaccessioned dataset file edit fix. #10901

Merged
merged 10 commits into from
Oct 3, 2024

Conversation

jp-tosca
Copy link
Contributor

@jp-tosca jp-tosca commented Oct 1, 2024

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:

  1. Deploy develop branch
  2. Create a dataset with file
  3. Publish the dataset
  4. Deaccession the dataset
  5. Try to delete a file and you will see an error
  6. Get the id of the file (lmk if you need help with this step)
  7. Try to delete the file with http://localhost:8080/api/files/ and you will succeed (you shouldn't)
  8. 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.
  9. 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

@jp-tosca jp-tosca self-assigned this Oct 1, 2024
@jp-tosca jp-tosca added Type: Bug a defect Feature: Deaccession Size: 10 A percentage of a sprint. 7 hours. labels Oct 1, 2024
@jp-tosca jp-tosca changed the title Fix Deaccessioned edition. Deaccessioned dataset file edit fix. Oct 1, 2024
@@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -

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.

This comment has been minimized.

@jp-tosca jp-tosca marked this pull request as ready for review October 1, 2024 18:36
@jp-tosca jp-tosca removed their assignment Oct 1, 2024

This comment has been minimized.

2 similar comments

This comment has been minimized.

This comment has been minimized.

@@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

This comment has been minimized.

1 similar comment

This comment has been minimized.

This comment has been minimized.

@jp-tosca
Copy link
Contributor Author

jp-tosca commented Oct 2, 2024

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.

This comment has been minimized.

@jp-tosca jp-tosca removed the Size: 10 A percentage of a sprint. 7 hours. label Oct 2, 2024
@jp-tosca 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.

Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

// Start of deaccession test.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

github-actions bot commented Oct 3, 2024

📦 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.

@jp-tosca jp-tosca removed their assignment Oct 3, 2024
@ofahimIQSS ofahimIQSS self-assigned this Oct 3, 2024
@ofahimIQSS
Copy link
Contributor

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

@ofahimIQSS ofahimIQSS merged commit 7be6c1b into develop Oct 3, 2024
19 checks passed
@ofahimIQSS ofahimIQSS deleted the 9351-edit-optionds-deaccessioned-dataset-fix branch October 3, 2024 18:13
@ofahimIQSS ofahimIQSS removed their assignment Oct 3, 2024
@pdurbin pdurbin added this to the 6.5 milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Deaccession FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) Type: Bug a defect
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

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