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 12 commits into from
Apr 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
32ece81
Properly translate errors in `ChangePassword.tsx`
MadLittleMods Apr 15, 2023
07a0a74
Make more useful
MadLittleMods Apr 15, 2023
c62c6c9
Update i18n strings
MadLittleMods Apr 15, 2023
7786dd1
No need to checkPassword since field validation already covers this
MadLittleMods Apr 15, 2023
a7c2fe4
Update i18n strings
MadLittleMods Apr 15, 2023
772c15d
Merge branch 'develop' into madlittlemods/9597-properly-translate-cha…
MadLittleMods Apr 15, 2023
329b868
Revert "No need to checkPassword since field validation already cover…
MadLittleMods Apr 20, 2023
93f238b
Update i18n strings
MadLittleMods Apr 20, 2023
e0fbd73
Add todo context to note that we can remove this logic in the future
MadLittleMods Apr 20, 2023
1f8bd91
Ensure is an error
MadLittleMods Apr 20, 2023
fc70a22
Merge branch 'develop' into madlittlemods/9597-properly-translate-cha…
MadLittleMods Apr 20, 2023
798bb19
Remove else
MadLittleMods Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 13 additions & 22 deletions src/components/views/settings/ChangePassword.tsx
View file
Open in desktop
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 setting 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,18 +205,6 @@ export default class ChangePassword extends React.Component<IProps, IState> {
});
}

private checkPassword(oldPass: string, newPass: string, confirmPass: string): { error: string } | undefined {
if (newPass !== confirmPass) {
return {
error: _t("New passwords don't match"),
};
} else if (!newPass || newPass.length === 0) {
return {
error: _t("Passwords can't be empty"),
};
}
}

private optionallySetEmail(): Promise<boolean> {
// Ask for an email otherwise the user has no way to reset their password
const modal = Modal.createDialog(SetEmailDialog, {
Expand Down Expand Up @@ -306,13 +303,7 @@ 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 {
return this.onChangePassword(oldPassword, newPassword);
}
return this.onChangePassword(oldPassword, newPassword);
};

private async verifyFieldsBeforeSubmit(): Promise<boolean> {
Expand Down
45 changes: 32 additions & 13 deletions src/components/views/settings/tabs/user/GeneralUserSettingsTab.tsx
View file
Open in desktop
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,37 @@ 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;
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,
});
} else {
errorMessageToDisplay = errorMessage;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think about doing something along the lines of:

let errorMessageToDisplay = errorMessage;
if(it is an HTTPError) {
  if(it is a 403) {
    reassign errorMessageToDisplay
  } else {
    reassign errorMessageToDisplay
  } 
}

I'm not completely sold on whether the nested if is better than what you have, but I'm a fan of being able to get rid of the last else with the initial assignment of errorMessageToDisplay at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are 🤷 type of problems to me in this case.

A bigger better change would be to remove the else if (underlyingError instanceof HTTPError) case altogether in favor of translated HTTPError in the matrix-js-sdk as we're not providing much value by just translating ... (HTTP status ...) here.

but I'm a fan of being able to get rid of the last else with the initial assignment of errorMessageToDisplay at least.

I've updated to do this ⏩


// 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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,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 setting password: %(error)s": "Error while setting 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 @@ -1545,7 +1546,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