[$500] Tag - Notification shows "tag" instead of custom name for tag

[lanitochka17]

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.33-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/34482

Action Performed:

Precondition:

• Admin and employee are part of Collect workspace
• The Collect workspace has custom name for Tag

1. [Employee] Go to workspace chat
2. [Employee] Create a manual expense with tag
3. [Employee] Navigate to request details page
4. {Employee] Click tag and select a different tag
5. [Admin} Note that the notification shows "tag" instead of custom name for tag

Expected Result:

The notification will show custom name for tag instead of the default tag name (Tag)

Actual Result:

The notification shows default tag name (Tag). On Android and iOS app, it shows "changed the request" with no details

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/Expensify/App/assets/78819774/32d2c01e-b207-4858-a9a3-af54963e9d2f

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016118ce4a2a30a489
• Upwork Job ID: 1752326219551039488
• Last Price Increase: 2024-01-30

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[lanitochka17]

We think that this bug might be related to #wave. CC @zanyrenney

[cead22]

Given this is push notification, I bet this is a back end issue that needs to be fixed internally

[johncschuster]

Thanks for grabbing it, @cead22!

[melvin-bot[bot]]

@cead22, @johncschuster, @ishpaul777 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[johncschuster]

Bumping to keep Melvin at bay. How's it going @cead22?

[cead22]

Sorry I'm still a bit behind and haven't gotten to this, but it's on my list

[cead22]

Looking more into this, this isn't a push notification, but a browser local notification, so I think it can be handled externally. There might be an inconsistency between how we show the report action message and the notification, but from a quick look

• BrowserNotifications.pushModifiedExpenseNotification calls
• ModifiedExpenseMessage.getForReportAction, which gets policyTagList name and passes it to
• buildMessageFragmentForValue

So I haven't gotten to the root cause, but I think we can leave that to contributors

[melvin-bot[bot]]

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

[cead22]

@johncschuster I think External and Help Wanted are the labels to apply to get this through the normal bug process, but correct me if I'm wrong

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[ishpaul777]

Awaiting proposals

[jcdiprose]

@ishpaul777 I cannot replicate the issue. I cannot see the A B C D option or even the tag option. If the instructions could be revised that would help

[Tony-MK]

Hi 👋 , @jcdiprose.

A B C D is the custom tag name.

The notification messages are supposed to be constructed from a template such as: change the {customOrTagName} to {newTagValue} (previously "{newTagValue}")

In this example, we want to replace the tag word with the custom tag name, A B C D.

Expected Result

changed the A B C D to "Saturn" (previously "Jupiter")

Actual Result

changed the tag to "Saturn" (previously "Jupiter")

I hope this helps. 🤔 Since I believe you are new, I don't mind you asking me questions on Slack.

@ishpaul777, Can you confirm? 🙏

[jcdiprose]

Thanks, @Tony-MK What's your name on slack? I have a couple of questions

[melvin-bot[bot]]

@cead22 @johncschuster @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[ishpaul777]

not overdue, awaiting proposals

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[ishpaul777]

bumped on slack for 👀 https://expensify.slack.com/archives/C01GTK53T8Q/p1708342485959849

[brandonhenry]

Proposal

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

The notification for a modified expense in a workspace displays the default word "tag" instead of the custom name for the tag defined in the workspace settings.

What is the root cause of that problem?

The root cause of the issue is that the notification message generation logic does not correctly handle custom tag names. Instead of using the custom name defined in the policy tags, it defaults to using the generic term "tag." This results in the notification displaying "tag" instead of the custom tag name.

https://github.com/Expensify/App/blob/3a95869505bf5165a039eb416d026d4f6d603c99/src/libs/ModifiedExpenseMessage.ts#L196

The line results in "tag" on ios and Android because for some reason, the PolicyUtils doesn't return the proper tag name.

This led me to the actual cause which is PolicyUtils.getTagListName. This function relies on the index of a tag passed in to provide the proper tagList name.

This is likely failing due to the ordering of list changing. I think we should not rely on index to get the tagListName

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

1. Update the getTagListName function to accept a tag key instead of an index:

import _ from 'underscore';
function getTagListName(policyTagList: OnyxEntry<PolicyTagList>, tagKey: string): string {
    if (!policyTagList) {
        return '';
    }

    return _.get(policyTagList, [tagKey, 'name'], '');
}

2. Update the hasModifiedTag block to use the tag key directly:

if (hasModifiedTag) {
    // same variables

    splittedTag.forEach((newTag, index) => {
        const oldTag = splittedOldTag[index] ?? '';
        const tagKey = `tag_${index}`;

        if (newTag !== oldTag) {
            const policyTagListName = PolicyUtils.getTagListName(policyTags, tagKey) || localizedTagListName;

            buildMessageFragmentForValue(
                PolicyUtils.getCleanedTagName(newTag),
                PolicyUtils.getCleanedTagName(oldTag),
                policyTagListName,
                true,
                setFragments,
                removalFragments,
                changeFragments,
                policyTagListName === localizedTagListName,
            );
        }
    });
}

This change uses the tag key directly, which eliminates the reliance on the order of the keys in the policyTags object. I believe this will fix the issue

[melvin-bot[bot]]

@cead22 @johncschuster @ishpaul777 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[Victor-Nyagudi]

How can I enable notifications on DEV or even staging?

Based on this Slack conversation, it seems contributors aren't able to test iOS notifications.

Come to think of it, I've never received notifications on Android Dev, only the production app. Ditto for the web and desktop app even though I have them enabled.

[cead22]

@Victor-Nyagudi this is a local browser notification, so I imagine you should be able to allow notifications in chrome to tests this

[Victor-Nyagudi]

I have them all on "allow" in site settings, but I don't get anything.

Any idea how I could troubleshoot this, @cead22 ?

What actions usually trigger a notification? Maybe I'm looking for them in the wrong places.

[ishpaul777]

I remember i got notifications on dev desktop in past for a PR i am reviewing https://github.com/Expensify/App/pull/35004 (on hold for some other PRs for a while), not sure about the web. For native apps i think we only repro. this on a deployed app (or a app with paid apple developer account) (100% sure for ios atleast)

[cead22]

@Victor-Nyagudi can you ask in #expensify-open-source? I don't know the answer but I bet others do, and that way we both learn

[Victor-Nyagudi]

I actually tried Slack first before asking here.

Didn't get much traction.

[Tony-MK]

Has anyone had luck enabling web notifications?

Observations

1. On Android, the notifications display changed the request.

2. When disabling Use multiple levels of tags, the created modified expense actions text displays changed the request after changing the tag.

3. The backend does not synchronize the tags when the Use multiple levels of tags gets disabled and the tag is changed. Hence, the tag's name in the notification seen below displays the previous children's tags.