Skip to content

Commit 11dc754

Browse files
jkasten2jkasten2
authored
Merge pull request #109 from OneSignal/get_tags_state_fixes
Fixed getTag internal merging logic after 1st call
2 parents be4baaa + c4012fe commit 11dc754

File tree

2 files changed

+83
-54
lines changed

2 files changed

+83
-54
lines changed

‎OneSignalSDK/app/src/test/java/com/test/onesignal/MainOneSignalClassRunner.java

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,7 +1063,7 @@ public void shouldGetTags() throws Exception {
10631063
}
10641064

10651065
@Test
1066-
public void shouldGetTagsFromServerOnFirstCall() throws Exception {
1066+
public void shouldGetTagsFromServerOnFirstCallAndMergeLocalAndRemote() throws Exception {
10671067
OneSignalInit();
10681068
threadAndTaskWait();
10691069

@@ -1075,9 +1075,35 @@ public void shouldGetTagsFromServerOnFirstCall() throws Exception {
10751075
Assert.assertEquals("value1", lastGetTags.getString("test1"));
10761076
Assert.assertEquals("value2", lastGetTags.getString("test2"));
10771077

1078+
// Makes sure a 2nd call to GetTags correctly uses existing tags and merges new local changes.
1079+
lastGetTags = null;
1080+
OneSignal.sendTag("test3", "value3");
10781081
GetTags();
1082+
threadWait();
1083+
Assert.assertEquals("value1", lastGetTags.getString("test1"));
1084+
Assert.assertEquals("value2", lastGetTags.getString("test2"));
1085+
Assert.assertEquals("value3", lastGetTags.getString("test3"));
1086+
threadAndTaskWait(); threadAndTaskWait();
1087+
// Also ensure only 1 network call is made to just send the new tags only.
1088+
Assert.assertEquals(4, ShadowOneSignalRestClient.networkCallCount);
1089+
1090+
StaticResetHelper.restSetStaticFields();
1091+
OneSignalInit();
10791092
threadAndTaskWait();
1080-
Assert.assertEquals(3, ShadowOneSignalRestClient.networkCallCount);
1093+
1094+
// Test that local pending changes are still applied but new changes made server side a respected.
1095+
lastGetTags = null;
1096+
OneSignal.deleteTag("test2");
1097+
OneSignal.sendTag("test4", "value4");
1098+
ShadowOneSignalRestClient.nextSuccessfulGETResponse = "{\"tags\": {\"test1\": \"value1\", \"test2\": \"value2\",\"test3\": \"ShouldOverride\",\"test4\": \"RemoteShouldNotOverwriteLocalPending\"}}";
1099+
GetTags();
1100+
threadAndTaskWait(); threadAndTaskWait();
1101+
Assert.assertEquals("value1", lastGetTags.getString("test1"));
1102+
Assert.assertFalse(lastGetTags.has("test2"));
1103+
Assert.assertEquals("ShouldOverride", lastGetTags.getString("test3"));
1104+
Assert.assertEquals("value4", lastGetTags.getString("test4"));
1105+
Assert.assertEquals(7, ShadowOneSignalRestClient.networkCallCount);
1106+
Assert.assertEquals("{\"test2\":\"\",\"test4\":\"value4\"}", ShadowOneSignalRestClient.lastPost.optJSONObject("tags").toString());
10811107
}
10821108

10831109
@Test

‎OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalStateSynchronizer.java

Lines changed: 55 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -58,9 +58,9 @@ class OneSignalStateSynchronizer {
5858
private static Context appContext;
5959

6060
private static final String[] LOCATION_FIELDS = new String[] { "lat", "long", "loc_acc", "loc_type"};
61-
private static final Set<String> LOCATION_FIELDS_SET = new HashSet<String>(Arrays.asList(LOCATION_FIELDS));
61+
private static final Set<String> LOCATION_FIELDS_SET = new HashSet<>(Arrays.asList(LOCATION_FIELDS));
6262

63-
// Object to synchronize to prevent concurrent modifications on syncValues and dependValues
63+
// Object to synchronize on to prevent concurrent modifications on syncValues and dependValues
6464
private static final Object syncLock = new Object() {};
6565

6666
static private JSONObject generateJsonDiff(JSONObject cur, JSONObject changedTo, JSONObject baseOutput, Set<String> includeFields) {
@@ -70,6 +70,11 @@ static private JSONObject generateJsonDiff(JSONObject cur, JSONObject changedTo,
7070
}
7171

7272
static private JSONObject synchronizedGenerateJsonDiff(JSONObject cur, JSONObject changedTo, JSONObject baseOutput, Set<String> includeFields) {
73+
if (cur == null)
74+
return null;
75+
if (changedTo == null)
76+
return baseOutput;
77+
7378
Iterator<String> keys = changedTo.keys();
7479
String key;
7580
Object value;
@@ -388,46 +393,50 @@ private void modifySyncValuesJsonArray(String baseKey) {
388393
private void persistStateAfterSync(JSONObject inDependValues, JSONObject inSyncValues) {
389394
if (inDependValues != null)
390395
OneSignalStateSynchronizer.generateJsonDiff(dependValues, inDependValues, dependValues, null);
396+
391397
if (inSyncValues != null) {
392398
OneSignalStateSynchronizer.generateJsonDiff(syncValues, inSyncValues, syncValues, null);
399+
mergeTags(inSyncValues, null);
400+
}
393401

394-
synchronized (syncLock) {
395-
if (inSyncValues.has("tags")) {
396-
JSONObject newTags;
397-
if (syncValues.has("tags")) {
398-
try {
399-
newTags = new JSONObject(syncValues.optString("tags"));
400-
} catch (JSONException e) {
401-
newTags = new JSONObject();
402-
}
403-
}
404-
else
402+
if (inDependValues != null || inSyncValues != null)
403+
persistState();
404+
}
405+
406+
void mergeTags(JSONObject inSyncValues, JSONObject omitKeys) {
407+
synchronized (syncLock) {
408+
if (inSyncValues.has("tags")) {
409+
JSONObject newTags;
410+
if (syncValues.has("tags")) {
411+
try {
412+
newTags = new JSONObject(syncValues.optString("tags"));
413+
} catch (JSONException e) {
405414
newTags = new JSONObject();
415+
}
416+
}
417+
else
418+
newTags = new JSONObject();
406419

407-
JSONObject curTags = inSyncValues.optJSONObject("tags");
408-
Iterator<String> keys = curTags.keys();
409-
String key;
420+
JSONObject curTags = inSyncValues.optJSONObject("tags");
421+
Iterator<String> keys = curTags.keys();
422+
String key;
410423

411-
try {
412-
while (keys.hasNext()) {
413-
key = keys.next();
414-
if (!"".equals(curTags.optString(key)))
415-
newTags.put(key, curTags.optString(key));
416-
else
417-
newTags.remove(key);
418-
}
424+
try {
425+
while (keys.hasNext()) {
426+
key = keys.next();
427+
if ("".equals(curTags.optString(key)))
428+
newTags.remove(key);
429+
else if (omitKeys == null || !omitKeys.has(key))
430+
newTags.put(key, curTags.optString(key));
431+
}
419432

420-
if (newTags.toString().equals("{}"))
421-
syncValues.remove("tags");
422-
else
423-
syncValues.put("tags", newTags);
424-
} catch (Throwable t) {}
425-
}
433+
if (newTags.toString().equals("{}"))
434+
syncValues.remove("tags");
435+
else
436+
syncValues.put("tags", newTags);
437+
} catch (Throwable t) {}
426438
}
427439
}
428-
429-
if (inDependValues != null || inSyncValues != null)
430-
persistState();
431440
}
432441
}
433442

@@ -691,43 +700,37 @@ static class GetTagsResult {
691700
}
692701
}
693702

694-
private static JSONObject lastGetTagsResponse;
703+
static boolean serverSuccess;
695704
static GetTagsResult getTags(boolean fromServer) {
696-
lastGetTagsResponse = null;
697-
698705
if (fromServer) {
699706
String userId = OneSignal.getUserId();
700707
OneSignalRestClient.getSync("players/" + userId, new OneSignalRestClient.ResponseHandler() {
701708
@Override
702709
void onSuccess(String responseStr) {
710+
serverSuccess = true;
703711
try {
704-
lastGetTagsResponse = new JSONObject(responseStr);
712+
JSONObject lastGetTagsResponse = new JSONObject(responseStr);
705713
if (lastGetTagsResponse.has("tags")) {
706-
lastGetTagsResponse = lastGetTagsResponse.optJSONObject("tags");
707-
currentUserState.syncValues.put("tags", lastGetTagsResponse);
714+
JSONObject dependDiff = generateJsonDiff(currentUserState.syncValues.optJSONObject("tags"),
715+
toSyncUserState.syncValues.optJSONObject("tags"),
716+
null, null);
717+
718+
currentUserState.syncValues.put("tags", lastGetTagsResponse.optJSONObject("tags"));
708719
currentUserState.persistState();
709720

710-
JSONObject tagsToSync = getTagsWithoutDeletedKeys(toSyncUserState.syncValues);
711-
if (tagsToSync != null) {
712-
Iterator<String> keys = tagsToSync.keys();
713-
while (keys.hasNext()) {
714-
String key = keys.next();
715-
lastGetTagsResponse.put(key, tagsToSync.optString(key));
716-
}
717-
}
721+
// Allow server side tags to overwrite local tags expect for any pending changes
722+
// that haven't been successfully posted.
723+
toSyncUserState.mergeTags(lastGetTagsResponse, dependDiff);
724+
toSyncUserState.persistState();
718725
}
719-
else
720-
lastGetTagsResponse = null;
721726
} catch (JSONException e) {
722727
e.printStackTrace();
723728
}
724729
}
725730
});
726731
}
727732

728-
if (lastGetTagsResponse == null)
729-
return new GetTagsResult(false, getTagsWithoutDeletedKeys(toSyncUserState.syncValues));
730-
return new GetTagsResult(true, lastGetTagsResponse);
733+
return new GetTagsResult(serverSuccess, getTagsWithoutDeletedKeys(toSyncUserState.syncValues));
731734
}
732735

733736
static void resetCurrentState() {

0 commit comments

Comments
 (0)