Conversation
…oid into token-migration
…ctivity and MigrationCallbackRegistry.
Generated by 🚫 Danger |
| import android.os.Parcelable | ||
| import kotlinx.parcelize.Parcelize | ||
|
|
||
| @Parcelize |
There was a problem hiding this comment.
This is Kotlin magic ✨. The compiler is able to implement the required functions to satisfy the Parcelable interface by itself since this is a data class.
We should do this for UserAccount in the future.
Generated by 🚫 Danger |
| val callbackKey = intent.getStringExtra(EXTRA_CALLBACK_ID) ?: run { | ||
| SalesforceSDKLogger.e(TAG, "Unable to parse MigrationResult callback id.") | ||
| finish() | ||
| return | ||
| } | ||
| val resultCallback = MigrationCallbackRegistry.consume(callbackKey) ?: run { | ||
| SalesforceSDKLogger.e(TAG, "Unable to retrieve MigrationResult callback.") | ||
| finish() | ||
| return | ||
| } |
There was a problem hiding this comment.
This should never happen, but I will admit it is a scary little gap in the flow. If we somehow hit an error here we can't invoke the error callback.
If anyone has a clever idea let me know. I'd rather not call startActivityForResult since it has been deprecated for years. And it's replacement registerForActivityResult needs to be called from an Activity.
There was a problem hiding this comment.
I haven't read everything yet, but could you use this@TokenMigrationActivity since that's captured in this closure?
| <!-- Token Migration Activity --> | ||
| <activity android:name="com.salesforce.androidsdk.ui.TokenMigrationActivity" | ||
| android:excludeFromRecents="true" | ||
| android:theme="@style/AccountSwitcher" |
There was a problem hiding this comment.
Should I rename this style to something more generic?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2837 +/- ##
============================================
+ Coverage 63.24% 63.87% +0.62%
- Complexity 2825 2856 +31
============================================
Files 216 219 +3
Lines 16982 17186 +204
Branches 2418 2452 +34
============================================
+ Hits 10741 10977 +236
+ Misses 5072 5008 -64
- Partials 1169 1201 +32
🚀 New features to boost your workflow:
|
| * and replace the existing credentials for the user. | ||
| */ | ||
| @Suppress("UnusedReceiverParameter") | ||
| fun UserAccountManager.migrateRefreshToken( |
There was a problem hiding this comment.
Taking the success and error callbacks as up-front parameters works so no change required. I wondered if this could benefit from returning a Result. The call would look something like this pseudo-code then:
UserAccountManager.getInstance().migrateRefreshToken(
userAccount = user, // defaults to current user if not set
appConfig = config
).onSuccess { user ->
// success lambda
}.onError { error -> // I saw there's currently three parameters, but maybe they all fold into the error here?
// error lambda
}
There was a problem hiding this comment.
I had the same thought. Originally I had a function in the AppFlowTester's RestUtils class that called migrateRefreshToken and returned a Result but I ended up just not using it.
I don't think it makes sense to change the signature to Result since the lambdas passed in flow all the way through AuthenticationUtilities. But I will take a look in case doing so lets us get rid of MigrationCallbackRegistry.
libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManagerExtension.kt
Outdated
Show resolved
Hide resolved
libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/TokenMigrationActivity.kt
Outdated
Show resolved
Hide resolved
| // Remove the existing Account from AccountManager so createAccount can create a fresh one | ||
| val existingAccount = userAccountManager.buildAccount(duplicateUserAccount) | ||
| if (existingAccount != null) { | ||
| SalesforceSDKManager.getInstance().clientManager.removeAccount(existingAccount) |
There was a problem hiding this comment.
What was the impact of not doing the removeAccount until now?
libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/TokenMigrationActivity.kt
Outdated
Show resolved
Hide resolved
wmathurin
left a comment
wmathurin
left a comment
There was a problem hiding this comment.
LGTM - the UI tests (especially the one combining multi users + migration) will catch anything we missed.
After writing that, I went and checked the UI tests on iOS and realized we don't have a test that does migration with multi users ;-) |
…iometricAuthPolicy and handleDuplicateUserAccount. Split out TokenMigrationView and add tests.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| androidTestImplementation("androidx.test.espresso:espresso-core:3.7.0") | ||
| androidTestImplementation("androidx.test.ext:junit:1.3.0") | ||
| androidTestImplementation("androidx.arch.core:core-testing:2.2.0") | ||
| androidTestImplementation("androidx.compose.ui:ui-test-junit4:$composeVersion") |
There was a problem hiding this comment.
| A newer version of androidx.compose.ui:ui-test-junit4 than 1.8.2 is available: 1.10.3 |
|
I will send a follow-up PR with UI tests as this one has gotten very large. |
3 participants
This PR adds a
migrateRefreshTokenfunction to an new KotlinUserAccountManagerExtentionclass.Example usage:
Implementing it in the existing Java class would have required defining a new public interface just for the callback functions since lambda functions can not be directly passed as parameters. I did not want to introduce the extra public API since I expect we will convert the class to Kotlin soon. I attempted to convert the class for this PR but the static references converted to companion properties and return type nullability would have introduced minor breaking changes (that would be better left for 14.0).
migrateRefreshTokenlaunches a newTokenMigrationActivitythat does most of the work. A newMigrationCallbackRegistryobject facilitates passing the callback functions from the entry point API to the activity, and a code comment explains why that is necessary. Most of the code inTokenMigrationActivityis theWebViewClientimplementation that is very similar to the one inLoginActivity, but unfortunately they are a little too coupled to share. There is very little else in the activity because it utilizes the existingLoginViewModelto do most of the work.The Activity UI is very minimal. When the WebView is loading, it and the Activity background are transparent with a 50% tint. The same progress indicator from standard login is reused, so if the app has customized it that should be reflected here as well.
No Interaction Required, Migration Failure, Interaction Required
TODO:
buildAuthWebview, AuthenticationUtilities, etc)UI Tests- Will send a follow-up PR.