Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MFA module name not showing on step completion #19581

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

codyc1515
Copy link
Contributor

Proposed change

MFA module name is not showing on step completion resulting in an incomplete and potentially confusing sententence. Videos from three years ago show the message showing "Setup done for Authenticator app". Today, it simply shows "Setup done for".

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

  1. Enable MFA

Final step does not show the module configured and so the sentence is incomplete.

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@codyc1515 codyc1515 marked this pull request as draft January 31, 2024 02:43
@codyc1515 codyc1515 marked this pull request as ready for review January 31, 2024 03:12
@@ -97,7 +97,7 @@ class HaMfaModuleSetupFlow extends LitElement {
? html`<p>
${this.hass.localize(
"ui.panel.profile.mfa_setup.step_done",
{ step: this._step.title }
{ step: this._step!.handler }
Copy link
Member

Choose a reason for hiding this comment

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

handler is a technical key, why don't we get title anymore? I don't think this is the right fix.

Copy link
Member

Choose a reason for hiding this comment

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

This should probably just be handled by translations?

this.hass.localize(
  `component.auth.mfa_setup.${
    this._step!.handler
  }.step.${
    (this._step! as DataEntryFlowStepForm).step_id
  }.description`,
  this._step!.description_placeholders
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with both your comments here. I spent entirely too long longer at this before eventually throwing in the towel and going with this approach, if not just to get a second opinion.

It looks like this was working (from looking at some YouTube videos) in 2018 and was then broken at some point in the years gone by - unnoticed - probably when there was an architecture change.

I thought too that we could probably, for now, safely just refer to a single translation as there is only one method.

Then, I found out that “Authenticator app” is hardcoded somewhere deep in the code (not translated)...

On your second point. This all started with me simply trying to take {qr_code} and wrap it with {url} to get a clickable method of enrolling MFA.

Don’t know if you’ve ever tried scanning a QR on mobile from your own screen (hint: it’s physically impossible, without another device).

With the current approach, I wasn’t able to get there :(

Copy link
Member

Choose a reason for hiding this comment

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

Don’t know if you’ve ever tried scanning a QR on mobile from your own screen (hint: it’s physically impossible, without another device).

I always make a screenshot and then run the image through a QR scanner 🙃

Copy link
Member

Choose a reason for hiding this comment

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

I'll check what is available and what was changed soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked and this is working.

@bramkragten bramkragten enabled auto-merge (squash) February 28, 2024 10:55
@bramkragten bramkragten merged commit 1d9fa15 into home-assistant:dev Feb 28, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOTP setup message is truncated
2 participants