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

[MadLittleMods]

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

Part of element-hq/element-web#9597 and also fixes that issue since it's the last piece mentioned (there could be other cases we log translated strings though).

Fix element-hq/element-web#9597

Related PRs:

• Replace newTranslatableError with UserFriendlyError (#10440 #10440
• Properly translate errors in AddThreepid.ts so they show up translated to the user but not in our logs #10432

Todo

• Write a few Cypress sanity check tests because this whole flow seems untested
  • This was only necessary previously when I was thinking about removing some redundant logic. In order to move forward with this PR as-is, I've restored that logic. See Properly translate errors in ChangePassword.tsx so they show up translated to the user but not in our logs #10615 (comment) for more details

Checklist

• Tests written for new code (and old code if feasible)
• Linter and other CI checks pass
• Sign-off given on the changes (see CONTRIBUTING.md)

---

Here's what your changelog entry will look like:

🐛 Bug Fixes

• Properly translate errors in ChangePassword.tsx so they show up translated to the user but not in our logs (#10615). Fixes We log the translated version of several errors element-hq/element-web#9597. Contributed by @MadLittleMods.

[MadLittleMods]

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

[MadLittleMods]

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

[alunturner]

Just one (non-blocking) comment.

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]

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.

[MadLittleMods]

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 ⏩

[MadLittleMods]

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)

[MadLittleMods]

Thanks for the review @alunturner and merge @andybalaam 🐅