feat: reimplement google smart lock for login #1572
Conversation
addiegupta
force-pushed
the
1535-googlesignin
branch
3 times, most recently
from
3ffb471 to
86900cb
Compare
|
@iamareebjamal please review |
|
Peer reviews first please |
|
@liveHarshit @angmas1 please review |
As you had asked me to work on this, I thought it best to directly ask you for a review as you would have the best insight on this |
|
Peer Reviews teach the reviewing peers on how to review. If they approve and I request some changes, then it means they missed something |
|
|
||
| override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) { | ||
|
|
||
| if (BuildConfig.FLAVOR == "playStore" && requestCode == RC_CREDENTIALS_READ) { |
There was a problem hiding this comment.
Declare string as constant.
| smartAuthViewModel.resolvableApiException.nonNull().observe(viewLifecycleOwner, Observer { | ||
| if (smartAuthViewModel.requestCode.value != null) { | ||
| SmartAuthUtil.handleResolvableApiException( | ||
| it, requireActivity(), smartAuthViewModel.requestCode.value!!) |
There was a problem hiding this comment.
Do not use non-null assertions
There was a problem hiding this comment.
In latest commit: As resolvableApiException and requestCode are always updated together, I have put them together in a Pair in the ViewModel and the null case is thus avoided.
|
Build is failing |
|
Run gradlew spotlessApply |
|
|
||
| override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) { | ||
|
|
||
| if (BuildConfig.FLAVOR == getString(R.string.playStore) && requestCode == RC_CREDENTIALS_READ) { |
There was a problem hiding this comment.
Define as const in the class itself, not in the resources file.
|
@liveHarshit any other changes that I should make? Or should I proceed with squashing commits? |
| import org.koin.androidx.viewmodel.ext.android.sharedViewModel | ||
| import org.koin.androidx.viewmodel.ext.android.viewModel | ||
|
|
||
| const val PLAY_STORE_BUILD_FLAVOR = "playStore" |
There was a problem hiding this comment.
Declare it once. It is public value, you can use it in different class also.
There was a problem hiding this comment.
updated. will squash once you approve
addiegupta
force-pushed
the
1535-googlesignin
branch
from
fa521a9 to
f1b0ee1
Compare
Fixes: fossasia#1535 Changes: - Smart Lock features added for Play Store Build Variant - At Login, an account is chosen. After entering password, Google Smart Lock prompts user to save password for future use - On next login, account and password data is entered automatically - In the previous implementation of this feature, viewmodel classes had some restricted classes being used Now all classes related to android imports are being handled in LoginFragment and MainActivity, keeping ViewModel android import free - A SmartAuthUtil class has been made to keep LoginFragment and MainActivity free of Google Play Services imports so that FDroid can be supported - SmartAuthUtil and SmartAuthViewModel classes for FDroid have also been made which don't do anything - Build Flavor is being checked before using smart lock features
addiegupta
force-pushed
the
1535-googlesignin
branch
from
f1b0ee1 to
bcc391b
Compare
|
Travis build was failing. removed unnecessary blank line |
4 participants
Fixes: #1535
Changes:
Smart Lock features added for Play Store Build Variant
Now all classes related to android imports are being handled in LoginFragment and MainActivity, keeping ViewModel android import free
The need for creating SmartAuthUtil arises as the other alternative is to create 2 files each of LoginFragment and MainActivity for both buildFlavors which might be undesirable
Screenshots for the change:
First Login
Second Login:
As I have disabled the AutoLogin in my Google Account settings, therefore I need to select 2 more things in the second GIF(first account and then password). This setting is enabled by default for all accounts and for those accounts, email and password will be added automatically.
Additional context:
Implementing these Google APIs has to be done in this scattered manner as it has been designed to used callbacks like
onActivityResultwhich are not very suitable for the architecture of this project. I have tried quite a few things but the best solution I could arrive at was this. Please guide me in improving this if you have any suggestions.