[$250] Tags - System message for disabling, adding tax code and deleting X: Y tag shows X\: Y tag

lanitochka17 commented 2 months ago

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: 9.0.23-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+kh05081@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Workspace is under Control plan.
1. Go to staging.new.expensify.com
2. Go to workspace settings > Tags
3. Add a tag in X: Y format
4. Disable, add tax code and delete the tag in X:Y format
5. Go to #admins

Expected Result:

The system message for disabling, adding tax code and deleting tag in X: Y format will display the tag correctly

Actual Result:

The system message for disabling, adding tax code and deleting tag in X: Y format displays the tag in X\: Y format

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/82c903e7-a874-4bbf-affe-946c767d4150

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01e7a0584538b48c5f
Upwork Job ID: 1826803032920988282
Last Price Increase: 2024-08-23

melvin-bot[bot] commented 2 months ago

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

lanitochka17 commented 2 months ago

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 2 months ago

We think that this bug might be related to #wave-collect - Release 1

abzokhattab commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-21 23:08:16 UTC.

Proposal

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

The system message for disabling, adding, and deleting the X: Y tag displays as "X: Y tag."

What is the root cause of that problem?

The comment message is only cleaned and parsed when the tag is added, but not when it is updated or deleted.

https://github.com/Expensify/App/blob/f80d19bdca0abc31ee5f0e0f3afb334738103337/src/pages/home/report/ReportActionItem.tsx#L652-L654

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

We should add conditions for the remove, disable, enable, and update actions, as well as any other actions that display the tag name in the comment.

} else if (
            action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.ADD_TAG ||
            action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG_ENABLED ||
            action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG_NAME ||
            action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.DELETE_TAG ||
            action.actionName === CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG
        ) {

we can also add other actions that show the tag name in the comment

And as daledah mentioned we need to do the same in the sideBar as well since the same problem also shown in the LHN data ... so the same fix would be applied here as well App/src/libs/SidebarUtils.ts Result:

What alternative solutions did you explore? (Optional)

N/A

daledah commented 2 months ago

Proposal

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

In LHN and report page, the system message for disabling, adding tax code and deleting tag in X: Y format displays the tag in X: Y format.

What is the root cause of that problem?

In LHN, we do not convert X: Y to X\: Y in case of system message for disabling, updating tag: https://github.com/Expensify/App/blob/ad7c1d5e394c8a9ee1ce7427233aa085350bb500/src/libs/SidebarUtils.ts#L428-L430
The same issue when displaying report actions: https://github.com/Expensify/App/blob/ad7c1d5e394c8a9ee1ce7427233aa085350bb500/src/pages/home/report/ReportActionItem.tsx#L657-L659

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

We should create a function to check whether the action is needed to clean the tag name:

const isActionNeedToCleanup(actionName: string) {
return [CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.ADD_TAG, CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG, CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG_ENABLED, CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.DELETE_TAG, CONST.REPORT.ACTIONS.TYPE.POLICY_CHANGE_LOG.UPDATE_TAG_NAME].includes(actionName)
}

Then: https://github.com/Expensify/App/blob/ad7c1d5e394c8a9ee1ce7427233aa085350bb500/src/libs/SidebarUtils.ts#L428 will be:
```
    } else if (isActionNeedToCleanup(lastAction?.actionName)) {
```
And: https://github.com/Expensify/App/blob/ad7c1d5e394c8a9ee1ce7427233aa085350bb500/src/pages/home/report/ReportActionItem.tsx#L657 will be:
```
    } else if (isActionNeedToCleanup(action?.actionName)) {
```

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

sakluger commented 2 months ago

@CortneyOfstad could you please help manage this issue while I'm OOO through next Friday, 8/30? Thanks!

daledah commented 2 months ago

@abzokhattab Please note the contributor guidelines:

If you want to make an entirely new proposal or update an existing proposal, please go back and edit your original proposal, then post a new comment to the issue in this format to alert everyone that it has been updated:

abzokhattab commented 2 months ago

Thank you for the note, Daledah.

The concept remains consistent across both files, which is why I didn't add a separate proposal update comment. The core issue we are addressing here focuses on the chat comments, while the LHN behavior, although related, can be seen as a sub-issue within the broader context as the result matches the expected output of the issue. Since the change in the other file is an extension of the same logic, I believe it doesn't fundamentally alter the original proposal.

nabeel-tariq commented 2 months ago

Proposal

Proposal: System Message for Disabling, Adding Tax Code, and Deleting Tag Shows X\: Y Tag

Issue: The system currently displays messages related to actions such as disabling, adding tax codes, and deleting tags with the X\: Y tag format.

Proposed Solution: To resolve this issue, we propose implementing a getCleanedTagName() function. This function will standardize the display of tags in system messages by removing unwanted escape characters and formatting tags properly.

Technical Explanation: Currently, system messages for actions such as adding, updating, enabling, and deleting tags display the tag names with the escape character () resulting in the format X\: Y. We will introduce a getCleanedTagName()` function that will clean and format tag names for display in system messages. This function will remove any escape characters and return the cleaned tag name in a user-friendly format. The addition of this function will require updates to the parts of the code responsible for generating system messages. Any existing code that directly displays tag names will be modified to use getCleanedTagName() to ensure consistency in tag display.

Implementation Plan: Implement the getCleanedTagName() function in the relevant utility. Refactor existing code that generates system messages to use getCleanedTagName() instead of directly referencing tag names. *Test the updated message formatting to ensure tags are displayed correctly.

Conclusion: Implementing the getCleanedTagName() function will standardize the display of tag names in system messages, removing unnecessary escape characters and enhancing readability. This approach will address the current formatting issue and ensure that system messages are clear and user-friendly o n all the platforms like Android, IOS, MAC and Web.

melvin-bot[bot] commented 2 months ago

📣 @nabeel-tariq! 📣 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:

Make sure you've read and understood the contributing guidelines.
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.
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.
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>
```

nabeel-tariq commented 2 months ago

Contributor details Your Expensify account email: nabeeltariq355@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~018b7fbb83ec91e8c3

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@sakluger, @CortneyOfstad, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 months ago

@sakluger, @CortneyOfstad, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

abdulrahuman5196 commented 2 months ago

Checking now

abdulrahuman5196 commented 2 months ago

@abzokhattab 's proposal here https://github.com/Expensify/App/issues/47804#issuecomment-2302659959 looks good and works well. Other proposal seems improvised version of core issue to fix system, so going with the original proposal.

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

❌ There was an error making the offer to @abzokhattab for the Contributor role. The BZ member will need to manually hire the contributor.

sakluger commented 2 months ago

Here's the offer for @abzokhattab: https://www.upwork.com/nx/wm/offer/103753777

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @francoisl, @sakluger, @abdulrahuman5196, @abzokhattab eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

abdulrahuman5196 commented 1 month ago

Hi @francoisl / @sakluger I think the automation failed here.

The change was pushed to production 2 weeks before. And should be up for payment.

francoisl commented 1 month ago

Changing to Daily so we can issue payments then @sakluger

sakluger commented 1 month ago

Sorry about that delay!

Summarizing payment on this issue:

Contributor: @abzokhattab $250, paid via Upwork Contributor+: @abdulrahuman5196 $250, please request on Newdot

sakluger commented 1 month ago

@abdulrahuman5196 please complete the BZ checklist:

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:

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

sakluger commented 1 month ago

We can close this as soon as the BZ checklist is completed.

abdulrahuman5196 commented 1 month ago

The PR that introduced the bug has been identified. Link to the PR: 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: 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:

Not a regression

Determine if we should create a regression test for this bug.

Yes.

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.

1) Workspace is under Control plan. 2) Go to workspace settings > Tags 3) Add a tag in X: Y format 4) Disable, add tax code and delete the tag in X:Y format 5) Go to #admins 6) The system message for disabling, adding tax code and deleting tag in X: Y format will display the tag correctly

sakluger commented 1 month ago

Thanks!

JmillsExpensify commented 1 month ago

$250 approved for @abdulrahuman5196

Expensify / App