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.

Sign up for GitHub

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

Jump to bottom

[DUOS-1815][risk=low] Add update own user properties endpoint #1649

Merged
merged 9 commits into from
Jul 15, 2022
Merged

[DUOS-1815][risk=low] Add update own user properties endpoint #1649

merged 9 commits into from
Jul 15, 2022

Conversation

connorlbark
Copy link
Contributor

@connorlbark connorlbark commented Jul 13, 2022 •
edited
Loading

Partially Addresses

https://broadworkbench.atlassian.net/browse/DUOS-1815

Adds needed PUT /api/user/ endpoint so non-admin users can update their properties.

Frontend component: DataBiosphere/duos-ui#1672

Also fixes bug wherein signing officials are not fully returned for non-admin users; this is necessary for the linked frontend issue, so I thought it was an appropriate place to fix it.

Have you read CONTRIBUTING.md lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@connorlbark connorlbark mentioned this pull request Jul 13, 2022
Merged
rushtong
rushtong requested changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

Nice work - see comments inline. We're kind of designing this API by PR, sorry we don't have more clearly laid out requirements.

src/main/java/org/broadinstitute/consent/http/service/UserService.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/consent/http/service/UserService.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/consent/http/resources/UserResource.java Outdated Show resolved Hide resolved
@connorlbark connorlbark marked this pull request as ready for review July 14, 2022 13:00
@connorlbark connorlbark requested a review from a team as a code owner July 14, 2022 13:00
rushtong
rushtong previously requested changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

We need an update to the swagger documentation as well. Have a look at api-docs.yaml. We've been migrating to describing the body of the api calls in a separate yaml file in resources/assets/paths. See other comments inline. This is looking good!

src/main/java/org/broadinstitute/consent/http/resources/UserResource.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/consent/http/service/UserService.java Outdated Show resolved Hide resolved
src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java Show resolved Hide resolved
@connorlbark
Copy link
Contributor Author

good catch on the swagger! totally forgot about that; just added it

@shaemarks shaemarks mentioned this pull request Jul 14, 2022
Merged
shaemarks
shaemarks suggested changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@shaemarks shaemarks left a comment

Choose a reason for hiding this comment

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

I left a couple of small comments, but looks good overall! I was able to update my own properties, and I got an error response when attempting to update roles without being an admin

src/main/resources/assets/paths/user.yaml Outdated Show resolved Hide resolved
src/test/java/org/broadinstitute/consent/http/resources/UserResourceTest.java Outdated Show resolved Hide resolved
src/main/java/org/broadinstitute/consent/http/util/InstitutionUtil.java Outdated Show resolved Hide resolved
lu-c
lu-c reviewed Jul 14, 2022
View reviewed changes
src/main/java/org/broadinstitute/consent/http/util/InstitutionUtil.java
return !isAdmin && !(fieldName.equals("id") || fieldName.equals("name") || fieldName.equals("signingOfficials"));

return !isAdmin && !(fieldName.equals("id")
|| fieldName.equals("name")
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, i LOVE this new formatting, it's really clean

lu-c
lu-c approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@lu-c lu-c left a comment

Choose a reason for hiding this comment

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

LGTM?

shaemarks
shaemarks approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@shaemarks shaemarks left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@connorlbark connorlbark merged commit a77d387 into develop Jul 15, 2022
@connorlbark connorlbark deleted the DUOS-1815-update-self-endpoint branch July 15, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaemarks shaemarks shaemarks approved these changes

@lu-c lu-c lu-c approved these changes

@rushtong rushtong Awaiting requested review from rushtong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@connorlbark @rushtong @lu-c @shaemarks