-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MBL-1123: Refactor PaymentSourceSelected to be an enum #1990
MBL-1123: Refactor PaymentSourceSelected to be an enum #1990
Conversation
d4e0064
to
67bb693
Compare
@@ -0,0 +1,25 @@ | |||
import Foundation | |||
public enum PaymentSourceSelected: Equatable { | |||
case paymentSourceId(String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions for better names welcome - as far as I know, this is an ID that comes from GraphQL and represents a particular payment methods stored in our own DB.
) | ||
PaymentSourceSelected.paymentSourceId(UserCreditCards.visa.id), | ||
PaymentSourceSelected | ||
.setupIntentClientSecret("seti_1LVlHO4VvJ2PtfhK43R6p7FI_secret_MEDiGbxfYVnHGsQy8v8TbZJTQhlNKLZ") | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example of how these are different now.
67bb693
to
9572634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good. Great cleanup. I suggest running through our normal pledge flow to make sure we're all good there before merging. Even though tests are passing and this is more of a cleanup, changing our checkout code makes me paranoid
public var paymentSourceId: String? { | ||
if case let .paymentSourceId(value) = self { | ||
return value | ||
} else { | ||
return nil | ||
} | ||
} | ||
|
||
public var setupIntentClientSecret: String? { | ||
if case let .setupIntentClientSecret(value) = self { | ||
return value | ||
} else { | ||
return nil | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any particular reason you didn't want to use switch statements here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not really. Just tried it out and it's a little neater looking, so I'll swap these.
paymentSourceId: selectedPaymentSheetPaymentMethodCardId, | ||
isSetupIntentClientSecret: true | ||
) | ||
return PaymentSourceSelected.setupIntentClientSecret(selectedPaymentSheetPaymentMethodCardId) | ||
case let (.some(selectedPaymentMethodCardId), .none): | ||
return PaymentSourceSelected( | ||
paymentSourceId: selectedPaymentMethodCardId, | ||
isSetupIntentClientSecret: false | ||
) | ||
return PaymentSourceSelected.paymentSourceId(selectedPaymentMethodCardId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so much clearer!
9572634
to
fefa917
Compare
fefa917
to
5bb1591
Compare
Ran two manual tests on staging - I made a pledge with an existing card, and another pledge that was adding a new card. |
📲 What
Refactor
PaymentSourceSelected
to be anenum
, instead of astruct
.🤔 Why
For
MBL-1123
, I'm adding another type of payment source toPledgeViewModel
. We start to createPaymentIntent
secrets, which are a separate kind of client secret from our existingSetupIntent
secrets.Before, the different between payment types was modeled by storing the payment source ID and a boolean, which flagged whether or not the string was a stripe token or a Kickstarter ID.
After, this is modeled as an enum, so that it should be much clearer what kind of payment source we're passing around the codebase. As a bonus, this actually deleted a few lines of redundant code, since it makes the plumbing a little simpler.
When I finish this ticket, we'll have an enum like this: