Repository navigation
Conversation
…cted expense Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and push updates, run: Then you can close this PR and open a new one from your fork. |
This comment has been minimized.
This comment has been minimized.
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.
|
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Fixed the failing
|
|
🤖 Android test steps verification results: The Rejected suggested search appears with its thumbs-down icon once a rejection exists and opens the
|
|
🤖 Web test steps verification results: ✅ pass The Rejected suggested search appears only after a rejection, stays visible after the rejection is resolved, and opens
|
|
@MelvinBot resolve conflicts |
…tedSearch Co-authored-by: Krishna <Krishna2323@users.noreply.github.com> # Conflicts: # tests/unit/Search/SearchUIUtilsTest.ts
|
I resolved the merge conflicts with The only conflict was in
All 46 |
This comment has been minimized.
This comment has been minimized.
|
The failing Analysis: Running
Evidence: None of the 21 files this PR changes has a type error. Once |
|
I didn't push a fix because nothing has changed since my previous comment.
Once |
| const hasRejectedTransactionSelector = (allTransactionViolations: OnyxCollection<TransactionViolations>): boolean => | ||
| Object.values(allTransactionViolations ?? {}).some((transactionViolations) => hasTransactionBeenRejected(transactionViolations)); | ||
|
|
||
| // A whole-report rejection doesn't add a violation to the report's transactions, so it has to be checked on the report. | ||
| const hasRejectedReportSelector = (allReports: OnyxCollection<Report>): boolean => Object.values(allReports ?? {}).some((report) => hasReportBeenRejectedToSubmitter(report)); |
There was a problem hiding this comment.
These selectors also match rejections on expenses the current user doesn't own.
Repro: an approver rejects one expense and one report from a submitter. On the approver's account, the server copy of transactionViolations_* still has autoReportedRejectedExpense with rejectedBy set to the approver, and the report matches hasReportBeenRejectedToSubmitter with ownerAccountID set to the submitter.
So hasRejectedExpense is true for the approver, the NVP gets set, and "Rejected" stays in their nav even though none of their own expenses were rejected.
@MelvinBot please investigate
There was a problem hiding this comment.
You're right. Neither selector checks who owns the expense, so an approver or admin who rejects someone else's expense trips the hook and gets the NVP set.
Root cause
hasTransactionBeenRejectedonly checks the violation name. The violation holdsrejectedBy,comment, andrejectedDate(RejectMoneyRequest.ts:865-873), but nothing about the submitter. Approvers and admins do keep it in Onyx:shouldShowViolationshows it to the submitter or a policy admin.hasReportBeenRejectedToSubmitteronly checksstateNumandnextStep.messageKey. It doesn't checkownerAccountID.useHasRejectedExpense.ts:13-17uses both helpers as-is.
Recommended fix: limit both checks to reports the current user owns. The violation doesn't identify the submitter, so the expense path has to go from violation to transaction (reportID) to report (ownerAccountID). isCurrentUserSubmitter already does the owner check. Checking rejectedBy !== currentUserLogin isn't enough, because a second admin who didn't reject the expense would still match.
Sketch
This uses closure selectors, the same pattern as useReportTransactionViolations.ts:15. The last selector still returns a boolean, so the nav only re-renders when the answer changes.
function useHasRejectedExpense(): boolean {
const {accountID} = useCurrentUserPersonalDetails();
// 1. IDs of transactions with the rejected-expense violation (usually a short list)
const [rejectedTransactionIDs = []] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, {selector: rejectedTransactionIDsSelector});
// 2. The reports those transactions live on
const [rejectedTransactionReportIDs = []] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {selector: reportIDsForTransactionsSelector(rejectedTransactionIDs)});
// 3. True only if the current user owns a report that was rejected, or that holds a rejected expense
const [hasRejectedExpense = false] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {
selector: hasOwnRejectionSelector(accountID, rejectedTransactionReportIDs),
});
return hasRejectedExpense;
}hasOwnRejectionSelector returns report.ownerAccountID === accountID && (hasReportBeenRejectedToSubmitter(report) || rejectedTransactionReportIDs.includes(report.reportID)).
Test accounts that already have nvp_hasSeenRejectedSuggestedSearch set will keep seeing "Rejected" after the fix. Reset that NVP before you retest.
There's a related gap: the suggested query is type:expense declined:yes with no from:me. An approver who has their own rejected expense may also see others' rejections in the results, depending on how the backend scopes declined:yes. I couldn't verify that from the App repo.
Reply with @MelvinBot implement this to push the fix to this PR, or @MelvinBot <your feedback> to change the approach.
There was a problem hiding this comment.
Done in 97af161. "Rejected" now appears only when one of your own expenses or reports was rejected.
- Counts a whole-report rejection only on a report you own.
- Traces each rejected expense to its report and counts it only if you own that report.
- Counts a rejected expense that has no report. Rejecting an expense on an instantly submitted report moves it out of the report, and unreported expenses sync only to their owner. Without this, the submitter would never see "Rejected" in that case.
- Skips expenses you rejected yourself. The approver's device keeps an optimistic copy of the expense with no report, so the rule above would otherwise match it.
I added useHasRejectedExpenseTest.tsx. It covers the submitter, the approver who rejected, and a second admin. Lint, typecheck, the React Compiler check, and spellcheck pass on the changed files. All 706 tests pass, including the Search tests.
Before you retest, reset nvp_hasSeenRejectedSuggestedSearch on the approver account. If that flag is already set, "Rejected" stays visible.
I haven't tested this in the App. One case is unconfirmed: whether the server syncs an unreported rejected expense to a second admin. If it does, that admin would still see "Rejected".
| const hasRejectedTransactionSelector = (allTransactionViolations: OnyxCollection<TransactionViolations>): boolean => | ||
| Object.values(allTransactionViolations ?? {}).some((transactionViolations) => hasTransactionBeenRejected(transactionViolations)); | ||
|
|
||
| // A whole-report rejection doesn't add a violation to the report's transactions, so it has to be checked on the report. | ||
| const hasRejectedReportSelector = (allReports: OnyxCollection<Report>): boolean => Object.values(allReports ?? {}).some((report) => hasReportBeenRejectedToSubmitter(report)); |
There was a problem hiding this comment.
@MelvinBot please investigate: the report check here shows "Rejected" for whole-report rejections, but declined:yes doesn't seem to return those expenses.
Repro: an approver rejects two expenses individually and one whole report (5778504925457019). The report matches hasReportBeenRejectedToSubmitter, but type:expense declined:yes only returns the two individually rejected expenses (reports 4298380125836590 and 8678739946184850). Nothing from the rejected report comes back. I checked this from the approver's account.
Can you confirm whether the backend's declined:yes match includes expenses in a fully rejected report? If not, a submitter whose whole report was rejected will see "Rejected" in the nav but get "No rejected expenses".
|
No. As far as I can tell, I couldn't read the backend search code from here, so I haven't confirmed the exact backend condition. The evidence comes from the PR that added the filter, and it all points the same way:
This conflicts with the issue. [$175] Add a "Rejected" and a "Held" suggested search that appears once a user has rejected/held expenses says: "If an entire report is rejected, the expenses in that report should also display in that filter." What's needed: an internal engineer has to change the backend so that How the two checks differ
|
|
@MelvinBot resolve conflicts |
|
|
|
@MelvinBot resolve conflicts |
…tedSearch Co-authored-by: Krishna <Krishna2323@users.noreply.github.com> # Conflicts: # src/hooks/useSearchTypeMenuSections.ts
|
I merged
Checks:
|
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index 16950479..64175f48 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -10319,10 +10319,7 @@ Fügen Sie weitere Ausgabelimits hinzu, um den Cashflow Ihres Unternehmens zu sc
title: 'Keine zu bezahlenden Ausgaben',
subtitle: 'Glückwunsch! Du hast die Ziellinie überquert.',
},
- emptyRejectedResults: {
- title: 'Keine abgelehnten Ausgaben',
- subtitle: 'Nichts zu korrigieren. Gute Arbeit!',
- },
+ emptyRejectedResults: {title: 'Keine abgelehnten Ausgaben', subtitle: 'Nichts zu beheben. Gute Arbeit!'},
emptyExportResults: {
title: 'Keine Spesen zum Exportieren',
subtitle: 'Zeit, es ruhig angehen zu lassen – gute Arbeit.',
diff --git a/src/languages/el.ts b/src/languages/el.ts
index 13ed8bb5..4c435336 100644
--- a/src/languages/el.ts
+++ b/src/languages/el.ts
@@ -10526,10 +10526,7 @@ ${reportName}`,
title: 'Δεν υπάρχουν έξοδα προς πληρωμή',
subtitle: 'Συγχαρητήρια! Περάσατε τη γραμμή τερματισμού.',
},
- emptyRejectedResults: {
- title: 'Δεν υπάρχουν απορριφθέντα έξοδα',
- subtitle: 'Τίποτα για διόρθωση. Μπράβο!',
- },
+ emptyRejectedResults: {title: 'Δεν υπάρχουν απορριφθείσες δαπάνες', subtitle: 'Δεν υπάρχει τίποτα για διόρθωση. Μπράβο!'},
emptyExportResults: {
title: 'Δεν υπάρχουν έξοδα για εξαγωγή',
subtitle: 'Ώρα να χαλαρώσετε, μπράβο σας.',
diff --git a/src/languages/es.ts b/src/languages/es.ts
index 145d96fa..9209baea 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -10157,10 +10157,7 @@ ${reportName}`,
title: 'No hay gastos para pagar',
subtitle: '¡Felicidades! Has cruzado la línea de meta.',
},
- emptyRejectedResults: {
- title: 'No hay gastos rechazados',
- subtitle: 'Nada que corregir. ¡Buen trabajo!',
- },
+ emptyRejectedResults: {title: 'No hay gastos rechazados', subtitle: 'Nada que corregir. ¡Buen trabajo!'},
emptyExportResults: {
title: 'No hay gastos para exportar',
subtitle: 'Es hora de relajarse, buen trabajo.',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index da0ace96..6a3f45ce 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -10345,10 +10345,7 @@ Ajoutez davantage de règles de dépenses pour protéger la trésorerie de l’e
title: 'Aucune dépense à payer',
subtitle: 'Félicitations ! Vous avez franchi la ligne d’arrivée.',
},
- emptyRejectedResults: {
- title: 'Aucune dépense rejetée',
- subtitle: 'Rien à corriger. Beau travail !',
- },
+ emptyRejectedResults: {title: 'Aucune dépense rejetée', subtitle: 'Rien à corriger. Beau travail !'},
emptyExportResults: {
title: 'Aucune dépense à exporter',
subtitle: 'Il est temps de se détendre, beau travail.',
diff --git a/src/languages/it.ts b/src/languages/it.ts
index 7ffc642b..faf3dddb 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -10273,10 +10273,7 @@ Aggiungi altre regole di spesa per proteggere il flusso di cassa aziendale.`,
title: 'Nessuna spesa da pagare',
subtitle: 'Congratulazioni! Hai tagliato il traguardo.',
},
- emptyRejectedResults: {
- title: 'Nessuna spesa rifiutata',
- subtitle: 'Niente da correggere. Ottimo lavoro!',
- },
+ emptyRejectedResults: {title: 'Nessuna spesa rifiutata', subtitle: 'Niente da correggere. Bel lavoro!'},
emptyExportResults: {
title: 'Nessuna spesa da esportare',
subtitle: 'È il momento di rilassarsi, bel lavoro.',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index d895db16..04be4494 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -10121,10 +10121,7 @@ ${reportName}`,
title: '支払う経費はありません',
subtitle: 'おめでとうございます！ゴールに到達しました。',
},
- emptyRejectedResults: {
- title: '却下された経費はありません',
- subtitle: '修正が必要なものはありません。お疲れさまでした！',
- },
+ emptyRejectedResults: {title: '却下された経費はありません', subtitle: '修正する箇所はありません。よくできました！'},
emptyExportResults: {
title: 'エクスポートする経費はありません',
subtitle: 'ひと休みする時間です、お疲れさまでした。',
@@ -10374,7 +10371,7 @@ ${reportName}`,
submit: '下書き',
approve: '承認が必要',
pay: '支払いの準備完了',
- rejected: '却下済み',
+ rejected: '却下されました',
accounting: '会計',
export: 'エクスポート待ち',
unapprovedCash: '現金発生主義',
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index fff120a7..0821040a 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -10242,10 +10242,7 @@ er bestedingsregels toe om de kasstroom van het bedrijf te beschermen.`,
title: 'Geen uitgaven om te betalen',
subtitle: 'Gefeliciteerd! Je hebt de finish gehaald.',
},
- emptyRejectedResults: {
- title: 'Geen afgekeurde uitgaven',
- subtitle: 'Niets om te corrigeren. Goed gedaan!',
- },
+ emptyRejectedResults: {title: 'Geen afgekeurde uitgaven', subtitle: 'Niets om te herstellen. Goed gedaan!'},
emptyExportResults: {
title: 'Geen uitgaven om te exporteren',
subtitle: 'Tijd om rustig aan te doen, goed gedaan.',
@@ -10505,7 +10502,7 @@ er bestedingsregels toe om de kasstroom van het bedrijf te beschermen.`,
submit: 'Concepten',
approve: 'Moet worden goedgekeurd',
pay: 'Klaar om te betalen',
- rejected: 'Afgekeurd',
+ rejected: 'Afgewezen',
accounting: 'Boekhouding',
export: 'Wachten op export',
unapprovedCash: 'Kasverplichtingen',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index b4df9eb5..f73c316d 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -10258,10 +10258,7 @@ Dodaj więcej zasad wydatków, żeby chronić płynność finansową firmy.`,
title: 'Brak wydatków do opłacenia',
subtitle: 'Gratulacje! Przekroczyłeś linię mety.',
},
- emptyRejectedResults: {
- title: 'Brak odrzuconych wydatków',
- subtitle: 'Nie ma nic do poprawienia. Dobra robota!',
- },
+ emptyRejectedResults: {title: 'Brak odrzuconych wydatków', subtitle: 'Nic do poprawy. Dobra robota!'},
emptyExportResults: {
title: 'Brak wydatków do wyeksportowania',
subtitle: 'Czas trochę odetchnąć, dobra robota.',
@@ -10527,7 +10524,7 @@ Dodaj więcej zasad wydatków, żeby chronić płynność finansową firmy.`,
submit: 'Szkice',
approve: 'Wymaga zatwierdzenia',
pay: 'Gotowe do zapłaty',
- rejected: 'Odrzucone',
+ rejected: 'Odrzucono',
accounting: 'Księgowość',
export: 'Oczekuje na eksport',
unapprovedCash: 'Rozliczenia kasowe',
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index a4c4c81e..d96ecf89 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -10235,10 +10235,7 @@ Adicione mais regras de gasto para proteger o fluxo de caixa da empresa.`,
title: 'Nenhuma despesa para pagar',
subtitle: 'Parabéns! Você cruzou a linha de chegada.',
},
- emptyRejectedResults: {
- title: 'Nenhuma despesa rejeitada',
- subtitle: 'Nada para corrigir. Bom trabalho!',
- },
+ emptyRejectedResults: {title: 'Nenhuma despesa rejeitada', subtitle: 'Nada para corrigir. Bom trabalho!'},
emptyExportResults: {
title: 'Nenhuma despesa para exportar',
subtitle: 'Hora de relaxar, bom trabalho.',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index c40e62e7..3c2fc64b 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -9854,10 +9854,7 @@ ${reportName}`,
title: '暂无可支付的报销',
subtitle: '恭喜！你已冲过终点线。',
},
- emptyRejectedResults: {
- title: '暂无被拒绝的报销',
- subtitle: '没有需要修正的内容。干得好！',
- },
+ emptyRejectedResults: {title: '无被拒的报销', subtitle: '没什么需要修改的。干得好！'},
emptyExportResults: {
title: '没有可导出的报销费用',
subtitle: '该放松一下了，干得不错。',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
… expenses Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
Explanation of Change
Users lose track of expenses after they are rejected. This PR adds a Rejected suggested search to the Expense reports section of the Search left-hand nav, so users can find those expenses.
type:expense declined:yes, using the Rejected filter added in [Payment due @QichenZhu] Add Rejected search filter for expenses and expense reports. It uses the sameThumbsDownicon as the Rejected filter chip.useHasRejectedExpensehook returns true when any transaction has the rejected-expense violation (hasTransactionBeenRejected). It also returns true when any report was rejected as a whole (hasReportBeenRejectedToSubmitter), because whole-report rejections don't add per-transaction violations. Both checks use boolean Onyx selectors, so the nav only re-renders when the answer changes.useSearchTypeMenuSectionssets a newnvp_hasSeenRejectedSuggestedSearchNVP. This follows thenvp_hasSeenForYouTodopattern. The item then stays visible even after every rejection is resolved, so it doesn't appear and disappear.getSuggestedSearchesVisibility/createTypeMenuSectionstake a newshouldShowRejectedSuggestionflag. The item has its own empty state and translations in all locales.Not included in this PR:
held:yesfilter hasn't merged yet. It's still a draft in Add Held search filter for expenses. Held can follow the same pattern once that PR lands.Open questions for reviewers:
nvp_hasSeenRejectedSuggestedSearchNVP is written throughSetNameValuePair. An internal engineer should confirm the backend accepts this NVP name.declined:yes, which I couldn't verify from the App repo.Fixed Issues
$ #103339
PROPOSAL: #103339 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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