Ls login/create #331

langsmith · 2017-05-11T17:55:56Z

PR adds oAuth2 flow for non-mandatory account login/creation before viewing examples.

…come toast

…ername

langsmith · 2017-05-11T18:01:57Z

@mapbox/android , would love 👀 , review, and feedback.

All networking that isn't browser based in Chrome Custom Tabs, is happening in the background service named AccountRetrievalService

Small stuff still remains like adding checks (and alert dialogs) for internet connectivity. Wanted to get the ball rolling on this though...

Guardiola31337

Build is broken 😖
I pointed out some details to discuss before landing.
BTW well done 👍

Guardiola31337 · 2017-05-19T06:45:37Z

MapboxAndroidDemo/build.gradle

-  compile 'com.github.javiersantos:MaterialStyledDialogs:2.1'
-  compile 'com.getbase:floatingactionbutton:1.10.1'
-  compile 'com.squareup.picasso:picasso:2.5.2'
+    // Firebase crash report


Useless comments.

Guardiola31337 · 2017-05-19T06:50:20Z

MapboxAndroidDemo/build.gradle

+    compile 'com.squareup.retrofit2:retrofit:2.2.0'
+    compile 'com.squareup.retrofit2:converter-gson:2.2.0'
+    compile 'com.android.support.constraint:constraint-layout:1.0.2'
+    compile 'com.android.support:customtabs:25.3.1'


Why not using ${supportLibVersion} here?
Same thing happens with other dependencies.

Guardiola31337 · 2017-05-19T06:52:25Z

MapboxAndroidDemo/build.gradle

-  compile 'com.google.code.gson:gson:2.8'
-  compile 'com.squareup.retrofit2:retrofit:2.2.0'
-  compile 'com.squareup.retrofit2:converter-gson:2.2.0'
+// Other dependencies


There are some Google dependencies intermingled. What about rearranging them?

Guardiola31337 · 2017-05-19T06:54:46Z

MapboxAndroidDemo/src/main/AndroidManifest.xml

+            android:screenOrientation="portrait">
+            <intent-filter>
+                <action android:name="android.intent.action.MAIN"/>
+


Blank lines.

Guardiola31337 · 2017-05-19T07:01:09Z

...xAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/AccountRetrievalService.java

+  private String username;
+
+  public AccountRetrievalService() {
+    super("AccountRetrievalService");


In order to increase readability, what about replacing magic numbers with constants with a particular meaning?

Are you talking about these numbers that are passed through to the service via the intent?

If so, they're needed as is because they're part of the oAuth flow. Specifically given to me by our API team.

when I say "numbers", I actually meant Strings. You get my point...

Guardiola31337 · 2017-05-19T07:26:46Z

...oidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/MapboxAccountRetrofitService.java

+ * Service for making call to Mapbox API for account information
+ */
+
+public interface MapboxAccountRetrofitService {


Access can be package-private.

Fixed. It's now:

interface MapboxAccountRetrofitService {

Guardiola31337 · 2017-05-19T07:32:29Z

...oxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/Authorizations.java

@@ -0,0 +1,105 @@
+package com.mapbox.mapboxandroiddemo.model.usermodel;
+
+/**


Remove file header.
Turning off “created by” stamp when generating files in AS

Done. Thanks. Switched it to:

/** * TODO: Add a class header comment! */

Guardiola31337 · 2017-05-19T07:32:50Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/Geocodes.java

@@ -0,0 +1,47 @@
+package com.mapbox.mapboxandroiddemo.model.usermodel;
+
+/**


Guardiola31337 · 2017-05-19T07:33:33Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/Plan.java

+
+public class Plan {
+  private String satellite;
+


Why blank line between all variables?

I used http://www.jsonschema2pojo.org/ and that's how they deliver the classes. I've removed the blank lines in all of the usermodel classes.

Guardiola31337 · 2017-05-19T07:49:39Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/UserResponse.java

+
+public class UserResponse {
+
+  @SerializedName("authorizations")


Why @SerializedName? It's normally used for customizing field names (here the names are equal).
Further information on @SerializedName.

Yeaaa, no reason to have them. I thought I might have to use it but it turned out I didn't. Forgot to change. Removed

langsmith · 2017-05-20T00:11:27Z

@Guardiola31337 , all requested changes have been addressed. Except for the okhttp3/retrofit question.

Guardiola31337

Looking good @langsmith! Added some remarks.

Guardiola31337 · 2017-05-22T10:02:23Z

MapboxAndroidDemo/build.gradle


-    // Mapbox Android UI
+// Mapbox Android UI
+


Nit new lines and reformat code.

Guardiola31337 · 2017-05-22T10:15:40Z

MapboxAndroidDemo/src/gpservices/AndroidManifest.xml

    <uses-feature android:name="android.hardware.location.gps"/>

    <application
+        android:name=".MapboxApplication"


We should clean/remove all unnecessary stuff from this manifest and take advantage of Manifest Merger to add/merge what differs from the root manifest (MapboxAndroidDemo/src/main/AndroidManifest.xml).

Guardiola31337 · 2017-05-22T10:21:21Z

MapboxAndroidDemo/src/main/AndroidManifest.xml

+                <action android:name="android.intent.action.MAIN"/>
+                <category android:name="android.intent.category.LAUNCHER"/>
+            </intent-filter>
+            <action android:name="android.intent.action.VIEW"/>


Isn't <intent-filter> needed?

yea, not really sure why it's being displayed that way on Github. On this branch, studio shows:

<activity android:name=".account.LandingActivity" android:screenOrientation="portrait"> <intent-filter> <action android:name="android.intent.action.MAIN"/> <category android:name="android.intent.category.LAUNCHER"/> </intent-filter> <intent-filter> <action android:name="android.intent.action.VIEW"/> <category android:name="android.intent.category.DEFAULT"/> <category android:name="android.intent.category.BROWSABLE"/> <data android:host="authorize" android:scheme="mapbox-android-dev-preview"/> </intent-filter> </activity>

This is what we can use moving forward

@langsmith I'm still seeing previous code without the <intent-filter> 😞
Remember that there are 2 manifests MapboxAndroidDemo/src/gpservices/AndroidManifest.xml and MapboxAndroidDemo/src/main/AndroidManifest.xml. It seems that only the first one was updated.

Hm. Ok, both manifest files now have:

<activity android:name=".account.LandingActivity" android:screenOrientation="portrait"> <intent-filter> <action android:name="android.intent.action.MAIN"/> <category android:name="android.intent.category.LAUNCHER"/> </intent-filter> <intent-filter> <action android:name="android.intent.action.VIEW"/> <category android:name="android.intent.category.DEFAULT"/> <category android:name="android.intent.category.BROWSABLE"/> <data android:host="authorize" android:scheme="mapbox-android-dev-preview"/> </intent-filter> </activity>

Guardiola31337 · 2017-05-22T10:32:14Z

...xAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/AccountRetrievalService.java

+  private String username;
+
+  public AccountRetrievalService() {
+    super("AccountRetrievalService");


Are you talking about these numbers that are passed through to the service via the intent?

Literals in general. Further information: Replace Magic Number with Symbolic Constant.

Guardiola31337 · 2017-05-22T11:03:43Z

...xAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/AccountRetrievalService.java

+
+        JSONObject data = null;
+        try {
+          data = new JSONObject(json);


Referring to Retrofit's issue in branch ls-switching-last-okhttp3-to-retrofit-for-login.
As you can see in the logcat:

05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug D/RetrievalService: onResponse: response received 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug D/RetrievalService: onResponse: json = Response{protocol=http/1.1, code=404, message=Not Found, url=https://api.mapbox.com/oauth/access_token/grant_type=authorization_code&client_id=%7BclientId%7D&client_secret=%7BclientSecret%7D&redirect_uri=%7BredirectUri%7D&code=%7Bcode%7D} 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug D/RetrievalService: onResponse: jsonString = retrofit2.OkHttpCall$NoContentResponseBody@79f9c5f 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: org.json.JSONException: Value Response of type java.lang.String cannot be converted to JSONObject 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at org.json.JSON.typeMismatch(JSON.java:111) 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at org.json.JSONObject.<init>(JSONObject.java:160) 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at org.json.JSONObject.<init>(JSONObject.java:173) 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at com.mapbox.mapboxandroiddemo.account.AccountRetrievalService$1.onResponse(AccountRetrievalService.java:78) 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at retrofit2.ExecutorCallAdapterFactory$ExecutorCallbackCall$1$1.run(ExecutorCallAdapterFactory.java:68) 05-22 12:45:17.376 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at android.os.Handler.handleCallback(Handler.java:739) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at android.os.Handler.dispatchMessage(Handler.java:95) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at android.os.Looper.loop(Looper.java:148) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at android.app.ActivityThread.main(ActivityThread.java:5417) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at java.lang.reflect.Method.invoke(Native Method) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726) 05-22 12:45:17.377 11938-11938/com.mapbox.mapboxandroiddemo.debug W/System.err: at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

Retrofit was able to communicate with the given server, but the server could not find what was requested (code=404, message=Not Found) and data can't be created (see org.json.JSONException: Value Response of type java.lang.String cannot be converted to JSONObject).
So it seems that isn't a problem with Retrofit but with how the request is created/configured.

Yea, I knew that my configuration was off. I guess the question is/was whether it's worth spending the time to fiddle with the configuration until it works or just leave the okhttp3 as is. I think I know your answer 😉

Guardiola31337 · 2017-05-22T11:09:51Z

...xAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/AccountRetrievalService.java

+
+        String json = response.body().string();
+
+        JSONObject data = null;


Variable data initializer null is redundant.

Fixed to:

try { JSONObject data = new JSONObject(json);

Guardiola31337 · 2017-05-22T11:10:59Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/LandingActivity.java

+    setUpSkipDialog();
+  }
+
+  private void setUpButtons() {


Rearrange methods order (public, protected, private, etc.).

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

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/LandingActivity.java

+  }
+
+  private void openChromeCustomTab(boolean creatingAccount) {
+


Nit new lines.

Guardiola31337 · 2017-05-22T11:17:43Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/account/LandingActivity.java

+
+    if (getIntent() != null && getIntent().getAction().equals(Intent.ACTION_VIEW)) {
+      Uri uri = getIntent().getData();
+      if (uri.getQueryParameter("error") != null) {


Why not extracting uri.getQueryParameter("error") into a variable String error and being able to reuse it when logging (no need to query again)?

Ok. I believe I've fixed with:

if (uri.getQueryParameter("error") != null) { Toast.makeText(this, R.string.whoops_error_message_on_app_return, Toast.LENGTH_SHORT).show(); String error = uri.getQueryParameter("error"); Log.d("LandingActivity", "onResume: error = " + error);

@langsmith
Sorry, I meant replacing all 2 occurrences of uri.getQueryParameter("error"), define a new variable to store the value before the if check so it'll be available to reuse it inside and remove unnecessary String error inside the if.
Basically, move String error definition out of the if and reuse it as much as possible:

String error = uri.getQueryParameter("error"); if (error != null) { Toast.makeText(this, R.string.whoops_error_message_on_app_return, Toast.LENGTH_SHORT).show(); Log.d("LandingActivity", "onResume: error = " + error); }

Guardiola31337 · 2017-05-22T11:20:18Z

MapboxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/Mapviews.java

@@ -0,0 +1,44 @@
+package com.mapbox.mapboxandroiddemo.model.usermodel;
+
+/**


Remove file headers.

Done. Thanks. Switched it to:

/** * TODO: Add a class header comment! */

You can leave it blank if you want.

Ok, fixed. Template is blank.

@langsmith
👀 There are still classes with file headers: Mapviews, Plan, Services, UserDirections and UserResponse.

Removed from those classes

Guardiola31337 · 2017-05-22T15:47:30Z

One more thing @langsmith rebase/fix up the commits to represent what would be merged into master for easier review 😉

zugaldia · 2017-05-23T19:30:08Z

Thank you @langsmith @Guardiola31337, this is looking great. In order to unblock #336 I suggest that, if the OkHttp/Retrofit conversation is the one thing left here, that we move that work to a separate PR that allows us to merge this one. Happy to help reviewing that separate PR.

…r, etc.)

langsmith · 2017-05-24T20:43:34Z

Ok @zugaldia .
How about we move the following two remaining "issues" to separate future PRs so that this one can land:

Remaining OkHttp/Retrofit transition
Cleaning up manifest

I believe I've fixed all of the issues that @Guardiola31337 has mentioned in this PR and in our Slack DMs, besides the two above.

Guardiola31337 · 2017-05-26T09:35:04Z

...oxAndroidDemo/src/main/java/com/mapbox/mapboxandroiddemo/model/usermodel/Authorizations.java

@@ -0,0 +1,93 @@
+package com.mapbox.mapboxandroiddemo.model.usermodel;
+
+public class Authorizations {


@langsmith
Access can be package-private.
👀 the same thing happens in other classes inside usermodel package.

langsmith · 2017-05-26T17:20:16Z

Made adjustments for your latest round of review @Guardiola31337

langsmith · 2017-05-30T18:30:30Z

Next steps? @zugaldia , I've made separate tickets in this repo for the okHttp/Retrofit switch and for cleaning up the two manifest files, so that we don't forget about them.

I've made changes for all of @Guardiola31337 's requests besides rebasing/fixing up the commits to represent what would be merged into master for easier review.

Guardiola31337

Well done @langsmith

Langston and others added 18 commits May 8, 2017 17:56

Set up layout and beginnings of oAuth flow

1752a6c

Successfully got access token at this point

0e63493

Refactored and cleaned up code

d3611a0

added user porous

50c6e86

added user model porous

c696239

refactored login code

00b8138

Can successfully get account info with retrofit

319e853

Sign in flow works. Added sharedPref saving & string resource for wel…

110d5f1

…come toast

Deleted separate Retrofit client

c81f824

Create account flow works too. Added JWT auth parsing and grabbing us…

0e7d5d2

…ername

String additions and other solving other merge conflicts

407f380

Set up background service for account retrieval networking

3664186

Added astronaut gif to launcher activity

d756b84

Checkstyle adjustments to user pojos

6681c5f

Refactoring names and check style adjustments

c2f6743

Added gif support library to dependencies

70711d5

Checkstyle adjustments

d50f188

Added loading activity

3bc5392

langsmith requested review from Guardiola31337 and tobrun and removed request for Guardiola31337 and tobrun May 12, 2017 18:58

zugaldia requested a review from Guardiola31337 May 12, 2017 19:04

Bumped dependency to beta.2

b2be6f0

Guardiola31337 reviewed May 19, 2017

View reviewed changes

Langston added 3 commits May 19, 2017 10:26

Manifest adjustments (launchMode and <intent-filter> placement)

82b516d

Addressed all of Pablo's review requests except okhttp to retrofit

46a68a5

Merge branch 'master' into ls-login/create

2259895

gradle upgrade

4498d7c

Guardiola31337 reviewed May 22, 2017

View reviewed changes

Langston added 3 commits May 24, 2017 08:34

More adjustments based on Pablo's review (using literal in constructo…

ed47f0c

…r, etc.)

Token recognition logic in LandingActivity onCreate() works

1fbdb1d

Checkstyle adjustment

da0c418

Removed log lines

dfb49e1

langsmith mentioned this pull request May 24, 2017

Adding Segment analytics code and opt out toggling #336

Merged

Guardiola31337 reviewed May 26, 2017

View reviewed changes

Langston added 2 commits May 26, 2017 09:39

Adjustments based on Pablo's latest review

2e7bc3c

Making UserResponse class public

53af272

This was referenced May 30, 2017

Clean up manifest duplicates #343

Open

OkHttp to Retrofit #344

Open

Guardiola31337 approved these changes May 30, 2017

View reviewed changes

langsmith added the ⚠️ DO NOT MERGE ⚠️ label May 31, 2017

Langston added 7 commits May 31, 2017 17:27

Changed landing activity layout and got rid of skip dialog

514007a

Gradle changes for authFlow client

e54d24c

Added segment write key logic

1ec7c0f

Non-wearable adjustments

8294246

gradle adjustment

784725f

Added astronaut gif to landing activity

b8c1a1b

Merge branch 'master' into ls-login/create

f7cdf76

langsmith merged commit bff835f into master Jun 2, 2017

langsmith removed the ⚠️ DO NOT MERGE ⚠️ label Jun 2, 2017

Guardiola31337 deleted the ls-login/create branch June 4, 2017 15:18

		@@ -0,0 +1,105 @@
		package com.mapbox.mapboxandroiddemo.model.usermodel;

		/**

		@@ -0,0 +1,47 @@
		package com.mapbox.mapboxandroiddemo.model.usermodel;

		/**


		public class UserResponse {

		@SerializedName("authorizations")


		String json = response.body().string();

		JSONObject data = null;

		}

		private void openChromeCustomTab(boolean creatingAccount) {

		@@ -0,0 +1,44 @@
		package com.mapbox.mapboxandroiddemo.model.usermodel;

		/**

		@@ -0,0 +1,93 @@
		package com.mapbox.mapboxandroiddemo.model.usermodel;

		public class Authorizations {

Ls login/create #331

Ls login/create #331

Uh oh!

Conversation

langsmith commented May 11, 2017

Uh oh!

langsmith commented May 11, 2017

Uh oh!

Guardiola31337 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langsmith commented May 20, 2017

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langsmith May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Guardiola31337 left a comment •

edited

Loading

Guardiola31337 May 26, 2017 •

edited

Loading

langsmith May 26, 2017 •

edited

Loading

Guardiola31337 May 26, 2017 •

edited

Loading