@W-20978376: User Story - [Android] Remove supportWelcomeDiscovery flag in dev #2823
Conversation
Job Summary for GradlePull Request :: test-android
|
Job Summary for GradlePull Request :: test-android
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2823 +/- ##
============================================
- Coverage 63.27% 63.24% -0.04%
+ Complexity 2831 2825 -6
============================================
Files 216 216
Lines 16997 16980 -17
Branches 2414 2417 +3
============================================
- Hits 10755 10739 -16
+ Misses 5073 5072 -1
Partials 1169 1169
🚀 New features to boost your workflow:
|
| val isNewServer = viewModel.loginUrl.value?.startsWith(value) != true | ||
| val valueUrl = value.toUri() | ||
| val loginUrl = viewModel.loginUrl.value?.toUri() | ||
| val isNewServer = loginUrl?.host != valueUrl.host || loginUrl?.path != valueUrl.path |
There was a problem hiding this comment.
This looks like an unrelated behavior change. Why is it here?
There was a problem hiding this comment.
It's a fix for W-20935841 that we ran into when testing WSC and WSC/disco login. The fix was done in master in the same PR that removed supportWelcomeDiscovery.
There was a problem hiding this comment.
Ah, thank you I wasn't aware of that fix. Having both welcome.salesforce.com and welcome.salesforce.com/discovery seems like a very niche issue. And I know for a fact that I explicitly used startsWith instead of directly comparing the path to prevent bugs where login would fail from running getAuthorizationUrl when we shouldn't have as that causes the PKCE code and verifier to be set with new values. Perhaps the scenario this was intended to prevent does not exist anymore but I'm not 100% sure.
There was a problem hiding this comment.
Circling back:
- The
startsWithis not a carry over from the oldOAuthWebviewHelper. - Looking at the current code,
LoginUrlSourceis only ever called whenselectedServerchanges and I do not see a scenario. If it does not happen mid authentication there should not be any issues with getting new code/verifier.
Seems like whatever issue this did avoid is no longer possible.
There was a problem hiding this comment.
Wow! Thanks for the detailed analysis ⭐️⭐️⭐️⭐️⭐️
brandonpage
dismissed
their stale review
Unsure if issues raised is a bug or not.
| val intent = activity.intent | ||
| val data = intent.data | ||
| if (data == null) { | ||
| Log.i("CodeCov", "1") | ||
| } | ||
| if (data?.host?.equals(pendingServerUri.host) == true) { | ||
| Log.i("CodeCov", "2.0") | ||
| } | ||
| if (data?.host?.equals(pendingServerUri.host) == false) { | ||
| Log.i("CodeCov", "2.1") | ||
| } | ||
| if (data?.path?.equals(pendingServerUri.path) == true) { | ||
| Log.i("CodeCov", "3.0") | ||
| } | ||
| if (data?.path?.equals(pendingServerUri.path) == false) { | ||
| Log.i("CodeCov", "3.1") | ||
| } | ||
| if (intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host)) { | ||
| Log.i("CodeCov", "4.0") | ||
| } | ||
| if (!(intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host))) { | ||
| Log.i("CodeCov", "4.1") | ||
| } | ||
| if (((data?.host?.equals(pendingServerUri.host) == true).and(data?.path?.equals(pendingServerUri.path) == true)).or(intent.getStringExtra(EXTRA_KEY_LOGIN_HOST).equals(pendingServerUri.host))) { |
There was a problem hiding this comment.
@JohnsonEricAtSalesforce Was this intended to be checked in?
Edit: I see the "(CodeCov Testing - Hold Reviewing Commit)" now.
There was a problem hiding this comment.
Thanks for looking for that. This commit will be replaced with a much simpler commit once I get full coverage. Even with Windsurf helping, getting through all the logic branches and minimizing their count is still tedious.
There was a problem hiding this comment.
@brandonpage - Here's the finalized commit for code coverage, assuming it still produces the coverage I was looking for. 16daac0
Most of the updates were suggested by or reviewed by Windsurf. I wrote the tests by hand, though, since the generated code was overly verbose it seemed.
JohnsonEricAtSalesforce
force-pushed
the
feature/merge-13.1.1-master-to-13.2.0-dev
branch
7 times, most recently
from
16daac0 to
8d769b9
Compare
JohnsonEricAtSalesforce
force-pushed
the
feature/merge-13.1.1-master-to-13.2.0-dev
branch
from
8d769b9 to
4d2741f
Compare
JohnsonEricAtSalesforce
merged commit e0549ef
into
forcedotcom:dev
JohnsonEricAtSalesforce
deleted the
feature/merge-13.1.1-master-to-13.2.0-dev
branch
3 participants
🥁 Ready For Review 🎸
This catches 13.2.0
devup with today's commits to 13.1.1master