Add Self-service Password Change

[reivilibre]

This is still missing some polish, like:

• Self-service Password Change: UI defects around form validation #2855
• Self-service Password Change: add password strength bar #2854
• Self-service Password Change: support the state of not having a password if passwords are enabled #2862

Fixes #2148.

[cloudflare-workers-and-pages]

Deploying matrix-authentication-service-docs with    Cloudflare Pages

Latest commit:	2c8ed61
Status:	✅  Deploy successful!
Preview URL:	https://0e588458.matrix-authentication-service-docs.pages.dev
Branch Preview URL:	https://rei-fe-pw-change.matrix-authentication-service-docs.pages.dev

View logs

[sandhose]

A few things here and there, but that looks really nice!

[sandhose]

There is an existing Link component which should have the same style?

[reivilibre]

I stole this from UserEmail FWIW

[reivilibre]

the Link component does not have the underline or the white text colour, it just looks like plain grey text

[sandhose]

Fundamentally we're missing a good implementation of the Text Link component: https://www.figma.com/design/rTaQE2nIUSLav4Tg3nozq7/Compound-Web-Components?node-id=645-3494&t=OH2MjoxKqsJTB1vF-4

I'll leave the duplication for now, and will see to implement that in Compound

[sandhose]

Actually for those I would just throw within the component? None of those are not recoverable by the user, so showing a page 'crash' is probably the best thing?

[reivilibre]

some of these I'm on the fence about.

e.g. password change disabled: the functionality could have been disabled since you navigated to the page, it feels a bit crap to show a generic 'something went wrong' error when we know the actual reason.

not allowed / not found: these probably make more sense if we had an admin control panel in MAS, but agreed we can probably lose them

no current password: I'd be tempted to keep it, since it's explanatory of a real-world error you might hit?

[sandhose]

A few nitpicks but we're almost there!

[sandhose]

Arguably, we don't need to load the current user as we're not really displaying anything about them, but I guess we need this for showing a 404 if the user isn't logged in. Also that query is most certainly cached so 🤷

[reivilibre]

yeah I was on the fence about this one...

[sandhose]

I'm not sure I like the switch/case logic up there. Because we have some errors specific to fields, I would rather have a series of {result.data?.setPassword.status === SetPasswordStatus.NoCurrentPassword && (<Alert … />)}. wdyt?

Also could you stick alerts at the top of the form?

[reivilibre]

do you think we should not show the errors in an alert if they are specific to a form field? I was thinking that might make it a bit harder to notice but wasn't entirely sure.

Both the switch and the sequence of conditionally rendered alerts feel a bit tedious but struggling to think of anything better

[reivilibre]

that said: I think it's important to have a 'default' case (for unhandled errors etc) and by taking your approach, it's harder to see how we'd do that

[sandhose]

do you think we should not show the errors in an alert if they are specific to a form field? I was thinking that might make it a bit harder to notice but wasn't entirely sure.

Yep, as radix forms will take care of focusing the errored field. I don't think I've saw any form with an alert like that in the designs

that said: I think it's important to have a 'default' case (for unhandled errors etc) and by taking your approach, it's harder to see how we'd do that

that's a fair point. Although could be offset by doing something like:

  const status = result.data?.setPassword.status;
  switch (status) {
    // Cases that are handled by the form
    case SetPasswordStatus.InvalidNewPassword:
    case SetPasswordStatus.NoCurrentPassword:
    case SetPasswordStatus.PasswordChangesDisabled:
    case SetPasswordStatus.WrongPassword:
    case SetPasswordStatus.Allowed:
    case undefined:
      break;

    case SetPasswordStatus.NotFound:
    case SetPasswordStatus.NotAllowed:
      throw new Error(`unexpected error when changing password: ${status}`);

    default:
      unreachable(status);
  }

[reivilibre]

This means you then have to remember to keep that in sync with what the form shows. I wouldn't mind something different but I prefer to make sure that handling the error and knowing we handle the error are guaranteed to be the same

[sandhose]

I'm fine with the double-error message for now, will see during design review I guess wether we should change that or not

[reivilibre]

I updated the double-error thing to not show, if it does focus like you say then I think my concern has been dealt with. I also find it a bit weird to double-show so on balance I'll go with this for now.