[HOLD for payment 2024-04-25] [$500] Tags & Categories - Name row in item RHP is not grayed out when edited offline

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.57-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition:

• User is an admin of Collect workspace.

1. Go to staging.new.expensify.com.
2. Go to Collect workspace settings.
3. Go offline.
4. Go to Taxes.
5. Click on any tax and edit the name.
6. Note that the name row in RHP is grayed out.
7. Go to Tag > Settings > Custom tag name > Edit the name.
8. Note that the name row in RHP is grayed out.
9. Click on any tag > Edit tag name.
10. Repeat Step 9 with category.

Expected Result:

In Step 9 and 10, when renaming tag and category offline, the name row in RHP should be grayed out, just like when tax name and custom tag name are edited offline.

Actual Result:

In Step 9 and 10, when renaming tag and category offline, the name row in RHP is not grayed out.

Workaround:

Unknown

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

Bug6429587_1711592918726.bandicam_2024-03-28_10-23-16-717.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01656e560eca3f61c2
• Upwork Job ID: 1773756778962694144
• Last Price Increase: 2024-03-29
• Automatic offers:
• ntdiary | Reviewer | 0
• nkdengineer | Contributor | 0

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #wave-control.

[bernhardoj]

Proposal

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

WS category and tag name isn't grayed out in the setting page.

What is the root cause of that problem?

The name menu item for category and tag isn't wrapped with OfflineWithFeedback, so even there is a pending action, they won't be grayed out.

App/src/pages/workspace/categories/CategorySettingsPage.tsx

Lines 119 to 124 in 469b112

	<MenuItemWithTopDescription
	title={policyCategory.name}
	description={translate(`workspace.categories.categoryName`)}
	onPress={navigateToEditCategory}
	shouldShowRightIcon
	/>

Compared to WS tax name field where we wrap it with OfflineWithFeedback.

App/src/pages/workspace/taxes/WorkspaceEditTaxPage.tsx

Lines 113 to 127 in 469b112

	<OfflineWithFeedback
	errors={ErrorUtils.getLatestErrorField(currentTaxRate, 'name')}
	pendingAction={currentTaxRate?.pendingFields?.name}
	errorRowStyles={styles.mh5}
	onClose={() => clearTaxRateFieldError(policyID, taxID, 'name')}
	>
	<MenuItemWithTopDescription
	shouldShowRightIcon
	title={currentTaxRate?.name}
	description={translate('common.name')}
	style={[styles.moneyRequestMenuItem]}
	titleStyle={styles.flex1}
	onPress={() => Navigation.navigate(ROUTES.WORKSPACE_TAX_NAME.getRoute(`${policyID}`, taxID))}
	/>
	</OfflineWithFeedback>

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

Wrap both WS category and tag name menu item with offline with feedback and pass the correct pending action.

<OfflineWithFeedback ... pendingAction={currentPolicyTag.pendingAction}>

<OfflineWithFeedback ... pendingAction={policyCategory?.pendingAction}>

[nkdengineer]

Proposal

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

In Step 9 and 10, when renaming tag and category offline, the name row in RHP is not grayed out.

What is the root cause of that problem?

We don't wrap MenuItemWithTopDescription of category/tag name into OfflineWithFeedback as we do for tax

App/src/pages/workspace/categories/CategorySettingsPage.tsx

Line 119 in 5a8cb7f

<MenuItemWithTopDescription

App/src/pages/workspace/tags/TagSettingsPage.tsx

Line 123 in 5a8cb7f

<MenuItemWithTopDescription

App/src/pages/workspace/taxes/WorkspaceEditTaxPage.tsx

Line 113 in 5a8cb7f

<OfflineWithFeedback

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

We should wrap the menu item of tag and category name with OfflineWithFeedback and pass the pendingAction for this.
We should pass special pending action of name field via pendingFields since this is only updated when we edit the name. If we use pendingAction, the name is also grey out when we enable/disable the category/tag

<OfflineWithFeedback pendingAction={policyCategory.pendingFields?.name}>
    <MenuItemWithTopDescription
        title={policyCategory.name}
        description={translate(`workspace.categories.categoryName`)}
        onPress={navigateToEditCategory}
        shouldShowRightIcon
    />
</OfflineWithFeedback>

<OfflineWithFeedback pendingAction={currentPolicyTag.pendingFields?.name}>
    <MenuItemWithTopDescription
        title={currentPolicyTag.name}
        description={translate(`workspace.tags.tagName`)}
        onPress={navigateToEditTag}
        shouldShowRightIcon
    />
</OfflineWithFeedback>

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

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

[garrettmknight]

Repro'd - opening up.

[melvin-bot]

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

[garrettmknight]

@ntdiary we've already got a couple proposals if you can check!

[ntdiary]

@ntdiary we've already got a couple proposals if you can check!

@garrettmknight, thank you! Under review. :)

[ntdiary]

Eh, this issue looks simple, I think it's fine to move forward with OfflineWithFeedback and pendingFields?.name approach. Also, I noticed that the new tag name is not displayed in the list, but I'm not sure if this bug has already been reported. :)

test.mp4

[ntdiary]

ok, let's fix the grayed out issue first, we can move forward with @nkdengineer's proposal. :)

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

📣 @ntdiary 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @nkdengineer You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[garrettmknight]

Moving to daily to check for deploy and test.

[trjExpensify]

@garrettmknight as a bug that doesn't block the release, I'm moving this into Polish on the project board.

[melvin-bot]

@garrettmknight, @arosiclair, @ntdiary, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[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.62-17 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Tags & Categories - Name row in item RHP is not grayed out when edited offline #39584

If no regressions arise, payment will be issued on 2024-04-25. 🎊

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

• @ntdiary requires payment automatic offer (Reviewer)
• @nkdengineer requires payment (Needs manual offer from BZ)

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

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

[melvin-bot]

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

[garrettmknight]

@ntdiary @nkdengineer please accept the offers.

[ntdiary]

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:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: Create TagSettingsPage #38335
• [@ntdiary] 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/38335/files#r1581060910
• [@ntdiary] 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: No need
• [@ntdiary] Determine if we should create a regression test for this bug. don't have strong feeling
• [@ntdiary] 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. N/A
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

This issue is relatively minor and not a regression. It's simply that we didn't consider the offline behavior during feature development. So, I think it's fine to just comment on the original PR. :)

[ntdiary]

@ntdiary @nkdengineer please accept the offers.

@garrettmknight, thank you! Have just accepted it. 😄

[arosiclair]

@ntdiary @nkdengineer please accept the offers.

bump @nkdengineer

[garrettmknight]

All paid out!