Skip to content
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

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

Closed
bondydaa opened this issue Sep 20, 2024 · 40 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@bondydaa
Copy link
Contributor

bondydaa commented Sep 20, 2024

Greater context in this issue #27262

Problem

We have marked this method as deprecate and need to remove it

/**
* Returns the parentReportAction if the given report is a thread/task.
*
* @deprecated Use Onyx.connect() or withOnyx() instead
*/
function getParentReportAction(report: OnyxInputOrEntry<Report>): OnyxEntry<ReportAction> {
if (!report?.parentReportID || !report.parentReportActionID) {
return undefined;
}
return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID];
}

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.

const parentReportAction = getParentReportAction(report);

function getParentReportAction(report: OnyxEntry<OnyxTypes.Report>): OnyxEntry<ReportAction> {
// If the report is not a thread report, then it won't have a parent and an empty object can be returned.
if (!report?.parentReportID || !report.parentReportActionID) {
return undefined;
}
return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID];
}

selector: (parentReportActions) => getParentReportAction(parentReportActions, reportOnyx?.parentReportActionID ?? ''),

function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, parentReportActionID: string | undefined): OnyxEntry<OnyxTypes.ReportAction> {
if (!parentReportActions || !parentReportActionID) {
return;
}
return parentReportActions[parentReportActionID ?? '0'];
}

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 OwnerCurrent Issue Owner: @getusha
@bondydaa bondydaa added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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
Copy link
Contributor Author

doh ignore for now! 🤦

@bondydaa bondydaa changed the title Remove deprecated Report Remove deprecated ReportActionUtils.getParentReportAction method Sep 20, 2024
@bondydaa bondydaa removed Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Sep 20, 2024
@bondydaa bondydaa self-assigned this Sep 20, 2024
@bondydaa bondydaa added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 20, 2024
@melvin-bot melvin-bot bot changed the title Remove deprecated ReportActionUtils.getParentReportAction method [$250] Remove deprecated ReportActionUtils.getParentReportAction method Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

Copy link

melvin-bot bot commented Sep 20, 2024

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

@bondydaa bondydaa changed the title [$250] Remove deprecated ReportActionUtils.getParentReportAction method [$125] Remove deprecated ReportActionUtils.getParentReportAction method Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

Upwork job price has been updated to $125

@nkdengineer
Copy link
Contributor

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
Copy link
Contributor

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

* Returns the parentReportAction if the given report is a thread/task.
*
* @deprecated Use Onyx.connect() or withOnyx() instead
*/
function getParentReportAction(report: OnyxInputOrEntry<Report>): OnyxEntry<ReportAction> {
if (!report?.parentReportID || !report.parentReportActionID) {
return undefined;
}
return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`]?.[report.parentReportActionID];
}

@ChavdaSachin

This comment was marked as outdated.

@1subodhpathak
Copy link
Contributor

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.

      App/src/libs/actions/Task.ts

      Lines 1108 to 1119 in 9160fa5

      function getTaskAssigneeAccountID(taskReport: OnyxEntry<OnyxTypes.Report>): number | undefined {
      if (!taskReport) {
      return;
      }
      if (taskReport.managerID) {
      return taskReport.managerID;
      }
      const reportAction = getParentReportAction(taskReport);
      return reportAction?.childManagerAccountID;
      }
  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()

@mallenexpensify mallenexpensify added the NewFeature Something to build that is a new item. label Sep 23, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 23, 2024
Copy link

melvin-bot bot commented Sep 23, 2024

⚠️ 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 ⚠️

@bondydaa
Copy link
Contributor Author

bondydaa commented Oct 3, 2024

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

#49551 (comment)

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

describe('Task', () => {
it('does not export getParentReport', () => {
// @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
expect(Task.getParentReport).toBeUndefined();
});
});

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 3, 2024
Copy link

melvin-bot bot commented Oct 3, 2024

📣 @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 📖

Copy link

melvin-bot bot commented Oct 4, 2024

⚠️ 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.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 4, 2024
@mallenexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 17, 2024
@melvin-bot melvin-bot bot changed the title [$125] Remove deprecated ReportActionUtils.getParentReportAction method [HOLD for payment 2024-10-24] [$125] Remove deprecated ReportActionUtils.getParentReportAction method Oct 17, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 17, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

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

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

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

Copy link

melvin-bot bot commented Oct 17, 2024

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:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 23, 2024
@sonialiap
Copy link
Contributor

Payment summary:

Copy link

melvin-bot bot commented Oct 29, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Oct 29, 2024
@sonialiap
Copy link
Contributor

@getusha bumping for the checklist

Copy link

melvin-bot bot commented Oct 31, 2024

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

@getusha
Copy link
Contributor

getusha commented Nov 4, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 4, 2024
@garrettmknight
Copy link
Contributor

$125 approved for @getusha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants