Skip to content

LG 11140 Break up webauthn selection presenter#9467

Merged
kevinsmaster5 merged 12 commits intomainfrom
kmas-lg-11140-break-up-webauthn-selection-presenter
Nov 1, 2023
Merged

LG 11140 Break up webauthn selection presenter#9467
kevinsmaster5 merged 12 commits intomainfrom
kmas-lg-11140-break-up-webauthn-selection-presenter

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Oct 27, 2023

🎫 Ticket

LG-11140

🛠 Summary of changes

Split up Webauthn & Webauthn Platform presenters into Sign Up and Sign In classes as a follow up to #9211

📜 Testing Plan

Adding Security key and Face or touch unlock MFA should continue to work as expected. Sign in with Security key or Face or touch unlock MFA should continue to work as expected.

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review October 30, 2023 17:01
Copy link
Contributor

@mdiarra3 mdiarra3 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looking good. Couple minor comments

Comment on lines 3 to 6
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest reordering these methods so that the constructor is first.

Suggested change
def method
:webauthn_platform
end
def initialize(user:, configuration: nil)
@user = user
@configuration = configuration
end
def initialize(user:, configuration: nil)
@user = user
@configuration = configuration
end
def method
:webauthn_platform
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got that reordered.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove checks below to configuration.blank?, since I would expect that the configuration should never be blank in the "Sign in" classes. They should be replaced with false.

@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-11140-break-up-webauthn-selection-presenter branch from 3069918 to 4d2be38 Compare October 31, 2023 16:55
@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-11140-break-up-webauthn-selection-presenter branch from 4d2be38 to 3c30b5d Compare October 31, 2023 19:23
@kevinsmaster5 kevinsmaster5 merged commit ccb2192 into main Nov 1, 2023
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-11140-break-up-webauthn-selection-presenter branch November 1, 2023 12:38
Comment on lines +18 to +26
it 'raises with missing translation' do
expect(presenter.label).to eq(
t('two_factor_authentication.two_factor_choice_options.webauthn_platform'),
)
end
end

describe '#info' do
it 'raises with missing translation' do
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed these as I start work on a related ticket. These methods don't raise, so I think we should revise the expectation description accordingly.

Suggested change
it 'raises with missing translation' do
expect(presenter.label).to eq(
t('two_factor_authentication.two_factor_choice_options.webauthn_platform'),
)
end
end
describe '#info' do
it 'raises with missing translation' do
it 'returns the label text' do
expect(presenter.label).to eq(
t('two_factor_authentication.two_factor_choice_options.webauthn_platform'),
)
end
end
describe '#info' do
it 'returns the info text' do

@@ -17,19 +17,6 @@
end
end

describe '#render_in' do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we lost test coverage here, we still have a SetUpWebauthnSelectionPresenter#render_in method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants