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

MBL-1838: Wire up PLOT query to UI in no-shipping checkout flow #2225

Merged
merged 10 commits into from
Jan 2, 2025

Conversation

amy-at-kickstarter
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter commented Dec 19, 2024

📲 What

Hook up the BuildPaymentPlanQuery to the new PLOT UI.

🤔 Why

This means PLOT will start showing live data, instead of mock data.

🛠 How

  • I created some helpers (plus tests) to map between the GraphAPI response object and @jovaniks view model objects. While I didn't write an intermediary envelope object, those helpers serve a similar role. As an aside, I've been noticing that Apollo/GraphQL objects are painful to test, which is a downside to avoiding envelope objects.
  • I had to update our ISO8061 date formatter to work in both directions, since we need to parse dates.
  • I removed some fields we weren't using from the PledgePaymentIncrement fragment.

👀 See

PLOT.demo.small.mp4

📓 Todos

  • Fix failing screenshot tests
  • Discuss loading states. Do we want to block pledge until the PLOT module loads? What should it look like while it's loading?

@@ -625,3 +626,20 @@ private func distinctRewards(_ rewards: [Reward]) -> [Reward] {
return !rewardIds.contains(reward.id)
}
}

// TODO: Remove this when implementing the API [MBL-1851](https://kickstarter.atlassian.net/browse/MBL-1851)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this and changed the ticket number. It's no longer needed in NoShippingPledgeViewModel, but still needed here in ManagePledgeViewModel.

).materialize()
}

self.showPledgeOverTimeUI = 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.

@jovaniks My assumption is that, since this feature is so small, there's no designs for a separate error state. Is that correct? My approach was just to hide the entire element if an error occurs in the BuildPaymentPlan query.

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s correct, we don’t have error handling planned for this component at the moment. Do you think it would make sense to add a TODO to revisit this discussion once everyone is back in January

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll set myself a reminder in Slack for January! And I'll raise it in the next PLOT meeting.

@@ -5285,4 +5287,97 @@ final class NoShippingPledgeViewModelTests: TestCase {
self.showPledgeOverTimeUI.assertValues([false])
}
}

func testPledgeOverTimeConfigData_LoadsAfterBuildPledgeQuery() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are pretty bare-bones. I'd love to test that the mock query is actually making a GraphQL request for the right amount, but I don't think we have that capability with our current mock infrastructure.

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Awesome!

re: the loading state, the decision was made to block pledging until PLOT UI loads, or fails due to some error. If there's an error we'll just default to a normal (non-plot) pledge.

KsApi/queries/BuildPaymentPlanQuery.graphql Show resolved Hide resolved
Library/ViewModels/NoShippingPledgeViewModel.swift Outdated Show resolved Hide resolved
@@ -393,3 +405,51 @@ final class NoShippingPledgeViewControllerTests: TestCase {
}
}
}

private let buildPaymentPlanQueryJson_Eligible = """
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of note: if we upgrade Apollo, we'll get access to their mocks API. Then it will be really, really easy to mock GraphQL response objects in our tests without all of this verbose json nonsense.

Copy link
Contributor

@jovaniks jovaniks left a comment

Choose a reason for hiding this comment

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

Nice work!

@amy-at-kickstarter amy-at-kickstarter merged commit 2be9705 into main Jan 2, 2025
5 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/mbl-1838 branch January 2, 2025 21:56
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.

3 participants