Refactor SaveCredentialsActivity #343

SUPERCILEX · 2016-10-07T04:41:47Z

If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
- Refactor SaveCredentialsActivity #254

Placeholder PR. This refactoring is huge and in no way ready yet (as in it won't even compile), but I thought I'd inform you guys that I'm working on this and ask a few questions.

won't compile

SUPERCILEX · 2016-10-07T04:43:10Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

@@ -265,13 +265,13 @@ private AuthUI(FirebaseApp app) {
     * ({@code !result.isSuccess()}).
     */
    public Task<Void> signOut(@NonNull Activity activity) {
-        // Get helper for Google Sign In and Credentials API
+        // Get helper for Google Sign In and SmartLock API


Ignore this weirdness: intellij make some mistakes.

SUPERCILEX

This PR isn't ready at all yet, but I have a few basic questions. Since the calling activities are expecting an activity result code that no longer exists, how should I handle this? startResolutionForResult needs an activity and finishes in an onActivityResult where stuff needs to be sorted out. To not copy code, I've put this logic in SmartLock, but this makes things a pain (see below).

SUPERCILEX · 2016-10-07T04:44:12Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java

@@ -137,10 +137,8 @@ public void onSuccess(AuthResult authResult) {
                                        new OnSuccessListener<AuthResult>() {
                                            @Override
                                            public void onSuccess(AuthResult authResult) {
-                                                mActivityHelper.dismissDialog();
-                                                SmartlockUtil.saveCredentialOrFinish(
+                                                mSmartLock = SmartLock.saveCredentialOrFinish(


Any thoughts on how to do this better?

SUPERCILEX · 2016-10-07T04:44:51Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java

-        if (requestCode == RC_CREDENTIAL_SAVE) {
-            finish(RESULT_OK, new Intent());
+        if (mSmartLock != null) {
+            mSmartLock.onActivityResult(requestCode, resultCode);


I really don't like this, but I can't think of another way to do this without copying code between Activities. Thoughts?

There's nothing wrong with this pattern (having a non-Activity object that has an onActivityResult method) but I'd suggest having that method return a boolean that signifies whether or not mSmartLock was able to handle the result or if it should be delegated back to the Activity.

samtstern · 2016-10-07T15:34:58Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

-                .getStringExtra(ExtraConstants.EXTRA_PROFILE_PICTURE_URI);
-
-        mCredentialsApiClient = new GoogleApiClient.Builder(this)
+    public SmartLock(AppCompatBase activity,


If this class is going to be tied to an Activity, then one way to structure it would be a headless Fragment. You'd call SmartLock.getInstance(activity, ...) which would attach a Fragment to the Activity. Then that Fragment can actually launch activities for result and receive the results directly, rather than the constant if (mSmartLock != null) { smartLock.onActivityResult(...) }.

Great idea! I didn't know you could do that. Things are way nicer now.

…ctivities

samtstern · 2016-10-14T15:53:44Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

+     * @param password     (optional) password for email credential.
+     * @param provider     (optional) provider string for provider credential.
+     */
+    public static SmartLock getInstance(AppCompatBase activity,


It's more conventional to have this getInstance() method actually do the Fragment transaction and add itself to the Activity (first checking if there is already an instance of SmartLock attached to the Activity).

You can attach a SmartLock fragment to each Activity that needs it in onCreate. Then SmartLock could expose methods to kick off the various flows, rather than starting on Fragment attach.

So getInstance() would look more like this:

SmartLock result; FragmentManager fm = activity.getSupportFragmentManager(); FragmentTransaction ft = fm.beginTransaction(); Fragment fragment = fm.findFragmentByTag(SMARTLOCK_TAG); if (fragment == null || (!fragment instanceof SmartLock)) { shouldAdd = true; } else { result = (SmartLock) fragment; } if (shouldAdd) { result = new SmartLock(...); ft.add(result, SMARTLOCK_TAG).disallowAddToBackStack().commit(); } return result; }

Ohhhhh. I think I'm getting it now. You can pass in things like Activities to a fragment because they are a more conventional Java object: you can just do method injection.

samtstern · 2016-10-14T15:54:18Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

+                                        @Nullable String password,
+                                        @Nullable String provider) {
+        SmartLock smartLock = new SmartLock();
+        smartLock.mActivity = activity;


For parameters of the Fragment like this it's better to use setArguments() to attach them to the Fragment in a way that the Android system can keep track of.

How would I pass in things like an activity though?

You can just do getActivity() in the fragment, and if you need methods on AppCompatBase you can just do:

if (!(getActivity() instanceof AppCompatBase)) { throw new IllegalStateException("SmartLock can only be attached to AppCompatBase!"); } else { // Cast and call method }

Realistically you can just do the check --> throw thing once in the Fragment onAttach.

samtstern · 2016-10-14T20:17:21Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

    public void onConnected(@Nullable Bundle bundle) {
        if (mEmail == null) {
            Log.e(TAG, "Unable to save null credential!");
-            finish(RESULT_FIRST_USER, getIntent());
+            mActivity.finish(RESULT_OK, mActivity.getIntent());


Is there a reason we changed this to RESULT_OK here?

Yes. When this was an activity we finished with result RESULT_FIRST_USER, but the calling activity didn't actually care what the result was (all the calling activities finished with RESULT_OK). Since the fragment it closing the activity for it, we just finish everything with RESULT_OK.

SGTM, thanks for clarifying 👍

samtstern · 2016-10-14T20:20:47Z

Looks like you're still working on the tests, but this is starting to look very close to done! Thanks for all the work so far.

@amandle could you take a look through this as well? Removes the headless smartlock activity in favor of a Fragment-based system. Should be a no-op in terms of logic and functionality but will make the back stack nicer and the whole thing a little more modular.

SUPERCILEX · 2016-10-14T22:02:39Z

I think the SaveCredentialsActivity refactor is good now.

Also, I'll create separate PRs for the other zombie activities in #254.

samtstern · 2016-10-14T22:40:13Z

auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java

@@ -139,10 +132,8 @@ public void onSuccess(AuthResult authResult) {
                                        new OnSuccessListener<AuthResult>() {
                                            @Override
                                            public void onSuccess(AuthResult authResult) {
-                                                mActivityHelper.dismissDialog();


Just noticed this, why are we removing the calls to dismissDialog()?

I decided to get rid of them because when you close the activity, they get closed with it. Leaving them open means the loading dialog stays open during the credential save.

samtstern · 2016-10-14T22:42:35Z

LGTM! I'll let @amandle take a look though in case I missed anything. This came out really clean in the end.

amandle

This is a very nice clean up. A few comments about the tests and test intent.

amandle · 2016-10-14T23:05:46Z

auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java

-        ShadowActivity.IntentForResult nextIntent =
-                Shadows.shadowOf(authMethodPickerActivity).getNextStartedActivityForResult();
-        verifySaveCredentialIntent(nextIntent, FacebookAuthProvider.PROVIDER_ID);
+        Intent smartLockIntent = SmartLock.getInstance(authMethodPickerActivity).getIntentForTest();


These tests verify the intent when the intent is only created for the purposes of the test. It would be nice to update the tests to reflect the changes in SmartLock, ideally checking that the SmartLock API is called with the correct credential.

I'll try that again, but I keep getting a null pointer exception at line 221 in smartlock.java.

I figured that part out

Do we need the tests? All they were doing before is checking to see if the intent was passed in correctly... Right now I'm stuck on how to wait for SmartLock to save the credentials.

Yeah, the test before was testing the boundary between this activity and the smartlock activity, so a similar test doesn't make sense here. Perhaps it would make the most sense to test the smartlock capabilities in CredentialSignInHandlerTest where you can provide a mock smartlock instance and verify that the correct method is called with the right parameters.

Noooooooooooo!!!!!! I just spent two hours figuring this out!!

I'm almost done and then I'll update the status of this PR.

amandle · 2016-10-14T23:05:52Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

+                .build();
+    }
+
+    public Intent getIntentForTest() {


This is returning an intent when the SmartLock doesn't actually use intents. This makes the tests slightly misleading IMO. See the comments on the tests below.

amandle · 2016-10-14T23:10:16Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

        if (mProfilePictureUri != null) {
            builder.setProfilePictureUri(Uri.parse(mProfilePictureUri));
        }
-        mActivityHelper.getCredentialsApi()
+
+        Auth.CredentialsApi


This change makes it difficult to use a mock CredentialsApi in the tests.

amandle · 2016-10-14T23:14:02Z

auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java

-                mActivityHelper.getCredentialsApi()
+                Credential credential = new Credential.Builder(mEmail).setPassword(mPassword)
+                        .build();
+                Auth.CredentialsApi


Same comment here about the mocking

SUPERCILEX · 2016-10-15T07:47:43Z

auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java

-        verifySaveCredentialIntent(nextIntent, GoogleAuthProvider.PROVIDER_ID);
+        assertEquals(
+                Shadows.shadowOf(authMethodPickerActivity).getResultCode(),
+                Activity.RESULT_OK);


If I could get robolectric to wait for SmartLock to finish, Shadows.shadowOf(authMethodPickerActivity).getResultCode() would change from Activity.RESULT_CANCELED to Activity.RESULT_OK which would confirm that the something happened if not the credentials being saved. Any ideas?

SUPERCILEX · 2016-10-15T18:58:50Z

Do you guys want to merge the refactor of SaveCredentialsActivity and ChooseAccountActivity separately? For the ChooseAccountActivity refactor, I pulled changes from this PR because I'm completely reworking the Smart Lock stuff.

SUPERCILEX · 2016-10-16T03:09:53Z

Nevermind, I'm going to submit a separate PR because the changes are massive. It's mind blowing how fast and beautiful things are now!!! Pretty exciting! The one downside is that there are breaking changes to AuthUI, but I think it's definitely worth it.

SUPERCILEX · 2016-10-17T05:43:54Z

This PR and #357 should be released together because I'm checking how configuration changes affect the smart lock stuff.

…or-zombie-activities # Conflicts: # auth/src/main/java/com/firebase/ui/auth/util/SmartlockUtil.java # auth/src/main/res/values/styles.xml # auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java

…or-zombie-activities # Conflicts: # auth/src/main/AndroidManifest.xml # auth/src/main/java/com/firebase/ui/auth/ui/account_link/WelcomeBackPasswordPrompt.java # auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java # auth/src/main/java/com/firebase/ui/auth/ui/idp/CredentialSignInHandler.java # auth/src/main/java/com/firebase/ui/auth/ui/idp/IdpSignInContainerActivity.java # auth/src/main/java/com/firebase/ui/auth/util/SmartLock.java # auth/src/main/java/com/firebase/ui/auth/util/SmartlockUtil.java # auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java # auth/src/test/java/com/firebase/ui/auth/ui/idp/CredentialSignInHandlerTest.java

SUPERCILEX · 2016-10-19T04:27:49Z

@amandle I finished up the PR, what do you think of the tests? BTW, to make them work, I had to remove the mSmartLock initialization from onCreate to where saveCredentialsOrFinish is called because I'm using a sneeky version of injection to make the tests work. Feel free to ask me any questions!

…or-zombie-activities # Conflicts: # auth/src/test/java/com/firebase/ui/auth/ui/email/RegisterEmailActivityTest.java # auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java

amandle · 2016-10-19T22:19:27Z

@SUPERCILEX Tests look good to me, thanks for doing that!

SUPERCILEX · 2016-10-19T22:51:52Z

@amandle Awesome! You're welcome!

amandle · 2016-10-21T01:30:52Z

@samstern, any objections to me merging this now?

temporary: saving to work on another computer

05ce3b5

won't compile

SUPERCILEX commented Oct 7, 2016

View reviewed changes

samtstern reviewed Oct 7, 2016

View reviewed changes

SUPERCILEX added 6 commits October 8, 2016 13:28

Merge remote-tracking branch 'firebase/master' into refactor-zombie-a…

2391efb

…ctivities

Merge remote-tracking branch 'firebase/master' into refactor-zombie-a…

05a476a

…ctivities

Code compiles

2c0eaf5

Cleanup

8adeee1

Cleanup

11f3c2b

Cleanup

cb826f4

samtstern reviewed Oct 14, 2016

View reviewed changes

SUPERCILEX added 2 commits October 14, 2016 12:50

Update SmartLock.java with new fragment logic

3b788f5

Fix bug in SmartLock.java

9eaa4cd

samtstern reviewed Oct 14, 2016

View reviewed changes

SUPERCILEX added 2 commits October 14, 2016 13:17

Cleanup

3b76ac6

Cleanup

86d58a9

SUPERCILEX added 2 commits October 14, 2016 14:43

Fix broken tests

6ad0021

Cleanup

550acc9

SUPERCILEX changed the title ~~Refactor zombie activities~~ Refactor SaveCredentialsActivity Oct 14, 2016

SUPERCILEX mentioned this pull request Oct 14, 2016

Support account linking #309

Closed

7 tasks

samtstern reviewed Oct 14, 2016

View reviewed changes

amandle suggested changes Oct 14, 2016

View reviewed changes

SUPERCILEX added 2 commits October 15, 2016 00:39

Cleanup

b9d3ca5

Cleanup

a8b5cf5

SUPERCILEX commented Oct 15, 2016

View reviewed changes

Bug fix

9a4921e

SUPERCILEX mentioned this pull request Oct 16, 2016

Refactor chooseAccountActivity #357

Merged

10 tasks

SUPERCILEX changed the base branch from master to version-1.0.0-dev October 17, 2016 22:40

SUPERCILEX added 13 commits October 17, 2016 19:09

Cleanup

db96c91

Add interface to SmartLock

19bde41

Merge dev

9511a6d

working on tests

cbe0cfb

working on tests

a81bf5a

Revert interface, working on fixing tests

db05831

Fix tests

8c96216

Merge dev

cb6efad

Fix merge mistakes

af644be

Cleanup

4152c23

Final cleanup and fix tests

2cc8adf

SUPERCILEX added 3 commits October 18, 2016 21:50

Fix test

c138a51

Fix test

f88aab3

Merge remote-tracking branch 'firebase/version-1.0.0-dev' into refact…

ad115f0

…or-zombie-activities # Conflicts: # auth/src/test/java/com/firebase/ui/auth/ui/email/RegisterEmailActivityTest.java # auth/src/test/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivityTest.java

amandle approved these changes Oct 19, 2016

View reviewed changes

amandle merged commit baab107 into firebase:version-1.0.0-dev Oct 21, 2016

SUPERCILEX deleted the refactor-zombie-activities branch October 21, 2016 22:27

Refactor SaveCredentialsActivity #343

Refactor SaveCredentialsActivity #343

Conversation

SUPERCILEX commented Oct 7, 2016

Choose a reason for hiding this comment

SUPERCILEX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX Oct 7, 2016 • 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

samtstern commented Oct 14, 2016

SUPERCILEX commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samtstern commented Oct 14, 2016

amandle 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

SUPERCILEX commented Oct 15, 2016

SUPERCILEX commented Oct 16, 2016 • edited Loading

SUPERCILEX commented Oct 17, 2016

SUPERCILEX commented Oct 19, 2016

amandle commented Oct 19, 2016

SUPERCILEX commented Oct 19, 2016 • edited Loading

amandle commented Oct 21, 2016

SUPERCILEX Oct 7, 2016 •

edited

Loading

SUPERCILEX commented Oct 16, 2016 •

edited

Loading

SUPERCILEX commented Oct 19, 2016 •

edited

Loading