[HOLD for payment 2023-10-16] [$500] propTypes warning - policy is marked as required in MoneyRequestHeader

[marcaaron]

Note this one is maybe just holding on the TS migration since we are deprecating propTypes IIRC cc @hayata-suenaga

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d965fee45c4003a6
• Upwork Job ID: 1700249043663089664
• Last Price Increase: 2023-09-08

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[hayata-suenaga]

@marcaaron We're still requiring propTypes in JavaScript files. If there is an issue with what is specified in propTypes and the actual type being passed to a prop, we should keep fixing those errors 🙇

Having correct propTypes will also aid TypeScript migration process as migration engineers can rely on propTypes when defining TypeScript types.

[spcheema]

@marcaaron I have reported this issue here
https://expensify.slack.com/archives/C049HHMV9SM/p1693657079886579

[spcheema]

Here is proposal if I am allowed to have one:

Proposed solution

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

DEV: console error on request money thread

What is the root cause of that problem?**

After policy is deleted, we dont have data in onyx. So policy data returned is undefined which is throwing an error when undefined passed to MoneyRequestHeader.

policies[${ONYXKEYS.COLLECTION.POLICY}${report.policyID}]

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

We can pass empty object if policy is not found - ReportScreen.js

            <MoneyRequestHeader
                report={report}
-                policy={policy}
+               policy={policy || {}}
                personalDetails={personalDetails}
                isSingleTransactionView={isSingleTransactionView}
                parentReportAction={parentReportAction}
            />

So in this scenario, getPolicyName will return Unavailable workspace as the policy name and the default workspace avatar will be generated accordingly which is already in action

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[melvin-bot]

Current assignee @abekkala is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[azamuddin]

After made a quick analysis to the related files, here's what I found and my proposed solution:

Component Tree

How the policy is used in the context of this issue:

ReportScreen.js -> MoneyRequestHeader -> HeaderWithBackButton -> AvatarWithDisplayName 

• policy object is defined and first supplied in <ReportScreen/>

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

• <MoneyRequestHeader/> and <HeaderWithBackButton/> is only passing down the props
• Actual usage of policy object is in <AvatarWithDisplayName/> component

propTypes Inconsistencies

There is propTypes inconsistencies amongs components:

• Here is the propTypes policy in <MoneyRequestHeader/>:

/** The policy which the report is tied to */
policy: PropTypes.shape({
    /** Name of the policy */
    name: PropTypes.string,
}).isRequired,

• Here is the propTypes policy in <HeaderWithBackButton/>:

/** The report's policy, if we're showing the details for a report and need info about it for AvatarWithDisplay */
policy: PropTypes.shape({
    /** Name of the policy */
    name: PropTypes.string,
}),

• Here is the propTypes policy in <AvatarWithDisplayName/>:

/** The policy which the user has access to and which the report is tied to */
policy: PropTypes.shape({
    /** Name of the policy */
    name: PropTypes.string,
}),

As we can see from above codes, <MoneyRequestHeader/> marks policy as required, but <HeaderWithBackButton/> and <AvatarWithDisplayName/> is treating it as optional props.

Proposed Solution

Mark policy object as optional

If policy is actually not a required in the runtime usage, then the solution is simply remove isRequired mark in the <MoneyRequestHeader/> propTypes.policy.

This change will also fixes the propTypes inconsistencies amongs different components in the Component Tree I mentioned above.

Supporting evidence

If we are looking into the actual use of policy object in <AvatarWithDisplayName/> it is supplied to ReportUtils.getIcons, and if we open the method signature, it looks like this:

function getIcons(report, personalDetails, defaultIcon = null, isPayer = false, defaultName = '', defaultAccountID = -1, policy = undefined) {
  // the code
}

As we can see, policy here is supplied with default value of undefined, which means it is actually an optional parameter. So it makes sense to also mark propTypes.policy as optional.

Beyond The Problem in This Component Tree

The original problem on this github issue should be solved with the proposed solution above.

Here I just want to make an additional comments with regards to policy object usage in general.

More propTypes inconsistencies

In src/pages/home/ReportScreen.js policies propTypes is an object of shape of policy shape that have property of name and type :

policies: PropTypes.objectOf(
    PropTypes.shape({
        /** The policy name */
        name: PropTypes.string,

        /** The type of the policy */
        type: PropTypes.string,
    }),
),

Meanwhile, in src/pages/home/report/ReportActionsView.js propTypes policy is defined as below:

policy: PropTypes.shape({
    /** The name of the policy */
    name: PropTypes.string,

    /** The URL for the policy avatar */
    avatar: PropTypes.string,
})

It has avatar property.

So far we have 3 different shape of the same policy object.

1. with name property only (in <MoneyRequestHeader/>, <HeaderWithBackButton/> and <AvatarWithDisplayName/>)
2. with name and type as in src/pages/home/ReportScreen.js
3. with name and avatar as in src/pages/home/report/ReportActionsView.js

If you are about to migrate to typescript, you will need to unify the Policy type / interface and shares this accross component, this will make consistent.

[melvin-bot]

📣 @azamuddin! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[azamuddin]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/muhammada1457

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[tienifr]

Proposal

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

Warning shows on the MoneyRequestHeader if the policy is deleted.

What is the root cause of that problem?

In here, the policy is marked as required, but when the policy is deleted, it will be undefined, leading to the warning.

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

We should set default value for the policy in defaultProps here.

policy: {},

(Instead of default {}, we can potentially provide empty string as the value of the fields of policy as well)

Many other places in the app already follow this same defaultProps fix for the policy, for example here and here.

We can optionally remove the isRequired from the policy in propTypes to make it optional.

There're other places that have the same issue, like MoneyReportHeader, we should fix there as well.

What alternative solutions did you explore? (Optional)

Probably unrelated to this issue, but when the workspace is deleted, it's money report and money request can still have new messages sent into it, meanwhile the main workspace chat will display This workspace chat is no longer active because Deleted WP is no longer an active workspace., we can consider displaying the same for the money report and money request as well, for consistency, using the same condition.

[yusufipsum]

During component transitions, we must prevent it from moving to the next component before the policy prop data arrives.

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01f0c399b94f4fb448

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[melvin-bot]

@abekkala, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abekkala]

@sobitneupane have you had a chance to review the proposals that have come in?

[sobitneupane]

Thanks for proposal everyone. If policy is an optional prop, it is better to remove isRequired and add a default value for it. Proposal from @azamuddin looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

• fix: mark propTypes policy as optional and provides default value #28213

If no regressions arise, payment will be issued on 2023-10-16. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @azamuddin requires payment
• @sobitneupane does not require payment (Eligible for Manual Requests)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-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:

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

[abekkala]

PAYMENTS FOR OCT 16

• @azamuddin (Contributor) - [$500 - no bonus]
• @sobitneupane (Reviewer) [$500 - no bonus] 🚫 Upwork payment (please request manually in Expensify Chat)

[abekkala]

@sobitneupane can you complete the checklist above?

[abekkala]

@azamuddin payment made and contract ended - thank you! 🎉

[abekkala]

@sobitneupane can you complete the checklist above prior to requesting payment in Expensify Chat

[abekkala]

Last action needed here:

@sobitneupane completes the checklist

Payment for:
@sobitneupane (Reviewer) [$500 - no bonus] 🚫 Upwork payment (please request manually in Expensify Chat)

[abekkala]

@sobitneupane can you complete the checklist above?

[sobitneupane]

@abekkala Sorry for the delay. I was OOO. I will try to get it done in the weekend.

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#22484

• [@sobitneupane] 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:

https://github.com/Expensify/App/pull/22484/files#r1375904116

• [@sobitneupane] 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:

It is an edge case and appears only for deleted workspace. It is quite difficult to catch such issues.

• [@sobitneupane] Determine if we should create a regression test for this bug.

I don't think we can add regression test for this bug. So, it is only reproducible in dev.

[sobitneupane]

#26991 (comment)

Requested payment on newDot.

[melvin-bot]

@puneetlath, @azamuddin, @abekkala, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abekkala]

closing this as payment is handled in newdot and has already been requested

[JmillsExpensify]

$500 payment approved for @sobitneupane based on this comment.