@W-20618449: [MSDK 13.1][Android] Cannot login GUS using Welcome endpoint (13.1.1 Patch) #2812
Conversation
Generated by 🚫 Danger |
JohnsonEricAtSalesforce
force-pushed
the
master
branch
from
2437420 to
318e75c
Compare
JohnsonEricAtSalesforce
force-pushed
the
master
branch
from
544e855 to
c6db752
Compare
JohnsonEricAtSalesforce
changed the title
…point (Squashed From 13.2)
JohnsonEricAtSalesforce
force-pushed
the
master
branch
from
c6db752 to
cf39cab
Compare
|
I verified the two failed tests actually pass for me locally. To the best of my knowledge, current state on 13.1.0 did have known unreliable tests. Do we want to pull in the fixes for those as part of this? |
|
This has passed testing by the app team plus my own testing. It ought to be good-to-go (G2G!) |
brandonpage
left a comment
brandonpage
left a comment
There was a problem hiding this comment.
👍 Lets wait until everyone is back in the new year before merging.
| isUsingFrontDoorBridge: Boolean = viewModel.isUsingFrontDoorBridge, | ||
| singleServerCustomTabActivity: Boolean = viewModel.singleServerCustomTabActivity, | ||
| ) { | ||
| if ((singleServerCustomTabActivity.or(isBrowserLoginEnabled)).and(!isUsingFrontDoorBridge)) { |
There was a problem hiding this comment.
I don't think it matters for this context, but it should be noted that or and and differ from || and && in that they do not short-circuit.
There was a problem hiding this comment.
That's true. Kotlin's methods are beneficial here since they increase code coverage. The short-circuit operators produce untestable logic branches which are revealed by CodeCov. That seems well documented online. In many cases I was able to retain the operators and still hit 100% coverage, but this one stumped even the AI tools and they recommended the methods - which worked.
We wouldn't want to use Kotlin's methods if the expressions were costly or had side-effects, obviously. In this case, they're simple booleans.
There was a problem hiding this comment.
Oh, interesting! Great use case, especially for non-nullable values.
There was a problem hiding this comment.
Thanks
There was a problem hiding this comment.
We wouldn't want to use Kotlin's methods if the expressions were costly or had side-effects, obviously. In this case, they're simple booleans.
Maybe there are cases where you specifically would want to ensure a side-effect happens? Like:
if (override.and(atomicInt.addAndGet(1) > MAX))
and guarantees the add happens no matter the value of override. Seems pretty niche though.
| val pendingLoginServerUnwrapped: String = pendingLoginServer ?: return | ||
|
|
||
| // Recall this pending login server for reference by future updates. | ||
| previousPendingServer = pendingLoginServerUnwrapped |
There was a problem hiding this comment.
Seems like this could be combined into one line?
There was a problem hiding this comment.
This method makes multiple references to the local variable previousPendingServerUnwrapped, so this statement assigns the member value previousPendingServer while the method only uses the thread-safe local. There's also the nullability of the value since it's derived from a nullable MediatorLiveData. Those two factors were what kept this assignment separate when I looked at it.
We've had a couple places in the code lately where multiple references to mutable members in a method scope caused issues, like crashes in tests. That's why I kept the logic referring only to the local.
Years ago on another project I had used a custom non-null LiveData that helped simplify this kind of logic. I haven't looked into something like that in our context, but it could be helpful someday.
|
I verified once again that the failed tests here actually do pass locally. We're aware the configuration on |
JohnsonEricAtSalesforce
merged commit 7682fb1
into
forcedotcom:master
2 participants
🎸 Ready For Review! 🥁
This is the 13.1.1 back-port of #2807, which resolves issues when selecting Salesforce Welcome Discovery tiles that require browser-based auth using the device browser instead of the web view. That change is currently approved for 13.2.0 on
dev.Much of the description there applies here, since I cherry-picked those changes into 13.1.1 (
master). We've reviewed and tested that logic extensively and the actual conflict betweendevandmasterat this time is quite low. Also, the merge of 13.1.1 frommasterback todevfor 13.2.0 will be easier if the source is similar.I rolled back any changes related to increasing code coverage to minimize the footprint for 13.1.1 and reduce risk.
Here's what looks like the exact same test video, now recorded against this pull request, which shows the server picker in action plus the switch to WSC Discovery and selection of a tile that requires the external browser custom tab. Note, I cannot actually sign in because of the still present consumer key restrictions for the internal app. I should be able to do that within the next day.
Screen_recording_20251217_171130.mp4