Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #9875

gabrielluong · 2021-03-11T18:38:25Z

Fixes #9838

Introduces CreditCardValidationDelegate and a default implementation in DefaultCreditCardValidationDelegate
Adds a wipeLocal() to CreditCardsAddressesStorage to clear out local state in between tests
Implements onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate

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.

codecov · 2021-03-12T15:16:32Z

Codecov Report

Merging #9875 (719b601) into main (57e8d7b) will decrease coverage by 7.47%.
The diff coverage is 26.53%.

❗ Current head 719b601 differs from pull request most recent head b8be7e9. Consider uploading reports for the commit b8be7e9 to get more accurate results

@@             Coverage Diff              @@
##               main    #9875      +/-   ##
============================================
- Coverage     74.15%   66.67%   -7.48%     
+ Complexity     6240      768    -5472     
============================================
  Files           835      119     -716     
  Lines         31562     3712   -27850     
  Branches       5268      590    -4678     
============================================
- Hits          23405     2475   -20930     
+ Misses         5465      903    -4562     
+ Partials       2692      334    -2358

Impacted Files	Coverage Δ
...tofill/GeckoCreditCardsAddressesStorageDelegate.kt	`5.88% <0.00%> (ø)`
.../mozilla/components/service/sync/autofill/Types.kt	`71.64% <0.00%> (ø)`
...nc/autofill/AutofillCreditCardsAddressesStorage.kt	`64.00% <75.00%> (ø)`
...nc/autofill/DefaultCreditCardValidationDelegate.kt	`77.77% <77.77%> (ø)`
...eature/awesomebar/provider/SearchActionProvider.kt	`63.15% <0.00%> (-1.85%)`	⬇️
...a/mozilla/components/browser/storage/sync/Types.kt	`80.43% <0.00%> (-1.66%)`	⬇️
.../components/concept/engine/prompt/PromptRequest.kt	`78.09% <0.00%> (-1.00%)`	⬇️
...re/awesomebar/provider/SearchSuggestionProvider.kt	`76.31% <0.00%> (-0.61%)`	⬇️
...java/mozilla/components/concept/engine/Settings.kt	`100.00% <0.00%> (ø)`
...components/feature/awesomebar/AwesomeBarFeature.kt	`89.33% <0.00%> (ø)`
... and 729 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 57e8d7b...b8be7e9. Read the comment docs.

grigoryk

This is pretty close! A few questions throughout, and some specific suggestions.

...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt

grigoryk · 2021-03-23T01:08:40Z

...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt

+        data class CanBeUpdated(val foundCreditCard: CreditCard) : Result()
+
+        /**
+         * The [CreditCard] cannot be saved.


"saved or updated", right?

Also - do we get anything from the API we can use as a "reason" here?

...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt

...rc/main/java/mozilla/components/service/sync/autofill/AutofillCreditCardsAddressesStorage.kt

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

grigoryk · 2021-03-23T01:36:16Z

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

+                    )
+                }
+                is CreditCardValidationDelegate.Result.Error -> {
+                    // Do nothing since an error occurred and the credit card cannot be saved.


Do we not return any results to GV?

grigoryk · 2021-03-23T01:52:29Z

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

@@ -8,8 +8,10 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.async
+import kotlinx.coroutines.launch


nit: this class should probably be called DefaultCreditCardsAddressesStorageDelegate if there's nothing gecko-specific going on here.

This is kinda of Gecko specific. It is only consumed by the GeckoAutocompleteStorageDelegate. Its respective Login counterpart also uses the Gecko prefix. If we want to rename this, I would suggest we do it for both, and note that it will be a breaking change.

grigoryk · 2021-03-23T02:00:02Z

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

@@ -38,6 +40,22 @@ class GeckoCreditCardsAddressesStorageDelegate(
    }

    override fun onCreditCardSave(creditCard: CreditCard) {
-        TODO("Not yet implemented")
+        val validationDelegate = DefaultCreditCardValidationDelegate(storage)


If this method is called in a quick succession (and you're not in control of its consumers, so you don't know if it'll be called in this fashion or not), it's possible that one of the add/update calls to storage below will fail (and we'll crash) with a SQL_LOCKED exception coming out of storage.

That's because sqlite doesn't support consequent writers, and within AutofillCreditCardsAddressesStorage you only have a single connection instance to the storage that's shared for both reading and writing. And, the coroutineContext use there is defined over Dispatchers.IO. That means you call addCreditCard and then very quickly an updateCreditCard while first operation is still running, it's possible for things to crash - we'll end up using a different thread for the update operation, and will try to talk to the storage while it's busy.

For comparison, PlacesStorage defines two scopes - readScope and writeScope. The former uses a thread pool to make sure reads are quick, while the latter uses a single threaded executor to make sure write operations are sequential and never overlap.

Just to avoid this class of errors and not have to worry about what's possible here and what's not, this is the kind of split I'd introduce here as well.

grigoryk · 2021-03-23T02:02:42Z

...est/java/mozilla/components/service/sync/autofill/DefaultCreditCardValidationDelegateTest.kt

+
+            assertEquals(Result.CanBeCreated, result)
+        }
+}


Note how your tests assume that weird/bad things won't happen (they don't test for them). See my comment in the validate method.

...rc/main/java/mozilla/components/service/sync/autofill/DefaultCreditCardValidationDelegate.kt

mergify · 2021-04-08T16:25:24Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

mergify · 2021-04-20T18:52:36Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

mergify · 2021-04-29T14:43:26Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

mergify · 2021-05-19T14:59:14Z

This pull request has conflicts when rebasing. Could you fix it @gabrielluong? 🙏

pocmo · 2021-08-11T10:21:41Z

Looking through old PRs... Is this something we still want to get landed?

grigoryk · 2021-09-10T23:57:20Z

@pocmo yup, definitely worth landing this - and the pieces on the GV side to call into this delegate. I'll rebase/clean this up and will land it.

…of InputResultDetail We need to wait until having a response from GeckoView on how it handled the touch only after which we'll know whether to animate the toolbar or not. The edgecase scenario of having pull to refresh enabled even before having a response from GeckoView will still work because "canOverscrollTop()" only checks for the touch to not be handled by the browser to pan the page.

The previous implementation was affected by the fact that the class has 3 integer properties, all with values [0..4] (-1 was added recently) and it would just add those value to compute the hashcode. Collisions were a given. By using decimal places for each property the new implementation should avoid collisions while allowing for all the other expected guarantees.

This commented method seems to be a leftover from the previous refactoring. "behavior.forceExpand(..)" now calls "expandWithAnimation" for which we already have a test at line 450.

This version of A-S included breaking changes around the history metadata API, which this patch resolves.

…lla-mobile#10913) * Ensure we notice and repair a missing or expired push endpoint. The core problem is that the device constellation's observer is registered after we've updated the devices, so the code that checks if re-subscribing is necessary is never hit. The easiest way of fixing this was to move the `refreshDevices` call to the account state state-machine, rather than the authentication state-machine, meaning we can ensure the `onAuthenticated` notification is delivered before we refresh the devices. Note that I also removed the special-case for "SEND_TAB" devices - push is also used for account-related notifications, such as being disconnected. * Address CI failures (hopefully!) * Update comments about push renewal * Tweak suggested by Grisha for when to call refreshDevices * More CI failures Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

…ozilla-mobile#9827) * Accounts: Make webchannel work with custom fxa server * Accounts: Send overrideFxAServer message only to custom fxa server * Accounts: make webchannel background message name constant Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

grigoryk · 2021-09-11T01:22:27Z

Ummm. Yeah, that didn't go quite as planned... Going to close this in favour of #10964

gabrielluong requested review from grigoryk and a team as code owners March 11, 2021 18:38

gabrielluong force-pushed the 9838 branch from 5882595 to 7713049 Compare March 11, 2021 18:41

gabrielluong assigned grigoryk Mar 11, 2021

gabrielluong added the 🕵️‍♀️ needs review PRs that need to be reviewed label Mar 11, 2021

gabrielluong force-pushed the 9838 branch from 7713049 to d1f4baa Compare March 12, 2021 15:08

gabrielluong force-pushed the 9838 branch 3 times, most recently from 78b9e18 to 3932be0 Compare March 16, 2021 16:52

grigoryk suggested changes Mar 23, 2021

View reviewed changes

grigoryk added ✏️ needs changes PRs that need some changes/fixes before they can land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Mar 23, 2021

grigoryk reviewed Mar 23, 2021

View reviewed changes

...rc/main/java/mozilla/components/service/sync/autofill/DefaultCreditCardValidationDelegate.kt Outdated Show resolved Hide resolved

gabrielluong force-pushed the 9838 branch from 3932be0 to 719b601 Compare April 15, 2021 18:51

gabrielluong force-pushed the 9838 branch from 719b601 to 32e88a4 Compare May 13, 2021 15:09

gabrielluong mentioned this pull request May 13, 2021

For #18275: Handle duplicates when adding or updating a credit card. mozilla-mobile/fenix#19499

Closed

3 tasks

Mugurell and others added 7 commits September 10, 2021 17:54

For mozilla-mobile#10598 - Always display URLs left to right

aa28d7a

For mozilla-mobile#10555 - BrowserToolbarBehaviorTest cleanup

c836793

This commented method seems to be a leftover from the previous refactoring. "behavior.forceExpand(..)" now calls "expandWithAnimation" for which we already have a test at line 450.

Update GeckoView (Nightly) to 92.0.20210716094806.

62b492c

Import l10n.

bd5eb33

Update GeckoView (Nightly) to 92.0.20210717093616.

a58ba71

pocmo and others added 18 commits September 10, 2021 17:55

Set version to 94.0

3ba834d

Update changelog for 93.0 release

8b4bc85

Update to GeckoView 94.0.20210907094849

6154571

Upgrade to A-S 82.3.0

fde1394

This version of A-S included breaking changes around the history metadata API, which this patch resolves.

Mark new push exceptions as non-fatal

f0fb6b7

Remove unused import in AutoPushFeature

e4ebd38

Forward search terms to Tabs Tray Tabs

61fc12e

Update GeckoView (Nightly) to 94.0.20210908094702.

c79e524

Update GeckoView (Nightly) to 94.0.20210908213905.

87db1a3

Handle breaking changes from a-s 83.0.0

d0f109d

Fix linter errors

4f53e54

Test fixes

ece8f08

Adds history metadata delete API

46b5c8c

Issue mozilla-mobile#10922: AwesomeBar(): Add test tags for UI testing.

a155fe4

Update GeckoView (Nightly) to 94.0.20210910033856.

d72c8a9

Lazily register ktlintFormat task

b8868cb

gabrielluong requested review from Amejia481, csadilek, jonalmeida, mcomella, pocmo, rocketsroger and a team as code owners September 11, 2021 00:58

grigoryk mentioned this pull request Sep 11, 2021

Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #10964

Closed

4 tasks

grigoryk closed this Sep 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #9875

Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #9875

gabrielluong commented Mar 11, 2021 •

edited

Loading

codecov bot commented Mar 12, 2021 •

edited

Loading

grigoryk left a comment

grigoryk Mar 23, 2021

grigoryk Mar 23, 2021

grigoryk Mar 23, 2021

gabrielluong May 12, 2021

grigoryk Mar 23, 2021

grigoryk Mar 23, 2021

mergify bot commented Apr 8, 2021

mergify bot commented Apr 20, 2021

mergify bot commented Apr 29, 2021

mergify bot commented May 19, 2021

pocmo commented Aug 11, 2021

grigoryk commented Sep 10, 2021

grigoryk commented Sep 11, 2021

Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #9875

Issue #9838: Introduce CreditCardValidationDelegate and implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate #9875

Conversation

gabrielluong commented Mar 11, 2021 • edited Loading

Pull Request checklist

After merge

codecov bot commented Mar 12, 2021 • edited Loading

Codecov Report

grigoryk left a comment

Choose a reason for hiding this comment

grigoryk Mar 23, 2021

Choose a reason for hiding this comment

grigoryk Mar 23, 2021

Choose a reason for hiding this comment

grigoryk Mar 23, 2021

Choose a reason for hiding this comment

gabrielluong May 12, 2021

Choose a reason for hiding this comment

grigoryk Mar 23, 2021

Choose a reason for hiding this comment

grigoryk Mar 23, 2021

Choose a reason for hiding this comment

mergify bot commented Apr 8, 2021

mergify bot commented Apr 20, 2021

mergify bot commented Apr 29, 2021

mergify bot commented May 19, 2021

pocmo commented Aug 11, 2021

grigoryk commented Sep 10, 2021

grigoryk commented Sep 11, 2021

gabrielluong commented Mar 11, 2021 •

edited

Loading

codecov bot commented Mar 12, 2021 •

edited

Loading