For #10248 - Obfuscates credit card numbers autofill prompt #10256

codrut-topliceanu · 2021-05-13T14:41:24Z

Fixes #10248

Needs to land before mozilla-mobile/fenix#19420

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks .
Tests: This PR includes thorough tests.
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices.

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.

…ompt

gabrielluong · 2021-05-13T14:54:15Z

...mpts/src/main/java/mozilla/components/feature/prompts/creditcard/CreditCardItemViewHolder.kt

+        /**
+         * Number of digits to be displayed after ellipses on an obfuscated credit card number.
+         */


Suggested change

/**

* Number of digits to be displayed after ellipses on an obfuscated credit card number.

*/

// Number of digits to be displayed after ellipses on an obfuscated credit card number.

Still looking for this to be committed.

gabrielluong · 2021-05-13T14:57:18Z

components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt

+ * Returns the input string preceded by 4 ellipses, locked LTR, to be used for
+ * obfuscating credit card numbers.
+ */
+fun String.addEllipsesToCreditCardNumber(): String {


I thought our intention was to add a getter in the CreditCard data class to return the obfuscated card number? This seems ok, but maybe too specific for CreditCards. I will check in with some AC folks to see if there are any strong opinions.

I agree with @gabrielluong, this function is too spesific to CreditCard to be as String extension function. If we don't want to add the function directly to CreditCard we would make this to be an extension function of CreditCard.

Will request changes then.

@gabrielluong @Amejia481 I agree. I do want to add a getter, but there is more than one crediCard type object, as you mentioned, and I did not want to have the same code in two places. Should I just add the custom getter to both?

I think either option is fine. The CreditCard in concept-engine is more specific to the GV Autocomplete CreditCard, which will have the credit card decrypted at that point. You will also need to get the last 4 digits for that one. So, it seems easier for me to imagine 2 different getters due to the additional need to get the last 4 digits for the concept-engine CreditCard. I don't really know what a custom getter for both will look like, so feel free to post both version and we can work through it together.

gabrielluong · 2021-05-13T16:09:03Z

components/support/ktx/src/main/java/mozilla/components/support/ktx/kotlin/String.kt

+ * Returns the input string preceded by 4 ellipses, locked LTR, to be used for
+ * obfuscating credit card numbers.
+ */
+fun String.addEllipsesToCreditCardNumber(): String {


Will request changes then.

…ditCards

codecov · 2021-05-17T13:03:35Z

Codecov Report

Merging #10256 (2fe0c06) into master (ffa8a0f) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master   #10256      +/-   ##
============================================
- Coverage     74.18%   74.15%   -0.04%     
+ Complexity     6290     5176    -1114     
============================================
  Files           841      650     -191     
  Lines         31830    25765    -6065     
  Branches       5294     4348     -946     
============================================
- Hits          23613    19105    -4508     
+ Misses         5499     4365    -1134     
+ Partials       2718     2295     -423

Impacted Files	Coverage Δ	Complexity Δ
...lla/components/concept/engine/prompt/CreditCard.kt	`90.00% <66.66%> (-10.00%)`	`8.00 <1.00> (+1.00)`	⬇️
...ure/prompts/creditcard/CreditCardItemViewHolder.kt	`100.00% <100.00%> (ø)`	`4.00 <1.00> (ø)`
...vice/nimbus/ui/NimbusExperimentsAdapterDelegate.kt
...illa/components/feature/share/RecentAppsStorage.kt
...wser/menu2/adapter/SmallMenuCandidateViewHolder.kt
...nts/lib/crash/service/SendCrashTelemetryService.kt
...components/service/digitalassetlinks/ext/Client.kt
...lla/components/concept/menu/candidate/TextStyle.kt
...java/mozilla/components/browser/domains/Domains.kt
...zilla/components/concept/menu/ext/MenuCandidate.kt
... and 183 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 ffa8a0f...2fe0c06. Read the comment docs.

.../src/test/java/mozilla/components/feature/prompts/creditcard/CreditCardItemViewHolderTest.kt

gabrielluong · 2021-05-17T15:57:21Z

...mpts/src/main/java/mozilla/components/feature/prompts/creditcard/CreditCardItemViewHolder.kt

+        /**
+         * Number of digits to be displayed after ellipses on an obfuscated credit card number.
+         */


Still looking for this to be committed.

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

components/concept/engine/src/main/java/mozilla/components/concept/engine/prompt/CreditCard.kt

gabrielluong · 2021-05-18T15:45:34Z

components/concept/engine/src/main/java/mozilla/components/concept/engine/prompt/CreditCard.kt

@@ -25,4 +25,24 @@ data class CreditCard(
    val expiryMonth: String,
    val expiryYear: String,
    val cardType: String
-) : Parcelable
+) : Parcelable {
+    val obfuscatedCardNumber: String


We have a test for this https://searchfox.org/mozilla-mobile/source/android-components/components/concept/engine/src/test/java/mozilla/components/concept/engine/prompt/CreditCardTest.kt#13. Can you add a check that to confirm that we output the right value?

gabrielluong · 2021-05-18T15:46:13Z

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

@@ -192,7 +192,24 @@ data class CreditCard(
    val timeLastUsed: Long?,
    val timeLastModified: Long,
    val timesUsed: Long
-) : Parcelable
+) : Parcelable {
+    val obfuscatedCardNumber: String


Perhaps add the same test for this too.

…ompt (mozilla-mobile#10256) * For mozilla-mobile#10248 - Obfuscates credit card numbers autofill prompt * For mozilla-mobile#10248 - Changed string extension to getters in CreditCards * For mozilla-mobile#10248 - Addressed comments * For mozilla-mobile#10248 - Updates tests Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

For mozilla-mobile#10248 - Obfuscates credit card numbers autofill pr…

6513c15

…ompt

codrut-topliceanu requested a review from Amejia481 as a code owner May 13, 2021 14:41

codrut-topliceanu requested a review from gabrielluong May 13, 2021 14:41

codrut-topliceanu mentioned this pull request May 13, 2021

For #18270 - Adds ellipses+last 4 digits to CC display mozilla-mobile/fenix#19420

Merged

3 tasks

gabrielluong reviewed May 13, 2021

View reviewed changes

gabrielluong suggested changes May 13, 2021

View reviewed changes

For mozilla-mobile#10248 - Changed string extension to getters in Cre…

5468b12

…ditCards

codrut-topliceanu requested review from csadilek, grigoryk and pocmo as code owners May 17, 2021 12:51

gabrielluong suggested changes May 17, 2021

View reviewed changes

gabrielluong self-assigned this May 17, 2021

For mozilla-mobile#10248 - Addressed comments

039e24d

gabrielluong reviewed May 18, 2021

View reviewed changes

For mozilla-mobile#10248 - Updates tests

37ef4b8

gabrielluong approved these changes May 19, 2021

View reviewed changes

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

Merge branch 'master' into 10248_obfuscated_cc_numbers

2fe0c06

mergify bot merged commit b6ee8b3 into mozilla-mobile:master May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #10248 - Obfuscates credit card numbers autofill prompt #10256

For #10248 - Obfuscates credit card numbers autofill prompt #10256

codrut-topliceanu commented May 13, 2021 •

edited by gabrielluong

gabrielluong May 13, 2021

gabrielluong May 17, 2021

gabrielluong May 13, 2021

Amejia481 May 13, 2021

gabrielluong May 13, 2021

codrut-topliceanu May 14, 2021

gabrielluong May 14, 2021

gabrielluong May 13, 2021

codecov bot commented May 17, 2021 •

edited

gabrielluong May 17, 2021

gabrielluong May 18, 2021

gabrielluong May 18, 2021

For #10248 - Obfuscates credit card numbers autofill prompt #10256

For #10248 - Obfuscates credit card numbers autofill prompt #10256

Conversation

codrut-topliceanu commented May 13, 2021 • edited by gabrielluong

Pull Request checklist

After merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 17, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codrut-topliceanu commented May 13, 2021 •

edited by gabrielluong

codecov bot commented May 17, 2021 •

edited