[Insights: Violations]: Expanding group & Suggested search - #97067
[Insights: Violations]: Expanding group & Suggested search#97067nkdengineer wants to merge 17 commits into
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
nkdengineer
changed the title
nkdengineer
changed the title
|
@neil-marcellini From the doc, I added a violation column. But I think it's unnecessary as we displayed the violation below each transaction like this. What do you think? 
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@cretadn22 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] |
![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: d43b90fa27
ℹ️ 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".
| const isEligibleForTopSpendersSuggestion = isGroupPolicyEligible && (isAdmin || isAuditor || isUserApprover) && memberCount >= 2; | ||
| const isEligibleForTopCategoriesSuggestion = isGroupPolicyEligible && policy.areCategoriesEnabled === true; | ||
| const isEligibleForTopMerchantsSuggestion = isGroupPolicyEligible; | ||
| const isEligibleForViolationsBySubmitterSuggestion = isGroupPolicyEligible && (isAdmin || isAuditor) && arePolicyRulesEnabled(policy) && memberCount >= 2; |
There was a problem hiding this comment.
For migrated Control workspaces where areRulesEnabled is undefined but Classic category rules exist, this call always treats rules as disabled because it does not pass policy categories. arePolicyRulesEnabled() explicitly relies on hasAnyCategoryRules(policyCategories) for that migration case, so admins and auditors of these workspaces never see the new suggestion despite having applicable submitted violations.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🚧 dubielzyk-expensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
I'm seeing the LHN but not any data even though there is a violation. Is it because it's myself? CleanShot.2026-07-27.at.18.28.27.mp4 |
|
@dubielzyk-expensify You can check the transaction's date. Because the violation suggested search filter is for the date last month, we need to update the date to June |
|
Even when I remove it I don't get any though? 
|
|
@dubielzyk-expensify Can you try to submit the expense report above? |
|
@dubielzyk-expensify to be clear the violations only display for expenses that have been submitted because that's what the search is looking for. Also, I'm noticing that we never display the "Violations" column on mobile, or the narrow layout. Currently the narrow layout for a transaction item on the search page has a fixed card that doesn't include all columns. I think we should update it so the narrow layout on the suggest search shows the same key info as the wide layout. 
The group item should display "From, Expenses, Total" and the transaction items should display the "Violations" column, maybe instead of the current violations? We kind of missed designing the UI for the narrow view during the design doc. What do you think. Looping in @trjExpensify as well. |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Looking pretty good, but a couple of fixes are needed. Please also add unit tests for these functions, as the design doc mentions: buildQueryStringFromFilterFormValues, getSuggestedSearches, getSuggestedSearchesVisibility.
|
Ahh, I see. It worked now that's it submitted. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f502aaac8
ℹ️ 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".
|
Why "Expense too old" for the name of this one, and not either "Date older than X days" or "Max expense age" which are elsewhere in the product when referring to this violation? 
Also, @neil-marcellini it seems like with the date we're filtering this expense out but it was actually submitted this month? 🤔 
|
I'm not sure about this one, I think it's ok to display both since they are different.
I don't think this is necessarily a problem. The Violations column is supposed to show every unique violation that was present at submit time, which might not match the current violations on the expense. |
It does seem like there's some kind of prioritisation logic @neil-marcellini for either the itemised receipt or receipt required violation depending on the limit amounts set, so it kinda' makes sense to me it reflects which one was flagged and ignored: 
|
Yes I agree it should be something like "Date older than max expense age".
The date filter for the query operates based on the expense itself, not when the report was submitted, so that the date filter behavior matches the behavior when it's used for other expense searches. |
Yeah, I understand why it's happening, but I'm questioning if using expense date is the right call for these canned suggested searches. We have |
Yeah it probably does make more sense to use the submitted date, so we should swap it to submitted:last-month instead of the plain date filter. Maybe we should spin up issues to do that before launching release 1? |
|
Yeah, though I guess for violations by approver, the |
@neil-marcellini Should |
Yes it should only appear when the data is present, such as when that filter is applied. |
|
@cretadn22 Ready for another review. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f31cf65dc
ℹ️ 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".
| return translate(`violations.shortName.${violationName}`); | ||
| } | ||
|
|
||
| return violationName; |
There was a problem hiding this comment.
When a submitted snapshot contains current violation names omitted from SUBMITTED_TRANSACTION_VIOLATION_SHORT_NAMES (tagOutOfPolicy, overLimitAttendee, or customRules), this fallback returns the backend enum and TextCell displays it directly. Fresh evidence after the prior translation finding is that these are still valid CONST.VIOLATIONS entries but were not added to the short-name set, so users can still see camelCase identifiers instead of localized labels; please add parameter-free labels or otherwise translate every known violation before falling back only for truly unknown names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@neil-marcellini Can you give the sort name for these violations?
|
|
| topSpenders: 'Höchste Ausgaben', | ||
| topCategories: 'Topkategorien', | ||
| topMerchants: 'Top-Händler', | ||
| violationsBySubmitter: 'Violations by submitter', |
| function getSubmittedViolationDisplayName(violationName: string, translate: LocalizedTranslate): string { | ||
| if (!isValidViolationName(violationName)) { | ||
| return violationName; | ||
| } | ||
|
|
||
| if (isSubmittedTransactionViolationShortName(violationName)) { | ||
| return translate(`violations.shortName.${violationName}`); | ||
| } | ||
|
|
||
| return violationName; | ||
| } |
There was a problem hiding this comment.
| function getSubmittedViolationDisplayName(violationName: string, translate: LocalizedTranslate): string { | |
| if (!isValidViolationName(violationName)) { | |
| return violationName; | |
| } | |
| if (isSubmittedTransactionViolationShortName(violationName)) { | |
| return translate(`violations.shortName.${violationName}`); | |
| } | |
| return violationName; | |
| } | |
| function getSubmittedViolationDisplayName(violationName: string, translate: LocalizedTranslate): string { | |
| return isSubmittedTransactionViolationShortName(violationName) ? translate(`violations.shortName.${violationName}`) : violationName; | |
| } |
|
@neil-marcellini Should expenses be hidden under "Violations by submitter" if a user retracts the expense or fixes the violation? @nkdengineer I think the expense should be hidden in the above case, but currently, it is not. |
|
The expense count is displaying incorrectly. In my test, it shows 16 expenses, but the list only contains 10. Screen.Recording.2026-08-02.at.11.57.41.mov |
|
@neil-marcellini In this case, the Violations column displays "Receipt required," which is confusing since the receipt has already been uploaded Screen.Recording.2026-08-02.at.12.00.12.mov |
@cretadn22 If it's correct, we need to fix this from the backend site as it's the result from backend. |
Yeah, so violations reporting isn't about "current" violations. It's about highlighting bottlenecks and inefficiencies in the process at the time of submission and at the time of approval - whether or not that is rectified later. For example, if an employee submitted an expense with a missing receipt, that would hold up the process to approve, potentially for weeks. If we didn't include "violations that were later cleared" the company would never identify that employee as being a bottleneck. |
5 participants
Explanation of Change
feat: Expanding group & Suggested search
Fixed Issues
$ #95859
PROPOSAL:
Tests
Offline tests
QA Steps
Same as test
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
Screen.Recording.2026-07-27.at.14.32.35.mov
Android: mWeb Chrome
Screen.Recording.2026-07-27.at.14.25.08.mov
iOS: Native
Screen.Recording.2026-07-27.at.14.29.43.mov
iOS: mWeb Safari
Screen.Recording.2026-07-27.at.14.27.52.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-27.at.14.20.06.mov