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
Fix NSNull Attributes crash #359
Conversation
…ure that the right value is cached for postAttributionData
|
||
@implementation NSDictionary (RCExtensions) | ||
|
||
- (NSDictionary *)removingNSNullValues { |
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.
turns out this wasn't necessary, but it might be a useful utility. I think there's actually a couple of places where we're doing this manually. So I wasn't sure whether to remove it - there are also tests for it, so I imagine it can't hurt to have it around.
@@ -155,7 +155,7 @@ - (void)postAttributionData:(NSDictionary *)data | |||
forAppUserID:appUserID | |||
completion:^(NSError *_Nullable error) { | |||
if (error == nil) { | |||
[self.deviceCache setLatestNetworkAndAdvertisingIdsSent:newData | |||
[self.deviceCache setLatestNetworkAndAdvertisingIdsSent:newDictToCache |
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.
this is the fix. And totally my bad.
@@ -55,5 +65,64 @@ class AttributionFetcherTests: XCTestCase { | |||
|
|||
expect { self.attributionFetcher.rot13(randomized) } .to(equal(expected)) | |||
} | |||
|
|||
func testPostAttributionDataSkipsIfAlreadySent() { |
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 overestimated how tested this was - I thought that tests passing for the refactor meant that we were all good, but there weren't really any tests for this stuff.
from: .adjust, | ||
forNetworkUserId: userID) | ||
|
||
expect(backend.invokedPostAttributionDataCount) == 1 |
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.
confirmed that this test fails on develop
* added test that fails if NSNull value is passed into attribution * added NSDictionary extension to filter out NSNull values * fixed a bug in the test * fix regression * removed unnecessary usage of removingNSNullValues, added tests to ensure that the right value is cached for postAttributionData
Addresses a crash that's been reported through ZenDesk and in RevenueCat/purchases-flutter#105, where calling
addAttributionData
withNSNull
values inside theNSDictionary
would make the app crash.The problem is actually a regression in 3.7.0 in the caching of the latest sent values for each network.
breaking line:
https://github.com/RevenueCat/purchases-ios/pull/331/files#diff-16824aa266a8f5da37f049ed477c425bR158
original line:
https://github.com/RevenueCat/purchases-ios/pull/331/files#diff-1f8164986a491f4cc448a01a1161e2bbL395
note that the object cached should have been
newDictToCache
, notnewData
.