-
Notifications
You must be signed in to change notification settings - Fork 166
LG 11145 Break up MFA selection presenter classes for Phone Presenters #9560
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
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
004f4f2
changelog: Internal, tech debt, Break up MFA presenter class for phone
kevinsmaster5 a05eee9
split phone, voice, and sms presenter classes up
kevinsmaster5 a5ac190
add tests cases for sign_in and set_up phone presenter class
kevinsmaster5 fa1eb1c
split setup signin presenter spec for voice and sms
kevinsmaster5 473c08e
lint fix
kevinsmaster5 23c0ad2
remove old phone selection presenter spec
kevinsmaster5 8c5a20f
rename phone presenter in spec
kevinsmaster5 a77d9e6
remove unneeded configuration variable, leverage user for type method
kevinsmaster5 761176e
remove info method from phone sub classes
kevinsmaster5 f1ffce1
update options presenter spec with newly split classes
kevinsmaster5 5c39c4a
merge sms and voice presenters
kevinsmaster5 28b47f2
revise specs according to merged classes
kevinsmaster5 07e563d
remove deprecated spec and lint fix
kevinsmaster5 620c7e8
change info to switch and fix regression with disabled? method
kevinsmaster5 365f8fe
lint fix
kevinsmaster5 c218120
fix spec
kevinsmaster5 b4f693d
remove deprecated translations from setup presenter
kevinsmaster5 931b8f0
move reader :method to phone sign in presenter
kevinsmaster5 cb5a3c4
fix lint
kevinsmaster5 ad0955b
fix lint
kevinsmaster5 5e14f17
clean up selection presenter class
kevinsmaster5 d7b8914
remove unneeded configuration setting
kevinsmaster5 fd859d4
remove configuration from set up presenter spec
kevinsmaster5 c72de0d
add sms and voice outage spec, standardize spec syntax
kevinsmaster5 5591554
clarify some syntax
kevinsmaster5 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 3 additions & 7 deletions
10
...thentication/phone_selection_presenter.rb → ...ation/set_up_phone_selection_presenter.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
55 changes: 55 additions & 0 deletions
55
app/presenters/two_factor_authentication/sign_in_phone_selection_presenter.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| module TwoFactorAuthentication | ||
| class SignInPhoneSelectionPresenter < SignInSelectionPresenter | ||
| attr_reader :configuration, :user, :method | ||
|
|
||
| def initialize(user:, configuration:, method:) | ||
| @user = user | ||
| @configuration = configuration | ||
| @method = method | ||
| end | ||
|
|
||
| def type | ||
| if MfaContext.new(configuration&.user).phone_configurations.many? | ||
| "#{super}_#{configuration.id}" | ||
| else | ||
| super | ||
| end | ||
| end | ||
|
|
||
| def label | ||
| if method.to_s == 'sms' | ||
| t('two_factor_authentication.login_options.sms') | ||
| elsif method.to_s == 'voice' | ||
| t('two_factor_authentication.login_options.voice') | ||
| end | ||
| end | ||
|
|
||
| def info | ||
| case method | ||
| when :sms | ||
| t( | ||
| 'two_factor_authentication.login_options.sms_info_html', | ||
| phone: configuration.masked_phone, | ||
| ) | ||
| when :voice | ||
| t( | ||
| 'two_factor_authentication.login_options.voice_info_html', | ||
| phone: configuration.masked_phone, | ||
| ) | ||
| else | ||
| t('two_factor_authentication.two_factor_choice_options.phone_info') | ||
| end | ||
| end | ||
|
|
||
| def disabled? | ||
| case method | ||
| when :sms | ||
| OutageStatus.new.vendor_outage?(:sms) | ||
| when :voice | ||
| OutageStatus.new.vendor_outage?(:voice) | ||
| else | ||
| OutageStatus.new.all_phone_vendor_outage? | ||
| end | ||
| end | ||
| end | ||
| end | ||
22 changes: 0 additions & 22 deletions
22
app/presenters/two_factor_authentication/sms_selection_presenter.rb
This file was deleted.
Oops, something went wrong.
22 changes: 0 additions & 22 deletions
22
app/presenters/two_factor_authentication/voice_selection_presenter.rb
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
92 changes: 92 additions & 0 deletions
92
spec/presenters/two_factor_authentication/sign_in_phone_selection_presenter_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe TwoFactorAuthentication::SignInPhoneSelectionPresenter do | ||
| let(:user) { create(:user, :with_phone) } | ||
| let(:configuration) { create(:phone_configuration, user: user) } | ||
|
|
||
| let(:presenter) do | ||
| described_class.new(user: user, configuration: configuration, method: 'phone') | ||
| end | ||
|
|
||
| describe '#type' do | ||
| it 'returns phone appended with configuration id' do | ||
| expect(presenter.type).to eq "phone_#{configuration.id}" | ||
| end | ||
| end | ||
|
|
||
| describe '#info' do | ||
| it 'raises with missing translation' do | ||
| expect(presenter.info).to eq( | ||
| t('two_factor_authentication.two_factor_choice_options.phone_info'), | ||
| ) | ||
| end | ||
| context 'with sms method' do | ||
| let(:presenter) do | ||
| described_class.new(user: user, configuration: configuration, method: :sms) | ||
| end | ||
| it 'returns the correct translation for sms' do | ||
| expect(presenter.info).to eq( | ||
| t( | ||
| 'two_factor_authentication.login_options.sms_info_html', | ||
| phone: configuration.masked_phone, | ||
| ), | ||
| ) | ||
| end | ||
| end | ||
| context 'with voice method' do | ||
| let(:presenter) do | ||
| described_class.new(user: user, configuration: configuration, method: :voice) | ||
| end | ||
| it 'returns the correct translation for voice' do | ||
| expect(presenter.info).to eq( | ||
| t( | ||
| 'two_factor_authentication.login_options.voice_info_html', | ||
| phone: configuration.masked_phone, | ||
| ), | ||
| ) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe '#disabled?' do | ||
| let(:user_without_mfa) { create(:user) } | ||
| let(:phone) { build(:phone_configuration, phone: '+1 888 867-5309') } | ||
| let(:presenter_without_mfa) do | ||
| described_class.new(configuration: phone, user: user_without_mfa, method: 'phone') | ||
| end | ||
| it { expect(presenter_without_mfa.disabled?).to eq(false) } | ||
|
|
||
| context 'all phone vendor outage' do | ||
aduth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| before do | ||
| allow_any_instance_of(OutageStatus).to receive(:all_phone_vendor_outage?).and_return(true) | ||
| end | ||
|
|
||
| it { expect(presenter_without_mfa.disabled?).to eq(true) } | ||
| end | ||
|
|
||
| context 'voice vendor outage' do | ||
| let(:presenter_without_mfa) do | ||
| described_class.new(configuration: phone, user: user, method: method) | ||
| end | ||
| let(:method) { :voice } | ||
| before do | ||
| allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:voice). | ||
| and_return(true) | ||
| end | ||
|
|
||
| it { expect(presenter_without_mfa.disabled?).to eq(true) } | ||
| end | ||
|
|
||
| context 'sms vendor outage' do | ||
| let(:presenter_without_mfa) do | ||
| described_class.new(configuration: phone, user: user, method: method) | ||
| end | ||
| let(:method) { :sms } | ||
| before do | ||
| allow_any_instance_of(OutageStatus).to receive(:vendor_outage?).with(:sms).and_return(true) | ||
| end | ||
|
|
||
| it { expect(presenter_without_mfa.disabled?).to eq(true) } | ||
| end | ||
| end | ||
| end | ||
52 changes: 0 additions & 52 deletions
52
spec/presenters/two_factor_authentication/sms_selection_presenter_spec.rb
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
methodis a ruby built-in method so I feel like it would be good to avoid overriding that if we can? Could we name the attribute something else? likedelivery_method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. It would have to be changed on all of the MFA presenters if so.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a good idea, though in this case we're porting over an existing pair of methods:
identity-idp/app/presenters/two_factor_authentication/sms_selection_presenter.rb
Lines 3 to 5 in 59755ae
identity-idp/app/presenters/two_factor_authentication/voice_selection_presenter.rb
Lines 3 to 5 in 59755ae
It's also already used throughout every other 2FA presenter:
That being said, the only place I see it actually being used is in the presenter
typemethod, and the only purpose that seemed to serve was for the label method's switches, which should all be effectively removed as of this pull request. I could probably clean this up as part of the follow-on work I mentioned at #9560 (comment) .