Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Tag - "Missing tag" error appears & disappears when change Category on created request #37644

Closed
4 of 6 tasks
lanitochka17 opened this issue Mar 1, 2024 · 32 comments
Closed
4 of 6 tasks
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Mar 1, 2024

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

Action Performed:

Precondition:

  • User is an employee of Collect workspace on Old Dot
  • "People must tag expenses" option is disabled by Admin
  1. As employee navigate to Workspace chat> +> Request money> Manual
  2. Add amount> Select Category and add Merchant> Complete the flow
  3. Go to created IOU detail page
  4. Edit Category section and add new category

Expected Result:

Selected category should be displayed and no error messages should occur

Actual Result:

"Missing tag" error message appears and disappears when change Category on created Manual request

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

Bug6398779_1709325409504.Recording__2445.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fd47387167e68ba4
  • Upwork Job ID: 1763729758329421824
  • Last Price Increase: 2024-03-02
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Mar 1, 2024
Copy link
Contributor

github-actions bot commented Mar 1, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 1, 2024

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave36
CC @greg-schroeder

@lanitochka17
Copy link
Author

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

@deetergp deetergp added the External Added to denote the issue can be worked on by a contributor label Mar 2, 2024
@melvin-bot melvin-bot bot changed the title Tag - "Missing tag" error appears & disappears when change Category on created request [$500] Tag - "Missing tag" error appears & disappears when change Category on created request Mar 2, 2024
Copy link

melvin-bot bot commented Mar 2, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 2, 2024
Copy link

melvin-bot bot commented Mar 2, 2024

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

@s77rt
Copy link
Contributor

s77rt commented Mar 2, 2024

Not able to reproduce

Screen.Recording.2024-03-02.at.2.37.40.AM.mov

@omarnagy91
Copy link

Proposal

Problem:

When creating a manual expense request, if the user edits the category after initial submission, a "Missing tag" error briefly flashes even though the "People must tag expenses" setting is disabled.

Root Cause:

The issue is caused by incorrect validation logic in the hasMissingSmartscanFields function or the getViolationsOnyxData function. These functions is incorrectly triggering the "Missing tag" error even when the policy does not require tags.

Proposed Changes:

Modify the validation logic in the hasMissingSmartscanFields function:

function hasMissingSmartscanFields(transaction: OnyxEntry<Transaction>): boolean {
    const parentReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${transaction?.reportID}`] ?? null;
    const policy = ReportUtils.getPolicy(parentReport?.policyID ?? '');

    // Only check for missing tags if the policy requires them
    if (policy?.requiresTag) {
        return TransactionUtils.getTag(transaction) === '';
    }

    // Otherwise, only check for missing amount, date, and merchant
    return TransactionUtils.areRequiredFieldsEmpty(transaction);
}

This change ensures that the "Missing tag" error is only triggered when the policy actually requires tags.

Alternatively, modify the getViolationsOnyxData function:

If the issue is in the getViolationsOnyxData function, you would need to modify the logic that checks for missing tags to also consider the policy requirements.

Alternative Solutions:

Another possible solution could be to investigate the state management in the MoneyRequestView component to see if there is a problem with how the state is being updated when the category is changed. However, based on the information provided, fixing the validation logic seems like the most likely solution.

By implementing the proposed changes, we can ensure that the "Missing tag" error is only displayed when it is truly required by the policy, preventing confusion and improving the user experience.

@s77rt
Copy link
Contributor

s77rt commented Mar 3, 2024

@omarnagy91 Thanks for the proposal. Were you able to reproduce the bug? Seeing this is a deploy blocker, were you able to find the offending PR?

@omarnagy91
Copy link

@s77rt
I can't find the offending PR through git bisect yet
I was able to reproduce the bug

@omarnagy91
Copy link

@s77rt
could you please provide me with any specifics you need about the issue ?

@s77rt
Copy link
Contributor

s77rt commented Mar 4, 2024

@omarnagy91 I'm still unable to reproduce. I can't validate the root cause but seeing this is a deploy blocker and no recent changes to hasMissingSmartscanFields, it's probably not the root cause and something broke somewhere else.

@omarnagy91
Copy link

@s77rt
commit 631d206
Author: dukenv0307 [email protected]
Date: Fri Feb 23 13:38:18 2024 +0700

fix replace hasMissingRequiredFields by hasMissingSmartscanFields

I think this is the commit

@omarnagy91
Copy link

@s77rt
should I update my proposal
I should just add the requiresTag code to it and It's complete
what's your comment ?

@omarnagy91
Copy link

commit caa025b
Author: dukenv0307 [email protected]
Date: Tue Feb 6 11:49:24 2024 +0700

fix error message disappear

@omarnagy91
Copy link

@s77rt it's one of those three commits I am still analyzing everything
I am just doubtful that's why I provided all three
commit 02296e8
Author: VickyStash [email protected]
Date: Tue Jan 16 12:42:24 2024 +0100

Migrate MoneyRequestView to TypeScript

@s77rt
Copy link
Contributor

s77rt commented Mar 4, 2024

@omarnagy91 Both #34564 and #35871 are already on production, they can't be the offending PRs.

@deetergp
Copy link
Contributor

deetergp commented Mar 4, 2024

I'm able to reproduce in dev, but not yet sure what the root cause is.

Screen.Recording.2024-03-04.at.10.13.59.AM.mov

@roryabraham
Copy link
Contributor

roryabraham commented Mar 4, 2024

@deetergp try searching console logs for Onyx and seeing what updates are getting applied? without further investigation my guess would be that the front-end and the back-end aren't in agreement about whether a tag is required. So something like this is happening:

  • You take an action to update a category
  • the front-end optimistically computes violations and decides that a missing tag is a violation
  • you send the request to update the category
  • the back-end decides there's no violations and updates Onyx accordingly

@deetergp deetergp removed the DeployBlockerCash This issue or pull request should block deployment label Mar 4, 2024
@deetergp
Copy link
Contributor

deetergp commented Mar 4, 2024

Since the violations feature is in beta, this does not need to be a blocker. Also, one of our internal engineers has a draft PR going that should address this https://github.com/Expensify/App/pull/37461/files

@deetergp deetergp added Weekly KSv2 and removed Hourly KSv2 labels Mar 4, 2024
@roryabraham roryabraham removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 4, 2024
@roryabraham
Copy link
Contributor

removing Help Wanted and putting this on HOLD for #37461

@roryabraham roryabraham changed the title [$500] Tag - "Missing tag" error appears & disappears when change Category on created request [HOLD 37461][$500] Tag - "Missing tag" error appears & disappears when change Category on created request Mar 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 12, 2024
@s77rt
Copy link
Contributor

s77rt commented Mar 12, 2024

#37461 is merged. Can someone please retest this if it's still reproducible?

@melvin-bot melvin-bot bot removed the Overdue label Mar 12, 2024
@deetergp
Copy link
Contributor

I just requested a retest in our QA Slack channel. I'll report back when I know more.

@kavimuru
Copy link

Tester is still able to reproduce.

Recording.2588.mp4

@s77rt
Copy link
Contributor

s77rt commented Mar 18, 2024

@deetergp Let's reopen this. I was able to reproduce, I had to logout and login again. This bug is internal as the BE returns policy.requiresTag as true even if it's false in old dot.

Screenshot 2024-03-19 at 12 38 12 AM

@roryabraham roryabraham changed the title [HOLD 37461][$500] Tag - "Missing tag" error appears & disappears when change Category on created request [$500] Tag - "Missing tag" error appears & disappears when change Category on created request Mar 21, 2024
@roryabraham roryabraham added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Mar 21, 2024
Copy link

melvin-bot bot commented Mar 21, 2024

Current assignee @s77rt is eligible for the Internal assigner, not assigning anyone new.

@deetergp
Copy link
Contributor

I have not had a chance to dig much deeper into this. I hope to get to it before the week is out.

@deetergp
Copy link
Contributor

deetergp commented Apr 3, 2024

Hoping to get to this today.

@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
@deetergp
Copy link
Contributor

Still have not had a chance to look into this. Hopefully sometime this week.

@melvin-bot melvin-bot bot removed the Overdue label Apr 15, 2024
@deetergp
Copy link
Contributor

I just retested this on staging and am not able to reproduce. Can anyone else?

@roryabraham
Copy link
Contributor

@cead22 was this the same as the bug we tried debugging for a few hours on a call about a month ago? I know that one was also inconsistently reproducible, but you also traced the source and fixed it right?

@cead22
Copy link
Contributor

cead22 commented Apr 19, 2024

Yeah this should be fixed

@cead22 cead22 closed this as completed Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants