AuthManager code review

[mochiruntime]

This is a code review for the AuthManager implementation.

Styling

Don't forget to style your code and remove your unused packages. You can simply use the automated linter in Android Studio by pressing Ctrl+Alt+o (optimize imports) and Ctrl+alt+l (format code) in every file.

Packages

Try using more packages. As you create more files, your main package will become overcrowded and it will be difficult to find specific bits of code. You could modularize your files hierarchy, for instance :

ch.epfl.sweng.studdybuddy/
    models/
        Account.java
        // your other model classes here
    providers/
        AuthManager.java
        FirebaseAuthManager.java
        OnLoginCallback.java
        // any other external wrapper such as database accesses
    activities/
        // your android activities here

Using Android Studio's integrated refactoring will help as it will automatically resolve all paths (right-click on a file -> refactor -> move).

Account class

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/Account.java

Lines 7 to 9 in 65460e9

	private String displayName;
	private String uid;
	private String idToken;

Is your class supposed to be immutable? If yes, don't forget to add the final keyword.

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/Account.java

Line 16 in 65460e9

public Account(){}

What is this empty constructor used for ? The constructed object would not be very useful since you can't get any existing property.

AuthManager

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/AuthManager.java

Lines 6 to 11 in 65460e9

	public interface AuthManager {
	void login(Account acct, OnLoginCallback f, String TAG);
	Task<Void> logout();
	void startLoginScreen();
	Account getCurrentUser();
	}

The abstraction is good. It could be more consistent by using Tasks only or callbacks only rather than mixing the 2 (this helps if another developer is using the interface). Read further below.

FirebaseAuthManager

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/FirebaseAuthManager.java

Lines 25 to 32 in 65460e9

	public FirebaseAuthManager(Activity currentActivity, String clientID){
	this.ctx = currentActivity;
	GoogleSignInOptions gso = new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_SIGN_IN)
	.requestIdToken(clientID)
	.requestEmail()
	.build();
	this.client = GoogleSignIn.getClient(currentActivity, gso);
	}

You don't need to pass clientID as a String argument. You can simply add another line to retrieve the string from the context :

    this.ctx = currentActivity;
    String clientID = ctx.getResources().getString(R.string.default_web_client_id)
    GoogleSignInOptions gso = ... ;

This is in fact better as it hides another implementation detail from the caller.

OnCompleteListenerWrapper

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/FirebaseAuthManager.java

Line 35 in 65460e9

OnCompleteListener ocl = new OnCompleteListenerWrapper<AuthResult>(acct, f, TAG, "SignInWithCredentials");

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/OnCompleteListenerWrapper.java

Lines 9 to 33 in 65460e9

	public class OnCompleteListenerWrapper<TResult> implements OnCompleteListener<TResult> {
	private Account acct;
	private OnLoginCallback f;
	private String TAG;
	private String action;
	public OnCompleteListenerWrapper(Account acct, OnLoginCallback f, String TAG, String action){
	this.acct = acct;
	this.f = f;
	this.TAG = TAG;
	this.action = action;
	}
	public void onComplete(Task<TResult> task) {
	if (task.isSuccessful()) {
	// Sign in success, update UI with the signed-in user's information
	Log.d(TAG, action+": success");
	f.then(acct);
	} else {
	// If sign in fails, display a message to the user.
	Log.w(TAG,action+": failure", task.getException());
	f.then(acct);
	}
	}
	}

I don't really understand the motive behind this class. From what I've understood, you want to define what happens after the user has logged in, so that would be related to the user interface and the activities. But your new class does not help this, as you have no access to the context calling the login process. Besides, you would want to define your UI logic in the activities itself. Why not return the task returned by signInWithCredential directly so that you can handle the resolution on call site ? Your API would also be more consistent as login would also return a task and you can even ditch the passed callback. You can then also drop the TAG string parameter (btw this also makes more sense since this is an activity specific property, it should not leave its scope).

So you would change the signature of your implementation (and your interface) :

    public Task<Void> login(Account acct)

Now you can change your implementation by simply returning the login task :

    public Task<Void> login(Account acct) {
        AuthCredential credential = GoogleAuthProvider.getCredential(acct.getIdToken(), null);
        return mAuth.signInWithCredential(credential)
    }

You can then delete your OnCompleteListenerWrapper.java and resolve the task on site (in your calling activity) :

  authManager.logout().addOnCompleteListener(new OnCompleteListener() {
    @Override
    public void onComplete(Task<TResult> task) {
      if (task.isSuccessful()) {
          // update UI, which is possible since we are in the caller activity's context
      } else {
          // handle failure, again easy because we are in UI context
      }
    }
  })

DummyGoogleSignInActivity and DummyMainActivity

I think you have misunderstood the usage of dummy classes. You should only implement dummy classes to swap implementations that are not testable. By creating these dummy activities (aka UI controllers) you actually introduce more issues :

• this breaks the separation of concerns : you now have logic definitions for your AuthManager inside UI files
• you now have files that should not be part of the final system that live at the same level as your production files (they get compiled into your product)
• you only use them for rewriting the auth manager
• so now basically you don't really test your actual UI but rather another controller that "replaces" your real UI. But this is actually counter-productive : what you originally wanted was to keep the exact same UI class (so that you can still black box test it and its interactions) even if you can't test other parts of the system such as the authentication system.

These 2 files (dummy activities) should not exist at all. The only dummy class should exist in the test environment only, and should swap at runtime against the untestable code (FirebaseAuthManager) that only describes logic (no UI interaction).

What you want is a DummyAuthManager, defined in your test package, that you swap at runtime when you're instantiating your activities' instrumented tests. That way, you can still test the UI without the need to test the Firebase code and the dummy code never leaves the test environment. This DummyAuthManager will contain contain trivial implementations for your AuthManager interface contract (for instance login and logout would return a resolved task, startLoginScreen would call the tested activity's onActivityResult and getCurrentUser could return some default Account).

So in your androidTest package, you could have a definition that looks like this (I use TaskCompletionSource) to emulate the Task result:

public class DummyAuthManager implements AuthManager {

    private Activity context;

    public DummyAuthManager(Activity act) {
        this.context = act;
    }

    public Task<Void> login() {
        TaskCompletionSource<Void> tcs = new TaskCompletionSource<>();
        tcs.setResult(null); // maybe you'll need to delay it, if this doesn't work see google API
        return tcs.getTask();
    }

    public Task<Void> logout() {
        // same thing
    }

    public void startLoginScreen() {
        // you should put the dummy test code here
        context.onActivityResult(1, 0, null);
    }

    public void getCurrentUser() {
        return new Account("John Doe", "testid", "testtoken");
    }
}

Now when you instantiate your tests, you could swap the implementation in the before rule : your instrumented tests should now look like this :

@RunWith(AndroidJunit4.class)
public class YourClassActivityTest {
    @Rule
    public ActivityTestRule<YourClassActivity> rule = new ActivityTestRule<>(ActivityTestRule.class);

    // swap implementation when the tests start
    @Before
    public void beforeEachTest() {
           AuthManager dummy = new DummyAuthManager(rule.getActivity());
           rule.getActivity().setAuthManager(dummy);
    }
}

MainActivity and GoogleSignInActivity

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/MainActivity.java

Lines 55 to 60 in 65460e9

	public AuthManager getAuthManager(){
	if (mAuth == null){
	mAuth = new FirebaseAuthManager(this, getString(R.string.default_web_client_id));
	}
	return mAuth;
	}

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Lines 100 to 105 in 65460e9

	public AuthManager getAuthManager(){
	if (mAuth == null){
	mAuth = new FirebaseAuthManager(this, getString(R.string.default_web_client_id));
	}
	return mAuth;
	}

It looks like you are trying to implement some sort of Singleton pattern. However, this does not apply here as you're directly controlling the instantiation of the object (recall that the Singleton pattern is useful when the object itself wants to manage its own instances). Why not use basic instantiation and dependency injection ?

This could become :

public class YourClassActivity extends AppCompatActivity {
  private AuthManager auth; // unfortunately we can't instantiate here, because we're missing the context

  @Override
  public void onCreate(Bundle savedInstanceState) {
    ...
    auth = new FirebaseAuthManager(this);
    ...
  }

  public void setAuthManager(AuthManager auth) {
    this.auth = auth;
  }
}

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Lines 107 to 109 in 65460e9

	public boolean onTest(){
	return false;
	}

This code should not exist here. There should be no test related code inside the production code. I see that you are calling this function several times to change the actions if the activity is in test mode or not. But why not abstract everything so that swapping the AuthManager instance takes care of it for you ? Hence you won't need any testing logic inside your UI logic (which is exactly what you want).

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Lines 32 to 36 in f787ecc

	if(onTest()){
	onActivityResult(1,0,null);
	}else{
	getAuthManager().startLoginScreen();
	}

Same goes for this code, but now that you have reimplemented startLoginScreen, you don't even need to test this condition anymore. Just call the function.

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Line 51 in f787ecc

startActivity(new Intent(GoogleSignInActivity.this, MainActivity.class));

this instead of GoogleSignInActivity makes more sense as this is not a static property, we are in the same scope.

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Lines 63 to 81 in e27314d

	if (requestCode == RC_SIGN_IN) {
	GoogleSignInWrapper gsw = new GoogleSignInWrapper(onTest());
	Task<GoogleSignInAccount> task = gsw.getTask(data);
	try {
	// Google Sign In was successful, authenticate with Firebase
	Account account = getRightAccount(task);
	getAuthManager().login(account, new OnLoginCallback() {
	@Override
	public void then(Account acct) {
	if (acct != null) {
	startActivity(new Intent(GoogleSignInActivity.this, MainActivity.class));
	}
	}
	}, TAG);
	} catch (ApiException e) {
	// Google Sign In failed, update UI appropriately
	Log.w(TAG, "Google sign in failed", e);
	}
	}

Wouldn't it make more sense for this part to be part of the FirebaseAuthManager ? I mean that's debatable. Part of me thinks that these are implementation details that can be part of the concrete login() implementation (hence you would also avoid rewriting all of this if you decide to implement user login elsewhere). You could argue that these are not part of Firebase authentication itself, but I'd say that the GoogleSignIn.getSignedInAccountFromIntent(data) call cannot be tested anyway, so I think it would be better to abstract all of this out into the login() function. But you would need to call login with the Intent data instead of an Account. I believe it still makes more sense, or you could add a loginFromIntent method to the interface and concrete classes. However these test related parts :

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Line 64 in e27314d

GoogleSignInWrapper gsw = new GoogleSignInWrapper(onTest());

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Line 68 in e27314d

Account account = getRightAccount(task);

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInActivity.java

Lines 96 to 98 in e27314d

	private Account getRightAccount(Task<GoogleSignInAccount> task) throws ApiException {
	return onTest() ? new Account() : Account.from(task.getResult(ApiException.class));
	}

These should not exist either. That's why it's superior to put the whole previous bit about GoogleSignInAccount into login. Swapping the instance will do that for you, you really should not have any test related code in your controller.

GoogleSignInWrapper

StudyBuddy/app/src/main/java/ch/epfl/sweng/studdybuddy/GoogleSignInWrapper.java

Lines 17 to 29 in 65460e9

	public class GoogleSignInWrapper {
	private boolean onTest;
	public GoogleSignInWrapper(boolean onTest){
	this.onTest = onTest;
	}
	public Task<GoogleSignInAccount> getTask(Intent data){
	if(onTest){
	return null;
	}
	return GoogleSignIn.getSignedInAccountFromIntent(data);
	}
	}

I also don't understand why is this wrapper needed. As mentionned before, shouldn't this be part of the FirebaseAuthManager login method or with the rest of the code of onActivityResult ?