IQSS/10318 Uningest/Reingest UI #10319

qqmyers · 2024-02-13T22:58:22Z

What this PR does / why we need it: Per the issue, this adds a File page Edit menu Uningest/Reingest menu item for tabular files, visible to superusers, that allows clearing ingest errors/removing ingested tab files, and ingesting/reingesting a file (useful if a new Dataverse version may have fixes or if ingest limits change, etc.)

Which issue(s) this PR closes:

Closes #10318

Special notes for your reviewer: The PR as is, keeps these as requiring superuser. At QDR, we switched to allowing anyone who has permission to publish the dataset to uningest/reingest via API/UI. For QDR, that means curators can use these options whereas in a non-curated/self-publication setup it would allow anyone to do this in their draft datasets. If that's desired, the permission changes can be added to this PR.

Suggestions on how to test this: Go to the file page and try the uningest/reingest options on a tabular file. Verify that for a file where ingest has failed, that uningest clears the error and that for one where ingest succeeded, the .tab version is removed. Verify that reingest does an ingest (and fails or succeeds as the initial ingest did).

Does this PR introduce a user interface change? If mockups are available, please link/include them here: single entry (uningest or reingest) in the File page Edit menu, visible for superusers.

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

Additional documentation:

…reingest_UI

coveralls · 2024-02-13T23:07:01Z

coverage: 20.254% (-0.02%) from 20.27%
when pulling 6480896 on QualitativeDataRepository:IQSS/10318-uningest_reingest_UI
into dcd7c22 on IQSS:develop.

landreev · 2024-02-14T16:56:36Z

Jenkins run failed on edu.harvard.iq.dataverse.api.MoveIT.testMoveLinkedDataset. Hard to see how it could be related to the changes in the PR though.

pdurbin

Looks great! I left a few comments. I didn't run the code.

doc/sphinx-guides/source/user/tabulardataingest/ingestprocess.rst

pdurbin · 2024-02-14T16:56:43Z

doc/sphinx-guides/source/user/tabulardataingest/ingestprocess.rst

+who can see the draft version of the dataset containing the file that will indicate why ingest failed. When the file is published as 
+part of the dataset, there will be no indication that ingest was attempted and failed.
+
+If the warning message is a concern, the Dataverse software includes both an API call (see the Files section of the :doc:`/api/native-api` guide) 


A :ref: link to the specific endpoint would be nice.

pdurbin · 2024-02-14T16:56:56Z

doc/sphinx-guides/source/user/tabulardataingest/ingestprocess.rst

+This will remove the .tab version of the file that was generated.
+
+If a file is a tabular format but was never ingested, .e.g. due to the ingest file size limit being lower in the past, or if ingest had failed,
+e.g. in a prior Dataverse version, an reingest API (see the Files section of the :doc:`/api/native-api` guide) and a file page Edit/Reingest option


A :ref: link would be nice here as well.

pdurbin · 2024-02-14T16:57:37Z

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

+    public String ingestFile() throws CommandException{
+
+        User u = session.getUser();
+        if(!u.isAuthenticated() ||  !(permissionService.permissionsFor(u, file).contains(Permission.PublishDataset))) {


I was expecting to see a check for superuser here... Add a comment to say that the permission check is in the command (if it is)?

Actually, this is re-ingest - and there is no command involved. The corresponding API is superuser-only - so this should probably be .isSuperuser() here as well. (?)

(but note the "shouldn't happen" comment in the next line; i.e. I still believe it would make sense to check for superuser here, for consistency, but it's not as crucial)

pdurbin · 2024-02-14T16:58:24Z

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

+        if (!file.isTabularData()) {
+            if(file.isIngestProblem()) {
+                User u = session.getUser();
+                if(!u.isAuthenticated() ||  !(permissionService.permissionsFor(u, file).contains(Permission.PublishDataset))) {


Should this be superuser-only?

Ah, I see UningestFileCommand has a check for superuser. Maybe add a comment here to look at the command for the actual permission check?

This permission check does not appear to be for an actual attempt to uningest a file - it's just for clearing of the ingest failure warning from the UI (the file is not ingested to begin with), and yes, it makes perfect sense to allow authors to do this.
I hope I'm getting it right this time around.

src/main/webapp/file-edit-button-fragment.xhtml

landreev · 2024-02-14T17:34:30Z

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

+                if(!u.isAuthenticated() ||  !(permissionService.permissionsFor(u, file).contains(Permission.PublishDataset))) {
+                    logger.warning("User: " + u.getIdentifier() + " tried to uningest a file");
+                    //Shouldn't happen (choice not displayed for users who don't have the right permission), but check anyway
+                    JH.addMessage(FacesMessage.SEVERITY_WARN, BundleUtil.getStringFromBundle("file.ingest.cantUningestFileWarning"));


This entry may not be in the Bundle in the develop branch yet.

…reingest_UI

Co-authored-by: Philip Durbin <philipdurbin@gmail.com>

landreev · 2024-02-14T18:48:53Z

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

-              file.setIngestReport(null);
+            //Ingest never succeeded, either there was a failure or this is not a tabular data file
+            User u = session.getUser();
+            if (!u.isAuthenticated() || !u.isSuperuser()) {


OK, I thought what you had here before was on purpose - to give the dataset author a way to remove an ingest failure report - no?

(those are only visible to the owners/authors, so not a big deal either way)

Would be nice if that's OK. (Probably less work for superusers as that warning bothers the authors.)
At QDR all of this is with the publish permission.

pdurbin

All 6 Jenkins runs failed so far but I think Jenkins was having issues. I just kicked off a 7th run: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10319/7/

I found a typo in the docs. I'll probably just commit it.

There's one spot where I'm suggesting we lower logging to fine but I don't feel strongly about it.

I didn't run the code but I'd say it's safe to start manually testing. Hopefully the API test pass.

doc/sphinx-guides/source/user/tabulardataingest/ingestprocess.rst

pdurbin · 2024-02-15T21:56:20Z

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

+
+        }
+
+        dataFile.SetIngestScheduled();


Huh, it's unconventional that this and other methods start with capital "S" but whatever, something to address another time:

public void SetIngestScheduled() { ingestStatus = INGEST_STATUS_SCHEDULED; } public void SetIngestInProgress() { ingestStatus = INGEST_STATUS_INPROGRESS; } public void SetIngestProblem() { ingestStatus = INGEST_STATUS_ERROR; }

pdurbin · 2024-02-15T21:56:43Z

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

+
+            logger.warning("Ingest Status for file: " + dataFile.getId() + " : " + status);
+        }
+        logger.info("File: " + dataFile.getId() + " ingest queued");


Suggested change

logger.info("File: " + dataFile.getId() + " ingest queued");

logger.fine("File: " + dataFile.getId() + " ingest queued");

src/main/webapp/file-edit-button-fragment.xhtml

pdurbin

All 6 Jenkins runs failed so far but I think Jenkins was having issues. I just kicked off a 7th run: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10319/7/

This passed so I'm approving this.

@landreev I know you're assigned to this but I'll make sure neither one of us is assigned as this moves to "ready for QA", as is our custom.

There is still the one logger line we might want to lower but I'll leave that the to the person doing QA to decide if it adds value as-is.

Overall, this PR is looking much better than the first time I looked at it. Missing bundle properties have been added. Docs are better. I haven't run the code but looks good, approved.

sekmiller · 2024-02-28T20:30:26Z

@qqmyers Any thoughts about adding the option to the Edit Files button/kebab on the dataset page?

sekmiller · 2024-02-28T20:53:08Z

@qqmyers There are also merge conflicts that need to be resolved. thanks!

doc/release-notes/10318-uningest-and-reingest.md

IQSS/10318-uningest_reingest_UI

qqmyers · 2024-02-28T21:36:56Z

@qqmyers Any thoughts about adding the option to the Edit Files button/kebab on the dataset page?

Just removed the filePage restriction in the xhtml.

sekmiller · 2024-02-28T21:54:25Z

src/main/webapp/file-edit-button-fragment.xhtml

@@ -79,14 +79,14 @@
    </ui:fragment>

    <!-- Single file uningest/reingest -->
-    <ui:fragment rendered="#{isFilePg and (dataverseSession.user.superuser and FilePage.fileMetadata.dataFile.isTabularData()) or (FilePage.fileMetadata.dataFile.isIngestProblem() and FilePage.canPublishDataset())}">
+    <ui:fragment rendered="#{(dataverseSession.user.superuser and FilePage.fileMetadata.dataFile.isTabularData()) or (FilePage.fileMetadata.dataFile.isIngestProblem() and FilePage.canPublishDataset())}">


Now I'm getting a 500 error when I try to go to the dataset page because the FilePage.fileMetadata is null (jakarta.servlet.ServletException: Cannot invoke "edu.harvard.iq.dataverse.FileMetadata.getDataFile()" because "this.fileMetadata" is null)

Maybe it's not worth it to allow display the uningest on the dataset page. it will be pretty complicated - especially if the user selects multiple files from the list.

OK - the list part shouldn't be an issue if this is only in the kebab menu, but yeak, more work than just removing the isFilePage check - I'll restore that.

sekmiller · 2024-02-29T19:09:42Z

@qqmyers in all cases, whether the original ingest was successful or failed and whether it was an uningest or reingest the api does not create a new draft, but the ui does.

qqmyers · 2024-02-29T19:33:01Z

Thanks. I see that the new code was calling FilePage.save() to make sure the changes to the datafile were saved, but that triggers a draft. I changed that to just call DataFileServiceBean.save(DataFile) instead, which is what the API does.

qqmyers · 2024-02-29T19:34:32Z

Good catch btw - I think QDR only uses this on draft datasets, so not a lot of testing on published files.

sekmiller · 2024-02-29T20:52:34Z

All set., thanks for the follow-up.

scolapasta · 2024-02-29T21:14:42Z

Thanks. I see that the new code was calling FilePage.save() to make sure the changes to the datafile were saved, but that triggers a draft. I changed that to just call DataFileServiceBean.save(DataFile) instead, which is what the API does.

@qqmyers @sekmiller ideally, we don't want pages to call the service beans directly. That said, since this is in jsf and going away eventually AND it is superuser only anyway, I think it's fine for now.

qqmyers added 6 commits February 13, 2024 16:38

Ingest/Uningest from file page

240e851

Merge remote-tracking branch 'IQSS/develop' into IQSS/10318-uningest_…

5d241f8

…reingest_UI

missing imports/@ejb

fcdc246

Change command to publish perm

15ae19e

superuser only in command, add docs

262fb26

release note

130cfba

qqmyers added the Size: 3 A percentage of a sprint. 2.1 hours. label Feb 14, 2024

qqmyers requested a review from landreev February 14, 2024 16:14

landreev self-assigned this Feb 14, 2024

pdurbin self-assigned this Feb 14, 2024

pdurbin mentioned this pull request Feb 14, 2024

MoveIT.testMoveLinkedDataset fails intermittently #10189

Open

pdurbin reviewed Feb 14, 2024

View reviewed changes

landreev reviewed Feb 14, 2024

View reviewed changes

qqmyers and others added 5 commits February 14, 2024 13:19

update perms

1dc4825

add bundle strings

f151226

doc updates

14b280c

Merge remote-tracking branch 'IQSS/develop' into IQSS/10318-uningest_…

c5d1ed1

…reingest_UI

Apply suggestions from code review

5dffe36

Co-authored-by: Philip Durbin <philipdurbin@gmail.com>

landreev reviewed Feb 14, 2024

View reviewed changes

qqmyers added 8 commits February 14, 2024 16:21

change to require publish to uningest for a problem

70db48f

add uningest for a problem logic in api

3d7f72a

update docs

7de7f43

fix logic

51fe60c

typo/merge issues

31d7cbc

missing save

057d2c3

tweak api doc

beb5bf6

changelog, release note tweaks

00d4189

pdurbin reviewed Feb 15, 2024

View reviewed changes

typo IQSS#10318

87b5a38

pdurbin approved these changes Feb 16, 2024

View reviewed changes

pdurbin unassigned pdurbin and landreev Feb 16, 2024

sekmiller self-assigned this Feb 28, 2024

sekmiller reviewed Feb 28, 2024

View reviewed changes

doc/release-notes/10318-uningest-and-reingest.md Outdated Show resolved Hide resolved

qqmyers added 2 commits February 28, 2024 16:26

Merge remote-tracking branch 'IQSS/develop' into

50c84bf

IQSS/10318-uningest_reingest_UI

typo

5760149

sekmiller reviewed Feb 28, 2024

View reviewed changes

qqmyers force-pushed the IQSS/10318-uningest_reingest_UI branch from 3f59722 to 5760149 Compare February 28, 2024 22:11

qqmyers added 2 commits February 29, 2024 14:30

just save the file, w/o calling FilePage.save() which creates a draft

9f34826

info to fine

6480896

sekmiller merged commit 2e3b333 into IQSS:develop Feb 29, 2024
12 checks passed

pdurbin added this to the 6.2 milestone Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IQSS/10318 Uningest/Reingest UI #10319

IQSS/10318 Uningest/Reingest UI #10319

qqmyers commented Feb 13, 2024 •

edited

coveralls commented Feb 13, 2024 •

edited

landreev commented Feb 14, 2024

pdurbin left a comment

pdurbin Feb 14, 2024

pdurbin Feb 14, 2024

pdurbin Feb 14, 2024

landreev Feb 14, 2024

landreev Feb 14, 2024

pdurbin Feb 14, 2024

landreev Feb 14, 2024

landreev Feb 14, 2024

landreev Feb 14, 2024

landreev Feb 14, 2024

qqmyers Feb 14, 2024

pdurbin left a comment

pdurbin Feb 15, 2024

pdurbin Feb 15, 2024

pdurbin left a comment

sekmiller commented Feb 28, 2024

sekmiller commented Feb 28, 2024

qqmyers commented Feb 28, 2024

sekmiller Feb 28, 2024

sekmiller Feb 28, 2024

qqmyers Feb 28, 2024

sekmiller commented Feb 29, 2024

qqmyers commented Feb 29, 2024

qqmyers commented Feb 29, 2024

sekmiller commented Feb 29, 2024

scolapasta commented Feb 29, 2024

	logger.info("File: " + dataFile.getId() + " ingest queued");
	logger.fine("File: " + dataFile.getId() + " ingest queued");

IQSS/10318 Uningest/Reingest UI #10319

IQSS/10318 Uningest/Reingest UI #10319

Conversation

qqmyers commented Feb 13, 2024 • edited

coveralls commented Feb 13, 2024 • edited

landreev commented Feb 14, 2024

pdurbin left a 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

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

pdurbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdurbin left a comment

Choose a reason for hiding this comment

sekmiller commented Feb 28, 2024

sekmiller commented Feb 28, 2024

qqmyers commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sekmiller commented Feb 29, 2024

qqmyers commented Feb 29, 2024

qqmyers commented Feb 29, 2024

sekmiller commented Feb 29, 2024

scolapasta commented Feb 29, 2024

qqmyers commented Feb 13, 2024 •

edited

coveralls commented Feb 13, 2024 •

edited