-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create-post-confirmation-lambda-event-handler #400
Create-post-confirmation-lambda-event-handler #400
Conversation
✅ Deploy Preview for dailp canceled.
|
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.
None of these are blocking, but here are some nice to haves/questions.
.send() | ||
.await | ||
.map_err(|e| anyhow::Error::new(e)) | ||
.map(|x| ()) |
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.
Does this line map the result to the empty tuple? I think I see the usecase here and if this is the standard pattern I'm fine with it. It just throws me a bit.
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.
it maps to rust's unit type, ()
, which "is used when there is no other meaningful value that could be returned" according to the type's documentation.
Why aren't we returning a more meaningful type here?
The AWS operation cognito:AdminAddUserToGroup
returns an HTTP 200: Ok
with an empty body when successful (docs). When unsuccessful, it returns one of the many Cognito API Errors, usually a variation of HTTP 400: Bad Request
or 500: Internal Server Error
.
Because a success is simply an OK but errors can be one of many things, the most important information for the main lambda operation is whether or not the operation succeeded, so returning the unit type seemed appropriate.
admin-event-handlers/src/auth/post-confirmation/google_sheets_operations.rs
Outdated
Show resolved
Hide resolved
// First row is headers "Full Name" "Alt name" "DOB" "Role" "email" | ||
for row in self.values.into_iter().skip(1) { |
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.
We could also assert this instead of skipping
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.
To clarify, do you mean using Macro std::assert
here?
Share-spreadsheet-operations
Creates a new rust member for housing lambda event handlers that are not a core part of the application.
Creates a new rust binary with logic for a lambda that adds users to a group if their email is on a predefined list when they confirm their account.