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

[NT-354] Remove deprecated CreateApplePayBacking mutation #916

Merged
merged 4 commits into from
Oct 28, 2019

Conversation

justinswart
Copy link
Contributor

📲 What

Removes the deprecated CreateApplePayBacking mutation, updates our existing CreateBacking mutation to support Apple Pay.

🤔 Why

Changes on our back-end allowed for us to merge these together. This also helps us to keep this code DRYer.

🛠 How

  • Removed deprecated CreateApplePayBacking mutation.
  • Updated CreateBacking, CreateBackingInput, CreateBackingInput+Constructor, etc.
  • Updated tests.

✅ Acceptance criteria

  • There should be no regression in the experience of backing via Apple Pay.

shippingAmountDecimal = Decimal(shippingRule.cost)
shippingLocationId = String(shippingRule.location.id)
}

let pledgeTotal = NSDecimalNumber(decimal: pledgeAmountDecimal + shippingAmountDecimal)
let formattedPledgeTotal = Format.decimalCurrency(for: pledgeTotal.doubleValue)

let rewardId = reward == Reward.noReward ? nil : reward.graphID
let rewardId = reward.graphID
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to make Reward non-optional for this input so this kind've repeats the change in #915 🤷‍♂

.switchMap { input in
AppEnvironment.current.apiService.createBacking(input: input)
.ksr_delay(AppEnvironment.current.apiDelayInterval, on: AppEnvironment.current.scheduler)
.map { $0 as StripeSCARequiring }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this type mapping higher up means we don't have to do so many signal merges further down.

.materialize()
}

let createOrUpdateEvent = Signal.merge(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All values and errors are just merged here now.

@@ -999,8 +999,16 @@ final class PledgeViewModelTests: TestCase {
)
}

func testGoToThanks() {
withEnvironment(apiService: MockService()) {
func testApplePay_GoToThanks() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to kind've prove that there were no regressions I left these tests largely as-is. But we can chat about potentially adding more? I just didn't want this PR to be 1000+ lines from the start 😅

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

Works great!

Tested:
✅creating a backing with a stored card
✅creating a backing with Apple Pay
✅updating an Apple Pay backing to a stored card
✅updating a credit card backing to an Apple Pay backing

Just had a couple small comments and questions.

- parameter applePay: The optional ApplePayParams.
- parameter locationId: The optional ID of the ShippingRule's Location.
- parameter paymentSourceId: The optional ID of the PaymentSource.
- parameter projectId: The ID of the Project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we differentiate in this documentation for future folks the difference between graphId and ID? projectId and rewardId both need to be graphIDs (or relayId, whatever terminology we want) whereas locationId is the regular id from the location.

"amount": amount,
"paymentSourceId": paymentSourceId,
// swiftlint:disable:next line_length
"paymentType": PaymentType.creditCard.rawValue, // this is temporary and will be removed once the mutation has been updated
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed since we made this object Encodable we removed this parameter. I believe it's still required 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per Slack convo - it's no longer required.

@@ -27,21 +60,26 @@ final class CreateBackingInputTests: XCTestCase {
func testCreateBackingInputDictionary_TestNilLocationAndReward() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the name of this test to reflect the fact that we now always send rewardId even if it's Reward.noReward? Maybe testCreateBackingInputDictionary_Location_IsNil()?

XCTAssertEqual(input.amount, "10.00")
XCTAssertNil(input.applePay)
XCTAssertNil(input.locationId)
XCTAssertEqual(input.projectId, project.graphID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also update this assertion to show the exact string that's expected, similar to rewardId below on line 35?

# Conflicts:
#	Library/CreateApplePayBackingInputConstructorTests.swift
#	Library/CreateBackingConstructorTests.swift
@ksr-ci-bot
Copy link

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

G2G!

@justinswart justinswart merged commit 85949dc into master Oct 28, 2019
@justinswart justinswart deleted the remove-deprecated-create-apple-pay branch October 28, 2019 21:38
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.

None yet

3 participants