-
Notifications
You must be signed in to change notification settings - Fork 393
@W-21006714: singleServerCustomTabActivity is malfunctioning (13.2) #2832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2832 +/- ##
============================================
- Coverage 62.64% 62.61% -0.03%
+ Complexity 2809 2803 -6
============================================
Files 216 216
Lines 16997 16982 -15
Branches 2414 2418 +4
============================================
- Hits 10647 10633 -14
+ Misses 5188 5187 -1
Partials 1162 1162
🚀 New features to boost your workflow:
|
libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
57d24ac to
2698993
Compare
…implification And Unit Tests For 100% Patch Code Coverage)
2698993 to
46f8c83
Compare
| ) : Observer<String?> { | ||
| override fun onChanged(value: String?) { | ||
| if (!sdkManager.isBrowserLoginEnabled && !viewModel.isUsingFrontDoorBridge && value != null) { | ||
| if (!sdkManager.isBrowserLoginEnabled && !singleServerCustomTabActivity && !viewModel.isUsingFrontDoorBridge && value != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part 1 is making sure loginUrl is not set when using singleServerCustomTabActivity since the web view will not be used.
| ) : Observer<String> { | ||
| override fun onChanged(value: String) { | ||
| if (sdkManager.isBrowserLoginEnabled && !viewModel.isUsingFrontDoorBridge) { | ||
| val useBrowserCustomTab = sdkManager.isBrowserLoginEnabled || singleServerCustomTabActivity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part 2 is making sure customBrowserTabUrl is set when using singleServerCustomTabActivity.
| if (activity.viewModel.singleServerCustomTabActivity) { | ||
| // Show blank page and spinner until PKCE is done. | ||
| activity.viewModel.loginUrl.value = ABOUT_BLANK | ||
| finish() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part 3 is restoring the missing finish call in the activity result.
The repeated cycle of tapping back and returning to a brand new instance of LoginActivity which re-displays the browser custom tab is because when LoginActivity finishes without authenticating the default behavior of SalesforceActivity.onResume() is to detect there is no authenticated REST Client and restart LoginActivity. That's also what caused both 13.0.0 and now this version to briefly show a new LoginActivity while authentication is occurring in the custom tab.
The behavior here now matches 13.0.0 so I reckon it resolves the bug. Would we ever need this to behave better in an app that defaults to login?
brandonpage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks you for the tests!
🎸 Ready For Review 🥁
This is the 13.2 counterpart for #2831
This resolves issues with the function of the
singleServerCustomTabActivityproperty which was introduced in MSDK 13.0.0. The logic worked as intended at that time, but had regressed partially in 13.1.0 and fully in the pending 13.1.1 release.The MSDK sample apps behave in exactly the same way with this update as they did in 13.1.0. Here's a video that demonstrates that in the REST Explorer sample app. Note that the activity timing in this sample app isn't perfect since this app is not built as the target app would be and defaults to the login view instead of an app activity.
Screen_recording_20260122_180115.webm
Also, this property now has detailed unit tests and high code coverage to prevent future regression.