Repository navigation
feat: Workflow payer page - #37629
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2024-03-12_16.02.45.mp4Android: mWeb Chromeandroid-chrome-2024-03-12_15.52.39.mp4iOS: Nativeios-native-2024-03-12_15.36.20.mp4iOS: mWeb Safariios-safari-2024-03-12_15.43.43.mp4MacOS: Chrome / Safaridesktop-chrome-2024-03-11_15.09.12.mp4MacOS: Desktopdesktop-app-2024-03-12_14.43.10.mp4 |
|
Looking pretty good now, I think! The only thing is my concern here, I'll let @luacmartins make the last call on that. |
|
@jjcoffee if changes looks good to you i'l proceed with the final videos. would be great if we can get this merged today.. |
|
ahh.. we commented on same time |
jjcoffee
left a comment
There was a problem hiding this comment.
EOD for me now so I'll approve in order to keep this moving! We're just pending the rest of the test videos from @ishpaul777.
|
Added the videos... 😀 |
|
I think we can address this one as a follow up. I'll create an issue for it. |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Issue created here - #38153 |
|
Tests passed when I merged. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.51-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.51-3 🚀
|
| /> | ||
| <> | ||
| <MenuItem | ||
| titleStyle={styles.textLabelSupportingNormal} |
There was a problem hiding this comment.
We needed to display the default label in a different font size here. Fixed in #38584
| Policy.openPolicyWorkflowsPage(policy?.id ?? route.params.policyID); | ||
| }; | ||
|
|
||
| useNetwork({onReconnect: fetchData}); |
There was a problem hiding this comment.
When offline, this optimistic isLoading never resets until the network request fails, causing the page to briefly show the loader and flicker.
|
|
||
| const isOwner = policy?.owner === details?.login; | ||
| const isAdmin = policyMember.role === CONST.POLICY.ROLE.ADMIN; | ||
| const shouldSkipMember = isDeletedPolicyMember(policyMember) || PolicyUtils.isExpensifyTeam(details?.login) || (!isOwner && !isAdmin); |
There was a problem hiding this comment.
👋 the condition to skip members if isExpensifyTeam hides all members on my test workspaces, and I assume it hides all members on Expensify's production workspaces. Why is this condition here?
AI says:
Expensify team accounts (e.g. bills@expensify.com, concierge@expensify.com) are often added as policy members for internal reasons, but they should never appear as selectable payers in the workspace Authorized Payer list. Without this guard, an Expensify internal service account could show up as a payer option for the workspace owner to select — which would be nonsensical.
I find this hard to believe. Which accounts ending in @expensify.com are admins on policies?
There was a problem hiding this comment.
I see accountspayable@expensify.com as admin in a bunch of policies
There was a problem hiding this comment.
Do you mean policies owned by real users (customers)?
There was a problem hiding this comment.
i could be wrong but if i remember correctly at some point, guide accounts are admin in costumer WS this was to hide them in authorized payer list
Details
Fixed Issues
$ #37197
PROPOSAL: n/a
Tests
workspace/<id>/workflows, toggle on/off this option make sure it persists after refresh.Click on authorized payer selector, verify it should show list of all admins and owner of the workspace
Select a admin as authorized and verify that it should show as authorized payer and persist after refresh.
Failing Scenario tests:
Offline tests
QA Steps
same as tests and offline tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Android: Native
Screen.Recording.2024-03-12.at.9.46.58.PM-1.mov
Android: mWeb Chrome
Screen.Recording.2024-03-12.at.5.13.45.AM.mov
iOS: Native
Screen.Recording.2024-03-11.at.5.17.06.PM.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-03-11.at.4.57.07.PM.mov
MacOS: Desktop
Screen.Recording.2024-03-12.at.9.55.02.PM-1.mov