Require UserId for MapLoadEvent #41
Conversation
There was a problem hiding this comment.
I don't know if this PR is ready for review because it's labeled as in progress but a review was requested.
Anyways, some details to take into account before merging it.
We should also include some unit tests around userId now being a required field for map load events.
| @@ -234,6 +235,12 @@ private void checkLoad(Event.Type type) { | |||
| } | |||
| } | |||
|
|
|||
| private void checkUserId(String userId) { | |||
| if (userId == null && userId.isEmpty()) { | |||
There was a problem hiding this comment.
This would cause a NullPointerException 💥 if userId is null.
Method invocation 'isEmpty' may produce 'java.lang.NullPointerException'
The IDE hints you about this potential issue/crash.
There was a problem hiding this comment.
IllegalArgumentException should be thrown in two situations:
- If
userIdisnull - If
userIdis notnullbut it's empty
Why not using TelemetryUtils#isEmpty?
| @@ -29,7 +29,7 @@ public void checksMapLoadEvent() throws Exception { | |||
| initializeMapboxTelemetry(); | |||
| MapEventFactory aMapEventFactory = new MapEventFactory(); | |||
|
|
|||
| Event mapLoadEvent = aMapEventFactory.createMapLoadEvent(Event.Type.MAP_LOAD); | |||
| Event mapLoadEvent = aMapEventFactory.createMapLoadEvent(Event.Type.MAP_LOAD, "aUserId"); | |||
There was a problem hiding this comment.
This is actually a valid user id.
The same thing happens in the tests below 👇
We should add the missing not valid tests.
| public void checksLoadEmptyUserId() throws Exception { | ||
| initializeMapboxTelemetry(); | ||
| MapEventFactory aMapEventFactory = new MapEventFactory(); | ||
| Event.Type notALoadMapType = Event.Type.MAP_CLICK; |
There was a problem hiding this comment.
This test is passing but it's not testing that when creating a map load event (Event.Type.MAP_LOAD) with an empty user id it throws an IllegalArgumentException.
It's actually throwing an IllegalArgumentException because it's not a load map type (Event.Type.MAP_CLICK) which is already tested.
| public void checksLoadNullUserId() throws Exception { | ||
| initializeMapboxTelemetry(); | ||
| MapEventFactory aMapEventFactory = new MapEventFactory(); | ||
| Event.Type notALoadMapType = Event.Type.MAP_CLICK; |
There was a problem hiding this comment.
This test is passing but it's not testing that when creating a map load event (Event.Type.MAP_LOAD) with an null user id it throws an IllegalArgumentException.
It's actually throwing an IllegalArgumentException because it's not a load map type (Event.Type.MAP_CLICK) which is already tested.
| @@ -113,7 +113,7 @@ private Event obtainLoadEvent() { | |||
| when(mockedContext.getSystemService(Context.WINDOW_SERVICE)).thenReturn(mockedWindowManager); | |||
| initializeMapboxTelemetry(mockedContext); | |||
| MapEventFactory mapEventFactory = new MapEventFactory(); | |||
| Event loadEvent = mapEventFactory.createMapLoadEvent(Event.Type.MAP_LOAD); | |||
| Event loadEvent = mapEventFactory.createMapLoadEvent(Event.Type.MAP_LOAD, "aUserId"); | |||
There was a problem hiding this comment.
This should have been rebased from master instead of merged.
Could you do that? Most changes come from #42 (already reviewed and merged) making hard this PR's revision.
electrostat
force-pushed
the
aa-userid-meets-spec
branch
from
1dd0b89 to
ab63351
Compare
- require a String userId when creating maploadevent - add excemption for invalid userId - update and tweak tests
electrostat
force-pushed
the
aa-userid-meets-spec
branch
from
ab63351 to
3a6ae6d
Compare
Spec changed to require UserId for MapLoadEvent