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

HTTPClient: replaced X-StoreKit2-Setting with X-StoreKit2-Enabled #2118

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Dec 5, 2022

This will be used for https://github.com/RevenueCat/khepri/pull/4843, to fix SDKONCALL-160 and #2020.

X-StoreKit2-Setting only told us if the apps had that setting enabled. However, when running on an older device, SK2 would still be disabled.

@NachoSoto NachoSoto requested a review from a team December 5, 2022 23:19
@@ -60,6 +60,11 @@ extension StoreKit2Setting {

/// - Returns: `true` iff SK2 is enabled and it's available.
var shouldOnlyUseStoreKit2: Bool {
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 decided to keep this other property too because it has slightly different implications, even if it has the same implementation (right now).
This is used to determine whether to not enable SK1 stuff (see #1882 for example).

return self.isEnabledAndAvailable
}

/// - Returns: `true` iff SK2 is enabled and it's available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - Returns: `true` iff SK2 is enabled and it's available.
/// - Returns: `true` if SK2 is enabled and it's available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iff is not a typo, it means "if and only if": https://simple.wikipedia.org/wiki/If_and_only_if

Sources/Misc/StoreKit2Setting.swift Show resolved Hide resolved
@aboedo
Copy link
Member

aboedo commented Dec 7, 2022

I left some questions in the khepri PR, but general thoughts:

  • if this is information that khepri can figure out on its own, we should do that
  • having 2 headers that look similar seems like it'll get confusing to maintain

@NachoSoto
Copy link
Contributor Author

NachoSoto commented Dec 7, 2022

Yeah I wish we had done this originally :/ I feel like long term we should deprecate the other one. If anything changes in iOS I think the SDK is better suited to determine what's actually running and it seems scary to put that logic in the backend.

In the short term I agree that we should do that to fox promo purchases for existing users though. Since we're gonna do that, should we just close this?

@NachoSoto
Copy link
Contributor Author

Oh but as we discussed we need a client-side fix anyway for base64, so maybe this would be less risky that comparing version numbers?

@NachoSoto
Copy link
Contributor Author

Well I agree it might be better to not introduce a new header. I'll close this then.

@NachoSoto NachoSoto closed this Dec 8, 2022
@vegaro vegaro deleted the sk2-enabled-header branch December 8, 2022 09:55
@aboedo
Copy link
Member

aboedo commented Dec 8, 2022

if it ended up being impossible to fix older versions through a backend patch, then I'd vote for killing the old header in favor of the new one, I don't think it's currently used for anything

@NachoSoto NachoSoto restored the sk2-enabled-header branch December 9, 2022 17:21
@NachoSoto NachoSoto deleted the sk2-enabled-header branch December 9, 2022 17:21
@NachoSoto NachoSoto restored the sk2-enabled-header branch December 9, 2022 17:21
@NachoSoto NachoSoto reopened this Dec 9, 2022
@NachoSoto NachoSoto changed the title HTTPClient: added X-StoreKit2-Enabled header HTTPClient: replaced X-StoreKit2-Setting with X-StoreKit2-Enabled Dec 9, 2022
@NachoSoto
Copy link
Contributor Author

Okay updated this to replace the header instead.

@NachoSoto NachoSoto requested a review from aboedo December 9, 2022 17:25
@NachoSoto NachoSoto merged commit dc41f03 into main Dec 9, 2022
@NachoSoto NachoSoto deleted the sk2-enabled-header branch December 9, 2022 18:39
NachoSoto added a commit that referenced this pull request Dec 9, 2022
Fixes [SDKONCALL-160], [SDKONCALL-182], #2114, and
RevenueCat/react-native-purchases#455.
Depends on #2118 and https://github.com/RevenueCat/khepri/pull/4843 /
https://github.com/RevenueCat/khepri/pull/4852.

## Changes:
- Base64decode `signature`
- Fixed signature on backend for SK2:
https://github.com/RevenueCat/khepri/pull/4843 /
https://github.com/RevenueCat/khepri/pull/4852
- Added tests
- Added specific error if signature can't be decoded:
> [Purchases] - ERROR: 😿‼️ The information associated with this
PromotionalOffer is not valid.
See https://rev.cat/ios-subscription-offers for more info. The signature
generated by RevenueCat could not be decoded: signature 914

[SDKONCALL-160]:
https://revenuecats.atlassian.net/browse/SDKONCALL-160?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[SDKONCALL-182]:
https://revenuecats.atlassian.net/browse/SDKONCALL-182?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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