[$250] QBO - Main tag row is not grayed out when tag name or subtag is edited offline

[lanitochka17]

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.81-1
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
Issue reported by: Applause - Internal Team

Issue found when executing PR #43005

Action Performed:

Precondition:

• Workspace is connected to QBO

1. Go to staging.new.expensify.com
2. Go to workspace settings
3. Go to Tags
4. Go offline
5. Click on any tag (Classes or Customers/Projects)
6. Edit the tag name or edit any subtag
7. Dismiss the RHP

Expected Result:

The main tag (Classes or Customers/Projects) will be grayed out when the tag name or subtag is edited

Actual Result:

The main tag (Classes or Customers/Projects) is not grayed out when the tag name or subtag is edited

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

Add any screenshot/video evidence

Bug6508123_1718029397049.20240610_221233.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019a5f373ffd1d9267
• Upwork Job ID: 1803445339997697727
• Last Price Increase: 2024-06-19
• Automatic offers:
• eh2077 | Reviewer | 102877092

Issue Owner

Current Issue Owner: @eh2077

[melvin-bot]

Triggered auto assignment to @miljakljajic (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]

@miljakljajic 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]

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

[devguest07]

Proposal

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

The main tag row is not grayed out when the tag name or subtag is edited offline.

What is the root cause of that problem?

On the Workspace tags page, for multi-level tags, there isn't a pendingAction field. This omission means that changes made offline do not trigger the necessary visual indicator.

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

Line 75 in 60530f6

if (isMultiLevelTags) {

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

Line 99 in 60530f6

pendingAction: tag.pendingAction,

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

For multi-level tags, we need to check all the child tags and determine if any have a pendingAction field. If so, we should update the main tag row to reflect this.

const hasPendingAction = Object.values(policyTagList.tags).some(tag => tag.pendingAction);

return { 
  // ..
  pendingAction: hasPendingAction,

What alternative solutions did you explore?

[Krishna2323]

Proposal

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

QBO - Main tag row is not grayed out when tag name or subtag is edited offline

What is the root cause of that problem?

For multi level tags we don't set the pendingAction field. We missed to add pendingAction: policyTagList.pendingAction and we also don't check for any pending action in the child tags.

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

Lines 75 to 90 in 009b050

	if (isMultiLevelTags) {
	return policyTagLists.map((policyTagList) => ({
	value: policyTagList.name,
	orderWeight: policyTagList.orderWeight,
	text: PolicyUtils.getCleanedTagName(policyTagList.name),
	keyForList: String(policyTagList.orderWeight),
	isSelected: selectedTags[policyTagList.name],
	enabled: true,
	required: policyTagList.required,
	rightElement: (
	<ListItemRightCaretWithLabel
	labelText={policyTagList.required ? translate('common.required') : undefined}
	shouldShowCaret={false}
	/>
	),
	}));

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

We should use policyTagList.pendingAction and also check if any child has a pending action and set the pendingAction field accordingly.

        if (isMultiLevelTags) {
            return policyTagLists.map((policyTagList) => {
                const hasChildPendingAction = Object.values(policyTagList.tags).some((tag) => tag.pendingAction);

                return {
                    value: policyTagList.name,
                    orderWeight: policyTagList.orderWeight,
                    text: PolicyUtils.getCleanedTagName(policyTagList.name),
                    keyForList: String(policyTagList.orderWeight),
                    isSelected: selectedTags[policyTagList.name],
                    enabled: true,
                    required: policyTagList.required,
                    pendingAction: policyTagList.pendingAction ?? (hasChildPendingAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : undefined),
                    rightElement: (
                        <ListItemRightCaretWithLabel
                            labelText={policyTagList.required ? translate('common.required') : undefined}
                            shouldShowCaret={false}
                        />
                    ),
                };
            });
        }

What alternative solutions did you explore? (Optional)

[melvin-bot]

@miljakljajic Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@miljakljajic Still overdue 6 days?! Let's take care of this!

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

Proposal updated

• Minor updated in RCA

[eh2077]

Thank you for your proposals!

@Krishna2323 focused on getting detail coding changes while their proposal doesn’t seem to have significant difference compared to the previous one.

I think we should go with @devguest07 's proposal as they're the first to point out the root cause. I also agreed with their idea to get it fixed.

For multi-level tags, we need to check all the child tags and determine if any have a pendingAction field. If so, we should update the main tag row to reflect this.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[Krishna2323]

@eh2077, I think you missed this part of my proposal, we also need to set the policyTagList.pendingAction for pending action value else the tag won't be grayed out when tag details are changed.

We missed to add pendingAction: policyTagList.pendingAction

Monosnap.screencast.2024-06-20.20-17-54.mp4

[eh2077]

@Krishna2323 Thanks for your comment but I think the selected proposal covers the change you mentioned.

[Krishna2323]

For multi-level tags, we need to check all the child tags and determine if any have a pendingAction field. If so, we should update the main tag row to reflect this.

const hasPendingAction = Object.values(policyTagList.tags).some(tag => tag.pendingAction);

return { 
  // ..
  pendingAction: hasPendingAction,

@eh2077, I can't find any mention about adding policyTagList.pendingAction.

[eh2077]

@Krishna2323 I appreciated your effort to discuss the specific detail coding changes. But I still don’t agree with you that your proposal has contributed significant difference compared to the selected one.

I believe @devguest07 's description about the change needed and together with pseudocode are enough for the solution.

[Krishna2323]

@eh2077, the proposal never suggested using policyTagList.pendingAction, and it clearly mentions adding the pending action only if the child tags have pending actions. It is very clear in the solution details and code snippet that it only checks for the pending action value of child tags. Adding policyTagList.pendingAction is important because of the bug I mentioned here. Please re-evaluate...

For multi-level tags, we need to check all the child tags and determine if any have a pendingAction field. If so, we should update the main tag row to reflect this.

The pending action should be:

pendingAction: policyTagList.pendingAction ?? (hasChildPendingAction ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : undefined),

cc: @AndrewGable

[eh2077]

@Krishna2323 Sorry, I still think the idea to update pendingAction is from the selected proposal. In this case (a relatively straightforward issue), I think you can't just translate their pseudocode to specific diff and then claim significant contributions from it.

I tend to stick with my original decision, so I also agreed to defer it to the internal engineering team.

cc @AndrewGable

[Krishna2323]

I think you can't just translate their pseudocode to specific diff and then claim significant contributions from it.

@eh2077, I think you didn't understand what I'm saying. Let me explain more clearly.

The selected proposal states that for multilevel tags, we will check if any child tag has a pending action and, based on that, we will add the pending action. This approach works when we update any child tag in offline mode. However, if we update the parent tag name or make change the required field, the row won't be greyed out, and this case is not covered in the selected proposal. I’m not translating the pseudocode, my proposal will also grey out the option if the parent tag has a pending action or if any child tag has one, whereas the selected proposal only checks for the child pending action.

[Krishna2323]

@AndrewGable, I still can't understand why my proposal wasn't selected. @eh2077 seems to prefer the other proposal, which explicitly states that we only need to check for pending actions in child tags. It never mentions that we also need to add policyTagList.pendingAction, thus failing to cover cases where the main tag is updated offline. In this issue, my proposal was dismissed because I included the change in the RCA section. However, if we compare this issue with that one, the selected proposal doesn't even acknowledge this adding policyTagList.pendingAction.I have no issues with @eh2077's preference, but I believe the change is crucial and was never stated in the selected proposal. Feel free to make the decision :)

For multi-level tags, we need to check all the child tags and determine if any have a pendingAction field. If so, we should update the main tag row to reflect this.

[devguest07]

PR in progress. I encountered an issue with running the iPhone simulator (this is my first time with this project). I will fix it ASAP and make the PR ready for review. Apologies for the delay.

[eh2077]

@devguest07 You can also post issues you encountered here or on the expensify-open-source slack channel

[devguest07]

@eh2077 Thank you for your help. The issue was with Mapbox; I was using the default key without read/write access. Regarding Slack, I still don't have access. I requested an invitation via email a few weeks ago but haven't received it yet. The PR will be ready in a few hours.

[eh2077]

@devguest07 You can refer to this post from Slack.

sk.eyJ1IjoiaGF5YXRhIiwiYSI6ImNsbG11NjRqcDI5aDUzZnFsemQ1bzJ6a2sifQ.0w52KN5Ak4AMiwiW-MnWHg

[devguest07]

@eh2077 @AndrewGable I think automation failed here!

[AndrewGable]

Yes, looks like it has been deployed for a while. @miljakljajic - Can we pay this one out?

[melvin-bot]

This issue has not been updated in over 15 days. @AndrewGable, @miljakljajic, @eh2077, @devguest07 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!

[eh2077]

Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I can't figure out the specific PR that caused this bug.
• [@eh2077] 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: N/A
• [@eh2077] 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: N/A
• [@eh2077] Determine if we should create a regression test for this bug. Yes

Regression test

Precondition:

Workspace is connected to QBO

• Go to staging.new.expensify.com

1. Go to workspace settings
2. Go to Tags
3. Go offline
4. Click on any tag (Classes or Customers/Projects)
5. Edit the tag name or edit any subtag
6. Dismiss the RHP
7. Verify that the main tag (Classes or Customers/Projects) will be grayed out.

Do we agree 👍 or 👎

[eh2077]

@miljakljajic This is due for payment, please take a look at this when you get a chance. Thx

[miljakljajic]

Apologies, paid!

[devguest07]

@miljakljajic I am not paid, my profile was frozen, can you pls pay me?
https://www.upwork.com/freelancers/~01020d097e810d1b07

[devguest07]

@miljakljajic can you pls pay me? Thank you

[devguest07]

@AndrewGable this is closed without getting paid! Can you pls help!

[AndrewGable]

@miljakljajic - Can you double check @devguest07's payment? Thank you!

[miljakljajic]

offer extended

[devguest07]

@miljakljajic offer accepted

[miljakljajic]

Paid!