Add Login Options to Dev Menu #2801
Conversation
| // If debug LoginOptions were changed reload the webview. | ||
| if (SalesforceSDKManager.getInstance().isDebugBuild && webView.url != viewModel.loginUrl.value) { | ||
| viewModel.reloadWebView() | ||
| } |
There was a problem hiding this comment.
If anyone has a better was to implement this I am all for it. Unfortunately, calling reload from LoginOptionsActivity does not update the webview (because it is not currently rendered, I assume).
There was a problem hiding this comment.
If login url is not changed, will that reload?
There was a problem hiding this comment.
Yes. If WebServer Flow is used getting a new authorization url cause the url to change because code verifier will be new. If UserAgent flow is used we set the URL to about_blank before getting a "new" authorization url (which will be identical to the old one if nothing else changes) to force the reload.
It is just annoying I have to call it here also.
There was a problem hiding this comment.
But if web server flow is not being used then loginUrl won't change.
On iOS, I restart authentication when coming out of showing the login options: https://github.com/forcedotcom/SalesforceMobileSDK-iOS/pull/3950/files#diff-1334e2942ea2928b6db7bca76fdebd960b717bc0463c404244b998a86ee08121R494
There was a problem hiding this comment.
The WebView does reload if User Agent Flow is used, that was an issue with 13.0 that we fixed.
Since we are just popping the LoginOptionsActivity off of the back stack I have no way of telling LoginActivity directly (like through intent data or something) that it needs to reload. I think the new loginDevMenuReload boolean I just added to SalesforceSDKManager is a decent enough solution though.
Generated by 🚫 Danger |
| @Preview(showBackground = true) | ||
| @Composable | ||
| fun OptionsTogglePreview() { |
There was a problem hiding this comment.
Pro Tip: If you simply move your Composables outside of the activity where you are calling things like SalesforceSDKManager.getInstance() you can add previews without rendering issues.
| } | ||
| } | ||
|
|
||
| val previewBootConfig = object : BootConfig() { |
There was a problem hiding this comment.
Maybe for the preview, we use a bogus remote access consumer key / redirect uri?
There was a problem hiding this comment.
Is this bootconfig file sticking around?
There was a problem hiding this comment.
Do you mean: are we keeping a bootconfig file going forward, or are we going to keep checking in the bootconfig file?
There was a problem hiding this comment.
Is that file going to continue to exist for the foreseeable future? AFAIK, it is. So I just read the values from it for the preview instead of having them hardcoded or making up bogus ones.
| OutlinedTextField( | ||
| value = dynamicScopes.value ?: "", | ||
| onValueChange = { dynamicScopes.value = it }, | ||
| label = { Text("Scopes (comma separated)") }, |
There was a problem hiding this comment.
On iOS, I expect space delimited scopes.
|
The iOS equivalent: forcedotcom/SalesforceMobileSDK-iOS#3950 |
| @Test | ||
| fun clearWebViewCache_CallsWebViewClearCache_WithTrueParameter() { | ||
| // Arrange | ||
| val mockWebView = mockk<WebView>(relaxed = true) | ||
|
|
||
| // Act | ||
| viewModel.clearWebViewCache(mockWebView) | ||
|
|
||
| // Assert | ||
| verify { mockWebView.clearCache(true) } | ||
| } | ||
|
|
There was a problem hiding this comment.
After many hours cannot explain this, but mocking the webview here causes all tests in LoginActivityTest class to hang and never finish. Killing the while test run.
Unmocking at the end of the suite, or in the test itself with a finally did not fix the issue. Creating a real webview and using spyk did not fix the issue.
The test only covers the one line passthrough of clearCache so I don't think it is worth keeping.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2801 +/- ##
============================================
+ Coverage 56.92% 61.26% +4.34%
- Complexity 2527 2747 +220
============================================
Files 211 214 +3
Lines 16919 16931 +12
Branches 2369 2453 +84
============================================
+ Hits 9631 10373 +742
+ Misses 6241 5394 -847
- Partials 1047 1164 +117
🚀 New features to boost your workflow:
|
On iOS, I also ended up adding more details in the dev infos screen (and changed it to have collapsible sections). See forcedotcom/SalesforceMobileSDK-iOS#3950 (comment) |
Love it! I was thinking about categories a lot like that but didn't get around to implementing it. I'll add it when I get a chance. |
…on change. Add test for LoginOptionsActivity.
Generated by 🚫 Danger |
|
Last thing to do for this PR is to match the new DevMenu sections from the iOS PR. I have tests written for |
|
New Dev Info Screen: |
| import java.text.SimpleDateFormat | ||
| import java.util.Locale | ||
|
|
||
| data class DevSupportInfo( |
There was a problem hiding this comment.
On iOS I kept the Dev Info "dumb" to be able to support arbitrary grouping of rows - including rows provided by the app or subclasses of SalesforceSDKManager e.g. SmartStoreSDKManager here.
There was a problem hiding this comment.
Hmmm, I wanted to keep as much of the sorting logic out of the UI as I could... but I forgot about the subclasses. It would be easy to add the subclasses data as additional categories, but if you think I should revert back to the old simple list I can.
There was a problem hiding this comment.
Right now the dev infos provided by subclasses of SalesforceSDKManager are no longer shown (e.g. https://github.com/forcedotcom/SalesforceMobileSDK-Android/blob/dev/libs/SmartStore/src/com/salesforce/androidsdk/smartstore/app/SmartStoreSDKManager.java#L470) .
At least we should fix that but we would still be breaking apps that implemented their own SDKManager and overrode getDevSupportInfos.
That's why I ended up going with the list that has "section:xxx" markers. It's a bit low tech ;-) but it keeps everything working.
Maybe the move to a typed dev infos could be done in 14.0??
There was a problem hiding this comment.
DevSupportInfo
var smartStoreValues: List<Pair<String, String>>? = null
SmartStoreSDKManger
@Override
public @NotNull DevSupportInfo getDevSupportInfo() {
DevSupportInfo devInfo = super.getDevSupportInfo();
devInfo.setSmartStoreValues(Arrays.asList(
new Pair<>("SQLCipher version", getSmartStore().getSQLCipherVersion()),
new Pair<>("SQLCipher Compile Options", TextUtils.join(", ", getSmartStore().getCompileOptions())),
new Pair<>("SQLCipher Runtime Setting", TextUtils.join(", ", getSmartStore().getRuntimeSettings())),
new Pair<>("User SmartStores", TextUtils.join(", ", getUserStoresPrefixList())),
new Pair<>("Global SmartStores", TextUtils.join(", ", getGlobalStoresPrefixList())),
new Pair<>("User Key-Value Stores", TextUtils.join(", ", getKeyValueStoresPrefixList())),
new Pair<>("Global Key-Value Stores", TextUtils.join(", ", getGlobalKeyValueStoresPrefixList()))
));
return devInfo;
}
I was thinking this would be okay? The idea of grouping values based on a string that could change feels fragile but I will take a closer look at the iOS implementation before committing anything.
There was a problem hiding this comment.
- DevSupportInfo should not know about SmartStore.
- That would work for SmartStoreSDKManager but would still break apps (if they exist) that override getDevSupportInfos().
Maybe DevSupportInfo could have a field for other infos which would be shown in the UI after everything else.
The DevSupportInfo that you create in SalesforceSDKManager could populate that new field by calling the existing getDevSupportInfos().
The getDevSupportInfos() method in SalesforceSDKManager would return an empty list.
SmartStoreSDKManager and other managers could be left unchanged and their additional infos would keep showing up in the UI.
If we want some grouping in the additional infos, we could do the "section:xxx" hack I did on iOS or do something more structured (but that would mean changing the subclasses to take advantage of it).
There was a problem hiding this comment.
- DevSupportInfo should not know about SmartStore.
I think if SmartStore data is optional it isn't a problem. The UI has to check if SmartStore data exists one way or another, this is just more structured.
- That would work for SmartStoreSDKManager but would still break apps (if they exist) that override getDevSupportInfos().
Good point. I am going to try to massage the list from the existing API (even if it has been modified by a subclass) into the DevSupportInfo class. If I make everything nullable and allow additional sections to be added it should work well.
There was a problem hiding this comment.
@wmathurin I have the approach in my latest commit. The DevSupportInfo class now allows for everything to be null on the main constructor, has a secondary constructor similar to the original constructor and features a createFromLegacyDevInfos function to instantiate the class from List<String>. A unit test insures that given the same input data the objects created by the secondary constructor and the createFromLegacyDevInfos function are identical.
So whatever is in the devSupportInfos (list of string) will be parsed to the expected categories. Removed values simply will not show up, completely empty sections will now show up and any additional info will be added to the first basic non-collapsable section. SmartStoreSDKManager's override of devSupportInfos works as expected, creating a new "Smart Store" section.
Code to be used post 14.0 is commented out in place.
Job Summary for GradlePull Request :: test-android
|
Job Summary for GradlePull Request :: test-android
|
…oid into login_options # Conflicts: # .github/workflows/reusable-workflow.yaml
…oid into login_options
Job Summary for GradlePull Request :: test-android
|
…om code coverage.
3 participants
In this PR:
The work to actually use a dynamically supplied boot config will be in the next PR.