Repository navigation
[CP Staging] Revert "Merge pull request #101226 from lorretheboy/fix-v2/95750" - #103057
Conversation
|
@chuckdries 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
MonilBhavsar
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85037a02b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (isGroupPolicy(policy) && | ||
| getReimbursementChoice(policy) === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL && | ||
| canMemberWrite(policy, currentUserLogin, CONST.POLICY.POLICY_FEATURE.WORKFLOWS_PAYMENTS)); |
There was a problem hiding this comment.
When a workspace uses REIMBURSEMENT_YES and the current payments admin is not the designated reimburser or a sharee of the policy bank account, isReportPayer is false and this fallback now admits only REIMBURSEMENT_MANUAL. As a result, canIOUBePaid hides Pay for an otherwise authorized workspace admin, reintroducing the inability for admins to pay approved reports from an account available to them. The admin/payment-permission fallback needs to cover configured direct reimbursement as well.
Useful? React with 👍 / 👎.
| // A paying admin can record `bankAccountID` on a workspace payment too, so the report type is what says | ||
| // whether this was an invoice, matching `getIOUReportActionDisplayMessage`. | ||
| const isFromInvoice = isInvoiceReport(report) && !!originalMessage?.bankAccountID; | ||
| const isFromInvoice = !!originalMessage?.bankAccountID; |
There was a problem hiding this comment.
For a normal workspace expense paid by VBBA with a recorded bankAccountID, this condition classifies the payment as an invoice even though report is not an invoice. The function consequently returns settleInvoicePersonal/settleInvoiceBusiness copy such as “paid $100 with personal account” instead of the workspace bank-account wording, and may omit the digits for viewers who do not have the payer's account locally. Include isInvoiceReport(report) in this classification; the duplicated English report-action branch needs the same correction.
Useful? React with 👍 / 👎.
| reportActions, | ||
| canNonPayerAdminPay: true, | ||
| }) && allExpensesHeld; |
There was a problem hiding this comment.
When every expense is held on a manual-reimbursement report, this preliminary isPrimaryPayAction call omits canNonPayerAdminPay: true, unlike the regular Pay check below. A non-designated admin with workflow-payment permission can therefore pay the report when expenses are unheld but fails isPayActionWithAllExpensesHeld, so the primary action does not redirect them to Remove hold even when expensesToHold confirms they can unhold the expenses.
Useful? React with 👍 / 👎.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🚧 MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
…37349012534-1 🍒 Cherry pick PR #103057 to staging 🍒
|
🚀 Cherry-picked to staging by https://github.com/MonilBhavsar in version: 9.5.2-3 🚀
|
|
No help site update is needed. This PR reverts Allow any workspace admin to pay reports before it reached production. The live article still describes the restored behavior: "The authorized payer and workspace admins the bank account has been shared with can pay reports." One follow-up: the draft docs PR for the reverted change, [No QA] Docs updates for E/A#101226 Let any admin pay with connected bank account, would now make that article wrong. Close it, or hold it until the change relands. |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.5.2-4 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Cherry-picked to staging by https://github.com/MonilBhavsar in version: 9.5.3-0 🚀
|
|
No help site update is needed. My earlier answer still holds: this revert shipped to production in 9.5.2-4 together with the change it undoes, so users never saw Allow any workspace admin to pay reports. The article still matches production: "The authorized payer and workspace admins the bank account has been shared with can pay reports." The draft [No QA] Docs updates for E/A#101226 Let any admin pay with connected bank account is still open. Merging it would make that sentence wrong. Close it, or hold it until the change is merged again. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.5.3-4 🚀
Bundle Size Analysis (Sentry): |
5 participants
Reverts PR #101226
This reverts commit 06d44f6, reversing changes made to 901155d.
Explanation of Change
Fixed Issues
$ #102967
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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