[HOLD for payment 2024-03-11] [$500] Required label in push row should be removed if value is present

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.42-1
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1708010208553859

Action Performed:

You need to have a workspace that required categories and tags

1. go to workspace chat
2. Click + and request money
3. Go to Manual
4. Add amount
5. Click continue
6. Add a category and a tag

Expected Result:

"Required" shouldn't appear on the category, tag, or merchant fields if already a value present. Note the required label isn't showing on the merchant field in the screenshot below because the PR to add that was just merged and I pulled the screenshot from stagign

Actual Result:

The required label persists even after the value is there.

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012f130f43465b030a
• Upwork Job ID: 1758185700555239424
• Last Price Increase: 2024-02-15
• Automatic offers:
• aimane-chnaif | Reviewer | 0
• Pujan92 | Contributor | 0

[melvin-bot]

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

[mkhutornyi]

Proposal

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

Required label in push row should be removed if value is present

What is the root cause of that problem?

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Line 762 in d20ada6

rightLabel={isCategoryRequired ? translate('common.required') : ''}

There's no logic to hide Required label when field is empty

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

update condition like this: !iouCategory && isCategoryRequired ? translate('common.required') : ''

Apply same to tag, merchant

[Pujan92]

Proposal

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

Required label shows even though the value is present for the push row

What is the root cause of that problem?

At the moment we do not have any condition to not show the required label when the value for the push item is present.

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

As I am seeing rightLabel added only for showing the required value for the MenuItems at the moment, we can update the prop name like showRequiredLabel with the type boolean. We can consider the title value to prevent rendering the required label.

{!title && showRequiredLabel && (
    <View style={styles.justifyContentCenter}>
        <Text style={styles.rightLabelMenuItem}>{translate('common.required')}</Text>
    </View>
)}

App/src/components/MenuItem.tsx

Lines 612 to 616 in 20ca041

	{!!rightLabel && (
	<View style={styles.justifyContentCenter}>
	<Text style={styles.rightLabelMenuItem}>{rightLabel}</Text>
	</View>
	)}

Old

I can see we used rightLabel only for showing the required value for the MenuItems, so to generic solution we can consider the title value to prevent rendering the rightLabel.

!title && !!rightLabel

App/src/components/MenuItem.tsx

Lines 605 to 609 in 2fe384c

	{!!rightLabel && (
	<View style={styles.justifyContentCenter}>
	<Text style={styles.rightLabelMenuItem}>{rightLabel}</Text>
	</View>
	)}

Result

Screen.Recording.2024-02-15.at.22.54.57.mov

[aimane-chnaif]

This is exactly the same issue I reported here.
Happy to take this as C+

[melvin-bot]

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

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

@cead22, @muttmuure, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[aimane-chnaif]

MenuItem is general component and having title doesn't always mean that right label should be hidden.

[aimane-chnaif]

@mkhutornyi's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

Update we're going with @Pujan92 's proposal

[melvin-bot]

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

[Pujan92]

@aimane-chnaif @cead22 I have updated my proposal, Can you plz give a look at it as I think it helps to reduce the conditions and be more generic.

[aimane-chnaif]

I still don't like introducing specific props/values in general component.

[Pujan92]

Ok, I thought it makes sense to add it as it defines the intended behavior. Bcoz for right side content we already have rightComponent prop.

[cead22]

I like @Pujan92 's proposal better because all the required labels work the same today in that if there's a value to display (title) in the menu item, we shouldn't show the required label. This is kinda similar to the approach that we took in this issue #35843 (comment), where the component has all the knwodlege it needs to decide whether to display something or not, and therefore, we're putting the logic inside the component and not outside of it (like with !iouCategory && isCategoryRequired ? translate('common.required') : '')

We wouldn't be introducing a new prop, we'd be replacing rightLabel with showRequiredLabel.

If we want to keep the change a bit smaller, and keep the ability of putting something other than Required on the right label, we can keep the rightLabel as is, and do something like this on MenuItem.tsx

{!title &&  (
    <View style={styles.justifyContentCenter}>
        <Text style={styles.rightLabelMenuItem}>{rightLabel}</Text>
    </View>
)}

[aimane-chnaif]

ok let's go with that given that there will be no other cases of showing right label other than "Required" (at least for now)

No problem at this moment but there might be cases of showing title and right label at the same time in the future, as MenuItem is general component

[melvin-bot]

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

[Pujan92]

@aimane-chnaif @cead22 To confirm, do we need to proceed by updating the prop to showRequiredLabel or keep the rightLabel and use title value to render?

[aimane-chnaif]

If we want to keep the change a bit smaller, and keep the ability of putting something other than Required on the right label, we can keep the rightLabel as is, and do something like this on MenuItem.tsx

I think ^

[cead22]

Yeah. Let's keep the rightLabel and use title value to render

[Pujan92]

Thanks, PR is ready for review!

[melvin-bot]

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

[melvin-bot]

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

• Fix: Required label in push row should be removed if value is present #37137

If no regressions arise, payment will be issued on 2024-03-11. 🎊

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

• @Pujan92 requires payment automatic offer (Contributor)
• @aimane-chnaif requires payment automatic offer (Reviewer)

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

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

[cead22]

This on the payment stage, so not overdue

[muttmuure]

Was ooo, handling now

[muttmuure]

All paid up

[muttmuure]

@aimane-chnaif @Pujan92 could you confirm if we need a regression test?

[aimane-chnaif]

Regression Test Proposal

1. Go to workspace chat and request money with manual option
2. In the confirmation screen, select value for any required item (i.e. Merchant)
3. Verify the "Required" label isn't shown for the items where value is present

[muttmuure]

Thanks!