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.

Sign up for GitHub

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

Support not showing Contact Customer Support if the user isn't on the most recent app version #4567

Closed
wants to merge 10 commits into from
Closed

Support not showing Contact Customer Support if the user isn't on the most recent app version #4567

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
494cb2f
add shouldWarnCustomerToUpdate to Support
fire-at-will Dec 10, 2024
345e4dd
only show contact support if on most recent app version when shouldWa…
fire-at-will Dec 10, 2024
25e93c7
control showing contact support on WrongPlatformView
fire-at-will Dec 10, 2024
d839fac
temporarily force showing WrongPlatformView
fire-at-will Dec 10, 2024
d43953d
revert temporary WrongPlatformView hack
fire-at-will Dec 10, 2024
0baa665
write tests
fire-at-will Dec 10, 2024
441f3f1
fix tests
fire-at-will Dec 10, 2024
c66d336
use flag to control AppUpdateWarningView
fire-at-will Dec 10, 2024
2972a4d
test fix
fire-at-will Dec 10, 2024
dac49f9
PR feedback
fire-at-will Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions RevenueCatUI/CustomerCenter/Data/CustomerCenterConfigTestData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ enum CustomerCenterConfigTestData {

@available(iOS 14.0, *)
// swiftlint:disable:next function_body_length
static func customerCenterData(lastPublishedAppVersion: String?) -> CustomerCenterConfigData {
static func customerCenterData(
lastPublishedAppVersion: String?,
shouldWarnCustomerToUpdate: Bool = false
) -> CustomerCenterConfigData {
CustomerCenterConfigData(
screens: [.management:
.init(
Expand Down Expand Up @@ -111,7 +114,10 @@ enum CustomerCenterConfigTestData {
"back": "Back"
]
),
support: .init(email: "[email protected]"),
support: .init(
email: "[email protected]",
shouldWarnCustomerToUpdate: true
),
lastPublishedAppVersion: lastPublishedAppVersion,
productId: 1
)
Expand Down
5 changes: 5 additions & 0 deletions RevenueCatUI/CustomerCenter/ViewModels/CustomerCenterViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ import RevenueCat
private(set) var appIsLatestVersion: Bool = defaultAppIsLatestVersion
private(set) var purchasesProvider: CustomerCenterPurchasesType

/// Whether or not the user needs to update their app version to contact support.
var appUpdateRequiredToContactSupport: Bool {
Copy link
Member

Choose a reason for hiding this comment

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

we don't entirely require but rather suggest, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this implementation, I built it the way the copy for the checkbox on the dashboard is worded and made it required to contact support in the WrongPlatformView & the Restore Purchases flow.

The checkbox's copy reads:

Require users to update to the latest version of your app before contacting support

That said, I don't think it's great to prevent people from contacting support and would suggest that we instead show a message or button in the alert urging the user to update in addition to letting them contact support.

return !appIsLatestVersion && (configuration?.support.shouldWarnCustomerToUpdate ?? false)
}

// @PublicForExternalTesting
@Published
var state: CustomerCenterViewState {
Expand Down
2 changes: 1 addition & 1 deletion RevenueCatUI/CustomerCenter/Views/RestorePurchasesAlert.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ struct RestorePurchasesAlert: ViewModifier {

case .purchasesNotFound:
let message = Text(localization.commonLocalizedString(for: .purchasesNotRecovered))
if let url = supportURL {
if let url = supportURL, !customerCenterViewModel.appUpdateRequiredToContactSupport {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm do we need to show the AppUpdateWarningView in the else case? As in, right now we won't be able to contact support if the app update is required right? Which I think we didn't want to do? Same for the WrongPlatformView.

return Alert(title: Text(""),
message: message,
primaryButton: .default(
Expand Down
5 changes: 4 additions & 1 deletion RevenueCatUI/CustomerCenter/Views/WrongPlatformView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ struct WrongPlatformView: View {
@State
private var subscriptionInformation: PurchaseInformation?

@EnvironmentObject
private var customerCenterViewModel: CustomerCenterViewModel

@Environment(\.localization)
private var localization: CustomerCenterConfigData.Localization
@Environment(\.appearance)
Expand Down Expand Up @@ -80,7 +83,7 @@ struct WrongPlatformView: View {
}
}
}
if let url = supportURL {
if let url = supportURL, !customerCenterViewModel.appUpdateRequiredToContactSupport {
Section {
AsyncButton {
openURL(url)
Expand Down
8 changes: 7 additions & 1 deletion Sources/CustomerCenter/CustomerCenterConfigData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,9 +412,14 @@ public struct CustomerCenterConfigData {
public struct Support {

public let email: String
public let shouldWarnCustomerToUpdate: Bool

public init(email: String) {
public init(
email: String,
shouldWarnCustomerToUpdate: Bool
) {
self.email = email
self.shouldWarnCustomerToUpdate = shouldWarnCustomerToUpdate
}

}
Expand Down Expand Up @@ -550,6 +555,7 @@ extension CustomerCenterConfigData.Support {

init(from response: CustomerCenterConfigResponse.Support) {
self.email = response.email
self.shouldWarnCustomerToUpdate = response.shouldWarnCustomerToUpdate ?? false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaults to false if it isn't included in the response from the backend since the checkbox's default value in the dashboard is false.

}

}
Expand Down
1 change: 1 addition & 0 deletions Sources/Networking/Responses/CustomerCenterConfigResponse.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ struct CustomerCenterConfigResponse {
struct Support {

let email: String
let shouldWarnCustomerToUpdate: Bool?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm thinking... the original idea was that, if the backend sent the lastPublishedAppVersion, the SDK would consider it "enabled" implicitly. The backend shouldn't send that version if it's disabled. So this property would be unnecessary I think?


}

Expand Down
5 changes: 4 additions & 1 deletion Tests/RevenueCatUITests/CustomerCenter/ContactSupportUtilitiesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ import XCTest

class ContactSupportUtilitiesTest: TestCase {

private let support: CustomerCenterConfigData.Support = .init(email: "[email protected]")
private let support: CustomerCenterConfigData.Support = .init(
email: "[email protected]",
shouldWarnCustomerToUpdate: false
)
private let localization: CustomerCenterConfigData.Localization = .init(locale: "en_US", localizedStrings: [:])

func testSupportEmailBodyWithDefaultDataIsCorrect() {
Expand Down
50 changes: 50 additions & 0 deletions Tests/RevenueCatUITests/CustomerCenter/CustomerCenterViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import XCTest

#if os(iOS)

// swiftlint:disable file_length
@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
@available(macOS, unavailable)
@available(tvOS, unavailable)
Expand Down Expand Up @@ -218,6 +219,55 @@ class CustomerCenterViewModelTests: TestCase {
}
}

func testAppUpdateRequiredToContactSupport_true() {
let mockPurchases = MockCustomerCenterPurchases()
let latestVersion = "3.0.0"
let currentVersion = "2.0.0"
let viewModel = CustomerCenterViewModel(
customerCenterActionHandler: nil,
currentVersionFetcher: { return currentVersion },
purchasesProvider: mockPurchases
)
viewModel.configuration = CustomerCenterConfigTestData.customerCenterData(
lastPublishedAppVersion: latestVersion,
shouldWarnCustomerToUpdate: true
)

expect(viewModel.appUpdateRequiredToContactSupport).to(beTrue())
}

func testAppUpdateRequiredToContactSupport_false() {
let mockPurchases = MockCustomerCenterPurchases()
let latestVersion = "3.0.0"
let viewModel = CustomerCenterViewModel(
customerCenterActionHandler: nil,
currentVersionFetcher: { return latestVersion },
purchasesProvider: mockPurchases
)
viewModel.configuration = CustomerCenterConfigTestData.customerCenterData(
lastPublishedAppVersion: latestVersion,
shouldWarnCustomerToUpdate: true
)

expect(viewModel.appUpdateRequiredToContactSupport).to(beFalse())
}

func testAppUpdateRequiredToContactSupport_false_blocked_by_config() {
Copy link
Member

Choose a reason for hiding this comment

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

the mix of casing here looks a little odd to my eye

Copy link
Member

Choose a reason for hiding this comment

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

I would maybe go with something more like
testAppUpdateRequiredToContactSupportReturnsFalseIfBlockedByConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sometimes I mix them when the test names get super long in my apps - I'll change them to use the normal style here :)

let mockPurchases = MockCustomerCenterPurchases()
let latestVersion = "3.0.0"
let viewModel = CustomerCenterViewModel(
customerCenterActionHandler: nil,
currentVersionFetcher: { return latestVersion },
purchasesProvider: mockPurchases
)
viewModel.configuration = CustomerCenterConfigTestData.customerCenterData(
lastPublishedAppVersion: latestVersion,
shouldWarnCustomerToUpdate: true
)

expect(viewModel.appUpdateRequiredToContactSupport).to(beFalse())
}

}

@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
Expand Down
5 changes: 4 additions & 1 deletion Tests/UnitTests/CustomerCenter/CustomerCenterConfigDataTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ class CustomerCenterConfigDataTests: TestCase {
)
],
localization: .init(locale: "en_US", localizedStrings: ["key": "value"]),
support: .init(email: "[email protected]")
support: .init(
email: "[email protected]",
shouldWarnCustomerToUpdate: false
)
),
lastPublishedAppVersion: "1.2.3",
itunesTrackId: 123
Expand Down