Disable vacation delegate entry point while offline#94425
Conversation
Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
I created this PR from an upstream branch since you don't have a fork I could push to. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 I looked into the failing checks — there's nothing for me to fix in the code. The only failing check is Every real CI check is green: ESLint, Prettier, Next step: Fill out the PR Author Checklist (and the |
|
@MelvinBot update your checklist to the following:
|
|
🤖 Updated the PR description per your request:
|
|
@MelvinBot mark this PR as ready |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.hybrid.mp4Android: mWeb Chromeandroid.chrome.mp4iOS: HybridAppios.hybrid.mp4iOS: mWeb Safariios.safari.mp4MacOS: Chrome / Safarimac.safari.mp4 |
trjExpensify
left a comment
There was a problem hiding this comment.
This doesn't seem like a bug to me. We very rarely, if at all, disable rows when offline. We show a full page blocking form if it requires being online - and that's what the current behaviour appears to be applying pattern D.
@mountiny do you agree?
|
@trjExpensify I'm not entirely sure, but from what I understand, disabling that row while offline prevents users from opening it through the normal UI flow. The full-page offline blocker, on the other hand, prevents users from accidentally accessing the page via a deep link. |
|
I agree with Tom, also mainly because the full page makes it clear to the user WHY they cannot take the action. The disabled row does not explain that its because they are offline |
|
Cool, so I think we should close this PR then? |
|
Agreed |
Explanation of Change
When offline, tapping the Vacation delegate row on Account > Profile > Status (or on the Domain member details page) navigated the user into the vacation delegate selection screen, which only renders a full-page "You appear to be offline" block. That dead-end happens because the selection screen wraps its content in
FullPageOfflineBlockingView, and the underlying SET action (setVacationDelegate) usesAPI.makeRequestWithSideEffects(not a queueableAPI.write) because it must read the server response to surface the policy-diff warning — so the action genuinely cannot complete offline.Per the approved proposal (recommended option 1), this change disables the entry point while offline so users aren't routed into a screen they can't use. Both entry points (
StatusPageandDomainMemberDetailsPage) render the sharedVacationDelegateMenuItemcomponent, so gating theMenuItem'sdisabledprop onuseNetwork().isOfflinethere fixes both at once. The existing per-row pending/error indicator (OfflineWithFeedback) is unaffected.Fixed Issues
$ #94183
PROPOSAL: #94183 (comment)
Tests
Same as Offline tests
Offline tests
QA Steps
Same as Offline tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari