Skip to content
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

API calls, e.g. related to edit and publish, fail if request access is false and no Terms of access #8859

Closed
qqmyers opened this issue Jul 26, 2022 · 3 comments · Fixed by #8906
Assignees
Milestone

Comments

@qqmyers
Copy link
Member

qqmyers commented Jul 26, 2022

What steps does it take to reproduce the issue? Noticed at QDR on existing datasets (pre v5.11) where file request access is false and there are no terms of access: API calls to add/delete files result in 400 Bad Request. Updating the terms via the UI (where one can see a warning for these datasets) resolves the API issue.

  • What did you expect to happen?
    Clearer warning or, better yet, update of existing drafts, new drafts created from existing pre v5.11 versions, to start with valid requestaccess/terms of access settings. (At QDR, where it was deemed OK to always allow request access to be true, we did a db update to set fileaccessrequest = true for all drafts and the latest version of published datasets when there was no draft. Given that changing the setting in published versions may not work everywhere, something like a flyway script to update drafts, along with new code to flip fileaccessrequest = true when a new draft is created from an existing version, might work. Or just clearer API errors that indicate what has to be changed (although the QDR case was with another app using the API rather than a user who could read/interpret the error.))

Any related open or closed issues to this bug report?
~ #8832, #8847, #8742 - these appear to be other symptoms triggered by having invalid requestaccess/ToA settings, none are specifically related to API errors.

@qqmyers
Copy link
Member Author

qqmyers commented Jul 26, 2022

Discussion in tech hours suggested that going with a 409/Conflict error for these calls makes sense (vs. trying to automate any db updates to change the state) plus adding info to the release notes to remind people again to work with users to update. Given that the exact set of API calls involved isn't know, the fix should at least cover the ones above. However, it may be that it is possible to make a change, e.g. at the level of the UpdateDatasetVersionCommand to send an exception/wrapped response that will cover all affected API calls.

@sekmiller sekmiller self-assigned this Jul 27, 2022
@sekmiller sekmiller removed their assignment Jul 27, 2022
@mreekie mreekie added the pm.jim label Aug 1, 2022
@mreekie
Copy link

mreekie commented Aug 1, 2022

We would like to see this in 5.12 because it relates to the change made in 5.11 related to fixing a bug. If it's in 5.12 we will have saved users an additional bug.

@mreekie
Copy link

mreekie commented Aug 3, 2022

Discussed that we give a good error message for these issues.
When we hit this validation bug, catch it and display the error.
The new code to check for this conflict gets triggered.
This needs to be found and caught further up the stack and dealt with. This adds a bit of complexity to the fix.

phil - include some text that indicates that the issue is related to setting the terms of access using the API.

@scolapasta scolapasta added this to the 5.12 milestone Aug 3, 2022
@sekmiller sekmiller self-assigned this Aug 4, 2022
sekmiller added a commit that referenced this issue Aug 31, 2022
sekmiller added a commit that referenced this issue Aug 31, 2022
pdurbin added a commit that referenced this issue Sep 7, 2022
sekmiller added a commit that referenced this issue Sep 14, 2022
sekmiller added a commit that referenced this issue Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants