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

lanitochka17 commented 4 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: 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 https://github.com/Expensify/App/pull/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?

[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/4f734ad9-7023-4ad9-95bb-a879d817c361

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[bot] commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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

devguest07 commented 4 months ago

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.

https://github.com/Expensify/App/blob/60530f643b30f30510914ed28e2a90ed5d37184a/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L75

https://github.com/Expensify/App/blob/60530f643b30f30510914ed28e2a90ed5d37184a/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L99

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 commented 4 months ago

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.

https://github.com/Expensify/App/blob/009b050d2e0ed54cdbdc8a7b6a0a9b8534e05aaa/src/pages/workspace/tags/WorkspaceTagsPage.tsx#L75-L90

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[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

Krishna2323 commented 4 months ago

Proposal updated

Minor updated in RCA

eh2077 commented 3 months ago

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[bot] commented 3 months ago

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

Krishna2323 commented 3 months ago

@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

https://github.com/Expensify/App/assets/85894871/2a364f7a-e127-4e9d-90c7-0dc4942e2acb

eh2077 commented 3 months ago

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

Krishna2323 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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.

melvin-bot[bot] commented 3 months ago

@AndrewGable @miljakljajic @eh2077 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!

miljakljajic commented 3 months ago

discussion still ongoing

miljakljajic commented 3 months ago

@AndrewGable whenever you're able to, your help choosing between these two proposals would be helpful.

melvin-bot[bot] commented 3 months ago

@AndrewGable, @miljakljajic, @eh2077 Huh... This is 4 days overdue. Who can take care of this?

eh2077 commented 3 months ago

Not overdue, waiting for @AndrewGable 's review on https://github.com/Expensify/App/issues/43379#issuecomment-2180888494

melvin-bot[bot] commented 3 months ago

📣 @eh2077 🎉 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[bot] commented 3 months ago

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

Krishna2323 commented 3 months ago

@AndrewGable @eh2077, Yesterday, in this issue, I had the first correct proposal, and the solution was straightforward. However, a contributor copied my proposal and added very minor points, and the C+ and internal engineer selected their proposal instead.

Now, in this issue, even though the other contributor completely missed adding policyTagList.pendingAction in their proposal and didn't even talk about it, their proposal got selected. I'm really confused about what's happening. Contributors copy proposals, add a few minor points, and get their proposals selected. And in this case, my proposal had a very crucial point, but it wasn't selected.

devguest07 commented 3 months ago

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 commented 3 months ago

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

devguest07 commented 3 months ago

@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 commented 3 months ago

@devguest07 You can refer to this post from Slack.

sk.eyJ1IjoiaGF5YXRhIiwiYSI6ImNsbG11NjRqcDI5aDUzZnFsemQ1bzJ6a2sifQ.0w52KN5Ak4AMiwiW-MnWHg

devguest07 commented 2 months ago

@eh2077 @AndrewGable I think automation failed here!

AndrewGable commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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 commented 2 months ago

Checklist

[x] [@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.
[x] [@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
[x] [@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
[x] [@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

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

Do we agree 👍 or 👎

eh2077 commented 2 months ago

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

miljakljajic commented 2 months ago

Apologies, paid!

devguest07 commented 1 month ago

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

devguest07 commented 1 month ago

@miljakljajic can you pls pay me? Thank you

devguest07 commented 1 month ago

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

AndrewGable commented 1 month ago

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

miljakljajic commented 1 month ago

offer extended

devguest07 commented 1 month ago

@miljakljajic offer accepted

miljakljajic commented 1 month ago

Paid!

Expensify / App