Closed davidpmccormick closed 2 years ago
Can you add a comment about why this is a new endpoint? Reading the PR, I was initially against it as it's not very RESTful at all and it's definitely duplication, but I changed my mind because I thought (a) it seems reasonable that these endpoints will diverge (eg, we might want to send transactional emails here at some point in future) and (b) we can do a check that the user is at the correct stage of the signup process here.
Further thoughts:
p
- this might be some useful context if you look at where it's used?aws_api_gateway_resource
, and then for each method (eg PUT
) that might be used you need a aws_api_gateway_method
, and for each response that a given method can have you need a aws_api_gateway_method_response
. Example for OPTIONS /users/:id
here.aws_api_gateway_integration
for each method. Example for GET /users/:id
here.The ID will always include a p - this might be some useful context if you look at where it's used?
Does it make sense to send the whole prefixed id (e.g. auth0|p12345678
) through to this app and remove the prefix here, or strip it before it gets here, just sending 12345678
?
This endpoint needs to match the others in the API - the auth0|p
prefix is private, and the routes should only include the plain 1234567
. The authorizer won't work if it's any other way.
I think I've addressed the comments with the exception of the API gateway stuff (which I think I'd prefer to do and understand in a separate PR).
Part of wellcomecollection/wellcomecollection.org#7808
Opted to create a new endpoint and handler instead of wrapping
updateUser
in conditionals based on whether we've got a password or not.I think the
user_id
we're going to pass from the NextJS app will be prefixed withauth0|
and I imagine we'll need to strip this off in order for thegetTargetUserId
function not to blow up. I've seen elsewhere we're checking if it starts withauth0|p
– I'm not sure where thatp
comes from (and it doesn't appear in the ids that I've been testing with)Edit: I've removed the
auth0|
(and maybep
) before it gets this far. I guess thep
is for 'patron', although I wasn't sure if it would be added to newly created Sierra-database records using Auth0 as a way to signup, so left it optional for now. If we know the prefix either definitely will or will not contain ap
, we can remove the?
or thep
from the regex, respectively.