Support custom URL paths in `ManageSubscriptionsView` #4382

tonidero · 2024-10-18T09:17:21Z

Description

This allows to specify paths with custom URLs in the manage subscriptions view which can be opened either externally or in_app as a bottom sheet.

IN_APP:

Simulator.Screen.Recording.-.iPhone.16.-.2024-10-18.at.12.51.42.mp4

EXTERNAL:

Simulator.Screen.Recording.-.iPhone.16.-.2024-10-18.at.12.52.36.mp4

tonidero · 2024-10-18T11:24:34Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

@@ -186,6 +200,18 @@ private extension ManageSubscriptionsViewModel {
            } catch {
                self.state = .error(error)
            }
+        case .customUrl:


Note this is only currently available in the ManageSubscriptionView. It won't be available in the NoSubscriptionsView or WrongPlatformView since we're not using the paths for those views.

interesting... It might be worth figuring out a way for those to work eventually since for those it might make even more sense - you can direct users to where you'd actually want them to manage

Yeah, I think we need to move some of the logic to a shared view for all 3 of them and handle paths in all 3. Right now the buttons are mostly hardcoded in those 2 other views. Probably not something for this PR though.

I agree we need to do that refactor yes

tonidero · 2024-10-18T11:25:15Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

 #endif

 }

+struct IdentifiableURL: Identifiable {


Not sure if this is the best way but I couldn't make URL Identifiable directly without getting a warning.

Are you getting the warning that it will not behave correctly if URL ever becomes Identifiable in the future? If so, yea I think this solution is the safest!

I think it makes sense

nitpick but there should be an empty line at the beginning and at the end of the struct

tonidero · 2024-10-18T11:26:03Z

RevenueCatUI/Views/SafariView.swift

+import SafariServices
+import SwiftUI
+
+struct SafariView: UIViewControllerRepresentable {


Lmk what you think of this. Seems to work fine and seemed like an easy solution :)

This is good yea! One thing though is that there's an existing (and pretty much identical) SafariView in ButtonComponentView.swift. This gives an "invalid redeclaration" error if PAYWALL_COMPONENTS is enabled.

The only difference between the 2 SafariViews is that the existing one uses

#if canImport(SafariServices)

and the new one uses

#if os(iOS)

Could we consolidate the 2?

Ohh I had missed that one! Thanks for the heads up! I can try to consolidate yeah

tonidero · 2024-10-18T11:27:03Z

Sources/CustomerCenter/CustomerCenterConfigData.swift

@@ -425,10 +452,23 @@ extension CustomerCenterConfigData.Localization {

 extension CustomerCenterConfigData.HelpPath {

-    init(from response: CustomerCenterConfigResponse.HelpPath) {
+    init?(from response: CustomerCenterConfigResponse.HelpPath) {


Note that I changed the constructor to be nullable. If we get a custom url path without a url or open method, we can filter it out and this was the easiest way to achieve that

Would it be possible to make it throw instead? Imo absence of data (null) is not the same as incorrect input data. The latter is an error, the former doesn't have to be.

(I also find it odd that Foundation's URL constructor is nullable, for the same reason.)

Hmm so for customer center I would specially take a very "lax" approach. As in, if there is something misconfigured in the backend, I would prefer to try to display as much as possible instead of failing entirely. For example in this case, it basically means that if somehow a misconfiguration happened, we would just hide the path. I think we should do this since we're still changing the schema every now and then and we should be flexible.

Having said that, we could indeed throw in the constructor and handle it when creating the paths. The code becomes a bit less legible IMO, since we need to handle the catch though... I'm not sure TBH, since I know optional constructors are actually quite common in Swift (even if I don't like them as much 😅)... Wdyt @vegaro @aboedo ?

yeah, I think it also kinda maps better to what's happening underneath - you're using an optional constructor to try to build the URL in any case

You're right, this is essentially validating JSON, which I agree should be lax. We could avoid handling the catch and just doing try?:

self.paths = response.paths.compactMap { try? CustomerCenterConfigData.HelpPath(from: $0) }

I don't feel super strongly about this btw, especially since optional constructors are common in Swift, as you said.

Hmm yeah that's not bad. I also don't feel too strongly about this TBH... But considering we don't do that many throws along the codebase, I'm thinking on leaving it as is... If you feel we should better throw an error, I'm ok changing to your suggestion.

No it's minor imo. Feel free to leave as is!

I would also leave this as nullable as well because of the optional constructor it is using underneath

JayShortway

This is gonna be good! The only thing is the duplicate SafariView, and had some other minor comments.

JayShortway · 2024-10-18T13:41:01Z

RevenueCatUI/Views/SafariView.swift

+import SafariServices
+import SwiftUI
+
+struct SafariView: UIViewControllerRepresentable {


This is good yea! One thing though is that there's an existing (and pretty much identical) SafariView in ButtonComponentView.swift. This gives an "invalid redeclaration" error if PAYWALL_COMPONENTS is enabled.

The only difference between the 2 SafariViews is that the existing one uses

#if canImport(SafariServices)

and the new one uses

#if os(iOS)

Could we consolidate the 2?

JayShortway · 2024-10-18T13:47:03Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

 #endif

 }

+struct IdentifiableURL: Identifiable {


Are you getting the warning that it will not behave correctly if URL ever becomes Identifiable in the future? If so, yea I think this solution is the safest!

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

JayShortway · 2024-10-18T13:53:17Z

Sources/CustomerCenter/CustomerCenterConfigData.swift

+        public enum OpenMethod: String {
+
+            case inApp = "IN_APP"
+            case external = "EXTERNAL"
+
+            init?(from rawValue: String?) {
+                switch rawValue {
+                case "IN_APP":
+                    self = .inApp
+                case "EXTERNAL":
+                    self = .external
+                default:
+                    return nil
+                }
+            }
+
+        }


I wonder if we should consolidate this with the existing PaywallComponent.ButtonComponent.URLMethod. On the one hand I'd say yes, because it is exactly the same concept we're describing so having 2 enums is confusing. On the other hand you could say that we're tying things together which might reduce flexibility.

Hmm right kinda same thinking as you here... It's internal APIs so I guess we can go either way... I would suggest keeping it separate for flexibility for now, and we can always refactor later if needed. For example, we are currently only adding inApp and external for customer center but we have a deeplink option for paywalls... Having said that, I think we could try to refactor everything into a "link handler" to be used by both paywalls and customer center since it's close to the same thing

JayShortway · 2024-10-18T13:59:05Z

Sources/CustomerCenter/CustomerCenterConfigData.swift

@@ -425,10 +452,23 @@ extension CustomerCenterConfigData.Localization {

 extension CustomerCenterConfigData.HelpPath {

-    init(from response: CustomerCenterConfigResponse.HelpPath) {
+    init?(from response: CustomerCenterConfigResponse.HelpPath) {


Would it be possible to make it throw instead? Imo absence of data (null) is not the same as incorrect input data. The latter is an error, the former doesn't have to be.

(I also find it odd that Foundation's URL constructor is nullable, for the same reason.)

aboedo

awesome!

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

aboedo · 2024-10-18T14:24:09Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

@@ -186,6 +200,18 @@ private extension ManageSubscriptionsViewModel {
            } catch {
                self.state = .error(error)
            }
+        case .customUrl:


interesting... It might be worth figuring out a way for those to work eventually since for those it might make even more sense - you can direct users to where you'd actually want them to manage

aboedo · 2024-10-18T14:29:15Z

Sources/CustomerCenter/CustomerCenterConfigData.swift

@@ -425,10 +452,23 @@ extension CustomerCenterConfigData.Localization {

 extension CustomerCenterConfigData.HelpPath {

-    init(from response: CustomerCenterConfigResponse.HelpPath) {
+    init?(from response: CustomerCenterConfigResponse.HelpPath) {


yeah, I think it also kinda maps better to what's happening underneath - you're using an optional constructor to try to build the URL in any case

JayShortway

Nice one!

RevenueCatUI/Templates/Components/Button/ButtonComponentView.swift

tonidero · 2024-10-18T14:52:00Z

@RCGitBot please test

vegaro · 2024-10-21T09:27:12Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

 #endif

 }

+struct IdentifiableURL: Identifiable {


I think it makes sense

nitpick but there should be an empty line at the beginning and at the end of the struct

vegaro · 2024-10-21T09:28:06Z

RevenueCatUI/CustomerCenter/ViewModels/ManageSubscriptionsViewModel.swift

@@ -186,6 +200,18 @@ private extension ManageSubscriptionsViewModel {
            } catch {
                self.state = .error(error)
            }
+        case .customUrl:


I agree we need to do that refactor yes

vegaro · 2024-10-21T09:31:23Z

Sources/CustomerCenter/CustomerCenterConfigData.swift

@@ -425,10 +452,23 @@ extension CustomerCenterConfigData.Localization {

 extension CustomerCenterConfigData.HelpPath {

-    init(from response: CustomerCenterConfigResponse.HelpPath) {
+    init?(from response: CustomerCenterConfigResponse.HelpPath) {


I would also leave this as nullable as well because of the optional constructor it is using underneath

tonidero · 2024-10-21T12:54:06Z

Sources/Networking/Responses/CustomerCenterConfigResponse.swift

@@ -167,4 +179,10 @@ extension CustomerCenterConfigResponse.HelpPath.PathType: CodableEnumWithUnknown

 }

+extension CustomerCenterConfigResponse.HelpPath.OpenMethod: CodableEnumWithUnknownCase {


@vegaro Note that I added this for the new enum

tonidero added pr:RevenueCatUI feat:Customer Center pr:feat A new feature labels Oct 18, 2024

tonidero commented Oct 18, 2024

View reviewed changes

tonidero marked this pull request as ready for review October 18, 2024 11:43

tonidero requested a review from a team October 18, 2024 11:43

JayShortway reviewed Oct 18, 2024

View reviewed changes

aboedo approved these changes Oct 18, 2024

View reviewed changes

JayShortway approved these changes Oct 18, 2024

View reviewed changes

RevenueCatUI/Templates/Components/Button/ButtonComponentView.swift Show resolved Hide resolved

vegaro approved these changes Oct 21, 2024

View reviewed changes

tonidero added 9 commits October 21, 2024 14:53

Support custom URL paths in ManageSubscriptionsView

2be7197

Fix tests

40229aa

More test fixes

4ef91aa

Add parsing and processing tests

a0b19e7

Consolidate SafariView

2730487

Rename method

3cf3e73

Another rename

ebcdd90

Fix import issue with SafariView not being compatible to some platforms

eb23ef7

Style fix

487472c

tonidero force-pushed the sdk-3601-customercentersdk-support-custom-url-paths branch from 9f127de to 487472c Compare October 21, 2024 12:53

tonidero commented Oct 21, 2024

View reviewed changes

tonidero enabled auto-merge (squash) October 21, 2024 13:12

tonidero merged commit 95aaf74 into main Oct 21, 2024
7 checks passed

tonidero deleted the sdk-3601-customercentersdk-support-custom-url-paths branch October 21, 2024 13:47

This was referenced Oct 21, 2024

[AUTOMATIC] Release/5.7.0 #4391

Closed

[AUTOMATIC] Release/5.7.0 #4395

Closed

[AUTOMATIC] Release/5.7.0 #4402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom URL paths in `ManageSubscriptionsView` #4382

Support custom URL paths in `ManageSubscriptionsView` #4382

tonidero commented Oct 18, 2024 •

edited

Loading

tonidero Oct 18, 2024

aboedo Oct 18, 2024

tonidero Oct 18, 2024

vegaro Oct 21, 2024

tonidero Oct 18, 2024

JayShortway Oct 18, 2024

vegaro Oct 21, 2024

tonidero Oct 18, 2024

JayShortway Oct 18, 2024

tonidero Oct 18, 2024

tonidero Oct 18, 2024

JayShortway Oct 18, 2024

tonidero Oct 18, 2024

aboedo Oct 18, 2024

JayShortway Oct 18, 2024 •

edited

Loading

tonidero Oct 18, 2024

JayShortway Oct 18, 2024

vegaro Oct 21, 2024

JayShortway left a comment

JayShortway Oct 18, 2024

JayShortway Oct 18, 2024

JayShortway Oct 18, 2024

tonidero Oct 18, 2024

JayShortway Oct 18, 2024

aboedo left a comment •

edited

Loading

aboedo Oct 18, 2024

aboedo Oct 18, 2024

JayShortway left a comment

tonidero commented Oct 18, 2024

vegaro Oct 21, 2024

vegaro Oct 21, 2024

vegaro Oct 21, 2024

tonidero Oct 21, 2024

		@@ -167,4 +179,10 @@ extension CustomerCenterConfigResponse.HelpPath.PathType: CodableEnumWithUnknown

		}

		extension CustomerCenterConfigResponse.HelpPath.OpenMethod: CodableEnumWithUnknownCase {

Support custom URL paths in ManageSubscriptionsView #4382

Support custom URL paths in ManageSubscriptionsView #4382

Conversation

tonidero commented Oct 18, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayShortway Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayShortway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayShortway left a comment

Choose a reason for hiding this comment

tonidero commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support custom URL paths in `ManageSubscriptionsView` #4382

Support custom URL paths in `ManageSubscriptionsView` #4382

tonidero commented Oct 18, 2024 •

edited

Loading

JayShortway Oct 18, 2024 •

edited

Loading

aboedo left a comment •

edited

Loading