Skip to content

Commit dbf2967

Browse files
JasonAHeronJasonAHeron
authored
Improve impression logging for experimental campaigns (#1232)
1 parent f7ae31d commit dbf2967

File tree

7 files changed

+98
-31
lines changed

7 files changed

+98
-31
lines changed

‎firebase-inappmessaging/src/main/java/com/google/firebase/inappmessaging/internal/DeveloperListenerManager.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ public ExecutorAndListener(Executor e) {
182182
}
183183
}
184184

185-
private class ImpressionExecutorAndListener
185+
private static class ImpressionExecutorAndListener
186186
extends ExecutorAndListener<FirebaseInAppMessagingImpressionListener> {
187187
FirebaseInAppMessagingImpressionListener listener;
188188

@@ -203,7 +203,7 @@ public FirebaseInAppMessagingImpressionListener getListener() {
203203
}
204204
}
205205

206-
private class ClicksExecutorAndListener
206+
private static class ClicksExecutorAndListener
207207
extends ExecutorAndListener<FirebaseInAppMessagingClickListener> {
208208
FirebaseInAppMessagingClickListener listener;
209209

@@ -223,7 +223,7 @@ public FirebaseInAppMessagingClickListener getListener() {
223223
}
224224
}
225225

226-
private class ErrorsExecutorAndListener
226+
private static class ErrorsExecutorAndListener
227227
extends ExecutorAndListener<FirebaseInAppMessagingDisplayErrorListener> {
228228
FirebaseInAppMessagingDisplayErrorListener listener;
229229

‎firebase-inappmessaging/src/main/java/com/google/firebase/inappmessaging/internal/DisplayCallbacksImpl.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ private void logActionNotTaken(String action, Maybe<String> reason) {
208208
Logging.logd(String.format("Not recording: %s. Reason: %s", action, reason));
209209
}
210210
// If a reason is not provided then check for a test message.
211-
else if (inAppMessage.getIsTestMessage()) {
211+
else if (inAppMessage.getCampaignMetadata().getIsTestMessage()) {
212212
Logging.logd(String.format("Not recording: %s. Reason: Message is test message", action));
213213
}
214214
// If no reason and not a test message check for data collection being disabled.
@@ -224,9 +224,9 @@ private void logActionNotTaken(String action) {
224224
}
225225

226226
private Completable logToImpressionStore() {
227-
Logging.logd("Attempting to record: message impression in impression store");
228-
String campaignId = inAppMessage.getCampaignId();
229-
227+
String campaignId = inAppMessage.getCampaignMetadata().getCampaignId();
228+
Logging.logd(
229+
"Attempting to record message impression in impression store for id: " + campaignId);
230230
Completable storeCampaignImpression =
231231
impressionStorageClient
232232
.storeImpression(

‎firebase-inappmessaging/src/main/java/com/google/firebase/inappmessaging/internal/ImpressionStorageClient.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package com.google.firebase.inappmessaging.internal;
1616

1717
import com.google.firebase.inappmessaging.internal.injection.qualifiers.ImpressionStore;
18+
import com.google.internal.firebase.inappmessaging.v1.CampaignProto;
1819
import com.google.internal.firebase.inappmessaging.v1.sdkserving.CampaignImpression;
1920
import com.google.internal.firebase.inappmessaging.v1.sdkserving.CampaignImpressionList;
2021
import io.reactivex.Completable;
@@ -84,7 +85,11 @@ private void clearInMemCache() {
8485
}
8586

8687
/** Returns {@code Single.just(true)} if the campaign has been impressed */
87-
public Single<Boolean> isImpressed(String campaignId) {
88+
public Single<Boolean> isImpressed(CampaignProto.ThickContent content) {
89+
String campaignId =
90+
content.getPayloadCase().equals(CampaignProto.ThickContent.PayloadCase.VANILLA_PAYLOAD)
91+
? content.getVanillaPayload().getCampaignId()
92+
: content.getExperimentalPayload().getCampaignId();
8893
return getAllImpressions()
8994
.map(CampaignImpressionList::getAlreadySeenCampaignsList)
9095
.flatMapObservable(Observable::fromIterable)

‎firebase-inappmessaging/src/main/java/com/google/firebase/inappmessaging/internal/InAppMessageStreamManager.java

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -186,19 +186,13 @@ public Flowable<TriggeredInAppMessage> createFirebaseInAppMessageStream() {
186186
content.getIsTestCampaign()
187187
? Maybe.just(content)
188188
: impressionStorageClient
189-
.isImpressed(content.getVanillaPayload().getCampaignId())
189+
.isImpressed(content)
190190
.doOnError(
191191
e ->
192192
Logging.logw("Impression store read fail: " + e.getMessage()))
193193
.onErrorResumeNext(
194194
Single.just(false)) // Absorb impression read errors
195-
.doOnSuccess(
196-
isImpressed ->
197-
Logging.logi(
198-
String.format(
199-
"Already impressed %s ? : %s",
200-
content.getVanillaPayload().getCampaignName(),
201-
isImpressed)))
195+
.doOnSuccess(isImpressed -> logImpressionStatus(content, isImpressed))
202196
.filter(isImpressed -> !isImpressed)
203197
.map(isImpressed -> content);
204198

@@ -288,6 +282,20 @@ private Maybe<ThickContent> getContentIfNotRateLimited(String event, ThickConten
288282
return Maybe.just(content);
289283
}
290284

285+
private static void logImpressionStatus(ThickContent content, Boolean isImpressed) {
286+
if (content.getPayloadCase().equals(ThickContent.PayloadCase.VANILLA_PAYLOAD)) {
287+
Logging.logi(
288+
String.format(
289+
"Already impressed campaign %s ? : %s",
290+
content.getVanillaPayload().getCampaignName(), isImpressed));
291+
} else if (content.getPayloadCase().equals(ThickContent.PayloadCase.EXPERIMENTAL_PAYLOAD)) {
292+
Logging.logi(
293+
String.format(
294+
"Already impressed experiment %s ? : %s",
295+
content.getExperimentalPayload().getCampaignName(), isImpressed));
296+
}
297+
}
298+
291299
private Maybe<TriggeredInAppMessage> getTriggeredInAppMessageMaybe(
292300
String event,
293301
Function<ThickContent, Maybe<ThickContent>> filterAlreadyImpressed,

‎firebase-inappmessaging/src/main/java/com/google/firebase/inappmessaging/model/ProtoMarshallerClient.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package com.google.firebase.inappmessaging.model;
1616

1717
import android.text.TextUtils;
18+
import androidx.annotation.NonNull;
1819
import com.google.common.base.Preconditions;
1920
import com.google.firebase.inappmessaging.MessagesProto;
2021
import com.google.firebase.inappmessaging.internal.Logging;
@@ -159,7 +160,7 @@ private static Button decode(MessagesProto.Button in) {
159160
private static Action decode(MessagesProto.Action protoAction, MessagesProto.Button protoButton) {
160161

161162
Action.Builder builder = decode(protoAction);
162-
if (protoButton != MessagesProto.Button.getDefaultInstance()) {
163+
if (!protoButton.equals(MessagesProto.Button.getDefaultInstance())) {
163164
Button.Builder buttonBuilder = Button.builder();
164165
if (!TextUtils.isEmpty(protoButton.getButtonHexColor())) {
165166
buttonBuilder.setButtonHexColor(protoButton.getButtonHexColor());
@@ -207,11 +208,14 @@ private static Text decode(MessagesProto.Text in) {
207208
/** Tranform {@link MessagesProto.Content} proto to an {@link InAppMessage} value object */
208209
public static InAppMessage decode(
209210
@Nonnull MessagesProto.Content in,
210-
String campaignId,
211-
String campaignName,
211+
@NonNull String campaignId,
212+
@NonNull String campaignName,
212213
boolean isTestMessage,
213214
@Nullable Map<String, String> data) {
214215
Preconditions.checkNotNull(in, "FirebaseInAppMessaging content cannot be null.");
216+
Preconditions.checkNotNull(campaignId, "FirebaseInAppMessaging campaign id cannot be null.");
217+
Preconditions.checkNotNull(
218+
campaignName, "FirebaseInAppMessaging campaign name cannot be null.");
215219
Logging.logd("Decoding message: " + in.toString());
216220
CampaignMetadata campaignMetadata =
217221
new CampaignMetadata(campaignId, campaignName, isTestMessage);

‎firebase-inappmessaging/src/test/java/com/google/firebase/inappmessaging/internal/ImpressionStorageClientTest.java

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@
1515
package com.google.firebase.inappmessaging.internal;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.firebase.inappmessaging.testutil.TestData.CAMPAIGN_ID_STRING;
1819
import static org.mockito.Matchers.any;
1920
import static org.mockito.Mockito.verify;
2021
import static org.mockito.Mockito.when;
2122
import static org.mockito.MockitoAnnotations.initMocks;
2223

24+
import com.google.internal.firebase.inappmessaging.v1.CampaignProto;
25+
import com.google.internal.firebase.inappmessaging.v1.CampaignProto.ThickContent;
2326
import com.google.internal.firebase.inappmessaging.v1.sdkserving.CampaignImpression;
2427
import com.google.internal.firebase.inappmessaging.v1.sdkserving.CampaignImpressionList;
2528
import com.google.protobuf.Parser;
@@ -39,11 +42,32 @@
3942
@RunWith(RobolectricTestRunner.class)
4043
@Config(manifest = Config.NONE)
4144
public class ImpressionStorageClientTest {
42-
private static final String CAMPAIGN_ID = "campaign_id";
45+
private static final ThickContent vanillaCampaign =
46+
ThickContent.newBuilder()
47+
.setVanillaPayload(
48+
CampaignProto.VanillaCampaignPayload.newBuilder().setCampaignId(CAMPAIGN_ID_STRING))
49+
.build();
50+
private static final ThickContent experimentalCampaign =
51+
ThickContent.newBuilder()
52+
.setExperimentalPayload(
53+
CampaignProto.ExperimentalCampaignPayload.newBuilder()
54+
.setCampaignId(CAMPAIGN_ID_STRING + "2"))
55+
.build();
56+
4357
private static final CampaignImpression campaignImpression =
44-
CampaignImpression.newBuilder().setCampaignId(CAMPAIGN_ID).build();
58+
CampaignImpression.newBuilder()
59+
.setCampaignId(vanillaCampaign.getVanillaPayload().getCampaignId())
60+
.build();
4561
private static final CampaignImpressionList campaignImpressionList =
4662
CampaignImpressionList.newBuilder().addAlreadySeenCampaigns(campaignImpression).build();
63+
64+
private static final CampaignImpression experimentImpression =
65+
CampaignImpression.newBuilder()
66+
.setCampaignId(experimentalCampaign.getExperimentalPayload().getCampaignId())
67+
.build();
68+
private static final CampaignImpressionList experimentImpressionList =
69+
CampaignImpressionList.newBuilder().addAlreadySeenCampaigns(experimentImpression).build();
70+
4771
@Mock private ProtoStorageClient storageClient;
4872
private ImpressionStorageClient impressionStorageClient;
4973
private Completable fakeWrite;
@@ -58,8 +82,8 @@ private static List<Object> getPlainValues(TestSubscriber<CampaignImpressionList
5882
public void setup() throws IOException {
5983
initMocks(this);
6084
impressionStorageClient = new ImpressionStorageClient(storageClient);
61-
6285
fakeRead = Maybe.fromCallable(() -> campaignImpressionList);
86+
wasWritten = false;
6387
fakeWrite =
6488
Completable.fromCallable(
6589
() -> {
@@ -195,15 +219,40 @@ public void getAllImpressions_readError_notifiesError() {
195219
@Test
196220
public void isImpressed_ifCampaignImpressed_isTrue() {
197221
TestSubscriber<Boolean> subscriber =
198-
impressionStorageClient.isImpressed(CAMPAIGN_ID).toFlowable().test();
222+
impressionStorageClient.isImpressed(vanillaCampaign).toFlowable().test();
199223

200224
assertThat(subscriber.getEvents().get(0)).containsExactly(true);
201225
}
202226

203227
@Test
204228
public void isImpressed_ifCampaignNotImpressed_isFalse() {
205229
TestSubscriber<Boolean> subscriber =
206-
impressionStorageClient.isImpressed("some_other_campaign_id").toFlowable().test();
230+
impressionStorageClient
231+
.isImpressed(
232+
ThickContent.newBuilder()
233+
.setVanillaPayload(
234+
CampaignProto.VanillaCampaignPayload.newBuilder().setCampaignId("WALRUS"))
235+
.build())
236+
.toFlowable()
237+
.test();
238+
239+
assertThat(subscriber.getEvents().get(0)).containsExactly(false);
240+
}
241+
242+
@Test
243+
public void isImpressed_ifExperimentImpressed_isTrue() {
244+
when(storageClient.read(any(CampaignImpressionsParser.class)))
245+
.thenReturn(Maybe.fromCallable(() -> experimentImpressionList));
246+
TestSubscriber<Boolean> subscriber =
247+
impressionStorageClient.isImpressed(experimentalCampaign).toFlowable().test();
248+
249+
assertThat(subscriber.getEvents().get(0)).containsExactly(true);
250+
}
251+
252+
@Test
253+
public void isImpressed_ifExperimentNotImpressed_isFalse() {
254+
TestSubscriber<Boolean> subscriber =
255+
impressionStorageClient.isImpressed(experimentalCampaign).toFlowable().test();
207256

208257
assertThat(subscriber.getEvents().get(0)).containsExactly(false);
209258
}
@@ -213,7 +262,7 @@ public void isImpressed_ifNoCampaigns_isFalse() {
213262
when(storageClient.read(any(CampaignImpressionsParser.class))).thenReturn(Maybe.empty());
214263

215264
TestSubscriber<Boolean> subscriber =
216-
impressionStorageClient.isImpressed(CAMPAIGN_ID).toFlowable().test();
265+
impressionStorageClient.isImpressed(vanillaCampaign).toFlowable().test();
217266

218267
assertThat(subscriber.getEvents().get(0)).containsExactly(false);
219268
}
@@ -224,7 +273,7 @@ public void isImpressed_onError_notifiesError() {
224273
.thenReturn(Maybe.error(new IOException()));
225274

226275
TestSubscriber<Boolean> subscriber =
227-
impressionStorageClient.isImpressed(CAMPAIGN_ID).toFlowable().test();
276+
impressionStorageClient.isImpressed(vanillaCampaign).toFlowable().test();
228277

229278
subscriber.assertError(IOException.class);
230279
}

‎firebase-inappmessaging/src/test/java/com/google/firebase/inappmessaging/internal/InAppMessageStreamManagerTest.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import static io.reactivex.BackpressureStrategy.BUFFER;
2727
import static io.reactivex.schedulers.Schedulers.trampoline;
2828
import static org.mockito.Matchers.any;
29-
import static org.mockito.Matchers.anyString;
3029
import static org.mockito.Mockito.never;
3130
import static org.mockito.Mockito.times;
3231
import static org.mockito.Mockito.verify;
@@ -202,7 +201,8 @@ public void setup() {
202201
when(campaignCacheClient.get()).thenReturn(Maybe.empty());
203202
when(campaignCacheClient.put(any(FetchEligibleCampaignsResponse.class)))
204203
.thenReturn(Completable.complete());
205-
when(impressionStorageClient.isImpressed(anyString())).thenReturn(Single.just(false));
204+
when(impressionStorageClient.isImpressed(any(ThickContent.class)))
205+
.thenReturn(Single.just(false));
206206
when(impressionStorageClient.getAllImpressions()).thenReturn(Maybe.just(CAMPAIGN_IMPRESSIONS));
207207
}
208208

@@ -560,7 +560,8 @@ public void stream_onCacheWriteFailure_AbsorbsError() {
560560

561561
@Test
562562
public void stream_whenCampaignImpressed_filtersCampaign() {
563-
when(impressionStorageClient.isImpressed(anyString())).thenReturn(Single.just(true));
563+
when(impressionStorageClient.isImpressed(any(ThickContent.class)))
564+
.thenReturn(Single.just(true));
564565
when(mockApiClient.getFiams(CAMPAIGN_IMPRESSIONS)).thenReturn(campaignsResponse);
565566

566567
appForegroundEmitter.onNext(ON_FOREGROUND_EVENT_NAME);
@@ -570,7 +571,7 @@ public void stream_whenCampaignImpressed_filtersCampaign() {
570571

571572
@Test
572573
public void stream_whenCampaignImpressionStoreFails_doesNotFilterCampaign() {
573-
when(impressionStorageClient.isImpressed(anyString()))
574+
when(impressionStorageClient.isImpressed(any(ThickContent.class)))
574575
.thenReturn(Single.error(new Exception("e1")));
575576
when(mockApiClient.getFiams(CAMPAIGN_IMPRESSIONS)).thenReturn(campaignsResponse);
576577

@@ -581,7 +582,7 @@ public void stream_whenCampaignImpressionStoreFails_doesNotFilterCampaign() {
581582

582583
@Test
583584
public void stream_whenCampaignImpressionStoreFail_doesNotFilterCampaign() {
584-
when(impressionStorageClient.isImpressed(anyString()))
585+
when(impressionStorageClient.isImpressed(any(ThickContent.class)))
585586
.thenReturn(Single.error(new Exception("e1")));
586587
when(mockApiClient.getFiams(CAMPAIGN_IMPRESSIONS)).thenReturn(campaignsResponse);
587588

0 commit comments

Comments
 (0)