MBL-1550: Don't change selected card when unavailable card is added to payment methods #2081
Conversation
…o payment methods
| @@ -1786,4 +1786,231 @@ final class PledgePaymentMethodsViewModelTests: TestCase { | |||
| XCTAssertTrue(allowedDelayedPaymentMethods) | |||
| } | |||
| } | |||
|
|
|||
| // A test for MBL-1550 | |||
| func testAddNewUnavailablePaymentMethod_userAlreadyHasCard_selectsValidPaymentMethod() { | |||
There was a problem hiding this comment.
One line to fix the bug, 229 lines to test it.
| XCTAssertEqual(self.reloadPaymentMethodsCards.lastValue?.count, 0) | ||
|
|
||
| // The discover is disabled and not selected. | ||
| let discoverCardData = self.reloadPaymentSheetPaymentMethodsCards.lastValue?.first(where: { ( |
There was a problem hiding this comment.
Can someone remind me if there's a more beautiful way to do this search? Maybe with a keypath?
There was a problem hiding this comment.
FWIW I did this because the cards weren't ordered the way I expected, but I also realized it doesn't matter what order two disabled cards are in.
There was a problem hiding this comment.
I don't know a nicer way 😭 Tbh, if it's deterministic, I think I'd just go by the index (which would mean that the test would need to be changed if we change the order in the future, so I think I do like this search better).
amy-at-kickstarter
requested review from
a team,
scottkicks and
ifosli
and removed request for
a team
|
|
||
| self.scheduler.run() | ||
|
|
||
| // There are now twos card, in the new section |
There was a problem hiding this comment.
minor nit: "two cards" instead of "twos card"
| XCTAssertEqual(self.reloadPaymentMethodsCards.lastValue?.count, 0) | ||
|
|
||
| // The discover is disabled and not selected. | ||
| let discoverCardData = self.reloadPaymentSheetPaymentMethodsCards.lastValue?.first(where: { ( |
There was a problem hiding this comment.
I don't know a nicer way 😭 Tbh, if it's deterministic, I think I'd just go by the index (which would mean that the test would need to be changed if we change the order in the future, so I think I do like this search better).
| let newCardData = self.reloadPaymentSheetPaymentMethodsCards.lastValue?.first | ||
| XCTAssertEqual(newCardData?.isEnabled, false) | ||
| XCTAssertEqual(newCardData?.isSelected, false) | ||
| XCTAssertNil(self.reloadPaymentMethodsSelectedCardId.lastValue ?? nil) |
There was a problem hiding this comment.
Nit: I haven't looked through all the tests again so this may be unnecessary, but should we also test that the pledge button doesn't get enabled in this test?
There was a problem hiding this comment.
Ahh that's a great idea but this test doesn't have the pledge button! It's just the isolated payment sheet. The test for pledges don't have access to the payment sheet itself. So I'm not sure we actually can test the complete behavior of the bug.
There was a problem hiding this comment.
The PledgeViewController watches the signal/delegate call for when a card is selected, so testing that behavior should hopefully be sufficient.
| let newCardData = self.reloadPaymentSheetPaymentMethodsCards.lastValue?.first | ||
| XCTAssertEqual(newCardData?.isEnabled, false) | ||
| XCTAssertEqual(newCardData?.isSelected, false) | ||
| XCTAssertNil(self.reloadPaymentMethodsSelectedCardId.lastValue ?? nil) |
📲 What
Don't change the selection on the Payments sheet if the newly added card is disabled.
🤔 Why
Now that we're saving cards immediately, it's possible to add a card that's not available for the current project. The previous logic would try and select the card, even if it was unavailable; this short-circuits that edge case. Fixing this prevents a bunch of unfortunate behavior as documented in the ticket.