added fix for empty strings as cta urls #107

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

malectro commented Jan 23, 2014

No description provided.

@alex-hofsteede alex-hofsteede commented on an outdated diff Jan 23, 2014

HelloMixpanel/HelloMixpanel/AppDelegate.m
@@ -5,7 +5,7 @@
#import "ViewController.h"
// IMPORTANT!!! replace with you api token from https://mixpanel.com/account/
-#define MIXPANEL_TOKEN @"YOUR_MIXPANEL_PROJECT_TOKEN"
+#define MIXPANEL_TOKEN @"metrics-1"

@alex-hofsteede alex-hofsteede commented on an outdated diff Jan 23, 2014

HelloMixpanel/HelloMixpanel/AppDelegate.m
@@ -24,6 +24,8 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
self.mixpanel.checkForNotificationsOnActive = YES;
self.mixpanel.showNotificationOnActive = YES; //Change this to NO to show your notifs manually.
+
+ [self.mixpanel identify:@"189321"];

@alex-hofsteede alex-hofsteede commented on the diff Jan 23, 2014

Mixpanel/MPNotification.m
if (URLString != nil && ![URLString isKindOfClass:[NSNull class]]) {
- if (![URLString isKindOfClass:[NSString class]] || [(NSString *)URLString length] == 0) {
@alex-hofsteede

alex-hofsteede Jan 23, 2014

Contributor

Can we add a test to make sure the empty cta_url passes

@malectro

malectro Jan 23, 2014

Contributor

added

@malectro malectro mend
fixed cta_url string parsing and added extra testing
be611af

@alex-hofsteede alex-hofsteede commented on the diff Jan 23, 2014

Mixpanel/MPNotification.m
@@ -59,17 +59,19 @@ + (MPNotification *)notificationWithJSONObject:(NSDictionary *)object
}
NSURL *callToActionURL = nil;
- NSObject *URLString = object[@"cta_url"];
+ NSString *URLString = object[@"cta_url"];
@alex-hofsteede

alex-hofsteede Jan 23, 2014

Contributor

I started calling this NSObject just to make clear that we don't know what type it is until we check isKindOfClass. But maybe its just more confusing and adds a ton of casts throughout the code. It can stay this way.

@malectro

malectro Jan 23, 2014

Contributor

I figured that if we went that route, we'd have to play the same way with all the other values we're parsing. It might actually merit its own pull request in the future.

malectro closed this Mar 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment