Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IQSS/10318 Uningest/Reingest UI #10319
IQSS/10318 Uningest/Reingest UI #10319
Changes from 6 commits
240e851
5d241f8
fcdc246
15ae19e
262fb26
130cfba
1dc4825
f151226
14b280c
c5d1ed1
5dffe36
70db48f
3d7f72a
7de7f43
51fe60c
31d7cbc
057d2c3
beb5bf6
00d4189
87b5a38
50c84bf
5760149
9f34826
6480896
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A :ref: link to the specific endpoint would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A :ref: link would be nice here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. (?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, it's unconventional that this and other methods start with capital "S" but whatever, something to address another time:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry may not be in the Bundle in the develop branch yet.