LG-5700 Fix replicated content for the "Select your authentication method" page#6261
LG-5700 Fix replicated content for the "Select your authentication method" page#6261
Conversation
changelog: Improvements, Content, Seperate phone and sms text labels
|
|
||
| return [] | ||
| else | ||
| [TwoFactorAuthentication::SmsSelectionPresenter.new] |
There was a problem hiding this comment.
looks like this line is not covered by any tests, should we add some?
| def options | ||
| webauthn_platform_option + webauthn_option + piv_cac_option + totp_option + | ||
| phone_options + backup_code_option | ||
| phone_options + sms_option + voice_option + backup_code_option |
There was a problem hiding this comment.
is sms_option + voice_option ever redundant with phone_options?
Realized these methods were not needed as two_factor_options_presenter is only called when adding mfa options
| def info | ||
| if configuration.present? | ||
| t( | ||
| 'two_factor_authentication.login_options.sms_info_html', |
There was a problem hiding this comment.
I was sorta confused why this string already existed. I see that it exists here, despite the fact that (a) it'd never be reached and (b) if it were reached, it would produce the wrong string, since we're not interpolating the %{phone} variable.
Should we remove those cases from the default class?
There was a problem hiding this comment.
Those are the for the labels. This would be overwriting info on these lines. Probably worth a delete if they are being overwritten anyways.
| 'two_factor_authentication.login_options.sms_info_html', | ||
| phone: configuration.masked_phone, | ||
| ) | ||
| end |
There was a problem hiding this comment.
Probably not necessary given how we're currently using these presenters only when a configuration is present, but if the parent class's info is meant to handle the absence of any configuration, we could defer to super in an else? I don't feel strongly.
| end | |
| else | |
| super | |
| end |
(Same applies for the other presenter)
…thod" page (#6261) Separate Voice and SMS option text changelog: Improvements, Content, Separate phone and sms text labels * add option to not show sms voice if phone option is available * remove voice and sms options from options_presenter
3 participants
Why?: Reduce the confusion in MFA by having "text message" for the text message field and "phone call" for the Phone Call field
changelog: Improvements, Content, Separate phone and sms text labels