Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Properly translate errors in ChangePassword.tsx so they show up translated to the user but not in our logs #10615

Merged
Merged
54 changes: 40 additions & 14 deletions src/components/views/settings/ChangePassword.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -183,7 +183,16 @@ export default class ChangePassword extends React.Component<IProps, IState> {
}
},
(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,
}),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SonarCloud coverage not happy with these files not tested at all but it's not something I'm going to invest in now.

Will need an exception here

},
)
.finally(() => {
Expand All @@ -196,15 +205,19 @@ export default class ChangePassword extends React.Component<IProps, IState> {
});
}

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");
}
}

Expand Down Expand Up @@ -307,11 +320,24 @@ export default class ChangePassword extends React.Component<IProps, IState> {
const oldPassword = this.state.oldPassword;
const newPassword = this.state.newPassword;
const confirmPassword = this.state.newPasswordConfirm;
const err = this.checkPassword(oldPassword, newPassword, confirmPassword);
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
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);
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
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,
}),
);
}
Comment on lines +330 to +340
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an aside that occurred to me reading this. I imagine that the pattern of usage for the UserFriendlyError in a catch block will frequently be => catch an error, check if it's an instance of Error, if it is, do something, if not, make a UserFriendlyError. I was wondering if there might be some use in rolling that logic into the UserFriendlyError class so that you could sling it the initial caught error, and it does this check for you.

-- @alunturner, #10615 (review)

I don't quite see this yet. I think the problem here arises more because this.props.onError(err) only accepts an error (which is my doing in this PR). We could update it to accept any and de-duplicate the UserFriendlyError creation there. But then every downstream consumer of ChangePassword.tsx would need to to do that. In our case, we only use it one place so maybe that is just better.

I'm having a hard time imagining how we can handle this generically vs actually wanting our user friendly error message for a given case.

Or maybe I'm just blind to what you're thinking and need an actual example 🙏. It's definitely possible there are better patterns left on the table here though and this just hasn't matured enough.

(feel free to continue conversation after merge)

}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand Down Expand Up @@ -253,18 +253,35 @@ export default class GeneralUserSettingsTab extends React.Component<IProps, ISta
PlatformPeg.get()?.setSpellCheckEnabled(spellCheckEnabled);
};

private onPasswordChangeError = (err: { error: string } & MatrixError): void => {
// 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, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining strict Typescript failures are unlrelated to this PR and the problem is generally tracked by element-hq/element-web#24899

title: _t("Error"),
description: errMsg,
title: _t("Error changing password"),
description: errorMessageToDisplay,
});
};

Expand Down
4 changes: 4 additions & 0 deletions src/i18n/strings/en_EN.json
Original file line number Diff line number Diff line change
Expand Up @@ -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?",
Expand Down Expand Up @@ -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",
Expand Down