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

2176 apiv3 users #2212

Merged
merged 22 commits into from
May 21, 2020
Merged

2176 apiv3 users #2212

merged 22 commits into from
May 21, 2020

Conversation

aviveiros11
Copy link
Contributor

Any background context you want to provide?

The effort to move to API v3 - specifically users

What's this PR do?

translates API v2 into v3 with auto schema helper

How should this be manually tested?

Use the Try It functionality rendered by the API page on SEED for v3

What are the relevant tickets?

#2176

@coveralls
Copy link

coveralls commented May 11, 2020

Coverage Status

Coverage decreased (-0.9%) to 76.757% when pulling c0be058 on 2176_apiv3_users into f15fa05 on develop.

Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments from my first pass. I didn't look at any of the endpoints that were just a "move" within the issue, but let me know if any are worth a review.

seed/utils/api_schema.py Show resolved Hide resolved
seed/utils/api_schema.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Show resolved Hide resolved
Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more follow up comments

seed/views/v3/users.py Show resolved Hide resolved
seed/views/v3/users.py Show resolved Hide resolved
seed/views/v3/users.py Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved some previous comments. Tested the endpoints, and for the most part they look good! A few potential changes, and this'll be good to go.

seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
@aviveiros11
Copy link
Contributor Author

@adrian-lara - it turns out for the UI to render the body field for "role" needs to say "string". My solution to keep the param optional was to case switch on "string" and if unchanged default the user to be a viewer following least privilege. Let me know if this works.

seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
seed/views/v3/users.py Outdated Show resolved Hide resolved
aviveiros11 and others added 2 commits May 19, 2020 15:32
Co-authored-by: Adrian Lara <30608004+adrian-lara@users.noreply.github.com>
Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - thanks for making those changes!

Each of the Swagger page v3 User endpoints work as expected. Feel free to merge once tests pass.

@aviveiros11 aviveiros11 merged commit 6dc9a66 into develop May 21, 2020
@adrian-lara adrian-lara deleted the 2176_apiv3_users branch August 31, 2020 19:58
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 this pull request may close these issues.

3 participants