Skip to content

[Customer Center] Select the right subscription to show#4046

Merged
JayShortway merged 17 commits into
integration/customer_support_workflowfrom
sdk-3470-select-subscription
Jul 16, 2024
Merged

[Customer Center] Select the right subscription to show#4046
JayShortway merged 17 commits into
integration/customer_support_workflowfrom
sdk-3470-select-subscription

Conversation

@JayShortway

Copy link
Copy Markdown
Member

In this PR

  • Refactored the test-fixture-building code in ManageSubscriptionsViewModelTests, to more easily fake scenarios of having multiple subscriptions.
    • Moved this code from the ManageSubscriptionsViewModelTests extension to a separate Fixtures class to avoid errors and warnings for calling static methods from outside its actor context.
  • Translated the subscription selection requirements to tests in ManageSubscriptionsViewModelTests.
  • Updated the logic in ManageSubscriptionsViewModel to pick the right subscription based on the requirements specified in the tests.

Not in this PR

  • Handling of lifetime subscriptions (let's discuss in the sync).
  • Any UI changes (will do a follow-up PR).

Request

Please let me know any conventions I broke, and any tips you have for more idiomatic Swift! 🙏

vegaro and others added 10 commits July 9, 2024 21:48
Base branch is `integration/customer_support_workflow` so we don't merge
into `main` yet

Borrows a lot from #3865

Creates a new `CustomerCenterView` that can be used as a customer
support workflow starting point.

All details can be found in
https://linear.app/revenuecat/project/sdk-support-workflow-cf7f6a1d5340/overview

---------

Co-authored-by: Will Taylor <wtaylor151@gmail.com>
Co-authored-by: James Borthwick <109382862+jamesrb1@users.noreply.github.com>
Adds `Purchases.shared.loadCustomerCenter()` that calls a new backend
endpoint that returns the customer center configuration

This API call doesn't exist yet and it will change. This PR is the
ground work so that we don't have to wait for the backend to add this
API and we can already pretend the API is there.

---------

Co-authored-by: RevenueCat Git Bot <72824662+RCGitBot@users.noreply.github.com>
Based off #3933 



https://github.com/RevenueCat/purchases-ios/assets/664544/11eae984-294a-4e14-8c40-7c2d50994c09

Can probably use some animations, but tuning that up will come up later

It will open a feedback survey for an option if there's one
@JayShortway JayShortway added the pr:feat A new feature label Jul 12, 2024
@JayShortway JayShortway requested a review from a team July 12, 2024 10:22
@JayShortway JayShortway self-assigned this Jul 12, 2024
Comment thread Tests/RevenueCatUITests/CustomerCenter/ManageSubscriptionsViewModelTests.swift Outdated
Comment thread Tests/RevenueCatUITests/CustomerCenter/ManageSubscriptionsViewModelTests.swift Outdated
Comment thread Tests/RevenueCatUITests/CustomerCenter/ManageSubscriptionsViewModelTests.swift Outdated
Comment on lines +296 to +299
let viewModel = ManageSubscriptionsViewModel(screen: ManageSubscriptionsViewModelTests.screen,
purchasesProvider: MockManageSubscriptionsPurchases(
customerInfo: customerInfo,
products: products

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

haven't looked in detail that the class, but this is the first thing that really jumps out at me as not very swiftUI-ish, the viewModel and the fact that it holds a reference to the screen.

Why do we hold that reference? Typically you'd have the viewModel own the properties, and have those be published, then a View (or many) observes them

@JayShortway JayShortway Jul 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Totally agree. In MVVM the ViewModel should be completely decoupled from the UI layer and just publish state, not caring about who listens.

I did not change this ViewModel, but I see that in this case, the Screen class is not a SwiftUI view, but a struct holding some input data. Also, it is unused. @vegaro Are there any future plans for this, or can it be removed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh this is confusing, but screen is the data of the screen that is being sent down in the JSON, so the screen properties like screen title, feedback survey options, etc, it doesn't have anything to do with view classes. It's been used in upward branches so it can't be removed, for example https://github.com/RevenueCat/purchases-ios/pull/3968/files#diff-121dcc7927012032acdbbf026716bf9daf66d91cee530ab12eae052d06526aa0R187

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe there's a better name for the struct, to avoid the confusion. Something like Destination maybe?

Also I don't see it being used still, but maybe I'm looking in the wrong place haha. It looks like it's only being set, never read, from what I can tell.

@vegaro vegaro force-pushed the integration/customer_support_workflow branch from 4028258 to f9f4736 Compare July 15, 2024 17:18
@JayShortway JayShortway merged commit ecf830d into integration/customer_support_workflow Jul 16, 2024
@JayShortway JayShortway deleted the sdk-3470-select-subscription branch July 16, 2024 13:16
vegaro pushed a commit that referenced this pull request Jul 17, 2024
vegaro pushed a commit that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:feat A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants