-
Notifications
You must be signed in to change notification settings - Fork 493
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
8859 update api error msg #8906
Conversation
I don't have time to do a full review but I spoke with @sekmiller and he said you definitely have to get the dataset out of compliance first (such as with I started by trying reproduce the problems on develop (1e52809). You definitely can't create a new draft from a published version. If I run this...
... I get this error:
I haven't tested other edits but it sounds like Steve addressed these. He said he couldn't reproduce the "delete file" bug. I'm not sure about "add file". We might want @qqmyers to take a look since he opened the issue and found some of these bugs. Update: I think I got add and delete reversed. Please check with Steve. Also, I switched to the branch for this PR and the error is better but still a little long and weird:
|
I haven't run anything but I'm not seeing how this change results in a 409 versus 500 error. If it does, great. Specific Qs after a discussion with @scolapasta :
I'm not sure how far to go here, whether some things should be documented as separate issues, etc. |
@sekmiller and I just talked about how this message when restricting a file (after disallowing "request access") doesn't make much sense:
Instead it should say something like "In order to restrict a file, you need to either allow 'request access' or set terms of use." |
On develop as of 9ac2244 I created a dataset with a restricted file and put it out of compliance with this:
Then I tried these methods from UtilIT (from testApiErrors in the PR). As expected, all gave cryptic and ugly messages:
Now, with this PR (as of 2aeb457), the same operations now yield CONFLICT (409) as the HTTP response status code (with the exception of SWORD, which is not easily changed). However, only one of the five messages (uploadFileViaNative) has been made more clear. Here's the complete list:
I assume we want all five of these messages to be made more clear (including removing noise like "ReferencePipeline"). |
Ok as of 9f23b6a all five errors messages above look good and say I've basically already done the QA for this PR so assuming the tests pass in Jenkins I'll merge it. |
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.
As discussed in PR comments, looking good now. Approving and merging. All tests passed.
What this PR does / why we need it: Update api endpoints and commands so that update/publishing on datasets that are out of compliance with the requirement that datasets containing restricted files must either allow access requests or provide terms of access will throw a wrapped response with the reason for the failure is made clear to the API User.
Which issue(s) this PR closes:
Closes #8859 API calls, related to edit and publish fail if request access is false and no Terms of Access (if there are restricted files on the dataset)
Special notes for your reviewer:
Suggestions on how to test this: Test the publish and edit dataset apis with a dataset that has restricted files with request access false and Terms of Access blank (you may have to manipulate the termsofuseandaccess with a sql query update)
Does this PR introduce a user interface change? If mockups are available, please link/include them here: no
Is there a release notes update needed for this change?: no
Additional documentation: none