[HOLD for Payment 2024-09-25] [$125] [Search v2.1] Open reports show __fake__ in To column

[luacmartins]

Problem

Coming from here, open reports show __Fake__ in the To column because they don't have a managerID.

Solution

We should just display an empty column in that case.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021831386678971022108
• Upwork Job ID: 1831386678971022108
• Last Price Increase: 2024-09-04
• Automatic offers:
• ikevin127 | Contributor | 103817750

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (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.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $125

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-09-04 17:51:18 UTC.

Proposal


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

[Search v2.1] Open reports show fake in To column

What is the root cause of that problem?

We just simply render the display name.

App/src/components/SelectionList/Search/TransactionListItemRow.tsx

Lines 391 to 396 in dc1407a

	<View style={[StyleUtils.getSearchTableColumnStyles(CONST.SEARCH.TABLE_COLUMNS.FROM)]}>
	<UserInfoCell
	participant={item.to}
	displayName={item.formattedTo}
	/>
	</View>

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


We should pass empty string when item.formattedTo === CONST.REPORT.OWNER_EMAIL_FAKE is true.

                    <UserInfoCell
                        participant={item.to}
                        displayName={item.formattedTo === CONST.REPORT.OWNER_EMAIL_FAKE ? '' : item.formattedTo}
                    />

• Same should be done here also, we should also check other places.

What alternative solutions did you explore? (Optional)

Result

[melvin-bot]

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

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-09-04 19:01:45 UTC.

Proposal

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

Open reports show fake in To column

What is the root cause of that problem?

We don't filter FAKE before displaying

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

Here we can check if formattedTo === CONST.POLICY.OWNER_EMAIL_FAKE then we can fallback to empty string. We follow a similar approach for merchant

App/src/libs/SearchUtils.ts

Line 64 in dc1407a

const formattedTo = to?.displayName ?? to?.login ?? '';

Pseudocode

 const formattedTo = to?.displayName ?? to?.login ?? '';
const againFormatted = formattedTo === CONST.POLICY.OWNER_EMAIL_FAKE ? '' : formattedTo 

We can do the same for any other field if required. In case if we decided to remove only _fake_ then we can use CONST.REPORT.OWNER_EMAIL_FAKE instead of CONST.POLICY.OWNER_EMAIL_FAKE
We can add a check item.formattedTo . If formattedTo is an empty string, the avatar will not be displayed

   {item.formattedTo && (
                    <View style={[StyleUtils.getSearchTableColumnStyles(CONST.SEARCH.TABLE_COLUMNS.FROM)]}>
                        <UserInfoCell
                            participant={item.to}
                            displayName={item.formattedTo}
                        />
                    </View>
                )}

---

Or we can do something like this (Pseudocode). We will not show UserInfoCell if managerID will be 0. This solution is enough to solve the problem

   {item.managerID !== 0  && (
                    <View style={[StyleUtils.getSearchTableColumnStyles(CONST.SEARCH.TABLE_COLUMNS.FROM)]}>
                        <UserInfoCell
                            participant={item.to}
                            displayName={item.formattedTo}
                        />
                    </View>
                )}

What alternative solutions did you explore? (Optional)

If we also want to check for CONST.REPORT.OWNER_EMAIL_FAKE then we can add another check

 const formattedTo = to?.displayName ?? to?.login ?? '';
const againFormatted = formattedTo === CONST.POLICY.OWNER_EMAIL_FAKE  || formattedTo === CONST.REPORT.OWNER_EMAIL_FAKE? '' : formattedTo 

[ikevin127]

@luacmartins Should we have one of the Contributors work on this based on proposals or will this be handled by a Contractor as most of the Search v2.1 issues ?

[luacmartins]

@ikevin127 yes, we can open this to other contributors

[ikevin127]

In that case, @Krishna2323's updated proposal LGTM, being the first to point out the root cause and suggest a working solution.

🎀👀🎀 C+ reviewed

Edit: Expected result changed in #48582 (comment).

[melvin-bot]

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[luacmartins]

Hmm we shouldn't render the avatar either. I think if item.managerID = 0 we should just avoid rendering the UserInfoCell data

[Nodebrute]

@ikevin127 Thank you for the review, Don't you think fixing it in the getTransactionItemCommonFormattedProperties function (mentioned in my proposal) might be a more comprehensive solution? This change could resolve the problem globally and ensure consistency across the application.

[ikevin127]

@luacmartins What do you think, should we strictly avoid rendering the UserInfoCell data where this component is located as mentioned in proposal 1 or do this from getTransactionItemCommonFormattedPropertie as suggested by the other proposal ?

Either way, both proposals need to be updated to comply with:

if item.managerID = 0 we should just avoid rendering the UserInfoCell data

and early return instead when managerID = 0.

[Nodebrute]

@ikevin127 Can you tell me the steps to reproduce where managerID will be 0?

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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.

[ikevin127]

♻️ Will review the updates in ~12h, starting with @Nodebrute since they were first to update according to the new expected result.

[ikevin127]

@luacmartins Unfortunately I am not able to reproduce the managerID = 0 and __fake__ in the way it was reproduced in the Slack report, meaning it happened without having to set any onyx data locally or hardcoding values.

Given this, to even begin to have proper proposals I think we first need to be able to reproduce the issue since without that I don't know the conditions in which the __fake__ would appear, it can be only if managerID = 0 or we can get it in other cases as well (❓) - which will dictate the proposal's root cause and solution.

I think there's two ways to to get this issue fixed:

1. Make it Internal since it would be easier for an Expensify engineer to both reproduce and fix the issue.
2. If issue remains External, we need steps to reproduce the issue as shown in the Slack report in order to be able to get complete proposals that Contributors can come up by actually reproducing the issue instead of improvising.

[greg-schroeder]

What do you think on the above @luacmartins?

[luacmartins]

@ikevin127 I think the steps below might reproduce the issue:

1. Create a policy in OldDot
2. Create an open report in OldDot
3. Add an expense to it
4. Open the Search page in NewDot (the open report should show up and have managerID = 0)

[ikevin127]

@luacmartins Unfortunately I cannot reproduce, when I follow the steps you provided - the transaction I get in ND does have a managerID. Not sure what to do in this case, I'm happy to pass the issue to another C+ if they can reproduce this.

[luacmartins]

@ikevin127 hmm it seems like the report in the OP had a pending card transaction, maybe that could be related. I'll try to reproduce this on my end and report back.

[Kicu]

Are we still accepting proposals from contributors? If not I can work on this if you assign me.
At this moment we're in the polishing phase for v2.1 and v2.2 so I have some free time on my hands for smaller bugs.

[luacmartins]

All yours @Kicu!

[greg-schroeder]

PR merged! Awaiting deploy to staging -> prod

[ikevin127]

⚠️ Automation failed here -> this should be paid on 2024-09-25 according to today's production deploy from Deploy Checklist: New Expensify 2024-09-17 which was shipped to production today.

cc @greg-schroeder

[greg-schroeder]

Updated

[greg-schroeder]

@bfitzexpensify gonna leave this one back in your capable hands now that you're back - should be set to go for 9/25!

[bfitzexpensify]

Offer sent @ikevin127

[ikevin127]

@bfitzexpensify Accepted, thank you!