refactor getMovedTransactionMessage to add policy in its param#86237
refactor getMovedTransactionMessage to add policy in its param#86237dukenv0307 wants to merge 13 commits intoExpensify:mainfrom
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@DylanDylann 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d8d1a08f
ℹ️ 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".
|
No product review needed |
|
@DylanDylann Done |
|
@dukenv0307 I suggest adding report to the params too, then we only need to pass one policy |
|
@DylanDylann Can you add more details? Why do we need to pass a report while we can get it from action? Lines 7041 to 7048 in e38712f |
|
Currently, we need to pass two policies, which seems redundant. If we determine the report earlier, we could pass only one policy. |
|
Since we’ll need to pass the report as a parameter in the future, I’d prefer to clean this up now instead of passing 2 polict object |
|
I think it'd be overkill. If we want to pass 1 policy, that means we have to determine the report before calling getMovedTransactionMessage. As you can see, getMovedTransactionMessage is used in too many places and refactoring the report is out of this issue's scope. TBH, passing 1 more param is much better than refactoring the report in this case |
|
@dukenv0307 In the current code, we already pass fromReportPolicy and toReportPolicy, which means both reports are already computed in the component. So I thought passing the report along with the policy shouldn’t be too complex. I worry that we might miss cleaning up the policy parameters when refactoring the report parameter. So could you add a comment to remind us that we should clean the policy object when refactoring the report parameter (also note the link to that issue) |
|
|
||
| const [movedFromReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(lastAction, CONST.REPORT.MOVE_TYPE.FROM)}`); | ||
| const [movedToReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(lastAction, CONST.REPORT.MOVE_TYPE.TO)}`); | ||
| const [movedFromReportPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${movedFromReport?.policyID}`); |
There was a problem hiding this comment.
@dukenv0307 This is only for the dependency list ???
| const movedFromReport = isActionOfType(parentReportAction, CONST.REPORT.ACTIONS.TYPE.MOVED_TRANSACTION) | ||
| ? reports?.[`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(parentReportAction, CONST.REPORT.MOVE_TYPE.FROM)}`] | ||
| : undefined; | ||
| const movedToReport = isActionOfType(parentReportAction, CONST.REPORT.ACTIONS.TYPE.MOVED_TRANSACTION) | ||
| ? reports?.[`${ONYXKEYS.COLLECTION.REPORT}${getMovedReportID(parentReportAction, CONST.REPORT.MOVE_TYPE.TO)}`] | ||
| : undefined; |
There was a problem hiding this comment.
we should avoid using reports going forward since they’re deprecated. Keeping this usage will increase the workload for future refactors.
But in this case, I’m fine with it to keep the scope under control
|
@DylanDylann can you please review this PR? |
|
@DylanDylann Kindly bump |
|
How about #86237 (comment)? |
| const parentReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`]; | ||
| const parentReportAction = isThread(report) ? reportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`]?.[report.parentReportActionID] : undefined; | ||
|
|
||
| const movedFromReport = isActionOfType(parentReportAction, CONST.REPORT.ACTIONS.TYPE.MOVED_TRANSACTION) |
There was a problem hiding this comment.
Since we’re using reports and policies from params, could you verify that they are fully passed in all usages?
Co-authored-by: DylanDylann <141406735+DylanDylann@users.noreply.github.com>
@DylanDylann getMovedTransactionMessage isn't just used in the component, it's used in the lib functions as well. For example, in |
|
| movedFromReportPolicy?: OnyxEntry<Policy>; | ||
| movedToReportPolicy?: OnyxEntry<Policy>; |
There was a problem hiding this comment.
With this, I afraid that we will miss passing these prams
| fromMovedReportPolicy?: Policy, | ||
| toMovedReportPolicy?: Policy, |
| movedFromReportPolicy?: OnyxEntry<Policy>; | ||
| movedToReportPolicy?: OnyxEntry<Policy>; |
| movedFromReportPolicy?: OnyxEntry<Policy>; | ||
| movedToReportPolicy?: OnyxEntry<Policy>; |
|
@dukenv0307 Since we only need these new policy params when the action is MOVED_TRANSACTION, could we optimize the subscription to avoid unnecessary re-renders, maybe by using a derived key? |
Explanation of Change
Fixed Issues
$ #66411
PROPOSAL:
Tests
Offline tests
QA Steps
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-03-24.at.22.12.21.mov