fix: migrate ReportActionItem to useOnyx for policyTags#80258
fix: migrate ReportActionItem to useOnyx for policyTags#80258Beamanator merged 19 commits intomainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
melvin-bot
bot
requested review from
MonilBhavsar and
heyjennahay
and removed request for
a team
|
@MonilBhavsar 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: 388ce58c06
ℹ️ 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".
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.
|
|
@linhvovan29546 do you want to review this one as C+, since you have context about the migration? |
|
@marcochavezf Thanks for mention! Please assign this to me, since it came from the original PR I reviewed as well. |
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
We still need QA, so could you please remove the [No QA] tag from the title?
src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx
Outdated
Show resolved
Hide resolved
This properly migrates the ReportActionItem component to use the useOnyx hook for policyTags data instead of relying on the deprecated module-level Onyx.connect pattern in ModifiedExpenseMessage.ts. Changes: - Import getForReportActionTemp instead of getForReportAction - Add useLocalize hook to get translate function - Add useOnyx hook to fetch policyTags for the reports policy - Pass translate, policy, and policyTags to getForReportActionTemp - Add eslint-disable comments for remaining Onyx.connectWithoutView calls (used by utility files that will be migrated in follow-up PRs)
The function was still using module-level currentUserLogin from Onyx.connect. Now it accepts currentUserLogin as a parameter for full migration. Changes: - Add currentUserLogin parameter to getForReportActionTemp - Update ReportActionItem.tsx to pass currentUserLogin from session - Update ContextMenuActions.tsx to pass currentUserLogin - Update BaseReportActionContextMenu.tsx to fetch session and pass currentUserLogin
marcochavezf
changed the title
- Use useCurrentUserPersonalDetails hook for currentUserLogin instead of session - Use policyForMovingExpensesID when policyID is fake for policy tags - Update ESLint disable comments with more specific justifications
marcochavezf
force-pushed
the
marcochavezf/migrate-modified-expense-to-useOnyx
branch
from
62f9930 to
128592e
Compare
|
@MelvinBot please fix the eslint and typescript errors |
|
Fixed the failing ESLint and TypeScript checks. Issue: The code was destructuring Fix: Changed the destructured variable from
|
Beamanator
left a comment
Beamanator
left a comment
There was a problem hiding this comment.
Both concerns from my previous review have been addressed in cbec8b1a3fb:
-
Billable/Reimbursable localization --
getForReportActionTempnow translates raw values (e.g."billable"→translate('common.billable').toLowerCase()) before passing tobuildMessageFragmentForValue, matching the originalgetForReportActionbehavior. -
AI-generated fallback -- The empty-message fallback with
reportActionOriginalMessage?.aiGeneratedcheck has been added, producing"changed the expense based on past activity"when appropriate.
Tests for all three cases (billable, reimbursable, AI fallback) have been added and verify the correct translated output.
The overall migration approach (adding useOnyx for policyTags in ReportActionItem, passing currentUserLogin from useCurrentUserPersonalDetails, and the policyIDForTags fallback for moved expenses) looks solid as PR 1 of the migration plan in #80263.
Minor note for future PRs: test coverage for tag changes, tax amount/rate, date, attendees, and policyRulesModifiedFields would further strengthen confidence in behavioral parity, but is not blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbec8b1a3f
ℹ️ 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".
Co-authored-by: Marco Chávez <marcochavezf@users.noreply.github.com>
|
Fixed the Prettier formatting in |
|
Regarding the codex-connector thread about This PR already handles the fake policyID case in const policyIDForTags = report?.policyID === CONST.POLICY.OWNER_EMAIL_FAKE && policyForMovingExpensesID ? policyForMovingExpensesID : report?.policyID;
const [policyTags] = useOnyx(...);The remaining concern is about the transient state where |
| getMessageLine(translate, `\n${translate('iou.removed')}`, removalFragments); | ||
|
|
||
| if (message === '') { | ||
| // If we don't have enough structured information to build a detailed message but we |
There was a problem hiding this comment.
Are these changes necessary for this PR? If not, can you please remove it so that it is more straight-forward to review?
There was a problem hiding this comment.
Yes, these changes are necessary. Beamanator flagged in his review that getForReportActionTemp was missing the aiGenerated fallback that exists in getForReportAction. Since this PR migrates ReportActionItem to call getForReportActionTemp instead of getForReportAction, without this fallback there would be a behavioral regression: AI-attributed expense modifications would display "changed the expense" instead of "changed the expense based on past activity."
The same reasoning applies to the billable/reimbursable localization fix just above — both were required to maintain parity with the function being replaced.
🤖 This comment was generated with the assistance of an AI tool.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0515fdaaf7
ℹ️ 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".
…Tags Update the comment on policyIDForTags to explain why a fake policyID requires falling back to policyForMovingExpensesID. Add CONST.POLICY.DEFAULT_TAG_LIST fallback when policyTags is undefined to prevent tag-only edits from degrading to the generic "changed the expense" message. Co-authored-by: Marco Chávez <marcochavezf@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d18ddbb6e
ℹ️ 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 originalReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${originalReportID}`]; | ||
| const isOriginalReportArchived = useReportIsArchived(originalReportID); | ||
| const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); | ||
| const {accountID: currentUserAccountID, login: currentUserLogin} = useCurrentUserPersonalDetails(); |
There was a problem hiding this comment.
useCurrentUserPersonalDetails() does not guarantee login is populated (the provider backfills email from session, but not login), so extracting login here and later passing currentUserLogin ?? '' into getForReportActionTemp can make isPolicyAdmin() evaluate false while personal details are still loading, which drops the admin-only workspace rules link from MCC-based modified-expense messages. The previous implementation used session-backed login directly, so this is a behavior regression; please fall back to email when login is absent (and apply the same fallback where this payload is built for context-menu copy).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — verified this is a real regression. The CurrentUserPersonalDetailsProvider backfills email from session but not login, so login can be undefined during loading. The old code used session.email which was always available.
@MelvinBot Please fix this by using email instead of login in both locations:
- In
src/pages/inbox/report/ReportActionItem.tsx:
// Change:
const {accountID: currentUserAccountID, login: currentUserLogin} = useCurrentUserPersonalDetails();
// To:
const {accountID: currentUserAccountID, email: currentUserEmail} = useCurrentUserPersonalDetails();And update the usage: currentUserLogin: currentUserEmail ?? ''
- In
src/pages/inbox/report/ContextMenuActions.tsx, change:
currentUserLogin: currentUserPersonalDetails?.login ?? '',
// To:
currentUserLogin: currentUserPersonalDetails?.email ?? '',This maintains parity with the old session-backed email value.
🤖 This comment was generated with the assistance of an AI tool.
CurrentUserPersonalDetailsProvider backfills email from session but not login, so login can be undefined during loading. Use email to maintain parity with the old session-backed value. Co-authored-by: Marco Chávez <marcochavezf@users.noreply.github.com>
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Updated and ready for another review |
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
Nice, that comment it better!
There was a problem hiding this comment.
Re-reviewed the latest changes (3 new commits since my last approval). Everything looks good:
-
login→emailfix — Correct choice sinceuseCurrentUserPersonalDetailsbackfillsemailfrom session but notlogin. PreventsisPolicyAdmin()from getting an empty string during loading, which would drop the admin workspace rules link. -
DEFAULT_TAG_LISTfallback —policyTags: policyTags ?? CONST.POLICY.DEFAULT_TAG_LISTmatches the originalgetForReportActionbehavior and prevents tag-only edits from degrading to the generic "changed the expense" during initial load/cache misses. -
Improved comment on
policyIDForTags— Now clearly explains why the fakepolicyIDfallback is needed, not just what it does.
Previous fixes (billable/reimbursable localization, AI fallback, test coverage) remain intact. No new concerns found. CI is green.
^ Added by Opus 4.6 - but I also manually reviewed and agree 🙏
Beamanator
deleted the
marcochavezf/migrate-modified-expense-to-useOnyx
branch
|
🚧 @Beamanator has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.3.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.18-8 🚀
|
7 participants
Explanation of Change
This PR properly migrates the
ReportActionItemandContextMenuActionscomponents to use theuseOnyxhook forpolicyTagsandsessiondata, addressing the code review feedback about deprecatedOnyx.connectpatterns.Changes:
ReportActionItem.tsx:getForReportActionTempinstead ofgetForReportActionuseLocalizehook to gettranslatefunctionuseOnyxhook to fetchpolicyTagsandsessiontranslate,policy,policyTags, andcurrentUserLogintogetForReportActionTempContextMenuActions.tsx:currentUserLoginto destructuring in copy to clipboard actioncurrentUserLogintogetForReportActionTempBaseReportActionContextMenu.tsx:useOnyxhook forsessioncurrentUserLoginin the payloadModifiedExpenseMessage.ts:currentUserLoginparameter togetForReportActionTempOnyx.connectWithoutViewcallsWhy this approach:
The
getForReportActionTempfunction accepts parameters instead of using module-level Onyx connections. This allows React components to properly re-render when the underlying data changes and follows the recommended pattern of making functions pure.Fixed Issues
Part of #80263
Tests
Offline tests
N/A - This change does not affect offline behavior. The data fetching pattern remains the same.
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
N/A - No UI changes, this is a refactor
Android: mWeb Chrome
N/A - No UI changes, this is a refactor
iOS: Native
N/A - No UI changes, this is a refactor
iOS: mWeb Safari
N/A - No UI changes, this is a refactor
MacOS: Chrome / Safari
N/A - No UI changes, this is a refactor