[android] Add support for external SDK clients tokens management #14631

Guardiola31337 · 2019-05-09T22:32:44Z

This PR builds on top of #14404 and adds support for external SDK clients to be able to manage the tokens on their side disabling the default implementation and returning the token from SharedPreferences i.e. the external SDK is responsible for managing the state, rotating and storing (using the proper keys defined in AccountsManager) the tokens.

This is an initial proposal, please let me know what you think.

cc @akitchen @frederoni

zugaldia · 2019-05-09T23:53:12Z

cc: @tobrun or @LukasPaczos for a first round of comments.

LukasPaczos

Initially LGTM 👍

One note that AccountsManager#PREFERENCE_SKU_TOKEN constant should be exposed so that it can be used downstream, without hardcoding the value.

tobrun · 2019-05-28T08:30:30Z

@Guardiola31337 any progress on this PR? if we want to hit the deadline for release-oolong, we need to start moving on this.

Guardiola31337 · 2019-05-28T14:18:54Z

@tobrun

any progress on this PR? if we want to hit the deadline for release-oolong, we need to start moving on this.

Sure thing! This is updated and ready for another round of 👀

cc @LukasPaczos

LukasPaczos

Looks good, minor comments below. Also, it'd be great if you could add some unit tests to the AccountsManagerTest

LukasPaczos · 2019-05-28T17:29:27Z

...android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/constants/MapboxConstants.java

+  public static final String KEY_PREFERENCE_SKU_TOKEN = "com.mapbox.mapboxsdk.accounts.skutoken";
+
+  /**
+   * Key used to switch SKU token management on/off in AndroidManifest.xml


NIT: I don't think we need this javadoc.

LukasPaczos · 2019-05-28T17:29:40Z

...android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/constants/MapboxConstants.java

+  public static final String KEY_META_DATA_MANAGE_SKU_TOKEN = "com.mapbox.ManageSkuToken";
+
+  /**
+   * Default value for KEY_META_DATA_MANAGE_SKU_TOKEN (default is on)


Same as above.

LukasPaczos · 2019-05-28T17:34:48Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java


  private long timestamp;
+  @Nullable


Let's ensure that the token is actually not null by fetching it from preferences in the constructor as well.

Guardiola31337 · 2019-05-29T17:33:32Z

@LukasPaczos updated and ready for another round of 👀

LukasPaczos · 2019-05-30T10:58:25Z

platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/AccountsManagerTest.java

+  public void checksSkuTokenExternalManagement() {
+    SharedPreferences mockedSharedPreferences = mock(SharedPreferences.class);
+    when(mockedSharedPreferences.getString(KEY_PREFERENCE_SKU_TOKEN, "")).thenReturn("external-sku-token");
+    boolean isNotManaged = false;


NIT: should be called isManaged.

That was on purpose for better readability when creating the AccountsManager object https://github.com/mapbox/mapbox-gl-native/blob/434eacd60cd8bdf933894c6bfe9b4fe32ad560c5/platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/AccountsManagerTest.java#L38

🤔 now the variable is called isNotManaged, and has value false, so that should mean that it is managed. Idk, just seems a little bit off to me.

👀 Working Effectively with Unit Tests

when writing tests you should prefer DAMP (Descriptive And Maintainable Procedures)

That includes a bunch of procedures Moving Towards Readability 😅

I highly recommend you reading that book (if you haven't already). It's 🔝 💯

Thanks! It had a reverse effect on me though haha 🤷‍♂️

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java

platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/AccountsManagerTest.java

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java

Guardiola31337 · 2019-05-30T13:55:12Z

@LukasPaczos I've answered back / addressed all your comments, this is ready for a final round of 👀

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java

…he tokens on their side

Guardiola31337 added the Android Mapbox Maps SDK for Android label May 9, 2019

Guardiola31337 requested a review from zugaldia May 9, 2019 22:32

Guardiola31337 self-assigned this May 9, 2019

Guardiola31337 force-pushed the pg-external-sku-handling branch from 61c4a2c to 32193e4 Compare May 9, 2019 22:36

zugaldia removed their request for review May 9, 2019 23:53

Guardiola31337 requested review from zugaldia and removed request for zugaldia May 10, 2019 03:14

LukasPaczos reviewed May 10, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-external-sku-handling branch from 32193e4 to 328a69e Compare May 10, 2019 13:52

Guardiola31337 force-pushed the pg-external-sku-handling branch 2 times, most recently from 08ce07c to 3110363 Compare May 23, 2019 13:32

Guardiola31337 force-pushed the pg-external-sku-handling branch from 3110363 to 1f91422 Compare May 28, 2019 14:16

Guardiola31337 marked this pull request as ready for review May 28, 2019 14:18

Guardiola31337 requested a review from LukasPaczos May 28, 2019 14:19

LukasPaczos reviewed May 28, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-external-sku-handling branch from 1f91422 to 007af2a Compare May 29, 2019 17:30

Guardiola31337 force-pushed the pg-external-sku-handling branch from 007af2a to 434eacd Compare May 29, 2019 17:40

LukasPaczos reviewed May 30, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-external-sku-handling branch from 434eacd to 75776ae Compare May 30, 2019 13:53

Guardiola31337 force-pushed the pg-external-sku-handling branch from 75776ae to cfc1533 Compare May 30, 2019 13:56

LukasPaczos reviewed May 30, 2019

View reviewed changes

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/AccountsManager.java Outdated Show resolved Hide resolved

LukasPaczos approved these changes May 30, 2019

View reviewed changes

Guardiola31337 force-pushed the pg-external-sku-handling branch from cfc1533 to 742a021 Compare May 30, 2019 14:11

[android] add support for external sdk clients to be able to manage t…

8ade647

…he tokens on their side

Guardiola31337 force-pushed the pg-external-sku-handling branch from 742a021 to 8ade647 Compare May 30, 2019 14:16

Guardiola31337 merged commit 0181ef8 into master May 30, 2019

Guardiola31337 deleted the pg-external-sku-handling branch May 30, 2019 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Add support for external SDK clients tokens management #14631

[android] Add support for external SDK clients tokens management #14631

Guardiola31337 commented May 9, 2019

zugaldia commented May 9, 2019

LukasPaczos left a comment

tobrun commented May 28, 2019

Guardiola31337 commented May 28, 2019

LukasPaczos left a comment

LukasPaczos May 28, 2019

LukasPaczos May 28, 2019

LukasPaczos May 28, 2019

Guardiola31337 commented May 29, 2019

LukasPaczos May 30, 2019

Guardiola31337 May 30, 2019

LukasPaczos May 30, 2019

Guardiola31337 May 30, 2019

LukasPaczos May 30, 2019

Guardiola31337 commented May 30, 2019

[android] Add support for external SDK clients tokens management #14631

[android] Add support for external SDK clients tokens management #14631

Conversation

Guardiola31337 commented May 9, 2019

zugaldia commented May 9, 2019

LukasPaczos left a comment

Choose a reason for hiding this comment

tobrun commented May 28, 2019

Guardiola31337 commented May 28, 2019

LukasPaczos 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

Guardiola31337 commented May 29, 2019

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 commented May 30, 2019