Representative demo app for password auth #69
Conversation
Simple two activity demo app for password authentication.
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
=========================================
Coverage 70.94% 70.94%
Complexity 436 436
=========================================
Files 58 58
Lines 2437 2437
Branches 241 241
=========================================
Hits 1729 1729
Misses 641 641
Partials 67 67Continue to review full report at Codecov.
|
| * Indicates whether a progress bar should be displayed to the user, while an asynchronous | ||
| * request is made. | ||
| */ | ||
| public final ObservableBoolean loading = new ObservableBoolean(true); |
There was a problem hiding this comment.
Compromise: renamed to showLoading (to make it clear that this impacts the UI). Generally I prefer to reserve "is" prefixes for boolean getter methods.
| /** | ||
| * Whether the password entry field should be displayed at this time. | ||
| */ | ||
| public final ObservableBoolean showPasswordField = new ObservableBoolean(false); |
There was a problem hiding this comment.
Nit: rename to shouldShowpasswordField
There was a problem hiding this comment.
Leaving as showPasswordField for consistency with showLoading.
| * Whether to show a generic error message to the user - this happens when requests fail | ||
| * in a way that we cannot recover from, other than asking the user to potentially try again. | ||
| */ | ||
| public final ObservableBoolean errorOccurred = new ObservableBoolean(false); |
There was a problem hiding this comment.
Nit: rename to hasErrorOccurred
|
|
||
| doManualAuthentication( | ||
| R.string.enter_email_prompt, | ||
| null); |
There was a problem hiding this comment.
Nit: add comment with param name, similarly for other non-obvious parameters.
| .setDisplayName(displayName) | ||
| .setDisplayPicture(displayPicture) | ||
| .setPassword(hintPassword) | ||
| .build()); |
There was a problem hiding this comment.
Opened a issue to add a utlity transformer for this operation here: #71
| public UserRepository(@NonNull OpenYoloDemoApplication application) { | ||
| mUserDatabase = Room.databaseBuilder(application, UserDatabase.class, "userdb") | ||
| .build(); | ||
| mSharedPrefs = application.getSharedPreferences("authInfo", Context.MODE_PRIVATE); |
There was a problem hiding this comment.
Nit: Move inline strings to constants.
| setCurrentUserEmail(null); | ||
| } | ||
|
|
||
| private String getCurrentUserEmail() { |
There was a problem hiding this comment.
To stay consistent add @Nullable annotation.
| boolean isExistingAccount(@NonNull String email); | ||
|
|
||
| /** | ||
| * Attempts to create and persist a new user account account with the specified properties. |
There was a problem hiding this comment.
Please document that returned boolean signals success/failure.
| @NonNull String password); | ||
|
|
||
| /** | ||
| * Attempts to authenticate an existing user with the provided credentials. |
| if (userPassword.isEmpty()) { | ||
| authPrompt.set(getResourceString(R.string.existing_account_enter_password)); | ||
| showPasswordField.set(true); | ||
| } else if (mUserDataSource.authWithPassword(userEmail, userPassword)) { |
There was a problem hiding this comment.
Nit: Avoid side effects in conditional expression. Similarly for if conditional below.
| /** | ||
| * A base class for use by view models that utilize Android data binding. | ||
| */ | ||
| public abstract class ObservableViewModel extends AndroidViewModel implements Observable { |
There was a problem hiding this comment.
Would you mind explaining why you are using this base class?
There was a problem hiding this comment.
In order to be able to bind a value in a layout, it must be an Observable. AndroidViewModel is not an observable, so I had to create a subclass which implements this to allow data binding. As this composition of view model and observable is reusable (it never really changes from view to view), it was worth creating a base type for extension by my other views, rather than extending AndroidViewModel and copying this implementation into every type.
| * Specifies the navigator for use in communicating with the activity environment. | ||
| */ | ||
| @MainThread | ||
| public void setNavigator(@NonNull MainNavigator navigator) { |
There was a problem hiding this comment.
The documentation (https://developer.android.com/reference/android/arch/lifecycle/ViewModel.html) I found suggests a ViewModel should "scary bold text" never have a reference it's associated activity. It looks like that's what you're doing here. IMO it seems nice to have the business logic of transitions in here. It's not clear why that divide is made. Any idea why they suggest avoiding this?
There was a problem hiding this comment.
It's to avoid leaking activity references, but I've copied the pattern here from their own architecture component demos:
However, they do hold it in a weak reference to avoid leaks, so I'll at least have to fix that.
Simple two activity demo app for password authentication.
This also has some conflated changes (apologies), which relax our Javadoc validation as I intend to switch it to Markdown based doc comments, like in AppAuth.