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-04-15] [$250] Remove deprecated ReportUtils.getPolicy() method #38959

Closed
Tracked by #27262
tgolen opened this issue Mar 25, 2024 · 38 comments
Closed
Tracked by #27262
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 Reviewing Has a PR in review

Comments

@tgolen
Copy link
Contributor

tgolen commented Mar 25, 2024

This is coming from #27262. You can read the issue description there to get context behind the problem being solved and the mess being cleaned up.

Problem

ReportUtils.getPolicy() is called from a view component and other action files which is an anti-pattern.

Why this is important to fix

It maintains a more pure and exact flow of data through the react application. If the view is using policy data, then it needs to subscribe to the policy in Onyx so that it's garaunteed that the data will never be stale or out-of-date.

Solution

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010ae8bb081ab888f1
  • Upwork Job ID: 1772360821776945152
  • Last Price Increase: 2024-03-25
  • Automatic offers:
    • DylanDylann | Contributor | 0
Issue OwnerCurrent Issue Owner: @CortneyOfstad
@tgolen tgolen 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 Mar 25, 2024
@tgolen tgolen self-assigned this Mar 25, 2024
Copy link

melvin-bot bot commented Mar 25, 2024

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

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

melvin-bot bot commented Mar 25, 2024

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

Copy link

melvin-bot bot commented Mar 25, 2024

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 25, 2024

Proposal

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

Remove deprecated ReportUtils.getPolicy() method

What is the root cause of that problem?

Remove deprecated ReportUtils.getPolicy() method

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

Steps:

  1. Need to remove getPolicy from here:

    App/src/libs/ReportUtils.ts

    Lines 570 to 579 in c1947c2

    /**
    * @deprecated Use withOnyx or Onyx.connect() instead
    */
    function getPolicy(policyID: string | undefined): Policy | EmptyObject {
    if (!allPolicies || !policyID) {
    return {};
    }
    return allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? {};
    }
  2. There are total 2 components & 18 util functions which uses getPolicy, from there we need to remove the use of getPolicy and add extra parameter for policy.

Note: For components we will use withOnyx()

Result

@abzokhattab
Copy link
Contributor

abzokhattab commented Mar 25, 2024

Proposal

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

Remove deprecated ReportUtils.getPolicy() method

What is the root cause of that problem?

Remove deprecated ReportUtils.getPolicy() method.

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

we need to remove the ReportUtil.getPolicy()

const policy = ReportUtils.getPolicy(policyID);

and, here

const policy = ReportUtils.getPolicy(policyID);

and replace it with oynx similar to the following snippet

withOnyx({
  policy: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.POLICY}${route.params.policyID}`,
    },
})

then here

const policy = ReportUtils.getPolicy(policyID);
, we should use Onyx.connect on top to get the policy using the given id.

also we need to the do same in all the occurencies inside the IOU.js and in the Policy.ts files

@ghost
Copy link

ghost commented Mar 25, 2024

Proposal

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

Remove deprecated ReportUtils.getPolicy() method

What is the root cause of that problem?

Identify the usage of ReportUtils.getPolicy() method.

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

After that we need to :

  • Use withOnyx() to load the data in view components
  • Use Onyx.connect() to load the data in other action files

What alternative solutions did you explore? (Optional)

N/A

@nayabatir1
Copy link

nayabatir1 commented Mar 25, 2024

Proposal

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

Remove deprecated ReportUtils.getPolicy().

What is the root cause of that problem?

Since ReportUtils.getPolicy() is deprecated need to replace it everywhere with withOnyx or Onyx.connect() to get data

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

  • List of files for implementing this change
    • SettlementButton.tsx
    • NextStepUtils.ts
    • ReportUtils.ts
    • Policy.ts
    • WorkspaceJoinUserPage.tsx
  • add test case to check getPolicy is not exported from ReportUtils.ts

What alternative solutions did you explore? (Optional)

@nkdengineer
Copy link
Contributor

Proposal

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

Remove deprecated ReportUtils.getPolicy() method

What is the root cause of that problem?

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

  1. For component we should get policy via withOnyx of if the component already subscribe the policy collection, we will get the policy from this
policy: {
    key: ({policyID}) =>  `${ONYXKEYS.COLLECTION.POLICY}${policyID}`
}

export default withOnyx<SettlementButtonProps, SettlementButtonOnyxProps>({

const policy = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`]

const policy = ReportUtils.getPolicy(policyID);

  1. For the libs action, in we should use Onyx.connect to get allPolicies and then use this to get the policy
let allPolicies: OnyxCollection<Policy>;
Onyx.connect({
    key: ONYXKEYS.COLLECTION.POLICY,
    waitForCollectionCallback: true,
    callback: (value) => (allPolicies = value),
});

OPTION: We can create a local function in action file to get the policy with policyID and use this in other place of this file that we want.

  1. Add a test here to verify that we do not export getPolicy function in ReportUtils
it('does not export getPolicy', () => {
    // @ts-expect-error the test is asserting that it's undefined, so the TS error is normal
    expect(ReportUtils.getPolicy).toBeUndefined();
});

describe('ReportUtils', () => {

What alternative solutions did you explore? (Optional)

NA

@samilabud
Copy link
Contributor

samilabud commented Mar 26, 2024

Proposal

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

Remove deprecated ReportUtils.getPolicy() method

What is the root cause of that problem?

ReportUtils.getPolicy() is called from a view component and other action files which is an anti-pattern.

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

We should do the next changes in SettlementButton component:

Add the prop policy like:

type SettlementButtonOnyxProps = {
    /** The last payment method used per policy */
    nvpLastPaymentMethod?: OnyxEntry<LastPaymentMethod>;
    /** Policy to which the report belongs to */
    policy: OnyxEntry<Policy>;
};

Receive as a param the policy like:

function SettlementButton({
    .
    .
    .
    policy,
}: SettlementButtonProps) {

Remove the line:

const policy = ReportUtils.getPolicy(policyID);

Also we should remove the export of the function 'getPolicy' of ReportUtils.ts

Additionally we should reuse the function getPolicity in Policy.ts and replace all the lines with 'ReportUtils.getPolicy(policyID)' by 'getPolicy(policyID)'.

Later we should make the next changes in WorkspaceJoinUserPage:

Add policy prop:

type WorkspaceJoinUserPageOnyxProps = {
    /** The list of this user's policies */
    policies: OnyxCollection<Policy>;
    /** Policy to which the report belongs to */
    policy: OnyxEntry<Policy>;
};

Add the policy param to the function:
function WorkspaceJoinUserPage({route, policies, policy}: WorkspaceJoinUserPageProps) {

Add the policy prop inside withOnyx:

export default withOnyx<WorkspaceJoinUserPageProps, WorkspaceJoinUserPageOnyxProps>({
    policies: {
        key: ONYXKEYS.COLLECTION.POLICY,
    },
    policy: {
        key: ({route}) => `${ONYXKEYS.COLLECTION.POLICY}${route.params.policyID}`,
    },
})(WorkspaceJoinUserPage);

Add this to EnforceActionExportRestrictions inside of ReportUtils section:

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

Lastly we should modify NextStepUtils.ts as indicated below:
importing these:

import type {NullishDeep, OnyxCollection} from 'react-native-onyx';
import type {Policy, ...} from '@src/types/onyx';

Adding this:

const allPolicies: OnyxCollection<Policy> = {};
Onyx.connect({
    key: ONYXKEYS.COLLECTION.POLICY,
    callback: (val, key) => {
        if (!key) {
            return;
        }
        if (val === null || val === undefined) {
            // If we are deleting a policy, we have to check every report linked to that policy
            // and unset the draft indicator (pencil icon) alongside removing any draft comments. Clearing these values will keep the newly archived chats from being displayed in the LHN.
            // More info: https://github.com/Expensify/App/issues/14260
            const policyID = key.replace(ONYXKEYS.COLLECTION.POLICY, '');
            const policyReports = ReportUtils.getAllPolicyReports(policyID);
            const cleanUpMergeQueries: Record<`${typeof ONYXKEYS.COLLECTION.REPORT}${string}`, NullishDeep<Report>> = {};
            const cleanUpSetQueries: Record<`${typeof ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${string}` | `${typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${string}`, null> = {};
            policyReports.forEach((policyReport) => {
                if (!policyReport) {
                    return;
                }
                const {reportID} = policyReport;
                cleanUpMergeQueries[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] = {hasDraft: false};
                cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] = null;
                cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}`] = null;
            });
            Onyx.multiSet(cleanUpSetQueries);
            delete allPolicies[key];
            return;
        }

        allPolicies[key] = val;
    },
});

/**
 * Get the policy from a given policy ID
 */
function getPolicy(policyID: string | undefined): Policy | EmptyObject {
    if (!allPolicies || !policyID) {
        return {};
    }
    return allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? {};
}

And replacing the 'ReportUtils.getPolicy' by the new function called 'getPolicy'

This last modifications should be similar in IOU.ts file, except by the OnyxTypes, should be not imported just using like OnyxTypes.Policy.

This is the temporary/draft PR to try out: #38976

@CortneyOfstad
Copy link
Contributor

@eVoloshchak — we have quite a few proposals above. Can you provide feedback by EOD? Thanks!

@CortneyOfstad
Copy link
Contributor

@tgolen I reviewed all of the linked PRs in the main issue you linked (#27262) and I wasn't able to find a connection to any Waves or VIPs. Can you provide some context into whether or not this falls into one of those classifications? Thanks!

@tgolen
Copy link
Contributor Author

tgolen commented Mar 26, 2024 via email

@CortneyOfstad
Copy link
Contributor

Sounds good, thanks @tgolen!

@tgolen
Copy link
Contributor Author

tgolen commented Mar 27, 2024

@eVoloshchak are you able to take a look at the proposals? If not today, I will reassign this tomorrow.

@DylanDylann
Copy link
Contributor

I am happy to take over this issue if he isn't available

@tgolen
Copy link
Contributor Author

tgolen commented Mar 28, 2024

@DylanDylann Can you please take this one? Thank you! I will reassign it.

@tgolen tgolen assigned DylanDylann and unassigned eVoloshchak Mar 28, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 28, 2024
Copy link

melvin-bot bot commented Mar 28, 2024

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

@DylanDylann
Copy link
Contributor

@nkdengineer Why do you think this point is necessary here?

Add a test here to verify that we do not export getPolicy function in ReportUtils

@nkdengineer
Copy link
Contributor

Remove the usage of ReportUtils.getPolicy() or at least prevent it from being exported by adding a test to https://github.com/Expensify/App/blob/main/tests/actions/EnforceActionExportRestrictions.ts

@DylanDylann This is a requirement here.

@DylanDylann
Copy link
Contributor

Waiting for @nkdengineer's PR to be ready for review

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Overdue Daily KSv2 labels Apr 1, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Apr 5, 2024

@nkdengineer @DylanDylann @tgolen Seems regression from this PR where the join workspace flow turns to an infinite loading and doesn't make API JoinWorkspaceViaInviteLink call. Earlier ReportUtils.getPolicy is returning with the {} for the unavailable policy but now it could be undefined as we are fetching differently. So this condition will always turn to true and returns from here early. I think that !policy should be removed from the condition which doesn't make sense there.

Screen.Recording.2024-04-05.at.19.37.17.mov

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [$250] Remove deprecated ReportUtils.getPolicy() method [HOLD for payment 2024-04-15] [$250] Remove deprecated ReportUtils.getPolicy() method Apr 8, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

Copy link

melvin-bot bot commented Apr 8, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 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-04-15. 🎊

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

Copy link

melvin-bot bot commented Apr 8, 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:

  • [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
  • [@DylanDylann] 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:
  • [@DylanDylann] 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:
  • [@DylanDylann] Determine if we should create a regression test for this bug.
  • [@DylanDylann] 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.
  • [@CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Apr 10, 2024
@nkdengineer
Copy link
Contributor

@DylanDylann Create a follow up PR here #39991 to fix the bug that is mentioned by @Pujan92 above #38959 (comment).

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2024
@CortneyOfstad
Copy link
Contributor

So there was a regression, so this will result in a price of $250 instead of $500 for both @DylanDylann and @nkdengineer.

@DylanDylann can you handle the checklist here by EOD?

@nkdengineer I sent you a manual offer in Upwork. Please let me know once you accept 👍

@DylanDylann
Copy link
Contributor

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: NA
[@DylanDylann] 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: NA
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. No
[@DylanDylann] 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.

@DylanDylann
Copy link
Contributor

This is clean up issue, so I think we don't need regression test step

@CortneyOfstad
Copy link
Contributor

Sounds good @DylanDylann!

@CortneyOfstad
Copy link
Contributor

Payment Summary

@DylanDylann — paid $250 via Upwork
@nkdengineer — paid $250 via Upwork

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 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants