-
Notifications
You must be signed in to change notification settings - Fork 393
@W-20784385: [MSDK Android] Only Show Authorized Hosts Device Management Parameter In-Operative #2816
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
@W-20784385: [MSDK Android] Only Show Authorized Hosts Device Management Parameter In-Operative #2816
Conversation
| PickerBottomSheet( | ||
| pickerStyle, | ||
| sheetState, | ||
| addButtonVisible = viewModel.serverPickerAddConnectionButtonVisible, |
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.
The view model is available at this level before each individual property is passed to the next composable, so we pick up the new property just like all the others. The view model knows specifically what it wants to hide and so does this method. The method receiving this only knows it it is or isn't showing the "add" button. That seems to make sense since "add" is a single logic condition at that method which covers multiple add button types.
| internal val defaultTitleText: String | ||
| get() = if (loginUrl.value == ABOUT_BLANK) "" else selectedServer.value ?: "" | ||
|
|
||
| internal val serverPickerAddConnectionButtonVisible = getRuntimeConfig(SalesforceSDKManager.getInstance().appContext).getBoolean(OnlyShowAuthorizedHosts) |
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.
This is the exact same logic as it was prior to MSDK 13.0.0.
Job Summary for GradlePull Request :: test-android |
…ent Parameter In-Operative
55c7a2b to
afb4336
Compare
| PickerBottomSheet(pickerStyle, sheetState, list, selectedListItem, onItemSelected, getValidServer, | ||
| addNewLoginServer, removeLoginServer, addNewAccount) | ||
| PickerBottomSheet( | ||
| addButtonVisible = true, |
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.
I added the parameter names here to help the method call evolve as parameters may be added, removed or re-arranged.
…ent Parameter In-Operative
| pickerStyle = pickerStyle, | ||
| sheetState = sheetState, |
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.
NIT: I don't see the point of naming the parameters when it adds no information.
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.
It really helps avoid parameter transposition bugs. I've been through enough of those in both Java and Kotlin that I do see the value. Here's a Medium that I thought poses the pros and cons well.
https://bterczynski.medium.com/some-arguments-in-favor-of-kotlin-named-parameters-7abd35808c35
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.
There is no a possible transposition bug here because here since they types aren't the same. It would be a compiler error, not a transposition but we could miss. Not a bad habit to get into though reguardless.
| PickerBottomSheet( | ||
| pickerStyle, | ||
| sheetState, | ||
| addButtonVisible = viewModel.serverPickerAddConnectionButtonVisible, |
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.
NIT: I find it cleaner to add parameters with default values to the end of the list.
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.
I did that in d8e5336
I used to do something like this years ago, but then realized it can be difficult to maintain if some parameters lose their default value or other parameters gain one as the codebase evolves. Re-arranging the parameters in the list to meet this expectation can be difficult as methods evolve also. It's also difficult to re-arrange later since it can introduce parameter transposition bugs.
Some of this, of course, is different for functions intended for DSL use.
| PickerBottomSheet(pickerStyle, sheetState, list, selectedListItem, onItemSelected, getValidServer, | ||
| addNewLoginServer, removeLoginServer, addNewAccount) | ||
| } | ||
| PickerBottomSheet( |
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.
Can we add a test that asserts the button is not shown when false is passed in?
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.
You bet. I was just waiting for the first CodeCov run to see where the coverage landed. I'll have that test or more in soon.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #2816 +/- ##
============================================
+ Coverage 61.60% 62.29% +0.68%
- Complexity 2759 2791 +32
============================================
Files 215 215
Lines 16995 16977 -18
Branches 2474 2470 -4
============================================
+ Hits 10470 10575 +105
+ Misses 5355 5222 -133
- Partials 1170 1180 +10
🚀 New features to boost your workflow:
|
…ent Parameter In-Operative (Move `addButtonVisible` In Positional Parameters)
…ent Parameter In-Operative (Move `addButtonVisible` In Positional Parameters, Tests `serverList_Displays_DisplaysAddNewConnectionButton` And `serverList_Displays_HidesAddNewConnectionButton`)
…ent Parameter In-Operative (Bug Fixes, Tests `pickerBottomSheet_publicApiUserAccountPicker_displaysUserAccountPicker` And `pickerBottomSheet_publicApiLoginServerPicker_displaysLoginServerPicker`)
| val loginServerManager = SalesforceSDKManager.getInstance().loginServerManager | ||
| val userAccountManager = SalesforceSDKManager.getInstance().userAccountManager | ||
| val activity = LocalContext.current.getActivity() | ||
| TestablePickerBottomSheet( |
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.
@brandonpage, I was looking at the test coverage and noticed that the public PickerBottomSheet method doesn't have any coverage, though it does have a lot of logic in it. I was wondering if that was because it was the layer that (internally) resolved all the external dependencies and extracted parameters for the internal PickerBottomSheet that's not visible to the public API.
I thought to increase the coverage we could proxy this dependency and parameter-resolution layer into a new internal method that's mockable and testable, then only expose a one-liner implementation to the public API. I recall we thought about doing that for other feature work in another discussion.
How's it look? It's not required at all, but I'm always eyeballing ways to increase test coverage.
| } | ||
|
|
||
| val context = getInstrumentation().targetContext | ||
| val button = composeTestRule.onNode(hasText(context.getString(sf__account_selector_text))) |
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.
@brandonpage, sorry for another change after your review but I did extract the strings in the tests so they'll be easier to maintain.
…ent Parameter In-Operative (Correct Context Mock In `ClientManagerMockTest.kt`)
62df659
into
forcedotcom:dev
🥁 Ready For Review 🎸
This restores logic from prior to version 13.0.0 which hides the "Add New Connection" button in the server picker according to the mobile device management properties.
See this documentation: https://developer.salesforce.com/docs/platform/mobile-sdk/guide/oauth-mdm.html
Here's a screenshot of the relevant logic as it existed prior to 13.0.0.

This is a recording on the manual test. The first run of the app allows adding connections and the second run of the app does not. Notice that the add user button is not affected though it is the same code.