Skip to content
This repository was archived by the owner on Dec 14, 2021. It is now read-only.

472: Adding autofill route if fingerprint onboarding is skipped#474

Merged
eliserichards merged 6 commits intomasterfrom
472-onboarding-nav
Mar 2, 2019
Merged

472: Adding autofill route if fingerprint onboarding is skipped#474
eliserichards merged 6 commits intomasterfrom
472-onboarding-nav

Conversation

@eliserichards
Copy link
Copy Markdown
Contributor

Fixes #472

Testing and Review Notes

Screenshots or Videos

To Do

  • double check the original issue to confirm it is fully satisfied
  • add testing notes and screenshots in PR description to help guide reviewers
  • add unit tests
    • optional: consider adding instrumentation (integration/UI) tests
  • consider running this branch in a debug simulator and check for memory leak notifications or any warnings
  • request the "UX" team perform a design review (if/when applicable)
  • make sure CI builds are passing (e.g.: fix lint and other errors)
  • check on the accessibility of any added UI

@eliserichards eliserichards requested review from a team and nickbrandt February 26, 2019 01:45
@ghost ghost assigned eliserichards Feb 26, 2019
@ghost ghost added the in progress label Feb 26, 2019
Copy link
Copy Markdown
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

I think the logic here needs some small amount of rework, but I may be misunderstanding.

} else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
if (SettingStore.shared.autofillAvailable) {
dispatcher.dispatch(RouteAction.Onboarding.Autofill)
}
Copy link
Copy Markdown
Contributor

@jhugman jhugman Feb 26, 2019

Choose a reason for hiding this comment

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

What happens in Android versions at or above O, but where autofillAvailable is false?

Should it just return without dispatching the confirm route?

Copy link
Copy Markdown
Contributor Author

@eliserichards eliserichards Feb 26, 2019

Choose a reason for hiding this comment

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

I believe that's what it should do. If autofill isn't available then we shouldn't tell them to use autofill
I see what you're saying now :)

sashei
sashei previously requested changes Feb 27, 2019
Copy link
Copy Markdown
Contributor

@sashei sashei left a comment

Choose a reason for hiding this comment

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

would like to see complete tests!

@eliserichards eliserichards dismissed sashei’s stale review February 27, 2019 23:32

addressed comments :)

@eliserichards eliserichards requested review from sashei and removed request for jhugman and nickbrandt February 27, 2019 23:32
Copy link
Copy Markdown
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

🚜 🚜 🚜

This is looking pretty good. The tests need a bit of attention, but the main code is 👍 .

Comment thread app/src/main/java/mozilla/lockbox/presenter/FxALoginPresenter.kt
private lateinit var preferences: SharedPreferences
@RequiresApi(Build.VERSION_CODES.O)
private lateinit var autofillManager: AutofillManager
open lateinit var autofillManager: AutofillManager
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👾

This is making AutofillManager part of the public interface for this store. If I understand correctly, this is to make it easier to test.

I think probably the best way of solving this problem is to mock out the autofill manager and context, then inject it using injectContext. The SettingStoreTest shows this quite nicely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ahh I think I tried every way of mocking the AutofillManager and SettingStore except whenCalled(context.getSystemService(AutofillManager::class.java)).thenReturn(autofillManager). Thanks!

Copy link
Copy Markdown
Contributor

@sashei sashei left a comment

Choose a reason for hiding this comment

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

nice work @eliserichards !! thanks for putting those tests together :))

Comment thread app/src/main/java/mozilla/lockbox/presenter/FxALoginPresenter.kt
@eliserichards eliserichards dismissed jhugman’s stale review March 1, 2019 23:40

Addressed comments and made the autofillManager private to the SettingStore again :)

@eliserichards eliserichards merged commit 694df0c into master Mar 2, 2019
@ghost ghost removed the in progress label Mar 2, 2019
@sashei sashei deleted the 472-onboarding-nav branch March 4, 2019 22:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When device security is not set, the Autofill onboarding screen is skipped on first run

3 participants