Make `STPSource` conform to `STPPaymentMethod`, exposing labels & images #976

joeydong-stripe · 2018-06-26T22:09:07Z

Summary

Adds STPPaymentMethod protocol implementation for STPSource. In particular, it enables source.label, source.image, and source.templateImage.

There's potentially room to improve the labels & images - right now non-cards just mirror whatever the Source type is.

Motivation

Issue originally raised after extensive investigation between Joey & Fred. See individual commits. More details in IOS-816.

Testing

Automated tests added for the new functionality.

We originally made it a private implementation because the `STPPaymentMethod` protocol implementations don't return anything useful for `label` and `image` but we're going to improve on that next.

danj-stripe

I've left some comments with questions/suggestions.

Also, I think you have a typo in the Pull Request description. I think this is enabling source.label, not source.length.

PS: is there a reason you didn't use the Pull Request template with Summary/Motivation/Testing?

danj-stripe · 2018-07-03T20:30:37Z

Stripe/STPSource.m

+        case STPSourceTypeEPS:
+            return @"EPS";
+        case STPSourceTypeMultibanco:
+            return @"Multibanco";


Do any of these need to be localizable strings?

My first thought was no, because they're brand/product names. On the other hand, I suspect it'd be weird to see "Alipay" instead of "支付宝" (according to wikipedia) in an app localized to Chinese.

Good point, we should probably wrap them in NSLocalizedString calls in case users want to localize it themselves in a particular way.

danj-stripe · 2018-07-03T20:34:06Z

Stripe/STPSource.m

+        case STPSourceTypeMultibanco:
+            return @"Multibanco";
+        case STPSourceTypeUnknown:
+            return [STPCard stringFromBrand:STPCardBrandUnknown];


wdyt about using @"Unknown" here instead of this layer of indirection with STPCardBrandUnknown?

Yeah why not. I first put this here because I was focused on card source types but it is a little out of place in the STPSourceTypeUnknown block.

danj-stripe · 2018-07-03T20:37:36Z

CHANGELOG.md

@@ -1,3 +1,5 @@
+* Adds `STPPaymentMethod` protocol implementation for `STPSource` [#976](https://github.com/stripe/stripe-ios/pull/976)


wdyt about: mentioning what this accomplishes? I suspect this won't mean much to our users

danj-stripe · 2018-07-03T20:44:10Z

Stripe/STPSource.m

+        case STPSourceTypeSofort:
+            return @"SOFORT";
+        case STPSourceTypeThreeDSecure:
+            return @"3D Secure";


Did you consider including more details from the source in the label strings?

3DS is an obvious one: it's wrapping a card, and the customer is probably more likely to recognize the card that's used in the Source than the fact that it was charged through 3DS. It'd also be hard for them to disambiguate if they had more than one.

I suspect the STPSourceParam factory methods are good references for what fields might be interesting on a per-SourceType basis.

Yeah I did want to include more information if available but it looks like we don't make API Version guarantees on what information is included in the source specific blocks. If I'm wrong about the guarantees, then yeah we should add them appropriately.

danj-stripe · 2018-07-12T21:40:11Z

Stripe/Resources/Localizations/en.lproj/Localizable.strings

 /* Title for Payment Method screen */
 "Payment Method" = "Payment Method";

 /* Caption for Phone field on address form */
 "Phone" = "Phone";

-/* Short string for postal code (text used in non-US countries). Please keep to 8 characters or less if possible. */


Were these character count suggestions removed because you re-ran genstrings? I wonder if we need to move them into the STPLocalizedString call

…eSource Conflicts: CHANGELOG.md Tests/Tests/STPFixtures.h Tests/Tests/STPFixtures.m

Also, fixing `testPaymentMethodTemplateImage()` for non-Card sources. It was missing the assertion

csabol-stripe

lgtm

csabol-stripe · 2018-08-03T20:35:49Z

CHANGELOG.md

@@ -1,3 +1,6 @@
+## XX.Y.Z


this is just a placeholder for when we make the next release?

Yeah. I think it's better for us to update the CHANGELOG during the release process, to avoid conflicts while merging PRs, but sometimes we do it during the PR.

…ro from #976 Same problem as d1e3669, but they were disabled to avoid build failures. Now that we have a (hack/workaround) for image equality tests, use it.

…ro from #976 (#1005) Same problem as d1e3669, but they were disabled to avoid build failures. Now that we have a (hack/workaround) for image equality tests, use it.

Release version 22.1.1

Make STPSource's implementation of STPPaymentMethod public

119a5c9

We originally made it a private implementation because the `STPPaymentMethod` protocol implementations don't return anything useful for `label` and `image` but we're going to improve on that next.

joeydong-stripe self-assigned this Jun 26, 2018

joeydong-stripe force-pushed the joeydong/displayableSource branch 8 times, most recently from 4583ba8 to 5922603 Compare June 27, 2018 16:55

Add suitable STPPaymentMethod protocol implementation in STPSource

15f81a0

joeydong-stripe force-pushed the joeydong/displayableSource branch from 5922603 to 15f81a0 Compare June 27, 2018 18:48

joeydong-stripe changed the title ~~[WIP] Displayable STPSource~~ Displayable STPSource Jun 27, 2018

joeydong-stripe assigned csabol-stripe and danj-stripe and unassigned joeydong-stripe Jun 27, 2018

danj-stripe reviewed Jul 3, 2018

View reviewed changes

danj-stripe assigned joeydong-stripe Jul 12, 2018

joeydong-stripe unassigned danj-stripe and csabol-stripe Jul 12, 2018

joeydong-stripe added 2 commits July 12, 2018 10:08

Merge branch 'master' into joeydong/displayableSource

4a6b87c

Add localization options for [STPSource label]

a9458b7

joeydong-stripe force-pushed the joeydong/displayableSource branch from 2151761 to a9458b7 Compare July 12, 2018 17:27

Adjust CHANGELOG entry

ed95cb8

danj-stripe reviewed Jul 12, 2018

View reviewed changes

danj-stripe added 4 commits August 2, 2018 13:38

Merge remote-tracking branch 'origin/master' into joeydong/displayabl…

d64dedc

…eSource Conflicts: CHANGELOG.md Tests/Tests/STPFixtures.h Tests/Tests/STPFixtures.m

Revert removal of the character length requests for zip/postal

f3fcec1

Fix test failures on iOS 9: compare PNG data of UIImages

d1e3669

Also, fixing `testPaymentMethodTemplateImage()` for non-Card sources. It was missing the assertion

Update documentation on STPPaymentMethod

24f9824

danj-stripe unassigned joeydong-stripe Aug 2, 2018

danj-stripe self-assigned this Aug 2, 2018

danj-stripe changed the title ~~Displayable STPSource~~ Make STPSource conform to STPPaymentMethod, exposing labels & images Aug 2, 2018

danj-stripe requested a review from csabol-stripe August 2, 2018 23:36

csabol-stripe approved these changes Aug 3, 2018

View reviewed changes

stripe-ci added approved labels Aug 3, 2018

danj-stripe merged commit df4fded into master Aug 3, 2018

danj-stripe deleted the joeydong/displayableSource branch August 3, 2018 20:44

jaimepark-stripe added a commit that referenced this pull request Apr 11, 2022

Merge pull request #976 from stripe-ios/releases/22.1.1

f8bba7f

Release version 22.1.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `STPSource` conform to `STPPaymentMethod`, exposing labels & images #976

Make `STPSource` conform to `STPPaymentMethod`, exposing labels & images #976

joeydong-stripe commented Jun 26, 2018 •

edited by danj-stripe

Loading

danj-stripe left a comment

danj-stripe Jul 3, 2018

joeydong-stripe Jul 12, 2018

danj-stripe Jul 3, 2018

joeydong-stripe Jul 12, 2018

danj-stripe Jul 3, 2018

danj-stripe Jul 3, 2018

joeydong-stripe Jul 12, 2018

danj-stripe Jul 12, 2018

csabol-stripe left a comment

csabol-stripe Aug 3, 2018

danj-stripe Aug 3, 2018

		@@ -1,3 +1,5 @@
		* Adds `STPPaymentMethod` protocol implementation for `STPSource` [#976](https://github.com/stripe/stripe-ios/pull/976)

Make STPSource conform to STPPaymentMethod, exposing labels & images #976

Make STPSource conform to STPPaymentMethod, exposing labels & images #976

Conversation

joeydong-stripe commented Jun 26, 2018 • edited by danj-stripe Loading

Summary

Motivation

Testing

danj-stripe left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabol-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `STPSource` conform to `STPPaymentMethod`, exposing labels & images #976

Make `STPSource` conform to `STPPaymentMethod`, exposing labels & images #976

joeydong-stripe commented Jun 26, 2018 •

edited by danj-stripe

Loading