Repository navigation
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot review |
ReviewThe fix looks correct. Search rows now read attendees from the same place as the attendees column and the expense view: I found no blocking problems. I have three optional suggestions:
I didn't run the App: reproducing this needs a Control workspace with a category that requires attendees and existing snapshot data. |
|
I've updated Regarding:
Good catch, but I think the root cause is different. The spec in #76155 says to flag the violation when the attendees list is empty or has no one besides the expense creator. Passing the report owner to Lines 71 to 72 in 5bd9516 The same issue was already flagged in the #79721 (comment) @MelvinBot, considering this, do you think it makes sense to do any adjustments in this PR? Or should it be solved separately? |
|
I agree with your diagnosis. I'd fix it in a separate PR and merge this one as is. You're right that the root cause is the viewer-vs-creator check. Why I'd keep it separate:
Attendee - Multiple attendee violation only applicable for the owner of the WS was closed after a CP hid the feature, with a note to fix this in a follow-up. That follow-up doesn't seem to exist, so a new issue linking that comment would track it. In that thread, |
|
@nkdengineer 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. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-21.at.18.21.14.movAndroid: mWeb ChromeScreen.Recording.2026-10-21.at.18.22.10.moviOS: HybridAppScreen.Recording.2026-10-21.at.18.20.30.moviOS: mWeb SafariScreen.Recording.2026-10-21.at.18.19.26.movMacOS: Chrome / SafariScreen.Recording.2026-10-21.at.18.18.05.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb778b36f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| policyCategories, | ||
| transaction?.category ?? transactionItem.category ?? '', | ||
| transaction?.comment?.attendees ?? transactionItem.attendees, | ||
| getAttendees(transaction ?? transactionItem), |
There was a problem hiding this comment.
Compare required attendees with the report owner
When a manager or approver views a Control-workspace expense in a category that requires attendees but has no non-owner attendee, getTransactionsSections inserts the report owner as the default snapshot attendee, and this new fallback passes that owner into syncMissingAttendeesViolation while the next argument remains the viewer's currentUserDetails. The helper therefore counts the submitter as a valid non-owner attendee and fails to add the real violation; the new getAttendees(transaction) call in ExpenseReportListItem.tsx:208 has the same owner/viewer mismatch and can remove the snapshot violation. Use the report owner/submitterLogin for the owner exclusion in both rows.
Useful? React with 👍 / 👎.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@VickyStash I think the owner vs viewer check needs fixing in this PR.
Say Alice submits an expense and is the only attendee. The backend flags it, because it leaves out the owner when it counts attendees. But syncMissingAttendeesViolation leaves out the viewer. So when Bob, the approver, looks at the row in Search, the check leaves out Bob instead of Alice, Alice counts as the extra attendee, and the violation is hidden.
Can we leave out the report owner's login instead, like the backend does, in both Search rows, with the current user as the fallback for unreported expenses?
There was a problem hiding this comment.
Ah, I misunderstood the codex comment. @lakchote is right.
There was a problem hiding this comment.
The backend flags it, because it leaves out the owner when it counts attendees. But
syncMissingAttendeesViolationleaves out the viewer.
Yeah, and other places in the app pass the current user instead of the owner in other places too:
- MoneyRequestView.tsx:820
- ViolationsUtils.ts:595
- useConfirmationValidation.ts:276. The current user is the creator here, so this one can keep its current value.
I've mentioned it here: #104540 (comment)
So if we do this:
Can we leave out the report owner's login instead, like the backend does, in both Search rows, with the current user as the fallback for unreported expenses?
we should change it everywhere, right?
There was a problem hiding this comment.
@nkdengineer I guess it's the same on staging?
There was a problem hiding this comment.
|
Reproducible on staging.
@VickyStash We can update the test step Precondition: Create a WS setup-requires-attendees rule for a category. Invite a member. As a member, create an expense with the category with the rule and add two attendees.
|
| policyCategories, | ||
| transaction?.category ?? transactionItem.category ?? '', | ||
| transaction?.comment?.attendees ?? transactionItem.attendees, | ||
| getAttendees(transaction ?? transactionItem), |
There was a problem hiding this comment.
@VickyStash I think the owner vs viewer check needs fixing in this PR.
Say Alice submits an expense and is the only attendee. The backend flags it, because it leaves out the owner when it counts attendees. But syncMissingAttendeesViolation leaves out the viewer. So when Bob, the approver, looks at the row in Search, the check leaves out Bob instead of Alice, Alice counts as the extra attendee, and the violation is hidden.
Can we leave out the report owner's login instead, like the backend does, in both Search rows, with the current user as the fallback for unreported expenses?
mountiny
left a comment
There was a problem hiding this comment.
@nkdengineer can you create the new issues using melvin please?
|
@MelvinBot Create a new issue based on this analysis and the image for the bug is here. |
|
I created Missing attendees violation is hidden when an approver views another member's expense. It covers the owner-vs-viewer mismatch from the review analysis and includes the creator and approver screenshots. It also lists every caller that passes the current user, and notes that the confirmation page can keep its current value. I found no open duplicate. Two issues are related:
I didn't add labels. |
|
@MelvinBot Let assign me, @mountiny and @VickyStash to this issue. |
|
|
I assigned |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.16-0 🚀
|
|
No help site update needed. This PR fixes a bug: Search showed a false "Multiple attendees required for this category" violation, and it no longer does. That doesn't make any sentence in an existing article wrong. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.18-0 🚀
|
|
No help site update needed. This bug fix stops a false "Multiple attendees required" violation from showing in Search rows, and the help site doesn't document the absence of an error. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.18-1 🚀
Bundle Size Analysis (Sentry): |



Explanation of Change
Bug:
In Spend -> Reports, a report row shows "Report contains expenses with violations.", but opening the report shows no errors.
In Spend → Expenses, its expenses show "Multiple attendees required for this category." until the expense or report is opened.
Root cause: Both Search rows re-check the missing-attendees violation on the client. For snapshot data they read the top-level
attendeesfield, which is never set. The attendees are actually incomment.attendees, so with snapshot data the client saw no attendees and added the violation.Expense row: it used
transaction?.comment?.attendees ?? transactionItem.attendees.transactionis the live transaction from Onyx (transactions_<id>). It only exists after the expense or its report has been opened, because opening it loads the transaction into Onyx. Search results live in a separate snapshot key.transactionItemis the snapshot row, and its top-levelattendeesis alwaysundefined.undefined, which shows the error. After it's opened, the livecomment.attendeesis used and the error disappears.Report row: it used
transaction.attendees, wheretransactioncomes fromreportItem.transactions, the snapshot rows. It never looks at the live transaction, so it gotundefinedeven after the report was opened, and the error never went away.Fix: Both rows (
ExpenseReportListItem,TransactionListItem) now read attendees withgetAttendees(), which returnsmodifiedAttendees ?? comment.attendees. That is the same source the Search attendees column already uses.Fixed Issues
$ #104546
PROPOSAL: N/A
Tests
Precondition: Create a WS setup-requires-attendees rule for a category. Invite a member. As a member, create an expense with the category with the rule and add two attendees.
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in Tests section
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
MacOS: Chrome / Safari
Shared in Slack: https://expensify.slack.com/archives/C05LX9D6E07/p1792579446644099?thread_ts=1792563208.522159&cid=C05LX9D6E07