Feat: Change Add unreported expense to Add existing expense in a report#85225
Feat: Change Add unreported expense to Add existing expense in a report#85225nyomanjyotisa wants to merge 6 commits intoExpensify:mainfrom
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 |
Codecov Report❌ Patch coverage is
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@sobitneupane 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] |
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/pages/AddUnreportedExpense.tsx
Outdated
| return Object.values(transactions || {}).filter((item) => { | ||
| const isUnreported = item?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || item?.reportID === ''; | ||
| if (!isUnreported) { | ||
| const isOnOpenExpenseReport = isOpenExpenseReport(getReportOrDraftReport(item?.reportID)); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
getReportOrDraftReport(item?.reportID) is called inside getUnreportedTransactions, which is used as a useOnyx selector for ONYXKEYS.COLLECTION.TRANSACTION. This function reads from a module-level allReports variable (another Onyx collection) rather than from the subscribed transaction data. The selector will only re-run when the transaction collection changes or when the useCallback dependencies change -- it will NOT re-run when a report's status changes (e.g., from open to submitted). This means isOpenExpenseReport(getReportOrDraftReport(item?.reportID)) can return stale results, showing transactions that should no longer appear or hiding transactions that should.
Consider subscribing to the reports collection separately (e.g., via a separate useOnyx call for the relevant reports) and passing the result into the filtering logic outside the selector, so that changes to report data trigger re-evaluation.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/pages/AddUnreportedExpense.tsx
Outdated
| } | ||
|
|
||
| return filteredTransactions.filter((item) => { | ||
| const isUnreported = item?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || item?.reportID === ''; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The check item?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || item?.reportID === '' is duplicated here and in getUnreportedTransactions (line 91). Both perform the same "is unreported" determination. If the definition of "unreported" changes, both locations must be updated, risking them getting out of sync.
Extract this into a shared helper function:
function isUnreportedTransaction(transaction: OnyxEntry<Transaction>): boolean {
return transaction?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || transaction?.reportID === '';
}Then use it in both getUnreportedTransactions and statusFilteredTransactions.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/pages/AddUnreportedExpense.tsx
Outdated
|
|
||
| return filteredTransactions.filter((item) => { | ||
| const isUnreported = item?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || item?.reportID === ''; | ||
| if (selectedStatusValues.includes(CONST.SEARCH.STATUS.EXPENSE.UNREPORTED) && isUnreported) { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
selectedStatusValues.includes(CONST.SEARCH.STATUS.EXPENSE.UNREPORTED) and selectedStatusValues.includes(CONST.SEARCH.STATUS.EXPENSE.DRAFTS) (line 206) do not depend on the iterator variable item but are called on every iteration of the .filter() callback. Hoist these outside the loop:
const includesUnreported = selectedStatusValues.includes(CONST.SEARCH.STATUS.EXPENSE.UNREPORTED);
const includesDrafts = selectedStatusValues.includes(CONST.SEARCH.STATUS.EXPENSE.DRAFTS);
return filteredTransactions.filter((item) => {
const isUnreported = item?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID || item?.reportID === '';
if (includesUnreported && isUnreported) {
return true;
}
if (includesDrafts && !isUnreported) {
return true;
}
return false;
});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 17516e5771
ℹ️ 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".
src/pages/AddUnreportedExpense.tsx
Outdated
| const isOnOpenExpenseReport = isOpenExpenseReport(getReportOrDraftReport(item?.reportID)); | ||
| if (!isUnreported && !isOnOpenExpenseReport) { |
There was a problem hiding this comment.
This filter now decides whether a transaction is a draft by reading isOpenExpenseReport(getReportOrDraftReport(item?.reportID)), but the surrounding useOnyx subscription is only on ONYXKEYS.COLLECTION.TRANSACTION. If a report moves from OPEN/OPEN to SUBMITTED while this page is mounted (for example from another client/session), no transaction change is required, so this selector can stay stale and keep showing now-ineligible expenses as selectable.
Useful? React with 👍 / 👎.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Change looks good for product. Let's see how the testing goes though.
2 participants
Explanation of Change
Fixed Issues
$ #84130
PROPOSAL: #84130 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Precondition: Have at least 15 expenses in Draft or Unreported state so that the search bar is visible.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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-Native.mp4
Android: mWeb Chrome
Android-mWeb.Chrome.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-mWeb.Safari.mp4
MacOS: Chrome / Safari
MacOS-Chrome.mp4