Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Card Metadata Service to CardNumberController #4573

Merged
merged 26 commits into from
Feb 25, 2022

Conversation

epan-stripe
Copy link
Contributor

Summary

Add the card metadata service to the CardNumberController to account for longer card lengths.

Testing

  • Added tests
  • Modified tests
  • Manually verified

@github-actions
Copy link
Contributor

Diffuse output:

Copy link
Contributor

@skyler-stripe skyler-stripe 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 some extra testings both in the unit tests and adding some notes in the top level description of this PR would be helpful.

@michelleb-stripe
Copy link
Contributor

You should be able to remove the string: card_number_longer_than_expected

@michelleb-stripe
Copy link
Contributor

Remove FormPreview, it isn't working anyway, and preview is probably better used on smaller units.

@michelleb-stripe
Copy link
Contributor

Let's just make sure the michelleb/credit-card-compose gets reviewed and approved before merging this branch in.

@epan-stripe
Copy link
Contributor Author

PTAL @michelleb-stripe

Comment on lines 25 to 26
private val cardAccountRangeRepository: CardAccountRangeRepository,
private val workContext: CoroutineContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

These are no longer required as member variables, just parameters so you can remove the private val.

}
override val fieldState: Flow<TextFieldState> = _fieldState

private val _hasFocus = MutableStateFlow(false)

@VisibleForTesting
val accountRangeService = CardAccountRangeService(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really cool, now all the logic for the service is tied up nicely in a bow in this CardAccountRangeService


internal class CardNumberController constructor(
private val cardTextFieldConfig: CardNumberConfig,
private val cardAccountRangeRepository: CardAccountRangeRepository,
private val workContext: CoroutineContext,
private val staticCardAccountRanges: StaticCardAccountRanges = DefaultStaticCardAccountRanges(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move StaticCardAccountRanges into the CardAccountRangeService. It feels like the CardNumberController shouldn't care about if it needs to query the StaticCardAccount or the server to get the AccountResult. All it needs to know is to call update (a new function) on CardAccountRangeService, and it will get updated on any required changes to the AccountResult through the callback.

Copy link
Contributor

@michelleb-stripe michelleb-stripe left a comment

Choose a reason for hiding this comment

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

A few more comments, it is looking really good!

@michelleb-stripe michelleb-stripe removed their assignment Feb 18, 2022
@epan-stripe
Copy link
Contributor Author

PTAL @michelleb-stripe

Comment on lines 24 to 26
val isLoading: Flow<Boolean> = cardAccountRangeRepository.loading.map {
it
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val isLoading: Flow<Boolean> = cardAccountRangeRepository.loading.map {
it
}
val isLoading: Flow<Boolean> = cardAccountRangeRepository.loading

Comment on lines +34 to +41
val staticAccountRange = staticCardAccountRanges.filter(cardNumber)
.let { accountRanges ->
if (accountRanges.size == 1) {
accountRanges.first()
} else {
null
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the check for whether the accountRange.size is equal to 1

@@ -3,5 +3,4 @@
<!--
Items here need to be entered in our translation tool
-->
<string name="card_number_longer_than_expected" tools:ignore="ExtraTranslation">The card number is longer than expected, but you can still submit it.</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

yay! excited to see this removed!

@michelleb-stripe
Copy link
Contributor

michelleb-stripe commented Feb 24, 2022

We don't have to address it in this PR, but a couple of things I am seeing:

  • On master the card brand doesn't populate until the last zero of 6205 50 is entered. Not sure what exactly is causing that different behavior
  • The CVV icon in michelleb/credit-card-compose is not showing up, but the placeholder text does change.
  • Next field from the card number appears to be the CVV instead of the date
  • Only the card number shows a red line under the text when it loses focus, in dark mode, but all fields have the line under in light mode. It should be that the CVC and expiration text have a red line when the field is in error.
    • When the CVC is in error it shows the card number error message
  • If only a CVC is entered the error message shows that the card number is valid. It should be blank, but not an error.

You can move these to the top level PR and address them as separate PRs. Thanks epan!

@epan-stripe epan-stripe merged commit a18c6c2 into michelleb/credit-card-compose Feb 25, 2022
@epan-stripe epan-stripe deleted the elena/card-metadataservice branch February 25, 2022 14:51
epan-stripe added a commit that referenced this pull request Apr 25, 2022
* [WIP]

* Display and receive input for Credit Card number and CVC.

* Add in the credit billing fields, need hidden visibility to work on address fields.

* Billing code now shows the correct fields.

* Billing now added to the credit element.

* Credit card number length verification corrected.

* Refactor Elements so each element (including sections) provide the form field values of it's elements.  This simplifies the form quite a bit!

* Add in the expiration date functionality.

* Refactor so that each element returns a flow of FormFieldEntries, so that we don't have to iterate through section element fields to get the form field values.

* Fix unit test

* Rename class

* More rework

* Get ready for merge with master

* Change from liveData to flow.

* Row is working but not expiry date

* Make the keypad just numbers.

* Make the keypad just numbers.

* Set the expiration month and date correctly in the formFieldValues

* Update the focus to visit the CVC

* Handle a full text field state.

* Unit tests

* ktlintFormat

* Format card number based on max pan for card number

* Cleanup date util for expiration month

* Make the label flowable

* Fix internals

* Cleanup

* Cleanup

* Fix unit tests

* ktlintFormat

* Undos

* ktlintFormat

* Ignore tests that cause other failures

* Working on the merge.

* Almost building

* Get closer to master

* Update files.

* Building

* Fix unit tests

* Cleanup

* Remove extra comment

* Fix failing tests

* Fix linting

* Fix SaveForFutureUseController.label not showing

* Cleanup

* apiDump

* apiDump

* Fix some failing tests, apiDump, ktlintFormat

* Fix remaining failing tests, apiDump, ktlintFormat

* Fix todo

* Simplify and add DateConfig tests and cleanup commented out code in TextFieldController.

* ktlintFormat apiDump

* Add icons to the IBAN, credit card and CVC fields. (#4359)

* Add Card Metadata Service to CardNumberController (#4573)

* Add card metadataservice for CardNumberController

* Add card metadataservice for CardNumberController

* Minor fixes

* Minor fixes

* Make more methods internal

* Fix tests

* Minor fixes

* Fix failing tests

* Fix failing tests

* Refactor CardNumberController and add tests

* Don't hardcode visualTransformation panLength

* Change interface to service

* Fix tests

* Update payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt

Co-authored-by: michelleb-stripe <[email protected]>

* Update payments-ui-core/src/main/java/com/stripe/android/ui/core/elements/CardNumberController.kt

Co-authored-by: michelleb-stripe <[email protected]>

* Move staticCardAccountRanges

* Add loading icon

* Add loading icon

* Minor fix

Co-authored-by: michelleb-stripe <[email protected]>

* Don't show cardbrand if there are multiple possibilities

* Don't show cardbrand if there are multiple possibilities

* Material Theme information gathering.

* Revert "Material Theme information gathering."

This reverts commit 067ea3b.

* Immediately detect invalid card numbers (no brand)

* Fix invalid vs incomplete dateconfig error text

* Fix CVC icon and next field focus

* Fix expiry date accessibility reader

* Fix merge issues

* Add ability to move focus on delete

* Fix dark mode error underline

* Fix cursor and spacing position in card details

* update icons and colors for future wardrobe work

* Add card information title

* Add tests for card number formatting

* Revert "Add tests for card number formatting"

This reverts commit 0c87580

* Add country list to card and sepa billing spec. (#4669)

* Credit Card Address and focus bug fixes (#4665)

* Fix width of rows (#4676)

* Fix row max width calculation

* Update visibleFields calculation method

* Fix spacing for AMEX and Discover cards (#4672)

* Fix card number spacing for AMEX and Discover

* Add tests for card number formatting

* Fix test fixtures

* Fix tests

* Don't show CVC error when CardBrand is unknown (#4681)

* Don't show CVC error when cardbrand is unknown

* Fix tests

* Stop accessibility talkback reading textfields twice (#4683)

* ktformat

* Fix detekt

* checkbox colors read from theme object now

* Initial implementation

* Fix camera icon

* Move design changes to different branch

* Update camera icon design

* Add string resource for scan card text and remove "test" name

* Use actual publishable key

* Add removeCardScanFragment to StripeCardScanProxy

* Restart fragment onResume and onPause

* Restart fragment onResume and onPause

* Update to match CardScanSheet changes

* Fix scan card button styling

* Fix scan card button styling

* Update CHANGELOG

* Move CARD_SCAN_PARCELABLE_NAME to companion object

* Stop passing context in and delete CardDetailsSpec

* Make CardScanActivity internal

* Make code segment more functional

* Fix comment

* Add tests for StripeCardScanProxy

* Fix tests

* Fix tests

* Stop setting Scan card button font weight

* Add context back to TransformSpecToElements

* Add CardDetailsSectionController so credit card form values are passed through

* Update CardScanActivity package

* Remove context from FormUI

* Add clarifying comment

* Remove unnecessary padding

* Create ScanCardButtonUI composable

* Move isStripeCardScan object to controller instead of Composable

* Use material camera icon

Co-authored-by: Michelle Brubaker <[email protected]>
Co-authored-by: michelleb-stripe <[email protected]>
Co-authored-by: James Woo <[email protected]>
Co-authored-by: Skyler Reimer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants