6558 validate files on publish #6790

landreev · 2020-04-03T01:31:08Z

What this PR does / why we need it:
See the issue; and the release note: https://github.com/IQSS/dataverse/blob/6558-validate-files-on-publish/doc/release-notes/6558-validate-files-on-publish.md

Which issue(s) this PR closes:

Closes #6558

Special notes for your reviewer:

Suggestions on how to test this:
Create a dataset, upload some files, mess up one of the files in the dataset directory or in the S3 bucket... try to publish.
Try it with both the synchronous and asynchronous ("too many files") publishing modes.
Does this PR introduce a user interface change?:
There is a new UI error message notifying the user about the failure to publish, explaining to them that some of their files do not pass validation, and telling them that they must contact support in order to fix it, before they can make another attempt to publish.
It uses the standard "red" error banner system at the top of the page.

Is there a release notes update needed for this change?:
yes, above.
Additional documentation:

…lication (#6558)

…6558)

…e. (#6558)

coveralls · 2020-04-03T01:38:42Z

Coverage decreased (-0.03%) to 19.626% when pulling a2079d2 on 6558-validate-files-on-publish into 076bc32 on develop.

sekmiller · 2020-04-06T17:29:40Z

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

+                    JH.addMessage(FacesMessage.SEVERITY_ERROR, BundleUtil.getStringFromBundle("dataset.publish.file.validation.error.message"),
+                            BundleUtil.getStringFromBundle("dataset.publish.file.validation.error.contactSupport"));
+                }
+                /* and now that we've shown the message to the user - remove the lock? */


Because we want the admin to intercede here we probably don't want to remove the lock, but should we update the edit data button so that if there is a file validation failure the user cannot add files? We would still allow them to make other edits?

Or maybe use the file validation failure lock to disable the publish button?

No, we don't want to remove that lock automatically - that was an idea I was entertaining during the development. I should remove that comment.
As for the second comment - that's exactly what that "failed validation" lock does. It disables the publish button, and will prevent an execution of the publish command, if called via API. But it shouldn't prevent the contributor from making changes. But yes, this should be documented clearly.

OK. I missed that the lock disables publish because it's if it's locked for any reason, not just file validation failure.

The way that page is dealing with/checking for various types of locks is a little scary.
Added a quick note to the "dataset management"/"publish dataset" section of the user guide.

sekmiller · 2020-04-06T18:20:58Z

...ain/java/edu/harvard/iq/dataverse/engine/command/impl/FinalizeDatasetPublicationCommand.java

+            for (DataFile dataFile : dataset.getFiles()) {
+                // TODO: Should we validate all the files in the dataset, or only 
+                // the files that haven't been published previously?
+                logger.log(Level.FINE, "validating DataFile {0}", dataFile.getId());


I think it would be desirable to attempt to validate all of the files before throwing an exception. It would make the admins job easier if they knew more than one file failed validation.

Thanks for the suggestion. I decided to quit on the first invalid file, so that it's faster. And then I tell the admin in the troubleshooting instruction that they need to verify all the files in the dataset.
I just had this idea: I should probably leave the finalizePublication command as is, stopping on the first file that fails - but then I should provide a one step API for the admin, that will validate all the files in the dataset (using the existing code), and spell out to them which ones need to be fixed - ?

OK, this is done.

…QSS/dataverse into 6558-validate-files-on-publish

…ve failed validation, that need to be fixed or removed before the dataset can be published. (#6558)

…in API (#6558)

…user guide. (#6558)

... from the troubleshooting guide; also moved around the diag. API and the example sections.

kcondon · 2020-04-10T00:33:33Z

@landreev
x1. Turning off validation does not seem to stop validation.
x2. There is an unusual error banner behavior, it displays initially as Error: with expected text about missing file and contact support as a link, then wait a few seconds, it refreshes, says Failed to Publish, then same text as before but says support as text, not link. Just seems like we'e duplicating functionality somewhere, otherwise works.
x 3. When in async mode (20 files for this test), dataset fails to publish with valid files, many logging entries, some regarding storage driver, ends with index error. No logging present on develop branch for same scenario. Update: I have also seen this fail but with index error only at end.
x4. Minor doc issue, preexisting but seems like we should fix since we now reference it 2x, :PIDAsynchRegFileCount does not have it's own entry in config/settings, only mentioned inline as a setting in file pid and now file validation settings.

landreev · 2020-04-10T00:36:31Z

Turning off validation does not seem to stop validation.

OK, will investigate and fix.

landreev · 2020-04-13T13:29:50Z

@kcondon
In item 3.:

No logging present on develop branch for same scenario. Update: I have also seen this fail but with index error only at end.

So does the dataset get published or not, w/ develop branch on dvn-build? (not 100 %sure how to read "same scenario" - so checking...)
As for the "logging", I'm going to assume that those were whitehat scan messages - is that safe?

kcondon · 2020-04-13T15:34:20Z

@landreev Yes, develop works

…ommand. #6558

…t's modified during the first stage of the async publishing. #6558.

landreev · 2020-04-14T13:02:57Z

Checked in fixes, should be ready to re-test.

landreev · 2020-04-14T13:29:00Z

@kcondon

4. Minor doc issue, preexisting but seems like we should fix since we now reference it 2x, :PIDAsynchRegFileCount does not have it's own entry in config/settings, only mentioned inline as a setting in file pid and now file validation settings.

It was really "preexisting", in that apparently it wasn't documented at all. I added that mention of it, to both the file reg. and the validation options sections. I wasn't sure it needed its own section, but sure, I just added its own paragraph.

landreev · 2020-04-14T14:22:57Z

There's one more thing - whether we want to validate every file, every time, where I was soliciting opinions on how to handle it. I'm leaning towards skipping it for minor versions; but validating every file, new and old on every major version. But this hasn't been implemented yet.

)

landreev · 2020-04-14T15:28:43Z

I'm leaning towards skipping it for minor versions; but validating every file, new and old on every major version.

OK, this has been checked in as well.

landreev added 16 commits March 27, 2020 09:03

the framework for the physical file validations (#655)

f1f5285

Physical file validation framework, refined; (#6558)

f149e12

one added TODO in the finalize command (#6558)

ad0960c

renamed DatasetLock.Reason.pidRegister DatasetLock.Reason.finalizePub…

9280665

…lication (#6558)

documentation for validate-files-on-publish. (#6558)

58ac83b

More changes/refinements, dedicated "validation failed" lock, etc. (#…

790a5e5

…6558)

refresh the changed error/lock message after a file validation failur…

fdfb767

…e. (#6558)

cleaned up lock refresh and messaging. (#6558)

a3df9c5

Merge branch 'develop' into 6558-validate-files-on-publish

d16bed5

a flyway script for purging any locks of type pidRegister.

7dd277d

another lock refresh tweak. (#6558)

3d21e9d

A release note for #6558.

811ab09

final (?) info messaging mechanism for the dataset page. (#6558)

2a6411a

rewrote/rearranged the troubleshooting guide for invalid files. (#6558)

bc6e37f

extra lock checking in the publish command. (#6558)

31f8df0

a typo/dropped word in the release notes. (#6558)

b102daf

typo

74badcb

sekmiller self-assigned this Apr 3, 2020

sekmiller reviewed Apr 6, 2020

View reviewed changes

landreev self-assigned this Apr 6, 2020

landreev added 5 commits April 6, 2020 22:38

Merge branch 'develop' into 6558-validate-files-on-publish

d126a72

Merge branch '6558-validate-files-on-publish' of https://github.com/I…

45cfa0f

…QSS/dataverse into 6558-validate-files-on-publish

Added /admin API call that an admin can use to find the files that ha…

c3fbad2

…ve failed validation, that need to be fixed or removed before the dataset can be published. (#6558)

Extra documentation entries for the validate files across dataset adm…

9ecb82c

…in API (#6558)

Another documentation entry, the "dataset management" section of the …

608228b

…user guide. (#6558)

kcondon self-assigned this Apr 8, 2020

landreev added 3 commits April 9, 2020 16:02

removed the incorrect/confusing "API" reference

c0340c8

... from the troubleshooting guide; also moved around the diag. API and the example sections.

cosmetic/style change

93160c9

"strongly recommened"

840f389

kcondon assigned kcondon and landreev and unassigned kcondon Apr 10, 2020

landreev added 3 commits April 13, 2020 18:14

rearranged the order of operations inside FinalizeDatasetPublicationC…

093ed6d

…ommand. #6558

one other rearrangment - changes where the dataset is merged, after i…

b055cad

…t's modified during the first stage of the async publishing. #6558.

Cleaned up messaging; removed some commented out code. #6558

402a624

djbrooke unassigned landreev Apr 14, 2020

added an entry for the file number limit for async. handling. #6558

bc389b3

kcondon self-assigned this Apr 14, 2020

as discussed, skipping file validation for minor version releases (#6558

a2079d2

)

kcondon merged commit 75f3b69 into develop Apr 14, 2020

kcondon deleted the 6558-validate-files-on-publish branch April 14, 2020 19:32

djbrooke added this to the Dataverse 5 milestone Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6558 validate files on publish #6790

6558 validate files on publish #6790

landreev commented Apr 3, 2020

coveralls commented Apr 3, 2020 •

edited

Loading

sekmiller Apr 6, 2020

sekmiller Apr 6, 2020

landreev Apr 6, 2020

sekmiller Apr 6, 2020

landreev Apr 7, 2020

sekmiller Apr 6, 2020

landreev Apr 6, 2020

landreev Apr 7, 2020

kcondon commented Apr 10, 2020 •

edited

Loading

landreev commented Apr 10, 2020

landreev commented Apr 13, 2020 •

edited

Loading

kcondon commented Apr 13, 2020 •

edited

Loading

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

6558 validate files on publish #6790

6558 validate files on publish #6790

Conversation

landreev commented Apr 3, 2020

coveralls commented Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcondon commented Apr 10, 2020 • edited Loading

landreev commented Apr 10, 2020

landreev commented Apr 13, 2020 • edited Loading

kcondon commented Apr 13, 2020 • edited Loading

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

landreev commented Apr 14, 2020

coveralls commented Apr 3, 2020 •

edited

Loading

kcondon commented Apr 10, 2020 •

edited

Loading

landreev commented Apr 13, 2020 •

edited

Loading

kcondon commented Apr 13, 2020 •

edited

Loading