Wootric / WootricSDK-iOS

Wootric iOS SDK to show NPS, CSAT and CES surveys
MIT License
13 stars 24 forks source link

Crash in checkEligibility #124

Closed samsiner closed 7 months ago

samsiner commented 9 months ago

Starting with this latest version (0.26.3), I'm getting a crash when the app loads. The stack trace says that it's happening on WTRApiClient checkEligibility:.

To configure Wootric in the app, we call the following within AppDelegate didFinishLaunchingWithOptions: Wootric.configure(withClientID: clientID, accountToken: accountToken). To show the survey, we call Wootric.showSurvey(in:). It's been working fine up to now.

Screenshot of the crash and the relevant Wootric code:

Screenshot 2024-01-31 at 9 45 12 AM
diegoserranoa commented 9 months ago

Hi @samsiner thanks for reporting this. Let me check this out and try to reproduce this issue.

diegoserranoa commented 9 months ago

Hi @samsiner I'm trying to replicate this issue but haven't been able to. Do you mind providing more details of your setup? If you're not comfortable sharing those details here you can send me an email to diego.serrano@inmoment.com

Thank you for your help!

samsiner commented 9 months ago

@diegoserranoa Yep, I will send it via e-mail. When I pinned the Wootric SDK to the 0.26.2, the crash went away, so whatever is happening seems related to the latest version.

samsiner commented 8 months ago

@diegoserranoa I was able to dig in more and figure out what's going on. When we set user properties with Wootric.setEndUserProperties(), we are assigning Doubles as values in that dictionary.

In the latest version, the way that Wootric parses user properties to check eligibility was changed. See here.

However, NSURLQueryItem expects a NSString value, but the code no longer checks to see if the value is a string. In our case, the value is a Double, but it could be anything besides a string.

I was able to fix the issue in this way in WTRApiClient.m, line 534-542:

- (NSArray *)addPropertiesToURL {
  NSMutableArray *items = [NSMutableArray new];
  if (_settings.customProperties) {
    for (NSString *key in _settings.customProperties) {
      [items addObject:[NSURLQueryItem queryItemWithName:[NSString stringWithFormat:@"%@[%@]",WTRPropertiesKey, key] value:[NSString stringWithFormat:@"%@", [_settings.customProperties objectForKey:key]]]];
    }
  }
  return items;
}

I'm unable to open a PR in the Wootric repo, so please feel free to open a PR on my behalf if you think this is the right solution.

diegoserranoa commented 8 months ago

@samsiner nice! Thank you for catching that and providing more details. I will start working on the patch. Will keep you posted.

rohdester commented 7 months ago

When will this be released? We also experience this crash. Also thanks to @samsiner for finding the cause. Makes it possible for us to code around it.

diegoserranoa commented 7 months ago

Hi @rohdester , someone is currently QAing the fix. Hopefully we'll release the new version this week or the next one.

diegoserranoa commented 7 months ago

The fix has been released (https://github.com/Wootric/WootricSDK-iOS/releases/tag/0.27.1)

Thank you all for the help and patience!

EvertEt commented 6 months ago

@diegoserranoa It looks like that version does not include the fix.

  1. I've confirmed the crash still happens
  2. https://github.com/Wootric/WootricSDK-iOS/releases/tag/0.27.1 shows 1 commit to master since this release

Could you take a look and push a new version? Thank you

diegoserranoa commented 6 months ago

@EvertEt do you have a log or something showing the crash with version 0.27.1?

EvertEt commented 6 months ago

@diegoserranoa Yep:

Last Exception Backtrace:
0   CoreFoundation                         0x1167c1561 __exceptionPreprocess + 226
1   libobjc.A.dylib                        0x112ad57e8 objc_exception_throw + 48
2   CoreFoundation                         0x1167d6683 +[NSObject(NSObject) instanceMethodSignatureForSelector:] + 0
3   CoreFoundation                         0x1167c5d04 ___forwarding___ + 1459
4   CoreFoundation                         0x1167c7f08 _CF_forwarding_prep_0 + 120
5   CoreFoundation                         0x11679cd7d _CFStringCreateByAddingPercentEncodingWithAllowedCharacters + 48
6   CoreFoundation                         0x1167baff4 _CFURLComponentsSetQueryItemsInternal + 888
7   Foundation                             0x11b9118fa -[__NSConcreteURLComponents setQueryItems:] + 242
8   XXX                        0x1060a6e68 -[WTRApiClient checkEligibility:] + 840 (WTRApiClient.m:368)
9   XXX                        0x1060b2fac -[WTREvent checkEligibility:] + 172 (WTREvent.m:79)
10  XXX                        0x1060b2eea -[WTREvent start] + 922 (WTREvent.m:76)

Podfile contents:

- WootricSDK (0.27.1)
...
WootricSDK: 7093f79a05eb215ced9efa37f85e361fac289d1a

frontend/ios/Pods/WootricSDK/WootricSDK/WootricSDK/WTRApiClient.m contains the following (unfixed)

    [queryItems addObject:[NSURLQueryItem queryItemWithName:WTRAccountTokenKey value:_accountToken]];
    [queryItems addObjectsFromArray:[self addVersionsToURL]];
    [queryItems addObject:[self addEmailToURL]];
    [queryItems addObjectsFromArray:[self addSurveyServerCustomSettingsToURL]];
    [queryItems addObjectsFromArray:[self addPropertiesToURL]];
    [queryItems addObject:[self addEventNameToURL]];
EvertEt commented 6 months ago

@diegoserranoa The problem is also visible if you look at the tags. 0.27.0 and 0.27.1 are on the same commit. I believe it should be on the last commit instead.

image
diegoserranoa commented 6 months ago

@EvertEt we're in the process of QA for another PR. Once that goes through we'll release a new version and that should contain this fix!

EvertEt commented 6 months ago

@diegoserranoa Do you have an update on the QA here? This is blocking the upgrade to the RN SDK 1.8.0 (https://github.com/Wootric/react-native-wootric/pull/34#issuecomment-2091710342)

diegoserranoa commented 5 months ago

Hi @EvertEt ! We just released version 0.28.0. Can you try that update and see if it fixes your issue? 🙏

EvertEt commented 5 months ago

It's not crashing the app anymore, so looking promising. We'll do some more tests over the next weeks.

diegoserranoa commented 5 months ago

@EvertEt thanks for the update. That's good to know. Keep me posted if anything comes up over the next weeks.