For #10331 - Allow dynamically toggling CC autofill #10332

Mugurell · 2021-05-25T15:56:27Z

Fixes #10331

Result in Fenix:

TogglingCCAutofill.mp4

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

gabrielluong · 2021-05-25T15:59:33Z

components/feature/prompts/src/main/java/mozilla/components/feature/prompts/PromptFeature.kt

@@ -131,6 +131,7 @@ class PromptFeature private constructor(
    private val shareDelegate: ShareDelegate,
    override val loginValidationDelegate: LoginValidationDelegate? = null,
    private val isSaveLoginEnabled: () -> Boolean = { false },
+    private val isCCAutofillEnabled: () -> Boolean = { false },


I think we should prefer saying CreditCard instead of CC. Can you also add the @Property?

Suggested change

private val isCCAutofillEnabled: () -> Boolean = { false },

private val isCreditCardAutofillEnabled: () -> Boolean = { false },

Sure, thank you!

gabrielluong · 2021-05-25T16:06:23Z

...in/java/mozilla/components/service/sync/autofill/GeckoCreditCardsAddressesStorageDelegate.kt

@@ -19,7 +20,8 @@ import mozilla.components.concept.storage.CreditCardsAddressesStorageDelegate
 */
 class GeckoCreditCardsAddressesStorageDelegate(
    private val storage: Lazy<CreditCardsAddressesStorage>,
-    private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO)
+    private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO),
+    private val isCCAutofillEnabled: () -> Boolean = { false }


I am not quite sure if we want to add this parameter to GeckoCreditCardsAddressesStorageDelegate. I think it should be enough if we just intercept the request and block it from showing.

We can disable this in PromptFeature. And this condition is indeed checked there also.
Here it's used to skip querying storage for data that won't be used.

This is also the same approach initially used for toggling logins autofill before there was a GeckoRuntimeSettings available for it.
I can remove this if you want but I feel like there's a real benefit in having this check also here.

codecov · 2021-05-25T16:45:59Z

Codecov Report

Merging #10332 (92023b7) into master (cfba5b3) will decrease coverage by 3.71%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master   #10332      +/-   ##
============================================
- Coverage     74.19%   70.48%   -3.72%     
+ Complexity     6298     1095    -5203     
============================================
  Files           841      192     -649     
  Lines         31837     5627   -26210     
  Branches       5296      994    -4302     
============================================
- Hits          23622     3966   -19656     
+ Misses         5495     1204    -4291     
+ Partials       2720      457    -2263

Impacted Files	Coverage Δ
...ozilla/components/feature/prompts/PromptFeature.kt	`78.38% <83.33%> (+0.31%)`	⬆️
...tofill/GeckoCreditCardsAddressesStorageDelegate.kt	`57.14% <100.00%> (+20.77%)`	⬆️
...mponents/feature/addons/ui/AddonsManagerAdapter.kt
...onents/browser/menu/item/BrowserMenuImageSwitch.kt
...owser/session/engine/middleware/CrashMiddleware.kt
...ill/authenticator/DeviceCredentialAuthenticator.kt
...ozilla/components/support/base/log/sink/LogSink.kt
...lla/components/feature/addons/worker/Extensions.kt
...mponents/browser/toolbar/display/DisplayToolbar.kt
...ure/media/middleware/RecordingDevicesMiddleware.kt
... and 641 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfba5b3...92023b7. Read the comment docs.

gabrielluong · 2021-05-25T16:49:17Z

...ava/mozilla/components/service/sync/autofill/GeckoCreditCardsAddressesStorageDelegateTest.kt

-            delegate.onCreditCardsFetch()
+            val storage: AutofillCreditCardsAddressesStorage = mock()
+            val storedCards = listOf<CreditCard>(mock())
+            Mockito.doReturn(storedCards).`when`(storage).getAllCreditCards()


Prehaps import doReturn

Suggested change

Mockito.doReturn(storedCards).`when`(storage).getAllCreditCards()

doReturn(storedCards).`when`(storage).getAllCreditCards()

Sure. Thank you!

…zilla-mobile#10332)

Mugurell requested a review from gabrielluong May 25, 2021 15:56

Mugurell requested a review from Amejia481 as a code owner May 25, 2021 15:56

gabrielluong reviewed May 25, 2021

View reviewed changes

Mugurell mentioned this pull request May 25, 2021

For #18263 - Allow dynamically toggling credit cards autofill mozilla-mobile/fenix#19649

Merged

3 tasks

Mugurell force-pushed the toggleCCAutofill branch from c5eee0c to f4b0d7f Compare May 25, 2021 16:34

gabrielluong approved these changes May 25, 2021

View reviewed changes

gabrielluong self-assigned this May 25, 2021

For mozilla-mobile#10331 - Allow dynamically toggling CC autofill

92023b7

Mugurell force-pushed the toggleCCAutofill branch from f4b0d7f to 92023b7 Compare May 25, 2021 16:59

Mugurell added the 🛬 needs landing (squash) PRs that are ready to land (squashed) label May 25, 2021

mergify bot merged commit 5e42e71 into mozilla-mobile:master May 25, 2021

Mugurell deleted the toggleCCAutofill branch May 25, 2021 17:17

grigoryk pushed a commit to gabrielluong/android-components that referenced this pull request Sep 11, 2021

For mozilla-mobile#10331 - Allow dynamically toggling CC autofill (mo…

1716531

…zilla-mobile#10332)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #10331 - Allow dynamically toggling CC autofill #10332

For #10331 - Allow dynamically toggling CC autofill #10332

Mugurell commented May 25, 2021 •

edited by gabrielluong

Loading

gabrielluong May 25, 2021

Mugurell May 25, 2021

gabrielluong May 25, 2021

Mugurell May 25, 2021

Mugurell May 25, 2021

codecov bot commented May 25, 2021 •

edited

Loading

gabrielluong May 25, 2021

Mugurell May 25, 2021

	private val isCCAutofillEnabled: () -> Boolean = { false },
	private val isCreditCardAutofillEnabled: () -> Boolean = { false },

	Mockito.doReturn(storedCards).`when`(storage).getAllCreditCards()
	doReturn(storedCards).`when`(storage).getAllCreditCards()

For #10331 - Allow dynamically toggling CC autofill #10332

For #10331 - Allow dynamically toggling CC autofill #10332

Conversation

Mugurell commented May 25, 2021 • edited by gabrielluong Loading

Pull Request checklist

After merge

gabrielluong May 25, 2021

Choose a reason for hiding this comment

Mugurell May 25, 2021

Choose a reason for hiding this comment

gabrielluong May 25, 2021

Choose a reason for hiding this comment

Mugurell May 25, 2021

Choose a reason for hiding this comment

Mugurell May 25, 2021

Choose a reason for hiding this comment

codecov bot commented May 25, 2021 • edited Loading

Codecov Report

gabrielluong May 25, 2021

Choose a reason for hiding this comment

Mugurell May 25, 2021

Choose a reason for hiding this comment

Mugurell commented May 25, 2021 •

edited by gabrielluong

Loading

codecov bot commented May 25, 2021 •

edited

Loading