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

Fixing redisplaying for dynamic trigger IAMs #745

Merged
merged 6 commits into from Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m
Expand Up @@ -85,12 +85,12 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
return YES;
}

#define ONESIGNAL_APP_ID_KEY_FOR_TESTING @"ONESIGNAL_APP_ID_KEY_FOR_TESTING"
#define ONESIGNAL_APP_ID_KEY_FOR_TESTING @"77e32082-ea27-42e3-a898-c72e141824ef"

+ (NSString*)getOneSignalAppId {
NSString* onesignalAppId = [[NSUserDefaults standardUserDefaults] objectForKey:ONESIGNAL_APP_ID_KEY_FOR_TESTING];
if (!onesignalAppId)
onesignalAppId = @"0ba9731b-33bd-43f4-8b59-61172e27447d";
onesignalAppId = @"77e32082-ea27-42e3-a898-c72e141824ef";

return onesignalAppId;
}
Expand Down
2 changes: 2 additions & 0 deletions iOS_SDK/OneSignalSDK/Source/OSDynamicTriggerController.h
Expand Up @@ -33,6 +33,8 @@ NS_ASSUME_NONNULL_BEGIN
@protocol OSDynamicTriggerControllerDelegate <NSObject>

- (void)dynamicTriggerFired;
// Alerts the observer that a trigger evaluated to true
- (void)dynamicTriggerCompleted:(NSString *)triggerId;

@end

Expand Down
21 changes: 12 additions & 9 deletions iOS_SDK/OneSignalSDK/Source/OSDynamicTriggerController.m
Expand Up @@ -58,7 +58,6 @@ - (instancetype)init {
}

- (BOOL)dynamicTriggerShouldFire:(OSTrigger *)trigger withMessageId:(NSString *)messageId {

if (!trigger.value)
return false;

Expand All @@ -79,10 +78,12 @@ - (BOOL)dynamicTriggerShouldFire:(OSTrigger *)trigger withMessageId:(NSString *)
// Check what type of trigger it is
if ([trigger.kind isEqualToString:OS_DYNAMIC_TRIGGER_KIND_SESSION_TIME]) {
let currentDuration = fabs([[OneSignal sessionLaunchTime] timeIntervalSinceNow]);

if ([self evaluateTimeInterval:requiredTimeValue withCurrentValue:currentDuration forOperator:trigger.operatorType])
if ([self evaluateTimeInterval:requiredTimeValue withCurrentValue:currentDuration forOperator:trigger.operatorType]) {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"session time trigger completed: %@", trigger.triggerId]];
[self.delegate dynamicTriggerCompleted:trigger.triggerId];
//[self.delegate dynamicTriggerFired:trigger.triggerId];
return true;

}
offset = requiredTimeValue - currentDuration;
} else if ([trigger.kind isEqualToString:OS_DYNAMIC_TRIGGER_KIND_MIN_TIME_SINCE]) {

Expand All @@ -92,9 +93,10 @@ - (BOOL)dynamicTriggerShouldFire:(OSTrigger *)trigger withMessageId:(NSString *)

let timestampSinceLastMessage = fabs([self.timeSinceLastMessage timeIntervalSinceNow]);

if ([self evaluateTimeInterval:requiredTimeValue withCurrentValue:timestampSinceLastMessage forOperator:trigger.operatorType])
if ([self evaluateTimeInterval:requiredTimeValue withCurrentValue:timestampSinceLastMessage forOperator:trigger.operatorType]) {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"time since last inapp trigger completed: %@", trigger.triggerId]];
return true;

}
offset = requiredTimeValue - timestampSinceLastMessage;
}

Expand All @@ -103,17 +105,18 @@ - (BOOL)dynamicTriggerShouldFire:(OSTrigger *)trigger withMessageId:(NSString *)
return false;

// If we reach this point, it means we need to return false and set up a timer for a future time
let timer = [NSTimer timerWithTimeInterval:offset
NSTimer *timer = [NSTimer timerWithTimeInterval:offset
target:self
selector:@selector(timerFiredForMessage:)
userInfo:@{@"trigger" : trigger}
repeats:false];
if (timer)
if (timer) {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"timer added for triggerId: %@, messageId: %@", trigger.triggerId, messageId]];
[[NSRunLoop mainRunLoop] addTimer:timer forMode:NSRunLoopCommonModes];
}

[self.scheduledMessages addObject:trigger.triggerId];
}

return false;
}

Expand Down
6 changes: 5 additions & 1 deletion iOS_SDK/OneSignalSDK/Source/OSInAppMessageDisplayStats.m
Expand Up @@ -111,7 +111,9 @@ - (BOOL)isDelayTimeSatisfied:(NSTimeInterval)date {
}

- (BOOL)shouldDisplayAgain {
return _displayQuantity < _displayLimit;
BOOL result = _displayQuantity < _displayLimit;
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"In app message shouldDisplayAgain: %hhu", result]];
return result;
}

- (void)incrementDisplayQuantity {
Expand All @@ -131,6 +133,7 @@ - (void)encodeWithCoder:(NSCoder *)encoder {
[encoder encodeInteger:_displayQuantity forKey:@"displayQuantity"];
[encoder encodeDouble:_displayDelay forKey:@"displayDelay"];
[encoder encodeDouble:_lastDisplayTime forKey:@"lastDisplayTime"];
[encoder encodeBool:_redisplayEnabled forKey:@"redisplayEnabled"];
}

- (id)initWithCoder:(NSCoder *)decoder {
Expand All @@ -139,6 +142,7 @@ - (id)initWithCoder:(NSCoder *)decoder {
_displayQuantity = [decoder decodeIntegerForKey:@"displayQuantity"];
_displayDelay = [decoder decodeDoubleForKey:@"displayDelay"];
_lastDisplayTime = [decoder decodeDoubleForKey:@"lastDisplayTime"];
_redisplayEnabled = [decoder decodeBoolForKey:@"redisplayEnabled"];
}
return self;
}
Expand Down
36 changes: 29 additions & 7 deletions iOS_SDK/OneSignalSDK/Source/OSMessagingController.m
Expand Up @@ -322,11 +322,13 @@ - (void)messageViewImpressionRequest:(OSInAppMessage *)message {
- (void)evaluateMessages {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:@"Evaluating in app messages"];
for (OSInAppMessage *message in self.messages) {
// Make changes to IAM if redisplay available
[self setDataForRedisplay:message];
// Should we show the in app message
if ([self shouldShowInAppMessage:message]) {
[self presentInAppMessage:message];
if ([self.triggerController messageMatchesTriggers:message]) {
// Make changes to IAM if redisplay available
[self setDataForRedisplay:message];
// Should we show the in app message
if ([self shouldShowInAppMessage:message]) {
[self presentInAppMessage:message];
}
}
}
}
Expand Down Expand Up @@ -491,8 +493,11 @@ - (void)hideWindow {

- (void)persistInAppMessageForRedisplay:(OSInAppMessage *)message {
// If the IAM doesn't have the re display prop or is a preview IAM there is no need to save it
if (![message.displayStats isRedisplayEnabled] || message.isPreview)
return;
if (![message.displayStats isRedisplayEnabled] || message.isPreview) {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"not persisting %@",message.displayStats]];
return;
}


let displayTimeSeconds = self.dateGenerator();
message.displayStats.lastDisplayTime = displayTimeSeconds;
Expand Down Expand Up @@ -697,9 +702,25 @@ - (void)addKeySceneToWindow:(UIWindow*)window {
}
}

- (void)dynamicTriggerCompleted:(NSString *)triggerId {
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"messageDynamicTriggerCompleted called with triggerId: %@", triggerId]];
[self makeRedisplayMessagesAvailableWithTriggers:@[triggerId]];
}

- (void)makeRedisplayMessagesAvailableWithTriggers:(NSArray<NSString *> *)triggerIds {
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic is deferring a bit from Android, do we want to come with a common logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After working on it I think that not duplicating the state in the redisplayedMessages might be better what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that can be confusing having 2 different lists, although I see that having the redisplay messages separated can optimize the iteration. Because on worst case redisplay list will be the same length as the iam list, but in the best case redisplay iams will be empty. But is also true that there are not huge lists, or they shouldn't be, so there is not that much impact on the performance. So if you see that is more readable to only iterate iams list then I'm ok with removing the redisplay iteration for trigger change check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right I think having it more readable and not trying to synchronize the state is the better approach here. Sorry for making you implement it in the first place!

Copy link
Contributor

Choose a reason for hiding this comment

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

ok agree then, will make the change under major release branch

for (OSInAppMessage *message in self.messages) {
if ([self.redisplayedInAppMessages objectForKey:message.messageId]
&& [_triggerController hasSharedTriggers:message newTriggersKeys:triggerIds]) {
message.isTriggerChanged = YES;
}
}
}

#pragma mark OSTriggerControllerDelegate Methods

- (void)triggerConditionChanged {
// We should re-evaluate all in-app messages
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:@"Trigger condition changed"];
[self evaluateMessages];
}

Expand Down Expand Up @@ -741,5 +762,6 @@ - (void)messageViewDidSelectAction:(OSInAppMessage *)message withAction:(OSInApp
- (void)webViewContentFinishedLoading {}
#pragma mark OSTriggerControllerDelegate Methods
- (void)triggerConditionChanged {}
- (void)dynamicTriggerCompleted:(NSString *)triggerId {}

@end
2 changes: 1 addition & 1 deletion iOS_SDK/OneSignalSDK/Source/OSTriggerController.h
Expand Up @@ -39,7 +39,7 @@ NS_ASSUME_NONNULL_BEGIN
It is also called when the app changes trigger values
*/
- (void)triggerConditionChanged;

- (void)dynamicTriggerCompleted:(NSString *)triggerId;
@end

@interface OSTriggerController : NSObject <OSDynamicTriggerControllerDelegate>
Expand Down
8 changes: 7 additions & 1 deletion iOS_SDK/OneSignalSDK/Source/OSTriggerController.m
Expand Up @@ -85,7 +85,9 @@ - (BOOL)hasSharedTriggers:(OSInAppMessage *)message newTriggersKeys:(NSArray<NSS
for (NSString *triggerKey in newTriggersKeys) {
for (NSArray <OSTrigger *> *andConditions in message.triggers) {
for (OSTrigger *trigger in andConditions) {
if ([triggerKey isEqual:trigger.property]) {
// Dynamic triggers depends on triggerId
// Common triggers changed by user depends on property
if ([triggerKey isEqual:trigger.property] || [triggerKey isEqualToString:trigger.triggerId]) {
// At least one trigger has changed
return YES;
}
Expand Down Expand Up @@ -263,4 +265,8 @@ - (void)dynamicTriggerFired {
[self.delegate triggerConditionChanged];
}

- (void)dynamicTriggerCompleted:(NSString *)triggerId {
[self.delegate dynamicTriggerCompleted:triggerId];
}

@end
3 changes: 1 addition & 2 deletions iOS_SDK/OneSignalSDK/Source/OneSignal.m
Expand Up @@ -1712,6 +1712,7 @@ + (void)registerUserInternal {
}

[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:@"Calling OneSignal create/on_session"];
sessionLaunchTime = [NSDate date];
Jeasmine marked this conversation as resolved.
Show resolved Hide resolved


if (mShareLocation && [OneSignalLocation lastLocation]) {
Expand Down Expand Up @@ -2801,7 +2802,6 @@ @implementation OneSignal (SessionStatusDelegate)
+ (void)onSessionEnding:(NSArray<OSInfluence *> *)lastInfluences {
if (_outcomeEventsController)
[_outcomeEventsController clearOutcomes];

[OneSignalTracker onSessionEnded:lastInfluences];
}

Expand Down Expand Up @@ -2850,7 +2850,6 @@ + (void)load {
injectToProperClass(@selector(onesignalSetApplicationIconBadgeNumber:), @selector(setApplicationIconBadgeNumber:), @[], [OneSignalAppDelegate class], [UIApplication class]);

[self setupUNUserNotificationCenterDelegate];

sessionLaunchTime = [NSDate date];
}

Expand Down
73 changes: 73 additions & 0 deletions iOS_SDK/OneSignalSDK/UnitTests/InAppMessagingIntegrationTests.m
Expand Up @@ -1422,6 +1422,79 @@ - (void)testIAMHTMLLoadWithDefaultLanguage {
XCTAssertTrue([url containsString:OS_TEST_MESSAGE_ID]);
}

- (void)testInAppMessageDisplayMultipleTimesSessionDurationTrigger {
[OneSignal pauseInAppMessages:NO];
let trigger = [OSTrigger dynamicTriggerWithKind:OS_DYNAMIC_TRIGGER_KIND_SESSION_TIME withOperator:OSTriggerOperatorTypeGreaterThanOrEqualTo withValue:@0];
let message = [OSInAppMessageTestHelper testMessageWithTriggers:@[@[trigger]] withRedisplayLimit:5 delay:@30];
let registrationJson = [OSInAppMessageTestHelper testRegistrationJsonWithMessages:@[message.jsonRepresentation]];

//Time interval mock
NSDateComponents* comps = [[NSDateComponents alloc]init];
comps.year = 2020;
comps.month = 9;
comps.day = 10;
comps.hour = 10;
comps.minute = 1;

NSCalendar* calendar = [NSCalendar currentCalendar];
NSDate* date = [calendar dateFromComponents:comps];
NSTimeInterval firstInterval = [date timeIntervalSince1970];

// Init OneSignal IAM with redisplay
[self initOneSignalWithRegistrationJSON:registrationJson];

[OSMessagingControllerOverrider setMockDateGenerator: ^NSTimeInterval(void) {
return firstInterval;
}];

XCTAssertEqual(OSMessagingControllerOverrider.messageDisplayQueue.count, 1);

// Dismiss IAM will make display quantity increase and last display time to change
[OSMessagingControllerOverrider dismissCurrentMessage];
// Check IAMs was removed from queue
XCTAssertEqual(OSMessagingControllerOverrider.messageDisplayQueue.count, 0);
// Check if data after dismiss is set correctly
XCTAssertEqual(OSMessagingControllerOverrider.messagesForRedisplay.count, 1);
let displayQuantity = OSMessagingControllerOverrider.messagesForRedisplay.allValues[0].displayStats.displayQuantity;
let displayTime = OSMessagingControllerOverrider.messagesForRedisplay.allValues[0].displayStats.lastDisplayTime;
XCTAssertEqual(displayQuantity, 1);
XCTAssertTrue(displayTime > 0);

[UnitTestCommonMethods runBackgroundThreads];

// 3. Kill the app and wait 31 seconds
[UnitTestCommonMethods backgroundApp];
[NSDateOverrider advanceSystemTimeBy:31];
[UnitTestCommonMethods runBackgroundThreads];
//Time interval mock
comps.minute = 32;
NSDate* secondDate = [calendar dateFromComponents:comps];
NSTimeInterval secondInterval = [secondDate timeIntervalSince1970];
[OSMessagingControllerOverrider setMockDateGenerator: ^NSTimeInterval(void) {
return secondInterval;
}];

// Init OneSignal IAM with redisplay
[self initOneSignalWithRegistrationJSON:registrationJson];

[OSMessagingControllerOverrider setMockDateGenerator: ^NSTimeInterval(void) {
return firstInterval;
}];
[UnitTestCommonMethods foregroundApp];
[UnitTestCommonMethods runBackgroundThreads];

XCTAssertEqual(OSMessagingControllerOverrider.messageDisplayQueue.count, 1);

[OSMessagingControllerOverrider dismissCurrentMessage];
// Check IAMs was removed from queue
XCTAssertEqual(OSMessagingControllerOverrider.messageDisplayQueue.count, 0);
// Check if data after dismiss is set correctly
XCTAssertEqual(OSMessagingControllerOverrider.messagesForRedisplay.count, 1);
let secondDisplayQuantity = OSMessagingControllerOverrider.messagesForRedisplay.allValues[0].displayStats.displayQuantity;
let secondDisplayTime = OSMessagingControllerOverrider.messagesForRedisplay.allValues[0].displayStats.lastDisplayTime;
XCTAssertEqual(secondDisplayQuantity, 2);
}

// Helper method that adds an OSInAppMessage to the IAM messageDisplayQueue
// Mock response JSON and initializes the OneSignal SDK
- (void)initOneSignalWithInAppMessage:(OSInAppMessage *)message {
Expand Down
2 changes: 1 addition & 1 deletion iOS_SDK/OneSignalSDK/UnitTests/InAppMessagingTests.m
Expand Up @@ -541,7 +541,7 @@ - (void)testDynamicTriggerSessionDurationLaunchesTimer {

XCTAssertFalse(triggered);
XCTAssertTrue(NSTimerOverrider.hasScheduledTimer);
XCTAssertTrue(fabs(NSTimerOverrider.mostRecentTimerInterval - 30.0f) < 0.1f);
XCTAssertTrue(fabs(NSTimerOverrider.mostRecentTimerInterval - 30.0f) < 1.1f);
}


Expand Down