fix: handle email in use #2092
fix: handle email in use #2092
Conversation
| const messageForEmailInUse = ` | ||
| <h3>There is a problem configuring your commenting account</h3> | ||
| <p> | ||
| Please contact ${this.options.linkContact ? `<a class="linkContact" href='${this.options.linkContact}'>Customer Care</a>` : `Customer Care`} referencing issue code E403. | ||
| </p> | ||
| `; |
There was a problem hiding this comment.
Thought: not your fault but all these consts instantiated and not even used are a missed optimisation. Everytime a const is create it's using memory and then taking resources because of the Garbage collection that needs to follow. See comment below...
| } else if (this.isTrialist) { | ||
| customMessageContainer.innerHTML = messageForTrial; | ||
| } else if (this.emailInUse) { | ||
| customMessageContainer.innerHTML = messageForEmailInUse; |
There was a problem hiding this comment.
As per my previous comment, this could code could be structured differently...
| customMessageContainer.innerHTML = messageForEmailInUse; | |
| customMessageContainer.innerHTML = `<h3>There is a problem configuring your commenting account</h3> | |
| <p> | |
| Please contact ${this.options.linkContact ? `<a class="linkContact" href='${this.options.linkContact}'>Customer Care</a>` : `Customer Care`} referencing issue code E403. | |
| </p> | |
| `; |
| user_type: this.emailInUse | ||
| ? 'registered-email-in-use' | ||
| : this.isTrialist | ||
| ? 'trialist' | ||
| : this.isRegistered | ||
| ? 'registered' | ||
| : 'anonymous' |
There was a problem hiding this comment.
Thought: Again, this was like this before...it would likely benefit of creating a model and use that pushing the logic in the model and keep this part dumb till you reach something like the following:
user_type: user.type << A user model
|
|
||
| // some users have coral accounts associated to old FT accounts that are using the same email address | ||
| // we need to remove this email before they can sign in to the new account | ||
| if (response.status === 403) { |
There was a problem hiding this comment.
Thought: Using only status code may not be a very scalable solution. What if tomorrow we introduce another condition that generates a 403 but we want a different message.
We could share the error code as part of a payload in the response.
Pseudo response...
status-code: 403
payload: {
error-code: `CommentError403`, << Or whatever
}
In this way is easy to track the error between customer care, frontend and backend, you can even search the repos for this.
|
Closing in favour of #2102 |
Describe your changes
We have found a bug that causes users that have previously had an FT account to comment with their old coral account. We will be adding an additional check to the auth endpoint in next-comments-api, which will return a 403 when an old coral account is found. This PR checks for this status and returns a user-friendly error to the client.
Additional context
Checklist before requesting a review
percylabel for o-[COMPONENT] orchromaticlabel for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md