Lg 6230 mfa selection count

[SammySteiner]

What

After users create an account with 1 mfa method, they are now shown a message suggesting they create a second mfa configuration. If they so choose, we want to display the mfa option they've already configured as checked and disabled.

How

• Update presenters with disabled? method to determine if the mfa option should be disabled.
• Presenter options should also know if they are default checked.
• There is a new style-guide release to cover these styles
• New SVG icons for the illustrated options work with filter now that we've removed the white background layer so we can grey out the icons with css
• Updated tests

Example

[SammySteiner]

Checkbox looked funny on zoomed out browser, here's a normal pic:

[aduth]

I'd have a soft-preference to either using the inline comment hint or just avoid the dynamic key, since it'd be easier to maintain over time closer to the source.

[SammySteiner]

I refactored this since the method in all 6 files was going to get long and put it into the presenter, with a mfa_config_count method that I could override in each individual presenter since that was the only real difference. Backup codes needed to be separate, but this is cleaner overall.

[zachmargolis]

1. looks like this top branch is not covered by tests
2. the return keyword is not needed
3. We can take advantage of Rails I18n built in pluralization: https://guides.rubyonrails.org/i18n.html#pluralization

So this can all be:

Suggested change

	if user.backup_code_configurations.unused.count == 1
	return t(
	'two_factor_authentication.two_factor_choice_options.unused_backup_code',
	count: mfa_configuration_count,
	)
	else
	return t(
	'two_factor_authentication.two_factor_choice_options.unused_backup_code_plural',
	count: mfa_configuration_count,
	)
	end
	t(
	'two_factor_authentication.two_factor_choice_options.unused_backup_code',
	count: mfa_configuration_count,
	)

If we change the translations to be structured like:

two_factor_authentication:
  two_factor_choice_options:
    unused_backup_code:
      one: 'one thing'
      other: '%{count} things'

[zachmargolis]

same comment about pluralization and %{count}

[zachmargolis]

Maybe consider making these keyword arguments?
Or could we derive user from configuration?

[zachmargolis]

I'd recommend using the attr accessor here (and in all these methods), because direct instance variable access will quietly return nil if typoed, but methods will throw errors

Suggested change

	[TwoFactorAuthentication::PivCacSelectionPresenter.new(nil, @user)]
	[TwoFactorAuthentication::PivCacSelectionPresenter.new(nil, user)]

[zachmargolis]

WDYT of renaming this method to mfa_configuration_description or something? Based on the name of the method it sounds like a type or something not user-facing

[zachmargolis]

should this default to zero? or should it throw unimplemented?

Suggested change

	def mfa_configuration_count; end
	def mfa_configuration_count
	raise NotImplementedError
	end

[SammySteiner]

I'm not sure NotImplementedError is the right approach here, https://ruby-doc.org/core-2.3.0/NotImplementedError.html, seems to be more about having things be not implemented depending on the environment or os.
There are some presenters that inherit the selection presenter and they should not get mfa_configuration_description/count called on them. I'm happy those returning nothing in that case as we have no requirements for what they should/would return if they are needed.

[zachmargolis]

one last change and then good to go I think