Skip to content

Commit

Permalink
ScreenView do not include previous screen state (close #363)
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexBenny committed Mar 25, 2020
1 parent 8d66ef8 commit b4627df
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -277,15 +277,26 @@ public void testTrackScreenView() throws Exception {
Map<String, Object> screenStateMap = (Map<String, Object>)screenStateMapWrapper.get(Parameters.DATA);
assertEquals("Unknown", screenStateMap.get(Parameters.SCREEN_NAME));

// Send screenView
ScreenView screenView = ScreenView.builder().name("screen1").build();
String screenId = (String)screenView.getData().getMap().get("id");

tracker.track(screenView);

screenStateMapWrapper = screenState.getCurrentScreen(true).getMap();
screenStateMap = (Map<String, Object>)screenStateMapWrapper.get(Parameters.DATA);
assertEquals("screen1", screenStateMap.get(Parameters.SCREEN_NAME));
assertEquals(screenId, screenStateMap.get(Parameters.SCREEN_ID));

// Send another screenView
screenView = ScreenView.builder().name("screen2").build();
String screenId1 = (String)screenView.getData().getMap().get("id");
tracker.track(screenView);

Map<String, Object> payload = (Map<String, Object>)screenView.getPayload().getMap().get("data");
assertEquals("screen2", payload.get(Parameters.SCREEN_NAME));
assertEquals(screenId1, payload.get(Parameters.SCREEN_ID));
assertEquals("screen1", payload.get(Parameters.SV_PREVIOUS_NAME));
assertEquals(screenId, payload.get(Parameters.SV_PREVIOUS_ID));
}

public void testTrackUncaughtException() throws InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.snowplowanalytics.snowplow;

This comment has been minimized.

Copy link
@paulboocock

paulboocock Mar 25, 2020

Contributor

Shouldn't this file be in a folder? It's at the snowplow root folder at the moment. Probably should be in the events folder?

This comment has been minimized.

Copy link
@AlexBenny

AlexBenny Mar 25, 2020

Author Contributor

It's definitely the wrong place. I'm going to remove the interface as it's quite pointless.
Due to the structure of the packages I can't use it to make these methods package-private. They end up to be public anyway so I just remove the interface and I let the methods be just public methods of ScreenState.


public interface ScreenStatePreviousFields {
String getPreviousId();
String getPreviousName();
String getPreviousType();
}
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,18 @@ protected ScreenView(Builder<?> builder) {
/**
* Update the passed screen state with the data related to
* the current ScreenView event.
* @apiNote ScreenState updates back the previous screen fields
* in the ScreenView event (if `previousId` not already set).
* @param screenState The screen state to update.
*/
public void updateScreenState(ScreenState screenState) {
public synchronized void updateScreenState(ScreenState screenState) {

This comment has been minimized.

Copy link
@paulboocock

paulboocock Mar 25, 2020

Contributor

I see that we're adding the previous screen information when sending a screen_view event but I don't see the previous screen information when we are adding the screen_view as a context.

I think this same logic should be added to ScreenState.java in SelfDescribingJson getCurrentScreen(Boolean debug): https://github.com/snowplow/snowplow-android-tracker/blob/master/snowplow-tracker/src/main/java/com/snowplowanalytics/snowplow/tracker/tracker/ScreenState.java#L59

This will ensure the previous information is also included on the screen_view context when setting screenContext to true as well as when setting screenviewEvents to true.

This comment has been minimized.

Copy link
@AlexBenny

AlexBenny Mar 25, 2020

Author Contributor

The screen context doesn't use the screen_view schema, it uses the screen schema: https://github.com/snowplow/iglu-central/blob/5d2cd055d8a7a1cafc84b3707a65a9d6291d6dcc/schemas/com.snowplowanalytics.mobile/screen/jsonschema/1-0-0
It doesn't involve the previous state in its fields and the schema specify "additionalProperties": false.
So I think we can't add previous fields there. But let me know if I missed something.

This comment has been minimized.

Copy link
@paulboocock

paulboocock Mar 25, 2020

Contributor

Yep, my bad!
I thought it was the same schema being used as an entity context and as an event.
Now I see one is screen_view and the other is screen.
Ignore this comment and proceed :)

if (screenState == null) return;
screenState.updateScreenState(id, name, type, transitionType, fragmentClassName, fragmentTag, activityClassName, activityTag);
if (previousId == null) {
previousId = screenState.getPreviousId();
previousName = screenState.getPreviousName();
previousType = screenState.getPreviousType();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package com.snowplowanalytics.snowplow.tracker.tracker;

import com.snowplowanalytics.snowplow.ScreenStatePreviousFields;
import com.snowplowanalytics.snowplow.tracker.constants.Parameters;
import com.snowplowanalytics.snowplow.tracker.constants.TrackerConstants;
import com.snowplowanalytics.snowplow.tracker.payload.SelfDescribingJson;
import com.snowplowanalytics.snowplow.tracker.payload.TrackerPayload;
import com.snowplowanalytics.snowplow.tracker.utils.Util;


public class ScreenState {
public class ScreenState implements ScreenStatePreviousFields {
private String name;
private String type;
private String id;
Expand Down Expand Up @@ -109,4 +109,20 @@ private String getValidName(String s1, String s2) {
return null;
}

// Implement ScreenStatePreviousFields

@Override
public String getPreviousId() {
return previousId;
}

@Override
public String getPreviousName() {
return previousName;
}

@Override
public String getPreviousType() {
return previousType;
}
}

0 comments on commit b4627df

Please sign in to comment.