[HOLD for payment 2024-04-15] [$250] Taxes - Transaction thread shows skeleton loading when saving 0.00 tax amount for second time

[izarutskaya]

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.60-8
Reproducible in staging?: Y
Reproducible in production?: N
Found when validating PR : #33927
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition:

• User is an employee of Collect workspace with tax rates.

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Create a manual request with tax exempt (0%).
4. Go to transaction thread.
5. Click Tax amount.
6. Save it without entering any amount.
7. Repeat Step 5 and 6.

Expected Result:

In Step 6, when saving 0.00 tax amount, the system message will not show $NaN amount,
In Step 7, when saving 0.00 tax amount for the second time, the app will not show skeleton loading.

Actual Result:

In Step 6, $NaN is shown briefly in the system message.
In Step 7, transaction thread shows skeleton loading when saving 0.00 tax amount for second time.

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

Bug6438827_1712298111790.20240405_141500.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01091a68b65c340a6a
• Upwork Job ID: 1776234872385998848
• Last Price Increase: 2024-04-05
• Automatic offers:
• DylanDylann | Reviewer | 0

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

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

[izarutskaya]

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

[izarutskaya]

We think this issue might be related to the #collect project.

[MonilBhavsar]

Looking here...

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01091a68b65c340a6a

[melvin-bot]

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

[MonilBhavsar]

Making it external to see if anyone has proposals as I am close to log off

[nkdengineer]

Proposal

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

In Step 6, $NaN is shown briefly in the system message.
In Step 7, transaction thread shows skeleton loading when saving 0.00 tax amount for second time.

What is the root cause of that problem?

When we try to save the tax amount while entering nothing, it's still saving the empty value (which is converted to NaN).

In this, we're not validating the currentAmount if it's tax amount form, so in case the amount is empty, it will also not validate.

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

We need to update this to

if (!currentAmount.length || (!isTaxAmountForm && isAmountInvalid(currentAmount))) {

So that the "amount empty" error will be validated in all cases, (tax amount form won't be an exception). But it will still allow tax amount to be 0 (since we still have the (!isTaxAmountForm && isAmountInvalid(currentAmount)) part)

What alternative solutions did you explore? (Optional)

NA

[MonilBhavsar]

When we try to save the tax amount while entering nothing, it's still saving the empty value

Should we not allow saving empty value at the first place?

[nkdengineer]

@MonilBhavsar You mean to show an error like Please enter a valid amount before continuing. right? We can do that (similar to the request amount page)

[nkdengineer]

But it will look like the tax amount is a required thing, it might confuse the users. In my proposal, the UX will be, we still dismiss the modal but we'll not save anything, which is similar to the Expected Result in the OP, and same as behavior when the tax amount exists, but unchanged.

Like if tax amount is $10 and the user edit the tax amount, then press Save, it's the same behavior.

[MonilBhavsar]

You mean to show an error like Please enter a valid amount before continuing. right? We can do that (similar to the request amount page)

Yes, we have a text Please enter a valid amount. We can use it.

But it will look like the tax amount is a required thing, it might confuse the users

It is! User cannot save empty value. They need to enter a number ranging from 0 to maximum allowed value, calculated on basis of expense amount

[nkdengineer]

Proposal updated with alternative solution that will show an error if the tax amount is empty.

@MonilBhavsar Let me know if that looks good 👍

[MonilBhavsar]

Looks good!

One thing I noticed while reviewing. If zero is entered we don't display zero, but a placeholder. Do you want to fix it too?

[MonilBhavsar]

And would you be please able to prioritize PR, given it is a deploy blocker?

[MonilBhavsar]

Okay fine with that. Thanks for looking

[nkdengineer]

Thanks, I'm working on the PR now, will raise in a bit

[cristipaval]

Thanks for jumping into this one, @MonilBhavsar

[nkdengineer]

@MonilBhavsar The PR is ready.

[MonilBhavsar]

Works fine on staging! Removing blocker

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Validate tax amount in edit flow and fix skeleton #39720

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

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

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @nkdengineer paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@bfitzexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1776234872385998848/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[bfitzexpensify]

I don't think a specific regression test is needed for this - it should be caught in regular tax testing.

Payment complete, so closing this one out.

[nkdengineer]

Sorry @bfitzexpensify It seems I wasn't paid here, could you reopen the issue to help with this?

TIA

[bfitzexpensify]

@nkdengineer this one was paid on April 16th - title for the Upwork job was Reviewer - [$250] Taxes - Transaction thread shows skeleton loading when saving 0.00 tax amount for second time