Repository navigation
Add a Rejected suggested search that appears once the user has a rejected expense #104438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import {hasReportBeenRejectedToSubmitter} from '@libs/ReportUtils'; | ||
| import {hasTransactionBeenRejected} from '@libs/TransactionUtils'; | ||
|
|
||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type {Report, TransactionViolations} from '@src/types/onyx'; | ||
|
|
||
| import type {OnyxCollection} from 'react-native-onyx'; | ||
|
|
||
| // We need direct access to useOnyx from react-native-onyx to avoid reading search snapshots instead of live data | ||
| // eslint-disable-next-line no-restricted-imports | ||
| import {useOnyx} from 'react-native-onyx'; | ||
|
|
||
| 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)); | ||
|
Comment on lines
+13
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MelvinBot please investigate: the report check here shows "Rejected" for whole-report rejections, but Repro: an approver rejects two expenses individually and one whole report ( Can you confirm whether the backend's |
||
|
|
||
| /** | ||
| * Whether the user currently has at least one rejected expense, either rejected on its own or as part of a rejected report. | ||
| * The selectors return a boolean so consumers only re-render when the answer changes. | ||
| */ | ||
| function useHasRejectedExpense(): boolean { | ||
| const [hasRejectedTransaction = false] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS, {selector: hasRejectedTransactionSelector}); | ||
| const [hasRejectedReport = false] = useOnyx(ONYXKEYS.COLLECTION.REPORT, {selector: hasRejectedReportSelector}); | ||
|
|
||
| return hasRejectedTransaction || hasRejectedReport; | ||
| } | ||
|
|
||
| export default useHasRejectedExpense; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 hasautoReportedRejectedExpensewithrejectedByset to the approver, and the report matcheshasReportBeenRejectedToSubmitterwithownerAccountIDset to the submitter.So
hasRejectedExpenseis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).isCurrentUserSubmitteralready does the owner check. CheckingrejectedBy !== currentUserLoginisn'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.hasOwnRejectionSelectorreturnsreport.ownerAccountID === accountID && (hasReportBeenRejectedToSubmitter(report) || rejectedTransactionReportIDs.includes(report.reportID)).Test accounts that already have
nvp_hasSeenRejectedSuggestedSearchset 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:yeswith nofrom:me. An approver who has their own rejected expense may also see others' rejections in the results, depending on how the backend scopesdeclined:yes. I couldn't verify that from the App repo.Reply with
@MelvinBot implement thisto push the fix to this PR, or@MelvinBot <your feedback>to change the approach.