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

Issue 14 user management via api #25

Merged
merged 27 commits into from Feb 19, 2021
Merged

Conversation

nhoening
Copy link
Contributor

@nhoening nhoening commented Feb 12, 2021

  • open up user data via the API (no creation and deletion though)
  • enable customisation of password reset emails
  • some updates to the asset API with learning from this PR, e.g. use webargs/marshmallow
  • show how we can use webargs/marshmallow for the older parts of the API, which will become a new PR

@nhoening nhoening requested a review from Flix6x Feb 12, 2021
@nhoening nhoening self-assigned this Feb 12, 2021
@nhoening nhoening linked an issue Feb 12, 2021 that may be closed by this pull request
Copy link
Contributor

@Flix6x Flix6x left a comment

Nice work. Your experience with FlaskSecurityToo and asset crud supported through our API really pays off here.

Mostly small comments, with the largest one probably being support for future API versions, and a recommendation to support the password reset endpoint with both a GET and a PATCH request.

flexmeasures/api/common/utils/validators.py Outdated Show resolved Hide resolved
)
v2_0_service_listing["services"].append(
{
"name": "GET /user/<id>/password-reset",
Copy link
Contributor

@Flix6x Flix6x Feb 16, 2021

Choose a reason for hiding this comment

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

Given that this endpoint may change the user object (even if the endpoint code doesn't already reset the password, we expect that the person receiving emailed instructions would reset it), shouldn't this be a PATCH? Or a slightly more elegant implementation: allow both GET and PATCH, with only_send_email=True and only_send_email=False, respectively.

Copy link
Contributor Author

@nhoening nhoening Feb 17, 2021

Choose a reason for hiding this comment

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

I don't think this is necessary. The endpoint itself isn't changing data. And it would add a lot of work.

flexmeasures/api/v2_0/routes.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Show resolved Hide resolved
flexmeasures/ui/crud/users.py Show resolved Hide resolved
flexmeasures/ui/crud/users.py Outdated Show resolved Hide resolved
flexmeasures/ui/templates/crud/user.html Show resolved Hide resolved
flexmeasures/ui/tests/test_user_crud.py Show resolved Hide resolved
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Feb 17, 2021

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Feb 17, 2021

@nhoening
Copy link
Contributor Author

@nhoening nhoening commented Feb 17, 2021

I'm actually working on the custom marshmallow validation right now, using exactly that function. I have to do less marshmallow acrobatics than I thought. But the effect on our API response is not clear to me yet.

nhoening added 3 commits Feb 18, 2021
…uration (as an example). Also add error handling for this type of validation error. Add the 422 response to relevant routes.
…op outdated code; make reset password endpoint simpler & offer as PATCH
Flix6x
Flix6x approved these changes Feb 18, 2021
@nhoening nhoening merged commit c85927f into main Feb 19, 2021
2 checks passed
@Flix6x Flix6x deleted the issue-14-User_management_via_API branch Mar 1, 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 this pull request may close these issues.

2 participants