Allow replacing a draft file#7337
Conversation
when replacing a draft-only file that was already a replacement itself
…QualitativeDataRepository/dataverse.git into IQSS/7149-Replace_file_in_draft
|
OK some notes on integration test failures: A couple were related to the PR code (a bug and a test that expected an error when trying to replace a draft file which now succeeds). The rest appear to be unrelated to the PR code (which was fun to debug since other branches appear to pass the tests - in some cases that appears to be timing and in others because build failures lead to IT tests not running without the green/red pass/fail indicator turning red.) A couple appear to be intermittent and probably related to timing: a second call to update a dataset fails when the same API call works manually. E.g. test_006_ReplaceFileGood and testForceReplaceAndUpdate in a recent build. I suspect that these need the same type of UtilIT.sleepForLock() calls that some of the ingest methods have, though my guess would be that the delay would be shorter. These have been added. (testCuratorSendsCommentsToAuthor also failed that build with a wait for lock error so things were definitely slow). Two others appear to be cases where the code and tests had ~matching minor errors (in 5.1.1 but the code in question is several years old so probably before that as well). In testAddFileBadJson and testReplaceFileBadJson, sending some types of bad json (such as a simple string in the tests) resulted in a 200/OK status and the file being added when it presumably should have failed due to bad json. The code was catching, logging, and ignoring a ClassCastException to do that. Other types of bad json, such as "{}:string" did fail but weren't tested. In the updated code, I stopped catching the ClassCastException in OptionalFileParams and then caught both that and the possible json parsing exception in the same clause in the calling classes to generate a 400 error (now with a Bundle message!). The behavior of the code and tests should match. |
|
OK, after significantly more work this week, I think this is ready to go. To get the new and existing functionality to all work, I had to dig back into the merge issues brought up in #5847 and I think I understand more, if not all of the problems that could occur in that code/ all of the different input states (in terms of whether filemetadata, datafiles, datasetversions are persisted, merged, not merged coming into the code). The rewrite of the file delete code has more commenting that can hopefully guide anyone making changes after this. To get all of the tests to work, I had to do some further tweaking to the sleepForLock code to handle null for the lock type. In doing that I realized that when the replace api call fails due to a lock it appears to still send an OK, but fails the test due to differences in what's in the "data" part of the response. There, and a few other places, I've added a ToDo - if any of those are worth adding an issue for, they could probably be small ones. |
|
Thanks @qqmyers - moving to code review |
| </ui:fragment> | ||
| <ui:fragment rendered="#{FilePage.fileMetadata.dataFile.released and FilePage.draftReplacementFile == false and !FilePage.fileMetadata.dataFile.filePackage}"> | ||
| <ui:fragment rendered="#{FilePage.draftReplacementFile == false and !FilePage.fileMetadata.dataFile.filePackage}"> | ||
| <li><!-- start: replace file link --> |
There was a problem hiding this comment.
We're getting multiple replace file links here because of the change in render. Once a released file has been replaced it can be replaced again, so taking out the "isReleased" gets two Replace links to render
There was a problem hiding this comment.
Good catch. The problem is the next lines though - the second Replace button that would trigger the warning dialog to not replace a file that's already being replaced in the draft version was showing and it shouldn't have. For now, I've just adjusted the logic there (only show the dialog if there is a draft replacement for the file, which is not true for the replacing file.)
The logic is simpler too. FWIW, I went back about 4 years in github to see if I could find out why the current terms were in there and didn't find a clear reason, so perhaps someone else might have a better sense and be able to see if there's anything my change misses?
However, I'm also not sure that dialog is needed anymore - a file that has been replaced in the latest version now shows a dialog when you click the 'Edit' button itself, so you never get to the individual menu items like replace and delete and therefore can't trigger the dialogs related to them. If it's true that there is no way to get to those items for a file that has been replaced/deleted in the current version, they (those replace and delete items and the dialogs they trigger) should probably be deleted (new issue?).
| // There are two datafile cases as well - the file has been released, so we're | ||
| // jsut removing it from the current draft version or it is only in the draft | ||
| // version and we completely remove the file. | ||
| if (!fmd.getDataFile().isReleased()) { |
There was a problem hiding this comment.
Getting a null pointer here when I try to delete a file via the EditDataFilesPage from the Dataset Page. Will investigate further.
There was a problem hiding this comment.
@sekmiller - The new commits should fix this - the test of the two dataset versions in line 208 needed to use ! .equals() instead of !=, and then an fmd merge was needed (removing the else around line 212.
There was a problem hiding this comment.
Ok. i'm able to delete a file from a draft version. Thanks!
Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDatasetVersionCommand.java
| // datafile | ||
| if (fmToDelete.getDataFile().getStorageIdentifier().equals(fmd.getDataFile().getStorageIdentifier())) { | ||
| // and a match on the datasetversion | ||
| if (fmToDelete.getDatasetVersion().getId() == null) { |
There was a problem hiding this comment.
Getting a null pointer here when I try the uncommon, but previously supported, action of uploading a number of files, but then deleting one before saving the rest to the draft version
There was a problem hiding this comment.
Resolved - the new util method assumed that an fmd would always have an associated datasetversion which is not true for this case. - BTW - thanks for the thorough testing!
sekmiller
left a comment
sekmiller
left a comment
There was a problem hiding this comment.
Looks good overall. Thanks Jim, for addressing the issues I've found. Gustavo asked me to keep it out of QA for now pending the merging of kebabs and any notes coming from design.
@kcondon sees a DB error persisting a file with null createtime during the GlobalIDServiceBean.getBean call which uses a set namedQuery to find the :DoiProvider. Create times for files are set above, but not merged prior to calling registerFilePidsIfNeeded. Assuming the namedQuery is forcing the file (without a merge) to persist which triggers the error. In IQSS#7337, the code is reworked so there is a merge prior to registerFilePidsIfNeeded. This commit adds one temporarily so this PR works indepdently of the other.
IQSS/7149-Replace_file_in_draft Conflicts: src/main/webapp/file.xhtml
IQSS/7149-Replace_file_in_draft Conflicts: src/main/java/edu/harvard/iq/dataverse/EditDatafilesPage.java
IQSS/7149-Replace_file_in_draft Conflicts: src/main/java/edu/harvard/iq/dataverse/datasetutility/FileReplacePageHelper.java src/main/java/propertyFiles/Bundle.properties
|
@TaniaSchlatter at long last I've gotten the ability to spin up EC2 instances on my laptop, and I've stood up this branch on AWS. It works as I expect. A few weeks ago I had flagged this for UI/UX review as it enables the workflow to replace draft files and to retain metadata on replace, which @qqmyers has outlined above. If you'd like to review let me know. I don't want to keep the instance up and incur cost, so I'll spin it down for now, but let me know when you're ready to take a look and I'll spin up another instance. I'll need ~20 min to get the instance going so just give me a little warning and we can step through it together or you can review independently. |
|
Reviewed the AWS instance. It would be consistent to have Replace in the kebab on the file table on the dataset page, as well as under "Edit" on the file page. |
|
Thanks @TaniaSchlatter - that's covered in #7376 so we'll implement this with the current workflows and get the consistency on entry points across dataset kebab/file page in #7376. |
* file pid reservation * add file pid reservation step to publish (analogous to dataset pid register if needed) Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDatasetVersionCommand.java src/main/java/propertyFiles/Bundle.properties * comment change * check if file PIDs used once, use constants - per comments * adding release note * release notes, API doc update * reflecting datasets and files for the PID endpoint * removing release note about pre-reg for file PIDs as this is not supported * file pid pre-reservation Conflicts: src/main/java/edu/harvard/iq/dataverse/engine/command/impl/AbstractDatasetCommand.java src/main/java/propertyFiles/Bundle.properties * avoid problem when GlobalIDServiceBean implicitly merges @kcondon sees a DB error persisting a file with null createtime during the GlobalIDServiceBean.getBean call which uses a set namedQuery to find the :DoiProvider. Create times for files are set above, but not merged prior to calling registerFilePidsIfNeeded. Assuming the namedQuery is forcing the file (without a merge) to persist which triggers the error. In #7337, the code is reworked so there is a merge prior to registerFilePidsIfNeeded. This commit adds one temporarily so this PR works indepdently of the other. * update theDataset * noting that PID reservation can cause old timeouts to be too short * more specifics * release note update * cleanup reformatting * further cleanup * set createTime earlier --------- Co-authored-by: Danny Brooke <danny_brooke@harvard.edu>
7 participants
What this PR does / why we need it: Enables replacing a file in draft, keeping the file metadata except label by default (user can then edit before saving if desired). (Note - not using the old file name is both because it is confusing and because one of the use cases is to, for example, replace a doc file with a pdf-a version that needs a different extension to get proper mimetyping).
Which issue(s) this PR closes:
Closes #7149
Closes #4380
Special notes for your reviewer: Also ties to #3913. There was some discussion about using a different name instead of 'replace' for this since the effect is slightly different for draft files when there is no prior released version (the new file gets the DOI of the old file because the old one is fully deleted, versus the new file getting a new DOI). Thinking through this further, it seems to be weirder to me to have ~replace on a draft file get called two different things depending on whether a released version exists, and it seems fairly natural to me for the DOI to be reused when it can. I also don't know what else I would call this. If there's still interest in making this a separate call and there's a name suggestion, I can update the PR.
Suggestions on how to test this: Replace of published files should work as before, but it should be possible to replace a file in a draft version as well. When doing so, metadata including description, tags, categories, path, on the file should remain by default (and be editable prior to hitting save as normal). Replacing a file that is only available in draft should delete the replaced file entry in the db and remove the physical file file from storage. The new file should retain the DOI of the one removed. Replacing a file in draft that has previously been released should not remove the old file (still available in prior versions) and it should get a new DOI.
Should work in both UI and API.
Should also text that #5847 (comment) doesn't reappear as this PR reverts one of the changes made in that issue (hopefully compensating to address the problem in a different way).
Does this PR introduce a user interface change? If mockups are available, please link/include them here: replace option available for draft files, file metadata is retained from replaced file by default.
Is there a release notes update needed for this change?:
Additional documentation: