From 16ab5e9db0f35b6f50559a37abfc55b63cf3f480 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 24 Apr 2023 03:41:09 -0500 Subject: [PATCH] Properly translate errors in `ChangePassword.tsx` so they show up translated to the user but not in our logs (#10615) * Properly translate errors in `ChangePassword.tsx` So they show up translated to the user but not in our logs. Part of https://github.com/vector-im/element-web/issues/9597 and also fixes it since it's the last piece mentioned (there could be other cases we log translated strings) Fix https://github.com/vector-im/element-web/issues/9597 * Make more useful * Update i18n strings * No need to checkPassword since field validation already covers this See https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1167363765 Both of the error cases are covered by the logic in `verifyFieldsBeforeSubmit()` just above and there is no way `checkPassword` would ever throw one of these errors since they are already valid by the time it reaches here. * Update i18n strings * Revert "No need to checkPassword since field validation already covers this" This reverts commit 7786dd151028e6fbf04d1a38a9c2cd47a3fbfc4b. * Update i18n strings * Add todo context to note that we can remove this logic in the future * Ensure is an error * Remove else See https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1173477053 --- .../views/settings/ChangePassword.tsx | 54 ++++++++++++++----- .../tabs/user/GeneralUserSettingsTab.tsx | 43 ++++++++++----- src/i18n/strings/en_EN.json | 4 ++ 3 files changed, 74 insertions(+), 27 deletions(-) diff --git a/src/components/views/settings/ChangePassword.tsx b/src/components/views/settings/ChangePassword.tsx index 8ea8326325..0a32646878 100644 --- a/src/components/views/settings/ChangePassword.tsx +++ b/src/components/views/settings/ChangePassword.tsx @@ -24,7 +24,7 @@ import { MatrixClientPeg } from "../../../MatrixClientPeg"; import AccessibleButton from "../elements/AccessibleButton"; import Spinner from "../elements/Spinner"; import withValidation, { IFieldState, IValidationResult } from "../elements/Validation"; -import { _t, _td } from "../../../languageHandler"; +import { UserFriendlyError, _t, _td } from "../../../languageHandler"; import Modal from "../../../Modal"; import PassphraseField from "../auth/PassphraseField"; import { PASSWORD_MIN_SCORE } from "../auth/RegistrationForm"; @@ -48,7 +48,7 @@ interface IProps { /** Was one or more other devices logged out whilst changing the password */ didLogoutOutOtherDevices: boolean; }) => void; - onError: (error: { error: string }) => void; + onError: (error: Error) => void; rowClassName?: string; buttonClassName?: string; buttonKind?: string; @@ -183,7 +183,16 @@ export default class ChangePassword extends React.Component { } }, (err) => { - this.props.onError(err); + if (err instanceof Error) { + this.props.onError(err); + } else { + this.props.onError( + new UserFriendlyError("Error while changing password: %(error)s", { + error: String(err), + cause: undefined, + }), + ); + } }, ) .finally(() => { @@ -196,15 +205,19 @@ export default class ChangePassword extends React.Component { }); } - private checkPassword(oldPass: string, newPass: string, confirmPass: string): { error: string } | undefined { + /** + * Checks the `newPass` and throws an error if it is unacceptable. + * @param oldPass The old password + * @param newPass The new password that the user is trying to be set + * @param confirmPass The confirmation password where the user types the `newPass` + * again for confirmation and should match the `newPass` before we accept their new + * password. + */ + private checkPassword(oldPass: string, newPass: string, confirmPass: string): void { if (newPass !== confirmPass) { - return { - error: _t("New passwords don't match"), - }; + throw new UserFriendlyError("New passwords don't match"); } else if (!newPass || newPass.length === 0) { - return { - error: _t("Passwords can't be empty"), - }; + throw new UserFriendlyError("Passwords can't be empty"); } } @@ -307,11 +320,24 @@ export default class ChangePassword extends React.Component { const oldPassword = this.state.oldPassword; const newPassword = this.state.newPassword; const confirmPassword = this.state.newPasswordConfirm; - const err = this.checkPassword(oldPassword, newPassword, confirmPassword); - if (err) { - this.props.onError(err); - } else { + try { + // TODO: We can remove this check (but should add some Cypress tests to + // sanity check this flow). This logic is redundant with the input field + // validation we do and `verifyFieldsBeforeSubmit()` above. See + // https://github.com/matrix-org/matrix-react-sdk/pull/10615#discussion_r1167364214 + this.checkPassword(oldPassword, newPassword, confirmPassword); return this.onChangePassword(oldPassword, newPassword); + } catch (err) { + if (err instanceof Error) { + this.props.onError(err); + } else { + this.props.onError( + new UserFriendlyError("Error while changing password: %(error)s", { + error: String(err), + cause: undefined, + }), + ); + } } }; diff --git a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx index 0827065fac..7b10119705 100644 --- a/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx +++ b/src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx @@ -21,9 +21,9 @@ import { SERVICE_TYPES } from "matrix-js-sdk/src/service-types"; import { IThreepid } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { IDelegatedAuthConfig, M_AUTHENTICATION } from "matrix-js-sdk/src/matrix"; -import { MatrixError } from "matrix-js-sdk/src/matrix"; +import { HTTPError } from "matrix-js-sdk/src/matrix"; -import { _t } from "../../../../../languageHandler"; +import { UserFriendlyError, _t } from "../../../../../languageHandler"; import ProfileSettings from "../../ProfileSettings"; import * as languageHandler from "../../../../../languageHandler"; import SettingsStore from "../../../../../settings/SettingsStore"; @@ -43,7 +43,7 @@ import Spinner from "../../../elements/Spinner"; import { SettingLevel } from "../../../../../settings/SettingLevel"; import { UIFeature } from "../../../../../settings/UIFeature"; import { ActionPayload } from "../../../../../dispatcher/payloads"; -import ErrorDialog from "../../../dialogs/ErrorDialog"; +import ErrorDialog, { extractErrorMessageFromError } from "../../../dialogs/ErrorDialog"; import AccountPhoneNumbers from "../../account/PhoneNumbers"; import AccountEmailAddresses from "../../account/EmailAddresses"; import DiscoveryEmailAddresses from "../../discovery/EmailAddresses"; @@ -260,18 +260,35 @@ export default class GeneralUserSettingsTab extends React.Component { - // TODO: Figure out a design that doesn't involve replacing the current dialog - let errMsg = err.error || err.message || ""; - if (err.httpStatus === 403) { - errMsg = _t("Failed to change password. Is your password correct?"); - } else if (!errMsg) { - errMsg += ` (HTTP status ${err.httpStatus})`; + private onPasswordChangeError = (err: Error): void => { + logger.error("Failed to change password: " + err); + + let underlyingError = err; + if (err instanceof UserFriendlyError && err.cause instanceof Error) { + underlyingError = err.cause; } - logger.error("Failed to change password: " + errMsg); + + const errorMessage = extractErrorMessageFromError( + err, + _t("Unknown password change error (%(stringifiedError)s)", { + stringifiedError: String(err), + }), + ); + + let errorMessageToDisplay = errorMessage; + if (underlyingError instanceof HTTPError && underlyingError.httpStatus === 403) { + errorMessageToDisplay = _t("Failed to change password. Is your password correct?"); + } else if (underlyingError instanceof HTTPError) { + errorMessageToDisplay = _t("%(errorMessage)s (HTTP status %(httpStatus)s)", { + errorMessage, + httpStatus: underlyingError.httpStatus, + }); + } + + // TODO: Figure out a design that doesn't involve replacing the current dialog Modal.createDialog(ErrorDialog, { - title: _t("Error"), - description: errMsg, + title: _t("Error changing password"), + description: errorMessageToDisplay, }); }; diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index dd9afaf5d7..003c0b49c8 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1344,6 +1344,7 @@ "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.": "If you want to retain access to your chat history in encrypted rooms you should first export your room keys and re-import them afterwards.", "You can also ask your homeserver admin to upgrade the server to change this behaviour.": "You can also ask your homeserver admin to upgrade the server to change this behaviour.", "Export E2E room keys": "Export E2E room keys", + "Error while changing password: %(error)s": "Error while changing password: %(error)s", "New passwords don't match": "New passwords don't match", "Passwords can't be empty": "Passwords can't be empty", "Do you want to set an email address?": "Do you want to set an email address?", @@ -1546,7 +1547,10 @@ "Set the name of a font installed on your system & %(brand)s will attempt to use it.": "Set the name of a font installed on your system & %(brand)s will attempt to use it.", "Customise your appearance": "Customise your appearance", "Appearance Settings only affect this %(brand)s session.": "Appearance Settings only affect this %(brand)s session.", + "Unknown password change error (%(stringifiedError)s)": "Unknown password change error (%(stringifiedError)s)", "Failed to change password. Is your password correct?": "Failed to change password. Is your password correct?", + "%(errorMessage)s (HTTP status %(httpStatus)s)": "%(errorMessage)s (HTTP status %(httpStatus)s)", + "Error changing password": "Error changing password", "Your password was successfully changed.": "Your password was successfully changed.", "You will not receive push notifications on other devices until you sign back in to them.": "You will not receive push notifications on other devices until you sign back in to them.", "Success": "Success",