[HOLD 47974] [$250] [Dupe detection] Tax field does not show the "Default" label on the confirmation page

IuliiaHerets commented 3 weeks 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: v9.0.24-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

Track tax is enabled in Workspace settings > Distance rates > Settings.
Distance rate has Tax rate and Tax reclaimable fields populated.

Go to staging.new.expensify.com
Go to workspace chat.
Submit two same distance expenses with the same distance rate that has tax rate and tax reclaimable on.
Go to transaction thread of any submitted expense.
Note that Tax field has the "Default" label.
Click Review duplicates.
Click Keep this on (any).
Proceed to confirmation page.

Expected Result:

Tax field will show the "Default" label on the confirmation page.

Actual Result:

Tax field does not show the "Default" label on the confirmation page and instead shows "None"

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/ea6e4514-e527-4b48-9f17-21a88c6c7d21

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~017284a9effe70faa8
Upwork Job ID: 1828590697351581643
Last Price Increase: 2024-08-28

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @stephanieelliott (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.

IuliiaHerets commented 3 weeks ago

We think that this bug might be related to #wave-control

IuliiaHerets commented 3 weeks ago

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

Krishna2323 commented 3 weeks ago

Proposal

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

Dupe detect - Tax field does not show the "Default" label on the confirmation page

What is the root cause of that problem?

To get the default tax rate we need comment?.customUnit?.customUnitRateID?.toString(); in the transactionID. https://github.com/Expensify/App/blob/73f97c930795b4c68512d0e25ee253f7ce7d4515/src/libs/TransactionUtils.ts#L748-L769
But when we call buildNewTransactionAfterReviewingDuplicates on confirmation page the comment object is only left with a comment property, all other properties are removed. https://github.com/Expensify/App/blob/73f97c930795b4c68512d0e25ee253f7ce7d4515/src/libs/TransactionUtils.ts#L952-L963

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

When we modify the transaction, we need to store the remaining properties of the comment object. To do this we need to make few changes.

We first need to store the comment object in keep object. https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/libs/TransactionUtils/index.ts#L991-L993

            if (allCommentsAreEqual || allCommentsAreEmpty) {
                keep[fieldName] = firstTransaction?.comment?.comment ?? firstTransaction?.comment;
                keep.comment = firstTransaction?.comment;

In buildNewTransactionAfterReviewingDuplicates, we need to merge the remaining properties of the comment object. https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/libs/TransactionUtils/index.ts#L1030
```
comment: {...reviewDuplicateTransaction?.comment, comment: reviewDuplicateTransaction?.description},
```
When updating the description in ReviewDescription, we need to find the correct comment object and update it in the REVIEW_DUPLICATES ONYX object. https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/pages/TransactionDuplicate/ReviewDescription.tsx#L34-L39

            const comment = compareResult.change.description?.find((d) => d?.comment === data.value);
            setReviewDuplicatesKey({description: data.value, comment});

Test Branch

What alternative solutions did you explore? (Optional) 

Alternatively we can store the comment object with updated comment instead of description text:

https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/libs/TransactionUtils/index.ts#L987-L992

NOTE: We also need to make changes in few other files. The changes are bit similar to solution above. Please checkout the text branch below for a better understanding.

Test Branch - Alternative Solution

Result

https://github.com/user-attachments/assets/2d8a009f-9b87-4cb3-a560-923d3882ae6d

stephanieelliott commented 3 weeks ago

updated the repro steps slightly -- actual result shows None instead of the expected default rate.

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~017284a9effe70faa8

melvin-bot[bot] commented 3 weeks ago

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

trjExpensify commented 3 weeks ago

and another one! CC: @dylanexpensify @pecanoro @parasharrajat @kubabutkiewicz

pecanoro commented 2 weeks ago

@alitoshmatov Could you review the proposals when you have some time?

parasharrajat commented 2 weeks ago

I can do that @pecanoro if needed.

pecanoro commented 2 weeks ago

Putting a HOLD here for now since it seems to be the same as https://github.com/Expensify/App/issues/47974

Krishna2323 commented 2 weeks ago

@pecanoro, I have proposals for both issues, and I can assure you that they are different. In this issue, we aren't persisting all the properties of the comment object, whereas in issue #47974, the comparison of the comment.comment string is incorrect.

I think we can remove the hold.

pecanoro commented 2 weeks ago

I still think we should wait since the proposals are really similar, and they will need to be updated accordingly.

melvin-bot[bot] commented 2 weeks ago

@pecanoro, @stephanieelliott, @alitoshmatov Huh... This is 4 days overdue. Who can take care of this?

stephanieelliott commented 1 week ago

Still holding on https://github.com/Expensify/App/issues/47974

stephanieelliott commented 1 day ago

Still holding on https://github.com/Expensify/App/issues/47974

pecanoro commented 23 hours ago

Still holding

Expensify / App