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
Conversation
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
…y already has authtoken_required; rename util function
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.
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.
) | ||
v2_0_service_listing["services"].append( | ||
{ | ||
"name": "GET /user/<id>/password-reset", |
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.
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.
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.
I don't think this is necessary. The endpoint itself isn't changing data. And it would add a lot of work.
I don't agree. If the endpoint changes the password by being triggered than
it is changing data, and that should not happen within a GET request. If
you don't want to distinguish between two types, then I suggest to use
PATCH only.
…On Wed, Feb 17, 2021, 12:01 Nicolas Höning ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In flexmeasures/api/v2_0/routes.py
<#25 (comment)>:
> +)
+v2_0_service_listing["services"].append(
+ {
+ "name": "GET /user/<id>",
+ "description": "Get a user.",
+ },
+)
+v2_0_service_listing["services"].append(
+ {
+ "name": "PATCH /user/<id>",
+ "description": "Edit a user.",
+ },
+)
+v2_0_service_listing["services"].append(
+ {
+ "name": "GET /user/<id>/password-reset",
I don't think this is necessary. The endpoint itself isn't changing data.
And it would add a lot of work.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHJ5BS7VMTXXYDJEM4SSVM3S7OOXTANCNFSM4XQUVGBA>
.
|
I assume defining a custom Marshmallow type involves writing some sort of
validator function. We could then simply use the isodate library (which is
already a FlexMeasures dependency) to try to parse the string as a
duration, right?
…On Wed, Feb 17, 2021, 10:45 Nicolas Höning ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In flexmeasures/api/common/utils/validators.py
<#25 (comment)>:
> @@ -186,16 +203,18 @@ def wrapper(fn):
@wraps(fn)
@as_json
def decorated_service(*args, **kwargs):
- form = get_form_from_request(request)
- if form is None:
- current_app.logger.warning(
- "Unsupported request method for unpacking 'duration' from request."
- )
- return invalid_method(request.method)
+ # TODO: marshmallow doesn't support timestamps in iso8601 str representation
It seems we would have to write our own Marshmallow type, which is not too
hard. It would be nice of course for having an API which gives you proper
responses about input. But we'd need a proper way to validate a ISO6801
duration string, like a Regex. Do you know where that stuff lives?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHJ5BS55675TSODTH5NTVZLS7OF3BANCNFSM4XQUVGBA>
.
|
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. |
…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
…/flexmeasures into issue-14-User_management_via_API