Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

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

Merged
merged 2 commits into from
May 4, 2022
Merged

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

merged 2 commits into from
May 4, 2022

Conversation

gabrielluong
Copy link
Member

Fixes #9838

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 gabrielluong added the work in progress Not ready to land yet. Work in progress (WIP). label Nov 8, 2021
@gabrielluong gabrielluong added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Nov 22, 2021
@gabrielluong gabrielluong marked this pull request as ready for review November 22, 2021 17:06
@gabrielluong gabrielluong added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Nov 24, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 24, 2021

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

@gabrielluong gabrielluong added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Nov 26, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

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

@mergify
Copy link
Contributor

mergify bot commented Mar 19, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2022

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

grigoryk
grigoryk approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@grigoryk grigoryk 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 this looks good overall! I left some feedback on how you can improve the code to make it a tad faster (and maybe neater).

...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt
* @property expiryYear The credit card expiry year.
* @property cardType The credit card network ID.
*/
data class CreditCardEntry(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could reduce duplication between CreditCardEntry and CreditCard by composing them, i.e. wrapping one inside another, perhaps? E.g. CreditCard could have cardData or card field which is an instance of CreditCardEntry.

Doing this could be awkward in other places, but maybe not. Just an idea! This sort of thing worked out well for RecoverableTab and TabState stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit awkward because we would need to take the GV Autocomplete.CreditCard which has the plain credit card number and encrypt it back to CreditCard. Using CreditCardEntry that simply wraps the GV Autocomplete.CreditCard which makes it easier to pass thing along through the GV <-> Prompt Feature. This currently also follows the LoginEntry.

...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt Outdated Show resolved Hide resolved
...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt Outdated Show resolved Hide resolved
...cept/storage/src/main/java/mozilla/components/concept/storage/CreditCardsAddressesStorage.kt Outdated Show resolved Hide resolved
...rc/main/java/mozilla/components/service/sync/autofill/DefaultCreditCardValidationDelegate.kt Outdated Show resolved Hide resolved
@grigoryk grigoryk removed the 🕵️‍♀️ needs review PRs that need to be reviewed label Apr 12, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2022

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

@gabrielluong
Issue mozilla-mobile#9838: Introduce CreditCardValidationDelegate and…
42e0097 
… implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate

- Introduces `CreditCardValidationDelegate` and a default implementation in `DefaultCreditCardValidationDelegate`
- Implements `onCreditCardSave` in `GeckoCreditCardsAddressesStorageDelegate`
- Refactors `CreditCard` from concept-engine to `CreditCardEntry` in concept-storage so that it can validated with the `CreditCardValidationDelegate`
@gabrielluong gabrielluong added 🛬 needs landing PRs that are ready to land and removed 🛬 needs landing PRs that are ready to land labels May 3, 2022
@gabrielluong
Copy link
Member Author

@Mergifyio requeue

@gabrielluong gabrielluong added 🛬 needs landing (squash) PRs that are ready to land (squashed) 🛬 needs landing PRs that are ready to land and removed 🛬 needs landing (squash) PRs that are ready to land (squashed) 🛬 needs landing PRs that are ready to land labels May 4, 2022
@mergify mergify bot merged commit c277fe7 into mozilla-mobile:main May 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rocketsroger rocketsroger rocketsroger approved these changes

@grigoryk grigoryk grigoryk approved these changes

Assignees

@grigoryk grigoryk

Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement onCreditCardSave in GeckoCreditCardsAddressesStorageDelegate
3 participants
@gabrielluong @grigoryk @rocketsroger