Feat/site creation form email #679
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kishore03109
reviewed
Apr 5, 2023
harishv7
reviewed
Apr 6, 2023
| const foundUser = await this.usersService.findByEmail(requesterEmail) | ||
| if (!foundUser) { | ||
| const foundRequester = await this.usersService.findByEmail(requesterEmail) | ||
| if (!foundRequester) { | ||
| const err = `Form submitter ${requesterEmail} is not an Isomer user. Register an account for this user and try again.` | ||
| await this.sendCreateError(requesterEmail, repoName, submissionId, err) |
There was a problem hiding this comment.
sorry a bit confused, this is an error flow right? Why are we returning 200 res after sendCreateError?
There was a problem hiding this comment.
This endpoint is only triggered by forms, which expects a 200 response, otherwise it could trigger a retry! The error handling is done separately via email
kishore03109
approved these changes
Apr 6, 2023
src/routes/formsgSiteCreation.ts
Outdated
| @@ -51,6 +52,7 @@ export class FormsgRouter { | |||
| const requesterEmail = getField(responses, REQUESTER_EMAIL_FIELD) | |||
| const siteName = getField(responses, SITE_NAME_FIELD) | |||
| const repoName = getField(responses, REPO_NAME_FIELD) | |||
| const ownerEmail = getField(responses, OWNER_NAME_FIELD)?.toLowerCase() | |||
There was a problem hiding this comment.
Nit: could we do a strip on the string?
worried if ops put space then the string equality operation fails and it registers as new email
(maybe this error case is captured downstream)
There was a problem hiding this comment.
good catch, added in 80cfbbd
alexanderleegs
added a commit
that referenced
this pull request
Apr 11, 2023
* Feat: add findOrCreateByEmail * Feat: add site member instead of github team * Feat: update forms parsing * Fix: update infraService initialisation * Chore: remove unused createTeamOnGithub * nit: update variable name * chore: update todo with issue number * Fix: trim user input
Closed
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies our existing site creation form to utilise the new email login flow instead. Currently, our site creation form creates a new website, but does not assign site members to the new site - thus our creation flow currently creates an Amplify site that is accessed via Github login, with access manually granted to users via Github. This PR changes the creation flow to associate an existing isomer user to the specified site, or create a new isomer user using the specified email if no such user exists. A new form has also been created for both staging and production (staging form link). Note that the person filling in the form is still specified as the owner of the form.
Env var changes before merging:
SITE_CREATE_FORM_KEYon production - we're using a new site create form (v3) just to be safe, so the form key needs to be swapped out to the new secret key