Repository navigation
Fix change workspace condition - #62062
Merged
Merged
Conversation
luacmartins
changed the title
|
@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Contributor
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari- ExpenseScreen.Recording.2025-05-21.at.17.46.41.mp4Uploading Screen Recording 2025-05-22 at 11.37.28.mp4… - IOUPaid Screen.Recording.2025-05-22.at.11.35.28.mp4UnPaid Uploading Screen Recording 2025-05-22 at 11.50.15.mp4… - Invoice & otherScreen.Recording.2025-05-22.at.11.58.34.mp4MacOS: Desktop- ExpenseScreen.Recording.2025-05-22.at.12.09.29.mp4- IOU & otherAndroid: HybridAppScreen.Recording.2025-05-22.at.12.11.19.mp4Android: mWeb ChromeScreen.Recording.2025-05-22.at.12.22.30.mp4iOS: HybridAppScreen.Recording.2025-05-22.at.12.26.54.mp4iOS: mWeb SafariScreen.Recording.2025-05-22.at.12.26.54.mov |
suneox
reviewed
May 20, 2025
Comment on lines
+10486
to
+10488
| function isWorkspaceEligibleForReportChange(newPolicy: OnyxEntry<Policy>, report: OnyxEntry<Report>, policies: OnyxCollection<Policy>): boolean { | ||
| const submitterEmail = getLoginByAccountID(report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID); | ||
| return isPaidGroupPolicyPolicyUtils(newPolicy) && (isPolicyMember(submitterEmail, newPolicy?.id) || isPolicyAdmin(newPolicy?.id, policies)); |
Contributor
There was a problem hiding this comment.
NAB: Currently, the solution section lacks a description of this change, so we can't verify this logic.
Contributor
Author
There was a problem hiding this comment.
Yea, I need to update the doc with this logic.
Contributor
There was a problem hiding this comment.
I think this change returns incorrect available workspaces
Screen.Recording.2025-05-21.at.17.25.16.mp4
Contributor
Author
There was a problem hiding this comment.
Ah the workspaces are correct, but this request is failing with 411 Cannot create policy expense chat for non Policy member. I have a BE PR to add support for this, but basically we'll need a method similar to MoveIOUReportToPolicyAndInviteSubmitter, but for paid policies
Contributor
Author
There was a problem hiding this comment.
BE PR is still WIP. @suneox let's continue review here and not block on that.
Contributor
There was a problem hiding this comment.
I got it. Overall, the change set is look good. I'm just a bit confused about this part, so I'll continue the checklist based on the current state.
Contributor
|
I need more time to compare with old dot and will complete the checklist today |
MarioExpensify
approved these changes
May 22, 2025
MarioExpensify
left a comment
MarioExpensify
left a comment
Contributor
There was a problem hiding this comment.
Code LGTM!! @suneox please let me know when it is ready to merge!
suneox
approved these changes
May 22, 2025
suneox
left a comment
suneox
left a comment
Contributor
There was a problem hiding this comment.
Overall, the change set LGTM. We can verify available ws issue later after BE ready
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
Bot
added
the
DeployBlockerCash
Contributor
Performance Comparison Report 📊 (1/3)Significant Changes To Duration
Show details
|
Contributor
Performance Comparison Report 📊 (2/3)Meaningless Changes To Duration (1/2)Show entries
Show details
|
Contributor
Performance Comparison Report 📊 (3/3)Meaningless Changes To Duration (2/2)Show entries
Show details
|
Contributor
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Contributor
|
🚀 Deployed to staging by https://github.com/MarioExpensify in version: 9.1.51-0 🚀
|
This was referenced May 25, 2025
Reports - Invoice is accessible when account does not have invoice, which opens not here page
#62735
Closed
6 of 8 tasks
Contributor
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.1.51-6 🚀
|
Contributor
|
For these steps, the “Move workspace” option shows up in the header, but the backend rejects it, saying that the IOU and workspace payer must be the same. Screen.Recording.2025-05-30.at.8.08.55.PM.movShould we show this option only if the IOU and workspace have the same payer? We are working on a PR that uses the isPayer logic for the workspace payer to show this option only when the backend condition matches. Is this fine? |
51 tasks done
luacmartins
removed
the
DeployBlockerCash
Contributor
Author
Yes. Additionally, besides the payer being the same, we can only update the IOU's workspace if all requests come from the same user. So for example: Scenario 1:
Scenario 2:
|
5 of 8 tasks
8 tasks done
53 tasks done
suneox
reviewed
Jan 14, 2026
Comment on lines
-10513
to
-10518
| isCurrentUserAdmin && | ||
| verifyState(report, [CONST.REPORT.STATE_NUM.APPROVED]) && | ||
| verifyStatus(report, [CONST.REPORT.STATUS_NUM.APPROVED, CONST.REPORT.STATUS_NUM.REIMBURSED, CONST.REPORT.STATUS_NUM.CLOSED]) | ||
| ) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Coming from the #76421 checklist: during the refactor and cleanup of conditions, we missed this case. As a result, the available workspace is shown, but the backend prevents the change
jjcoffee
reviewed
Feb 17, 2026
| return false; | ||
| function isWorkspaceEligibleForReportChange(newPolicy: OnyxEntry<Policy>, report: OnyxEntry<Report>, policies: OnyxCollection<Policy>): boolean { | ||
| const submitterEmail = getLoginByAccountID(report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID); | ||
| return isPaidGroupPolicyPolicyUtils(newPolicy) && (isPolicyMember(submitterEmail, newPolicy?.id) || isPolicyAdmin(newPolicy?.id, policies)); |
Contributor
There was a problem hiding this comment.
Coming from #79075. We should also check here for isPendingDeletePolicy otherwise the change workspace option will be shown whilst offline.
cretadn22
reviewed
May 24, 2026
Comment on lines
+3850
to
+3852
| if (isSubmitted) { | ||
| return (isSubmitter && isAwaitingFirstLevelApproval(report)) || isManager || isAdmin; | ||
| } |
Contributor
There was a problem hiding this comment.
This behavior is now considered incorrect and was removed in #88530
3 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Refactors the logic for displaying the
Change workspacesecondary action option as follows:Fixed Issues
$ #62337
Tests
Pre-condition:
Change workspaceoptionChange workspaceoptionChange workspaceChange workspaceChange workspaceOffline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Screen.Recording.2025-05-19.at.4.47.23.PM.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop