[HOLD for payment 2024-10-24] [$125] Remove deprecated ReportActionUtils.getParentReportAction method

[bondydaa]

Greater context in this issue https://github.com/Expensify/App/issues/27262

Problem

We have marked this method as deprecate and need to remove it https://github.com/Expensify/App/blob/97b51ac79d1fce0f09cfde5081594d138c817603/src/libs/ReportActionsUtils.ts#L337-L347

Solution

I'm not seeing any usages of this method so I think we can probably just remove it.

I do see these 2 instances of a similar methods but I believe those are just the local functions and not using the ReportActionUtils method.

https://github.com/Expensify/App/blob/513e6b3c714adbe24cd8b88c9fc9c20130a6c9d4/src/libs/actions/Task.ts#L947 https://github.com/Expensify/App/blob/513e6b3c714adbe24cd8b88c9fc9c20130a6c9d4/src/libs/actions/Task.ts#L919-L925

https://github.com/Expensify/App/blob/513e6b3c714adbe24cd8b88c9fc9c20130a6c9d4/src/pages/home/ReportScreen.tsx#L126 https://github.com/Expensify/App/blob/513e6b3c714adbe24cd8b88c9fc9c20130a6c9d4/src/pages/home/ReportScreen.tsx#L92-L97

If I missed something and we are using ReportActionUtils.getParentReportAction then we need to be sure to replace those with either withOnyx() or Onyx.connect()

cc @tgolen

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021837202794110877572
• Upwork Job ID: 1837202794110877572
• Last Price Increase: 2024-09-27
• Automatic offers:
• nkdengineer | Contributor | 104251488

Issue Owner

Current Issue Owner: @getusha

[melvin-bot[bot]]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[bondydaa]

doh ignore for now! 🤦

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~021837202794110877572

[melvin-bot[bot]]

Current assignee @sonialiap is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

[melvin-bot[bot]]

Upwork job price has been updated to $125

[nkdengineer]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove deprecated ReportActionUtils.getParentReportAction method

What is the root cause of that problem?

This is a new feature

What changes do you think we should make in order to solve the problem?

Remove all use of getParentReportAction in ReportActionUtils and for all lib files that we create a getParentReportAction function, we should add a test in EnforceActionExportRestrictions to ensure that we don't export this function to use in any other file

What alternative solutions did you explore? (Optional)

NA

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

Remove deprecated ReportActionUtils.getParentReportAction method

What changes do you think we should make in order to solve the problem?

New request

What alternative solutions did you explore? (Optional)

we should remove the getParentReportAction from here as its not used anywher

https://github.com/Expensify/App/blob/eab7d0e1a5b27386c7d5153355c3c8e37e7469d8/src/libs/ReportActionsUtils.ts#L337-L347

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-09-25 18:31:50 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove deprecated ReportActionUtils.getParentReportAction method

What is the root cause of that problem?

New request

• ReportActionUtils.getParentReportAction function is used by 2 files
  1. ReportUtils.ts
  2. Policy.ts

What changes do you think we should make in order to solve the problem?

• Remove ReportActionUtils.getParentReportAction() from - ReportActionsUtils.ts.
• On ReportUtils.ts we are already fetching allReportActions using onyx.connect() method similarly fetch allReportActions using onyx.connect() on Policy.ts

  let allReportActions: OnyxCollection<ReportActions>;
  Onyx.connect({
  key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
  waitForCollectionCallback: true,
  callback: (actions) => {
      if (!actions) {
          return;
      }
      allReportActions = actions;
  },
  });

• Create local getParentReportAction function in ReportUtils.ts and Policy.ts

  function getParentReportAction(report: OnyxInputOrEntry<Report>): OnyxEntry<ReportAction> {
  if (!report?.parentReportID || !report.parentReportActionID) {
      return undefined;
  }
  return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID];
  }

[!note] on task.ts file we already have local implementation which uses onyx.connect() to fetch allReportActions and hence needs no improvement

• Add EnforceActionExportRestrictions test to for files Task.ts, Policy.ts and ReportUtils.ts to avoid exporting getParentReportAction from the same files in future. (as we do for the similar methods which make use of onyx.connect() - Policy.getPolicy , ReportUtils.getPolicy)

  What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[1subodhpathak]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The getParentReportAction method in ReportActionsUtils.ts is deprecated and needs removal.

What is the root cause of that problem?

The method is outdated due to improved Onyx state management practices.

What changes do you think we should make in order to solve the problem?

1. Initially we need to delete the getParentReportAction function from ReportActionsUtils.ts
2. Then refactor task.ts to
   • update local getParentReportAction to use Onyx.connect() or withOnyx().
   • modify getTaskAssigneeAccountID which includes getParentReportAction to use the refactored version. https://github.com/Expensify/App/blob/9160fa585c26d3312fd6d6fbd69d334e66cb373d/src/libs/actions/Task.ts#L1108-L1119
3. We need to verify that the getParentReportAction function in ReportScreen.tsx does not rely on the deprecated method

What alternative solutions did you explore? (Optional)

None considered necessary, as the deprecation notice already suggests using Onyx.connect() or withOnyx()

[melvin-bot[bot]]

Current assignee @sonialiap is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot[bot]]

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

[melvin-bot[bot]]

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

[mallenexpensify]

d'oh, added NewFeature by mistake. @bondydaa , do you need/want a C+ assigned here to review proposals and the PR? If not, please unassign before @getusha comments. Thx

[bondydaa]

oh i think it's fine to keep a c+, go for it @getusha

[getusha]

@1subodhpathak could you provide a code snippet for this please?

update local getParentReportAction to use Onyx.connect() or withOnyx()

I agree we should update task.ts since the function is the exact same as the one in ReportActionUtils

[nkdengineer]

@getusha I think the only thing we need to do here is remove the unused getParentReportAction function in ReportActionUtils. For other getParentReportAction local functions in the lib file we need to make sure this is not exported by adding the test EnforceActionExportRestrictions. That is how we do for other functions like getParentReport, getReport.

[1subodhpathak]

@1subodhpathak could you provide a code snippet for this please?

update local getParentReportAction to use Onyx.connect() or withOnyx()

I agree we should update task.ts since the function is the exact same as the one in ReportActionUtils

Sure @getusha, Here's a code snippet

import { withOnyx } from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';

const getParentReportAction = withOnyx({
  parentReportActions: {
    key: ({ report }) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`,
  },
})(({ parentReportActions, report }) => {
  if (!report?.parentReportID || !report.parentReportActionID) {
    return undefined;
  }
  return parentReportActions?.[report.parentReportActionID];
});

Here -

• I’m using withOnyx() to connect the parentReportActions to the Onyx data
• The function checks for both parentReportID and parentReportActionID and returns the appropriate parentReportAction.

[1subodhpathak]

@getusha what do you think?

[getusha]

@getusha I think the only thing we need to do here is remove the unused getParentReportAction function in ReportActionUtils. For other getParentReportAction local functions in the lib file we need to make sure this is not exported by adding the test EnforceActionExportRestrictions. That is how we do for other functions like getParentReport, getReport.

@nkdengineer the function is basically copy paste from ReportActionUtils, i think we should replace that if we can

[nkdengineer]

@getusha The past problem is we used getParentReportAction of ReportActionUtils in the component and other lib files. We resolved it by creating the same local function in the lib file if we need to use it in many places of this lib file and using Onyx.connect. For component, we used withOnyx/useOnyx to resolve this problem. And for each local function created in the lib file we created, we add a test in EnforceActionExportRestrictions to make sure it isn't exported. That is how we do here

[1subodhpathak]

Removing the unused getParentReportAction function from ReportActionUtils should be a good move here. It will help declutter our codebase and remove deprecated code. I support adding the EnforceActionExportRestrictions test for the local getParentReportAction function in task.ts. This will ensure we're consistent with how we handle similar functions like getParentReport and getReport

[1subodhpathak]

@getusha what's your take on the code snippet I provided?

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-09-25 18:39:03 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove deprecated ReportActionUtils.getParentReportAction method

What is the root cause of that problem?

New request

• ReportActionUtils.getParentReportAction function is used by 2 files
  1. ReportUtils.ts
  2. Policy.ts

What changes do you think we should make in order to solve the problem?

• Remove ReportActionUtils.getParentReportAction() from - ReportActionsUtils.ts.
• On ReportUtils.ts we are already fetching allReportActions using onyx.connect() method similarly fetch allReportActions using onyx.connect() on Policy.ts

  let allReportActions: OnyxCollection<ReportActions>;
  Onyx.connect({
  key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
  waitForCollectionCallback: true,
  callback: (actions) => {
      if (!actions) {
          return;
      }
      allReportActions = actions;
  },
  });

• Create local getParentReportAction function in ReportUtils.ts and Policy.ts

  function getParentReportAction(report: OnyxInputOrEntry<Report>): OnyxEntry<ReportAction> {
  if (!report?.parentReportID || !report.parentReportActionID) {
      return undefined;
  }
  return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID];
  }

[!note] on task.ts file we already have local implementation which uses onyx.connect() to fetch allReportActions and hence needs no improvement

• Add EnforceActionExportRestrictions test to for files Task.ts, Policy.ts and ReportUtils.ts to avoid exporting getParentReportAction from the same files in future. (as we do for the similar methods which make use of onyx.connect() - Policy.getPolicy , ReportUtils.getPolicy)
• Exact same approach is used for getPolicy method across files Membets.ts , Policy.ts & ReportUtils.ts

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mallenexpensify]

@getusha 👀 on the above plz

[getusha]

The past problem is we used getParentReportAction of ReportActionUtils in the component and other lib files. We resolved it by creating the same local function in the lib file if we need to use it in many places of this lib file and using Onyx.connect. For component, we used withOnyx/useOnyx to resolve this problem. And for each local function created in the lib file we created, we add a test in EnforceActionExportRestrictions to make sure it isn't exported. That is how we do https://github.com/Expensify/App/pull/43632

@nkdengineer i am not sure i am following what you meant to say. are you suggesting to keep the function in Task.ts? if yes why?

[nkdengineer]

@nkdengineer are you suggesting to keep the function in Task.ts?

@getusha Yes

if yes why?

Because we're using this locally in Task.ts. You can see the same pattern for getReportOrDraftReport function.

[1subodhpathak]

@getusha what do you think on my proposal?

[getusha]

@nkdengineer is there any reason why we only want to deprecate the function in ReportActionUtils while keeping an exact same function in other files?

Because we're using this locally in Task.ts

@bondydaa what do you think?

[nkdengineer]

is there any reason why we only want to deprecate the function in ReportActionUtils while keeping an exact same function in other files?

@getusha Because we don't use this function anywhere in ReportActionUtils.

[getusha]

@getusha Because we don't use this function anywhere in ReportActionUtils.

@nkdengineer there are several functions in ReportActionUtils that aren't being used in ReportActionUtils

[nkdengineer]

These methods are anti-patterns because they are most always used for loading data into a component without using withOnyx(). This breaks the data flow of a react application. (data is coming from somewhere that is not props or state and cannot be debugged in react dev tools).

@getusha Another reason is this function is anti-patterns like the context here.

Switch all references to properly use withOnyx() for components and connect() for libs.

And here is the solution we did to refactor. And after refactoring, getParentReportAction in ReportActionUtils isn't used anywhere in this lib file then we can remove it.

[bondydaa]

Sorry I haven't been following too closely, just diving in a bit more.

https://github.com/Expensify/App/issues/49551#issuecomment-2374554496

I think the only thing we need to do here is remove the unused getParentReportAction function in ReportActionUtils. For other getParentReportAction local functions in the lib file we need to make sure this is not exported by adding the test EnforceActionExportRestrictions. That is how we do for other functions like getParentReport, getReport.

I see what you're saying now after looking at this file https://github.com/Expensify/App/blob/96acecaa7a47408fbc4f625f59b245189097c9b3/tests/actions/EnforceActionExportRestrictions.ts#L52-L57

👍, I wasn't aware we had tests like this, very cool and yeah makes sense to me. Going to assign you @nkdengineer

[melvin-bot[bot]]

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[mallenexpensify]

@getusha is the next step here for you to review the PR?

• https://github.com/Expensify/App/pull/50211

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.49-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/50211

If no regressions arise, payment will be issued on 2024-10-24. :confetti_ball:

For reference, here are some details about the assignees on this issue:

• @getusha requires payment through NewDot Manual Requests
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot[bot]]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@getusha] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] [@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] [@getusha] Determine if we should create a regression test for this bug.
• [ ] [@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [ ] [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

Payment summary:

• @getusha $125 please request in ND - please complete the checklist
• @nkdengineer $125 paid in upwork

[melvin-bot[bot]]

@bondydaa, @sonialiap, @getusha, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

[sonialiap]

@getusha bumping for the checklist

[melvin-bot[bot]]

@bondydaa, @sonialiap, @getusha, @nkdengineer Still overdue 6 days?! Let's take care of this!

[getusha]

This doesn't require a checklist. This issue is neither a bug nor a feature.