[NT-422] Payment source refactor #895
Conversation
KsApi/models/Backing.swift
Outdated
| public let expirationDate: String? | ||
| public let id: String? | ||
| public let lastFour: String? | ||
| public let paymentType: PaymentType? |
There was a problem hiding this comment.
I think this should be non-optional.
There was a problem hiding this comment.
I was following the description of this ticket
There was a problem hiding this comment.
I think the ticket says it should be non-optional? 🤔
There was a problem hiding this comment.
OMG! paymentType is non-optional, but type is! Got lost between types 😅
| import Prelude | ||
| import ReactiveExtensions | ||
| import ReactiveSwift | ||
| import UIKit |
There was a problem hiding this comment.
Can we get rid of the UIKit import here and just return the imageName as a string? I think we generally try to keep our VMs decoupled from UIKit.
|
|
||
| public protocol ManagePledgePaymentMethodViewModelOutputs { | ||
| /// Emits the card's image. | ||
| var cardImage: Signal<UIImage?, Never> { get } |
There was a problem hiding this comment.
Here this could be cardImageName: Signal<String?, Never>
|
|
||
| self.cardNumberAccessibilityLabel = self.paymentSourceSignal | ||
| .map { | ||
| [$0.type?.description, Strings.Card_ending_in_last_four(last_four: $0.lastFour ?? .init())] |
There was a problem hiding this comment.
Is it possible to set this up such that if type and lastFour are nil, we don't send anything back in the output? It doesn't seem like we'd ever want a situation where the label reads " , ending in " 🤔
There was a problem hiding this comment.
Good point. I will change that
| .map { Strings.Ending_in_last_four(last_four: $0) } | ||
|
|
||
| self.expirationDateText = self.paymentSourceSignal | ||
| .map { String($0.expirationDate?.dropLast(3) ?? "") } |
There was a problem hiding this comment.
Same here, I think we probably shouldn't have the signal output when the expirationDate is nil:
self.expirationDateText = self.paymentSourceSignal
.map { $0.expirationDate }
.skipNil()
.map { String($0.dropLast(3)) }
... etc
There was a problem hiding this comment.
I missed that one. Good catch!
Generated by 🚫 Danger |
| @@ -68,12 +91,12 @@ extension Backing: Argo.Decodable { | |||
| on that environment. This is a workaround to allow us to test on Staging and should be deleted once the | |||
| data is being returned normally. | |||
| */ | |||
| private func tryDecodePaymentSource(_ json: JSON?) -> Decoded<GraphUserCreditCard.CreditCard?> { | |||
| private func tryDecodePaymentSource(_ json: JSON?) -> Decoded<Backing.PaymentSource?> { | |||
There was a problem hiding this comment.
Do we still need this in this PR?
There was a problem hiding this comment.
We do. I tried removing it, but same issue still happens on Staging.
| @@ -91,8 +114,27 @@ extension Backing: EncodableType { | |||
| } | |||
| } | |||
|
|
|||
| extension Backing.PaymentSource: Argo.Decodable { | |||
There was a problem hiding this comment.
I think we could have this only conform to Swift's Decodable and do a similar function like before to return an Argo.Decodable success or failure?
There was a problem hiding this comment.
Just a thought, perhaps this PR is not the place to start doing that, but I think that can be an approach to help us to start moving away from Argo.
KsApi/models/Backing.swift
Outdated
| public static func decode(_ json: JSON) -> Decoded<Backing.PaymentSource?> { | ||
| return curry(Backing.PaymentSource.init) | ||
| <^> json <|? "expiration_date" | ||
| <*> (json <|? "id" <|> (json <| "id" >>- intToString)) |
There was a problem hiding this comment.
CJ merged the change to return a String instead of an Int so you should be able to remove this now! 🎉
|
|
||
| public protocol ManagePledgePaymentMethodViewModelOutputs { | ||
| /// Emits the card's image. | ||
| var cardImage: Signal<String, Never> { get } |
There was a problem hiding this comment.
Maybe cardImageName would be more accurate here?
| self.vm.inputs.configureWith(value: Backing.PaymentSource.applePay) | ||
|
|
||
| self.cardImage.assertValue("icon--apple-pay") | ||
| self.cardNumberTextShortStyle.assertLastValue("Ending in 1111") |
There was a problem hiding this comment.
Are we missing an assertion here for the accessibility label? And actually on second thought, do we want to update the accessibility label to indicate that the card is associated with Apple Pay?
There was a problem hiding this comment.
Regarding the accessibility label, it's a good idea, but I think this ends up being out of scope for this PR.
📲 What
PaymentSourceto be used on the manage pledge context.🤔 Why
GraphUserCreditCardmodel, but due some differences between the payloads returned byv1andgraphQL, we've decided to create a new model to avoid work arounds.🛠 How
PaymentSourceand making it Argo decodable.ManagePledgePaymentMethodViewModel👀 See
✅ Acceptance criteria
ManagePledgeViewControllerorManagePledgePaymentMethodView