Support custom URL paths in ManageSubscriptionsView
#4382
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // | ||
| // Copyright RevenueCat Inc. All Rights Reserved. | ||
| // | ||
| // Licensed under the MIT License (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // https://opensource.org/licenses/MIT | ||
| // | ||
| // SafariView.swift | ||
| // | ||
| // Created by Antonio Rico Diez on 2024-10-18. | ||
|
|
||
| #if canImport(SafariServices) && canImport(UIKit) | ||
|
|
||
| import SafariServices | ||
| import SwiftUI | ||
|
|
||
| struct SafariView: UIViewControllerRepresentable { | ||
|
There was a problem hiding this comment. Lmk what you think of this. Seems to work fine and seemed like an easy solution :) There was a problem hiding this comment. This is good yea! One thing though is that there's an existing (and pretty much identical) The only difference between the 2 #if canImport(SafariServices)and the new one uses #if os(iOS)Could we consolidate the 2? There was a problem hiding this comment. Ohh I had missed that one! Thanks for the heads up! I can try to consolidate yeah |
||
| let url: URL | ||
|
|
||
| func makeUIViewController(context: UIViewControllerRepresentableContext<SafariView>) -> SFSafariViewController { | ||
| return SFSafariViewController(url: url) | ||
| } | ||
|
|
||
| func updateUIViewController(_ uiViewController: SFSafariViewController, | ||
| context: UIViewControllerRepresentableContext<SafariView>) { | ||
| // No need to update the controller, as it's static in this case | ||
| } | ||
| } | ||
|
|
||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,15 +177,21 @@ public struct CustomerCenterConfigData { | |
|
|
||
| public let id: String | ||
| public let title: String | ||
| public let url: URL? | ||
| public let openMethod: OpenMethod? | ||
| public let type: PathType | ||
| public let detail: PathDetail? | ||
|
|
||
| public init(id: String, | ||
| title: String, | ||
| url: URL?, | ||
| openMethod: OpenMethod?, | ||
| type: PathType, | ||
| detail: PathDetail?) { | ||
| self.id = id | ||
| self.title = title | ||
| self.url = url | ||
| self.openMethod = openMethod | ||
| self.type = type | ||
| self.detail = detail | ||
| } | ||
|
|
@@ -203,6 +209,7 @@ public struct CustomerCenterConfigData { | |
| case refundRequest = "REFUND_REQUEST" | ||
| case changePlans = "CHANGE_PLANS" | ||
| case cancel = "CANCEL" | ||
| case customUrl = "CUSTOM_URL" | ||
| case unknown | ||
|
|
||
| init(from rawValue: String) { | ||
|
|
@@ -215,13 +222,33 @@ public struct CustomerCenterConfigData { | |
| self = .changePlans | ||
| case "CANCEL": | ||
| self = .cancel | ||
| case "CUSTOM_URL": | ||
| self = .customUrl | ||
| default: | ||
| self = .unknown | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| 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 | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
Comment on lines
+234
to
+250
There was a problem hiding this comment. I wonder if we should consolidate this with the existing There was a problem hiding this comment. 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 |
||
|
|
||
| public struct PromotionalOffer { | ||
|
|
||
| public let iosOfferId: String | ||
|
|
@@ -389,7 +416,7 @@ extension CustomerCenterConfigData.Screen { | |
| self.type = ScreenType(from: response.type.rawValue) | ||
| self.title = response.title | ||
| self.subtitle = response.subtitle | ||
| self.paths = response.paths.compactMap { CustomerCenterConfigData.HelpPath(from: $0) } | ||
| } | ||
|
|
||
| } | ||
|
|
@@ -425,10 +452,23 @@ extension CustomerCenterConfigData.Localization { | |
|
|
||
| extension CustomerCenterConfigData.HelpPath { | ||
|
|
||
| init?(from response: CustomerCenterConfigResponse.HelpPath) { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 ? There was a problem hiding this comment. 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 There was a problem hiding this comment. You're right, this is essentially validating JSON, which I agree should be lax. We could avoid handling the catch and just doing 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. No it's minor imo. Feel free to leave as is! There was a problem hiding this comment. I would also leave this as nullable as well because of the optional constructor it is using underneath |
||
| self.id = response.id | ||
| self.title = response.title | ||
| self.type = CustomerCenterConfigData.HelpPath.PathType(from: response.type.rawValue) | ||
| if self.type == .customUrl { | ||
| if let responseUrl = response.url, | ||
| let url = URL(string: responseUrl), | ||
| let openMethod = CustomerCenterConfigData.HelpPath.OpenMethod(from: response.openMethod?.rawValue) { | ||
| self.url = url | ||
| self.openMethod = openMethod | ||
| } else { | ||
| return nil | ||
| } | ||
| } else { | ||
| self.url = nil | ||
| self.openMethod = nil | ||
| } | ||
| if let promotionalOfferResponse = response.promotionalOffer { | ||
| self.detail = .promotionalOffer(PromotionalOffer(from: promotionalOfferResponse)) | ||
| } else if let feedbackSurveyResponse = response.feedbackSurvey { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,8 @@ struct CustomerCenterConfigResponse { | |
| let id: String | ||
| let title: String | ||
| let type: PathType | ||
| let url: String? | ||
| let openMethod: OpenMethod? | ||
| let promotionalOffer: PromotionalOffer? | ||
| let feedbackSurvey: FeedbackSurvey? | ||
|
|
||
|
|
@@ -53,6 +55,15 @@ struct CustomerCenterConfigResponse { | |
| case refundRequest = "REFUND_REQUEST" | ||
| case changePlans = "CHANGE_PLANS" | ||
| case cancel = "CANCEL" | ||
| case customUrl = "CUSTOM_URL" | ||
| case unknown | ||
|
|
||
| } | ||
|
|
||
| enum OpenMethod: String { | ||
|
|
||
| case inApp = "IN_APP" | ||
| case external = "EXTERNAL" | ||
| case unknown | ||
|
|
||
| } | ||
|
|
@@ -130,6 +141,7 @@ extension CustomerCenterConfigResponse.CustomerCenter: Codable, Equatable {} | |
| extension CustomerCenterConfigResponse.Localization: Codable, Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath: Codable, Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath.PathType: Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath.OpenMethod: Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath.PromotionalOffer: Codable, Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath.FeedbackSurvey: Codable, Equatable {} | ||
| extension CustomerCenterConfigResponse.HelpPath.FeedbackSurvey.Option: Codable, Equatable {} | ||
|
|
@@ -167,4 +179,10 @@ extension CustomerCenterConfigResponse.HelpPath.PathType: CodableEnumWithUnknown | |
|
|
||
| } | ||
|
|
||
| extension CustomerCenterConfigResponse.HelpPath.OpenMethod: CodableEnumWithUnknownCase { | ||
|
There was a problem hiding this comment. @vegaro Note that I added this for the new enum |
||
|
|
||
| static var unknownCase: Self { .unknown } | ||
|
|
||
| } | ||
|
|
||
| extension CustomerCenterConfigResponse: HTTPResponseBody {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the best way but I couldn't make
URLIdentifiabledirectly without getting a warning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you getting the warning that it will not behave correctly if
URLever becomesIdentifiablein the future? If so, yea I think this solution is the safest!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense
nitpick but there should be an empty line at the beginning and at the end of the struct