249/user cannot change email #6439
Conversation
| async changePassword(passwd: string, newPassword: string): Promise<void> { | ||
| const user = await this.getCurrentUser()!; | ||
|
|
||
| const credential = EmailAuthProvider.credential(user.email!, passwd); |
There was a problem hiding this comment.
We try to avoid ! even though we have it in some places.
Is it possible for this email to be empty? if yes - lets check it for null and through error otherwise. Credential should always be a email, right?
| { currentPassword: string; password: string } | ||
| >( | ||
| async ({ password, currentPassword }) => { | ||
| return authService.changePassword(currentPassword, password); |
There was a problem hiding this comment.
this should call services/auth service. We do not want this logic to spread everywhere. I suppose you need to add proxy to this method in AuthContext
|
|
||
| import { | ||
| Content, | ||
| SettingsCard, | ||
| } from "pages/SettingsPage/pages/SettingsComponents"; | ||
| import { FieldItem } from "packages/cloud/views/auth/components/FormComponents"; | ||
| import { LabeledInput } from "components/LabeledInput"; | ||
| import { Button } from "../../../../../../components/base/Button"; |
There was a problem hiding this comment.
Small style comment: let's order lib import ( react-query ) above our imports. And get rig of ../../../
| await userService.changeEmail(email); | ||
| }, | ||
| { | ||
| onSuccess: () => window.location.reload(), |
There was a problem hiding this comment.
Why do we need to reload page?
| }); | ||
| >( | ||
| async ({ email, passwd }) => { | ||
| await authService.changeEmail(email, passwd); |
There was a problem hiding this comment.
Logic leak here. Let's place it into AuthService just how all other auth services handle it. It already has access to authService and userService, so you won't need to do any initial initialization
…9/User-cannot-change-email
|
|
…9/User-cannot-change-email
| if (user) { | ||
| await this.reauthenticate(email, password); | ||
|
|
||
| return updateEmail(user, email); |
There was a problem hiding this comment.
hm, I think you could just use await updateEmail here
| return updateEmail(user, email); | ||
| } | ||
|
|
||
| return Promise.resolve(); |
There was a problem hiding this comment.
There is no need of this promise.resolve as void is returned, so you can just return; or do nothing.
| const cloudWorkspace = await this.fetch<CloudWorkspace>(`${this.url}/get`, { | ||
| public async get(workspaceId?: string | null): Promise<CloudWorkspace> { | ||
| if (!workspaceId) { | ||
| return Promise.resolve({ |
There was a problem hiding this comment.
you could just return object. No need to return Promise. ts will interfere types correctly as method is async
| @@ -0,0 +1,5 @@ | |||
| export type FormValues = { | |||
There was a problem hiding this comment.
No need to use d.ts extensions here as .d.ts is bridge between ts and js. We use just simple ts.
| disabled={!dirty} | ||
| onClick={() => resetForm()} | ||
| > | ||
| cancel |
There was a problem hiding this comment.
Hmmm, looks like we forgot to add it into locale. Let's add it.
…annot-change-email
* feat: change user email * feat: added password submit for change email * feat: password change section * feat: rename workspace * fix: workspace names * fix: auth refactoring * fix: change password onSuccess * fix: change workspaces name * fix: change user email hook
What