Support Multiple User Agents #19

electrostat · 2017-11-29T22:16:13Z

Use a public facing api to update the user agent for telemetry events. Validate this user agent and pass along to the Telemetry Client for sending along with events.

- create public facing call to retrieve user agent - validate given useragent - set new user agent within telemetry client for sending along with events data

Guardiola31337

We should validate both the accessToken and the userAgent when creating an instance of MapboxTelemetry and throw an error if any of them are not valid 👀
https://github.com/mapbox/mapbox-java/blob/3a4afbd88668224303360278ef927f26cd0f91ed/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L163-L168

We should also manage the generation of a full user agent within the library (TelemetryClient sounds like a good place to include that functionality) 👀
https://github.com/mapbox/mapbox-java/blob/3a4afbd88668224303360278ef927f26cd0f91ed/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L313-L316

Guardiola31337 · 2017-12-01T15:44:16Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryClient.java

@@ -35,6 +35,10 @@ public TelemetryClient(String accessToken, String userAgent, TelemetryClientSett
    this.logger = logger;
  }

+  void setUserAgent(String userAgent) {


get/set nomenclature is hijacked by the Java language and should be only used when defining POJOs/DTOs. What about updateUserAgent instead?

Guardiola31337 · 2017-12-01T15:46:04Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -277,4 +289,21 @@ public void onServiceDisconnected(ComponentName className) {
      serviceBound = false;
    }
  };
+
+  public void newUserAgent(String userAgent) {


NIT updateUserAgent sounds better IMO.

- rename methods - TelemetryException - full user agent creation

- update tests to handle method changes

Guardiola31337

Looking great.

Some minor details.
We should also include some unit tests around the new user agent behavior/functionality.

Guardiola31337 · 2017-12-05T11:07:39Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -124,6 +136,12 @@ IntentFilter obtainEventReceiverIntentFilter() {
  }

  private void initializeTelemetryClient(String accessToken, String userAgent) {
+    if (TextUtils.isEmpty(accessToken) || TextUtils.isEmpty(userAgent)) {


We're already checking ☝️ (👀 isTelemetryClientInitialized()). What about fixing isTelemetryClientInitialized() check?

Guardiola31337 · 2017-12-05T11:07:48Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -124,6 +136,12 @@ IntentFilter obtainEventReceiverIntentFilter() {
  }

  private void initializeTelemetryClient(String accessToken, String userAgent) {
+    if (TextUtils.isEmpty(accessToken) || TextUtils.isEmpty(userAgent)) {
+      throw new TelemetryException(
+        "Please, make sure you provide a valid access token and user agent."


NIT Magic number

Guardiola31337 · 2017-12-05T11:12:16Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryClient.java

+  private String createFullUserAgent(String userAgent) {
+    String appIdentifier = TelemetryUtils.getApplicationIdentifier(context);
+    String fullUserAgent = TextUtils.isEmpty(appIdentifier) ? userAgent : Util.toHumanReadableAscii(
+      String.format(TelemetryUtils.DEFAULT_LOCALE, "%s %s", appIdentifier, userAgent));


NIT Magic number

Guardiola31337 · 2017-12-05T11:12:41Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

+  public static String getApplicationIdentifier(Context context) {
+    try {
+      PackageInfo packageInfo = context.getPackageManager().getPackageInfo(context.getPackageName(), 0);
+      return String.format(DEFAULT_LOCALE, "%s/%s/%s", context.getPackageName(),


NIT Magic numbers

Guardiola31337 · 2017-12-05T11:22:00Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryClient.java

@@ -26,13 +30,20 @@
  private String userAgent = null;
  private final TelemetryClientSettings setting;
  private final Logger logger;
+  private Context context;


#19 (review)

TelemetryClient sounds like a good place to include that functionality

If we need to pass in a new dependency (Context), I'd rather leave it as it was and expose a method (createFullUserAgent) within TelemetryUtils. Sorry if I didn't make myself clear.

Guardiola31337 · 2017-12-05T11:22:49Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

@@ -21,4 +25,14 @@ static String obtainUniversalUniqueIdentifier() {
    String universalUniqueIdentifier = UUID.randomUUID().toString();
    return universalUniqueIdentifier;
  }
+
+  public static String getApplicationIdentifier(Context context) {


What about exposing a createFullUserAgent method instead? ☝️

get/set nomenclature is hijacked by the Java language and should be only used when defining POJOs/DTOs.

- cleaned up magic numbers - moved create full user agent from TelemetryClient -> TelemetryUtils - updated tests

- changed check and exception throw

- fix missing userAgent for tests to pass

- fix checkstyle error

Guardiola31337

Because we're now throwing an exception if the user passes in not valid inputs (accessToken and userAgent) I guess that

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

Line 201 in 0467990

if (isTelemetryClientInitialized()) {

check is not needed anymore because at the time of calling sendEvents both accessToken and userAgent must be valid (check is done at MapboxTelemetry creation time) so it should be avoided/removed.

We should also include some unit tests around the "failing" cases (i.e. accessToken equals null or empty, userAgent equals null or empty, MapboxTelemetryException, etc.).

Guardiola31337 · 2017-12-06T11:01:52Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

 import java.util.List;

 import okhttp3.Callback;

 import static com.mapbox.services.android.telemetry.EventReceiver.EVENT_RECEIVER_INTENT;

 public class MapboxTelemetry implements FullQueueCallback, EventCallback {
+  private final String ACCESS_TOKEN_USER_AGENT_EXCEPTION = "Please, make sure you provide a valid access token and user"


If ACCESS_TOKEN_USER_AGENT_EXCEPTION is a constant, we're missing the static modifier here.

Guardiola31337 · 2017-12-06T11:26:57Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    if (isTelemetryClientInitialized()) {
      telemetryClient = createTelemetryClient(accessToken, userAgent);
      queue.setTelemetryInitialized(true);
+    } else {
+      throw new TelemetryException(ACCESS_TOKEN_USER_AGENT_EXCEPTION);


Why not using a guard clause instead?
Meaning throwing TelemetryException in the exceptional case (i.e. the telemetry client can't be initialized because either accessToken or userAgent are not valid) and removing the else clause (we don't need it because throwing an exception breaks the execution). IMO it makes clearer the intention of the method.

Guardiola31337 · 2017-12-06T11:38:58Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+  public void updateUserAgent(String userAgent) {
+    if (isUserAgentValid(userAgent)) {
+      TelemetryUtils.createFullUserAgent(userAgent, context);


This is currently doing nothing.

In any case, we should also update the telemetry client with the new userAgent so everything behaves properly.

Guardiola31337 · 2017-12-06T11:41:16Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    }
  }

  private boolean isTelemetryClientInitialized() {
-    return accessToken != null && !accessToken.isEmpty();
+    return accessToken != null && !accessToken.isEmpty() && !userAgent.isEmpty() && userAgent != null;


!userAgent.isEmpty() would cause a NullPointerException 💥 if userAgent is null (order matters).

Guardiola31337 · 2017-12-06T11:52:58Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

  public MapboxTelemetry(Context context, String accessToken, String userAgent, Callback httpCallback) {
    this.context = context;
    this.accessToken = accessToken;
+    this.userAgent = userAgent;


We should break the execution as soon as the user passes in not valid inputs (accessToken and userAgent).
In order to avoid assigning/creating objects that might not be used, the check should be done at the very beginning whereas the assignment/creating after (because of that it might be interesting to pass in both accessToken and userAgent to the method in charge of doing that check as it was implemented before).

Guardiola31337 · 2017-12-06T12:05:50Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

 class TelemetryUtils {
  private static final String DATE_AND_TIME_PATTERN = "yyyy-MM-dd'T'HH:mm:ss.SSSZ";
  private static final SimpleDateFormat dateFormat = new SimpleDateFormat(DATE_AND_TIME_PATTERN, Locale.US);
+  public static final Locale DEFAULT_LOCALE = Locale.US;


Why DEFAULT_LOCALE needs to be public?

Guardiola31337 · 2017-12-06T12:07:28Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

+    try {
+      String packageName = context.getPackageName();
+      PackageInfo packageInfo = context.getPackageManager().getPackageInfo(packageName, 0);
+      String appIdentifier = String.format(DEFAULT_LOCALE, "%s/%s/%s", packageName,


NIT Magic number (Strings are also literals)
Further information: Replace Magic Number with Symbolic Constant.

Guardiola31337 · 2017-12-06T12:07:44Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

+
+      return appIdentifier;
+    } catch (Exception exception) {
+      return "";


NIT Magic number

Guardiola31337 · 2017-12-06T12:07:55Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryUtils.java

+
+  public static String createFullUserAgent(String userAgent, Context context) {
+    String appIdentifier = TelemetryUtils.obtainApplicationIdentifier(context);
+    String newUserAgent = Util.toHumanReadableAscii(String.format(DEFAULT_LOCALE, "%s %s", appIdentifier, userAgent));


NIT Magic number

Guardiola31337 · 2017-12-06T12:11:21Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

    EventsQueue mockedEventsQueue = mock(EventsQueue.class);
    TelemetryClient mockedTelemetryClient = mock(TelemetryClient.class);
    Callback mockedHttpCallback = mock(Callback.class);
    SchedulerFlusher mockedSchedulerFlusher = mock(SchedulerFlusher.class);
    Clock mockedClock = mock(Clock.class);
    LocalBroadcastManager mockedLocalBroadcastManager = mock(LocalBroadcastManager.class);
-    MapboxTelemetry theMapboxTelemetry = new MapboxTelemetry(mockedContext, aValidAccessToken, mockedEventsQueue,
-      mockedTelemetryClient, mockedHttpCallback, mockedSchedulerFlusher, mockedClock, mockedLocalBroadcastManager);
+    MapboxTelemetry theMapboxTelemetry = new MapboxTelemetry(mockedContext, aValidAccessToken, aValidAccessToken,


👀 we're using aValidAccessToken as a user agent instead of aValidUserAgent.

- renamed and reworked isAccessTokenAndUserAgentValid() - set telemetry client user agent when updated - magic numbers - cleaned up and added tests - removed redundant tests (throws exception before test runs)

Guardiola31337 · 2017-12-07T17:50:11Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

  public MapboxTelemetry(Context context, String accessToken, String userAgent, Callback httpCallback) {
+    isAccessTokenAndUserAgentValid(accessToken, userAgent);


This is checked twice, here and then within initializeTelemetryClient() 👇

Guardiola31337 · 2017-12-07T17:51:29Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

-  private boolean isTelemetryClientInitialized() {
-    return accessToken != null && !accessToken.isEmpty();
+  boolean isAccessTokenAndUserAgentValid(String accessToken, String userAgent) throws TelemetryException {
+    if (isAccessToeknValid(accessToken) && isUserAgentValid(userAgent)) {


NIT Typo isAccessToeknValid

Guardiola31337 · 2017-12-07T17:52:48Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+  }
+
+  private boolean isAccessToeknValid(String accessToken) {
+    return accessToken != null && !TextUtils.isEmpty(accessToken);


If we're going to use TextUtils.isEmpty, accessToken != null is not needed.

Guardiola31337 · 2017-12-07T18:06:42Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryClient.java

@@ -35,6 +34,10 @@ public TelemetryClient(String accessToken, String userAgent, TelemetryClientSett
    this.logger = logger;
  }

+  public void setUserAgent(String userAgent) {


Why does this need to be public?

get/set nomenclature is hijacked by the Java language and should be only used when defining POJOs/DTOs.

Guardiola31337 · 2017-12-07T18:16:31Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+  public void updateUserAgent(String userAgent) {
+    if (isUserAgentValid(userAgent)) {
+      telemetryClient.setUserAgent(TelemetryUtils.createFullUserAgent(userAgent, context));


We're not updating userAgent.

Guardiola31337 · 2017-12-07T18:21:48Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

  }

-  private boolean isTelemetryClientInitialized() {
-    return accessToken != null && !accessToken.isEmpty();
+  boolean isAccessTokenAndUserAgentValid(String accessToken, String userAgent) throws TelemetryException {


Why does throws TelemetryException is needed?

- rework accesstoken + useragent check - method order - reinstate check for sending events

- update tests - create updateUserAgent test - fixed bug with createFullUserAgent regarding spacing being off

- fixed 2 tests that are broken with exception refactor

- additional user agent tests - rework: checkRequiredParameters, isAccessTokenValid, isUserAgentValid, - bign back queue tests - cleanup code

Guardiola31337

Minor comments before merging this PR.

Guardiola31337 · 2017-12-11T12:09:49Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryException.java

+ * A {@code TelemetryException} is thrown by when MapboxTelemetry checks and finds that
+ * telemetry has not been properly configured.
+ */
+public class TelemetryException extends RuntimeException {


Class 'TelemetryException' is never used

Should be removed.

Guardiola31337 · 2017-12-11T12:12:28Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

  }

-  private boolean isTelemetryClientInitialized() {
-    return accessToken != null && !accessToken.isEmpty();
+  private void initializeTelemetryClient() {


We should revisit this because we could take advantage of the checkRequiredParameters check to not create the telemetry client and only do that if both the accessToken and the userAgent are valid so we don’t assign memory if not needed but carefully because we should still be calling queue.setTelemetryInitialized(true) in order to keep queuing events although we don’t send them.

Guardiola31337 · 2017-12-11T12:16:51Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -277,4 +299,25 @@ public void onServiceDisconnected(ComponentName className) {
      serviceBound = false;
    }
  };
+
+  private boolean isUserAgentValid(String userAgent) {
+    if (userAgent != null && !TextUtils.isEmpty(userAgent)) {


If we’re going to use TextUtils.isEmpty the null check is not needed, did you 👀 what TextUtils.isEmpty does internally?
If you check the implementation the only thing is doing is blablah != null && !blablah.isEmpty(), so I’d rather remove TextUtils (Android class that could affect Unit tests) and manage it by ourselves.

Guardiola31337 · 2017-12-11T12:17:05Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+  }
+
+  private boolean isAccessTokenValid(String accessToken) {
+    if (accessToken != null && !TextUtils.isEmpty(accessToken)) {


Same here ☝️

Guardiola31337 · 2017-12-11T12:22:24Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

@@ -309,6 +337,7 @@ public void checksFlusherUnregisteringWhenDisabled() throws Exception {
  public void checksOnFullQueueSendEventsNotCalledWhenNullTelemetryClient() throws Exception {
    Context mockedContext = mock(Context.class, RETURNS_DEEP_STUBS);
    String nullAccessToken = null;
+    String aValidUserAgent = null;


This is not a valid user agent.

Guardiola31337 · 2017-12-11T12:42:29Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String testUserAgent = "mapbox-navigation-android/";


Same comments as checksUserAgentTelemetryAndroid test.

Guardiola31337 · 2017-12-11T12:42:59Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String testUserAgent = "mapbox-navigation-ui-android/";


Same comments as checksUserAgentTelemetryAndroid test.

Guardiola31337 · 2017-12-11T12:43:33Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String testUserAgent = "MapboxEventsAndroid/";


Same comments as checksUserAgentTelemetryAndroid test.

Guardiola31337 · 2017-12-11T12:43:45Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String aInvalidUserAgent = "invalidUserAgent";


Same comments as checksUserAgentTelemetryAndroid test.

Why not using aValidUserAgent? aValidUserAgent is redundant. We should fix the name if we want to remark something around the access token variable. aInvalidUserAgent is 👍

Guardiola31337 · 2017-12-11T12:44:00Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String aNullUserAgent = null;


Same comments as checksUserAgentTelemetryAndroid test.

Why not using aValidUserAgent? aValidUserAgent is redundant. We should fix the name if we want to remark something around the access token variable. aNullUserAgent is 👍

- tweaked tests - created internal isEmpty() method to fix test failures - prevent telem client creation, if 'checkRequiredParameters' fails.

Guardiola31337

Minor minor comments and this PR should be good to go 🚀

Guardiola31337 · 2017-12-12T15:20:03Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

 import java.util.List;

 import okhttp3.Callback;

 import static com.mapbox.services.android.telemetry.EventReceiver.EVENT_RECEIVER_INTENT;

 public class MapboxTelemetry implements FullQueueCallback, EventCallback {
+  private static final String ACCESS_TOKEN_USER_AGENT_EXCEPTION = "Please, make sure you provide a valid access token"


Private field 'ACCESS_TOKEN_USER_AGENT_EXCEPTION' is never used

We should remove it.

Guardiola31337 · 2017-12-12T15:21:56Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/TelemetryException.java

+ * A {@code TelemetryException} is thrown by when MapboxTelemetry checks and finds that
+ * telemetry has not been properly configured.
+ */
+public class TelemetryException extends RuntimeException {


Should be removed.

Guardiola31337 · 2017-12-12T15:49:08Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

  }

  private TelemetryClient createTelemetryClient(String accessToken, String userAgent) {
+    if (!checkRequiredParameters(accessToken, userAgent)) {


We're doing the same exact check before 👀

mapbox-events-android/libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

Line 54 in 770b553

checkRequiredParameters(accessToken, userAgent);

What about extracting queue.setTelemetryInitialized(true) from initializeTelemetryClient() so we decide if creating the telemetryClient or not at construction time so we could remove this check from here and avoid to return null (which is generally a bad idea)?

Guardiola31337 · 2017-12-12T16:01:53Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    boolean accessTokentBool = theMapboxTelemetry.checkRequiredParameters(aValidAccessToken, aValidUserAgent);


👀 #19 (comment)
What about validRequiredParameters? The name of the test is also giving us a hint.

Guardiola31337 · 2017-12-12T16:02:32Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String nullAccessToken = null;


👀 #19 (comment)

Guardiola31337 · 2017-12-12T16:07:47Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    boolean checkRequiredParameters = theMapboxTelemetry.checkRequiredParameters(aValidAccessToken, aValidUserAgent);


Same comment as in checksValidAccessTokenValidUserAgent test. The name of the test is also giving us a hint.

Guardiola31337 · 2017-12-12T16:09:59Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    boolean checkRequiredParameters = theMapboxTelemetry.checkRequiredParameters(aValidAccessToken, aValidUserAgent);


Same comment as in checksValidAccessTokenValidUserAgent test. The name of the test is also giving us a hint.

Guardiola31337 · 2017-12-12T16:10:13Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+    theMapboxTelemetry.enable();
+
+    String aInvalidUserAgent = "invalidUserAgent";
+    boolean checkRequiredParameters = theMapboxTelemetry.checkRequiredParameters(aValidAccessToken, aInvalidUserAgent);


Same comment as in checksValidAccessTokenValidUserAgent test. The name of the test is also giving us a hint.

Guardiola31337 · 2017-12-12T16:13:28Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String aInvalidUserAgent = "invalidUserAgent";


Why not using aValidUserAgent? aValidUserAgent is redundant. We should fix the name if we want to remark something around the access token variable. aInvalidUserAgent is 👍

Guardiola31337 · 2017-12-12T16:14:13Z

libtelemetry/src/test/java/com/mapbox/services/android/telemetry/MapboxTelemetryTest.java

+      mockedLocalBroadcastManager);
+    theMapboxTelemetry.enable();
+
+    String aNullUserAgent = null;


Why not using aValidUserAgent? aValidUserAgent is redundant. We should fix the name if we want to remark something around the access token variable. aNullUserAgent is 👍

- remove unneded exception - rework 'initializeTelemetryClient' and its implementation - update tests

- remove unused method - update tests - initialize queue

Guardiola31337

Apart from this NIT comment this PR should be good to 🚢

Well done!

Guardiola31337 · 2017-12-13T10:53:56Z

libtelemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

@@ -35,22 +38,41 @@
  private boolean isOpted = false;
  private boolean serviceBound = false;

+  private static final String EVENTS_USERAGENT = "MapboxEventsAndroid/";


NIT The IDE warns us that might be a typo in EVENTS_USERAGENT. Could we separate USERAGENT with an underscore into USER_AGENT (Java's constants standard)? Same for the rest of the constants 👇

- renamed constant (added underscore between compound word) to meet java standards

- removal of spaces added when merging

Accept and Update UserAgent

1574a02

- create public facing call to retrieve user agent - validate given useragent - set new user agent within telemetry client for sending along with events data

electrostat added enhancement in progress labels Nov 29, 2017

electrostat self-assigned this Nov 29, 2017

electrostat requested a review from Guardiola31337 November 29, 2017 23:09

electrostat added ready for review and removed in progress labels Nov 29, 2017

Guardiola31337 requested changes Dec 1, 2017

View reviewed changes

electrostat added 2 commits December 1, 2017 15:17

Exception + FullUserAgent

76027f5

- rename methods - TelemetryException - full user agent creation

Update Tests

1e8735c

- update tests to handle method changes

electrostat requested a review from Guardiola31337 December 1, 2017 21:12

Guardiola31337 requested changes Dec 5, 2017

View reviewed changes

electrostat added 4 commits December 5, 2017 16:51

FullUserAgent changes

efb606b

- cleaned up magic numbers - moved create full user agent from TelemetryClient -> TelemetryUtils - updated tests

initializeTelemetryClient changes

e5845b9

- changed check and exception throw

Fix tests

fcd5c0f

- fix missing userAgent for tests to pass

Checkstyle fix

0467990

- fix checkstyle error

Guardiola31337 requested changes Dec 6, 2017

View reviewed changes

Access Token and User Agent Validation + test updates

20568d2

- renamed and reworked isAccessTokenAndUserAgentValid() - set telemetry client user agent when updated - magic numbers - cleaned up and added tests - removed redundant tests (throws exception before test runs)

electrostat requested a review from Guardiola31337 December 6, 2017 21:24

Guardiola31337 requested changes Dec 7, 2017

View reviewed changes

electrostat added 3 commits December 7, 2017 15:56

Rework and readd checks

7d4a45c

- rework accesstoken + useragent check - method order - reinstate check for sending events

Update and add to tests

95932a4

- update tests - create updateUserAgent test - fixed bug with createFullUserAgent regarding spacing being off

Fix tests

fe084a9

- fixed 2 tests that are broken with exception refactor

electrostat requested a review from Guardiola31337 December 7, 2017 21:49

Addiontal tests and validation reworking

4ba521d

- additional user agent tests - rework: checkRequiredParameters, isAccessTokenValid, isUserAgentValid, - bign back queue tests - cleanup code

Guardiola31337 requested changes Dec 11, 2017

View reviewed changes

isEmpty() method + telemClient creation tweak

770b553

- tweaked tests - created internal isEmpty() method to fix test failures - prevent telem client creation, if 'checkRequiredParameters' fails.

electrostat requested a review from Guardiola31337 December 11, 2017 19:12

Guardiola31337 requested changes Dec 12, 2017

View reviewed changes

electrostat added the RELEASE BLOCKING label Dec 12, 2017

initializeTelemetryClient rework + tests

5ada084

- remove unneded exception - rework 'initializeTelemetryClient' and its implementation - update tests

electrostat requested a review from Guardiola31337 December 12, 2017 19:25

Initialize Queue + clean-up

edd0ac8

- remove unused method - update tests - initialize queue

Guardiola31337 approved these changes Dec 13, 2017

View reviewed changes

electrostat added 3 commits December 13, 2017 11:57

Constant renaming

85b7ed3

- renamed constant (added underscore between compound word) to meet java standards

Merge branch 'master' into aa-multiple-user-agent-support

1cb735f

Checkstyle fixes

d23d619

- removal of spaces added when merging

electrostat merged commit 280da39 into master Dec 13, 2017

electrostat deleted the aa-multiple-user-agent-support branch December 13, 2017 22:01

		public MapboxTelemetry(Context context, String accessToken, String userAgent, Callback httpCallback) {
		isAccessTokenAndUserAgentValid(accessToken, userAgent);

Support Multiple User Agents #19

Support Multiple User Agents #19

Conversation

electrostat commented Nov 29, 2017

Guardiola31337 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment •

edited

Loading

Guardiola31337 left a comment •

edited

Loading