fix: [Dupe detection] Tax code review step shows empty option when workspace does not have tax rate #48958

Krishna2323 · 2024-09-11T06:19:15Z

Details

Fixed Issues

$ #47796
PROPOSAL: #47796 (comment)

Tests

Workspace A has tax rates, categories, and tags.
Workspace B does not have taxes, categories, and tags feature enabled.
Open the chat for Workspace A.
Submit an expense in Workspace A with a tax rate, category, and tag (use the same amount and merchant).
Submit another expense in Workspace A with all fields left empty (defaults).
Open the chat for Workspace B.
Submit an expense in Workspace B with the same amount and merchant as the previous entries.
Open the transaction thread for the expense submitted in Workspace B.
Click Review Duplicates.
Choose Keep this one for the first transaction and verify that the tax, tag, and category review step is skipped since Workspace B does not have these features enabled.
Return to the initial duplicate review step (where you select the transaction to keep).
Choose Keep this one for the second transaction and verify that the tax, tag, and category review step is not skipped, as Workspace A has these features enabled.
Repeat steps 11 and 12 for the third transaction, ensuring the tax, tag, and category review step is not skipped since it belongs to Workspace A.
Return to the chat for Workspace A and open any transaction thread for an expense submitted there.
Click Review Duplicates.
Choose Keep this one for the first transaction and verify that the tax, tag, and category review step is not skipped, as Workspace A has these features enabled.
Return to the initial duplicate review step.
Choose Keep this one for the second transaction and verify that the tax, tag, and category review step is not skipped since Workspace A has these features enabled.
Return to the initial duplicate review step again.
Choose Keep this one for the third transaction and confirm that the tax, tag, and category review step is skipped, as this transaction belongs to Workspace B, which does not have these features enabled.

Verify that no errors appear in the JS console

Offline tests

Dupe detection doesn't work in offline mode.

QA Steps

Workspace A has tax rates, categories, and tags.
Workspace B does not have taxes, categories, and tags feature enabled.
Open the chat for Workspace A.
Submit an expense in Workspace A with a tax rate, category, and tag (use the same amount and merchant).
Submit another expense in Workspace A with all fields left empty (defaults).
Open the chat for Workspace B.
Submit an expense in Workspace B with the same amount and merchant as the previous entries.
Open the transaction thread for the expense submitted in Workspace B.
Click Review Duplicates.
Choose Keep this one for the first transaction and verify that the tax, tag, and category review step is skipped since Workspace B does not have these features enabled.
Return to the initial duplicate review step (where you select the transaction to keep).
Choose Keep this one for the second transaction and verify that the tax, tag, and category review step is not skipped, as Workspace A has these features enabled.
Repeat steps 11 and 12 for the third transaction, ensuring the tax, tag, and category review step is not skipped since it belongs to Workspace A.
Return to the chat for Workspace A and open any transaction thread for an expense submitted there.
Click Review Duplicates.
Choose Keep this one for the first transaction and verify that the tax, tag, and category review step is not skipped, as Workspace A has these features enabled.
Return to the initial duplicate review step.
Choose Keep this one for the second transaction and verify that the tax, tag, and category review step is not skipped since Workspace A has these features enabled.
Return to the initial duplicate review step again.
Choose Keep this one for the third transaction and confirm that the tax, tag, and category review step is skipped, as this transaction belongs to Workspace B, which does not have these features enabled.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android_native.mp4

Android: mWeb Chrome

android_chrome.mp4

iOS: Native

ios_native.mp4

iOS: mWeb Safari

ios_safari.mp4

MacOS: Chrome / Safari

web_chrome.mp4

MacOS: Desktop

desktop_app.mp4

Signed-off-by: krishna2323 <[email protected]>

melvin-bot · 2024-09-11T06:19:21Z

@brunovjk Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Krishna2323 · 2024-09-11T06:22:40Z

Though the PR works, I'm a bit confused about the flow at the moment. I will comment on all the possible solutions today and move forward with whatever seems like the best approach.

brunovjk · 2024-09-11T13:08:10Z

Though the PR works, I'm a bit confused about the flow at the moment. I will comment on all the possible solutions today and move forward with whatever seems like the best approach.

Great :D Let me know if/when I can assit.

Krishna2323 · 2024-09-13T16:38:39Z

I apologize for the delay. Unfortunately, my health has not been well over the past two weeks, which has caused some setbacks. I understand that this PR, along with a few others, has been delayed, but I am making every effort to complete it this weekend. Much of my time has been spent away from my workplace. However, I am committed to finishing the PRs by the end of the weekend.

Thank you for your understanding, and I sincerely apologize again 🙏🏻.

Krishna2323 · 2024-09-15T21:17:37Z

Working on this, hopefully this will be ready for review in 1-2 hours.

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2024-09-16T13:00:24Z

Wasn't able to complete this yesterday :(, this once is bit complex. Will work again today.

brunovjk · 2024-09-16T13:58:10Z

Wasn't able to complete this yesterday :(, this once is bit complex. Will work again today.

Good work @Krishna2323 ! Let's me know if can help .

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2024-09-17T01:15:49Z

I have changed my approach a bit from my proposal here. However, the behavior remains the same.

We will skip the tax selection page if the policy does not include any tax codes extracted from different transactions. For example:

If we have T1, T2, and T3, which are duplicate transactions from different policies, and T1 and T2 have different tax codes, while T3 has no tax code:
- If we keep the T3 transaction, the tax code review will be skipped, as T3 does not have a matching tax code in the policy.
- If we keep T1 or T2, the tax code review will be shown if T1 or T2 have tax codes that match their respective policies.
- If T1’s policy does not include the tax code from the T2 transaction, the tax code review will be skipped, as there will be no tax code to select.

@brunovjk, what do you think about the implemented behaviour?

NOTE: in the video below, we are showing the tax code and tax amount field for the transaction does not have these fields, I'm working on it and this will be fixed soon.

Monosnap.screencast.2024-09-17.06-30-04.mp4

brunovjk · 2024-09-17T12:01:33Z

@Krishna2323 I'm dealing with some problems with my internet. I'll come back here as soon as I resolve it. Thank you

brunovjk · 2024-09-20T13:47:59Z

I'm back @Krishna2323 :D Now at full steam. I'll review it and let you know.

brunovjk · 2024-09-20T14:23:41Z

I have changed my approach a bit from my proposal here.

@pecanoro I’ve tested the recent changes, and everything runs fine without errors. The original issue appears to be resolved. However, before we proceed, could you confirm if the updated behavior, as mentioned in @Krishna2323's comment, aligns with our project expectations? Specifically, we need to ensure the behavior regarding the tax code selection is correct. Once you confirm, I will do a detailed code review and complete the checklist. Is it ok? Thank you!

pecanoro · 2024-09-20T15:35:06Z

@brunovjk @Krishna2323 Yeah, I think that's the best behavior. Thank you for handling edge cases.

brunovjk · 2024-09-20T16:31:15Z

@brunovjk @Krishna2323 Yeah, I think that's the best behavior. Thank you for handling edge cases.

Awesome!! Thank you very much @pecanoro, I will continue with the checklist.

brunovjk · 2024-09-23T14:15:31Z

@Krishna2323 Would you mind merging main to update the branch? Thanks.

brunovjk · 2024-10-30T14:27:40Z

That seems to be an unrelated bug to what we were trying to fix here. @brunovjk can you report that one in #expensify-bugs so we can get it fixed in a different issue?

Reported: https://expensify.slack.com/archives/C049HHMV9SM/p1730298247856019

brunovjk · 2024-11-01T11:31:42Z

Hi @Krishna2323, were you able to confirm the bug? How did your tests go? Any luck with the Android build? Thanks a lot! 😊

Krishna2323 · 2024-11-01T20:30:12Z

Yeah, I can reproduce that on. Will be on it in an hour.

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2024-11-01T21:55:26Z

@brunovjk, this one's fixed. Hopefully, we won't run into more bugs! 😬

brunovjk · 2024-11-01T22:05:01Z

Great!! I will test it on monday. Did you test it on different platforms? Thank you :D

Krishna2323 · 2024-11-04T04:44:31Z

@brunovjk, I have updated the test steps and added the recordings for all platforms.

brunovjk · 2024-11-04T15:00:42Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

48958_android_native.mov

Android: mWeb Chrome

48958_android_web.mov

iOS: Native

48958_ios_native.mov

iOS: mWeb Safari

48958_ios_web.mov

MacOS: Chrome / Safari

48958_web_chrome.mov

MacOS: Desktop

48958_web_desktop.mov

pecanoro · 2024-11-04T17:58:07Z

@brunovjk Are you having issues with the last platforms?

brunovjk · 2024-11-04T18:08:57Z

Great work, @Krishna2323! Everything looks spot on, with just one final detail I’d like to confirm: After confirming an expense, regardless of which one it is—say, keeping an expense from workspace 1 while previewing in workspace 2—where should the "back" button take us? I noticed some inconsistency in my tests, but I'm unsure of the intended behavior. What do you think? I believe we're nearly ready to approve, but I'd like to review this "back after confirming" behavior to avoid regressions. Thanks!

brunovjk · 2024-11-04T18:15:05Z

@brunovjk Are you having issues with the last platforms?

No. It seems the server encountered some issues, as I didn’t receive the "duplicate error" as expected. I checked https://status.expensify.com/ and there was a down during my testing, but everything appears to be back to normal now.

The PR is great in my opinion, I think we just need to adjust one last detail :D

Krishna2323 · 2024-11-05T03:53:17Z

After confirming an expense, regardless of which one it is—say, keeping an expense from workspace 1 while previewing in workspace 2—where should the "back" button take us?

@brunovjk, From what I gather, you're describing the "back" button behavior after confirming an expense. However, I'm not sure if this is directly related to our PR. Could you please share a video that walks through the scenario? It will help me understand the bug a bit better. Thank you!

brunovjk

Pre-requisites:

A small screen where the "back button" is visible.
Workspace 1: All features enabled (tax rate, category, and tag).
Workspace 2: All features disabled.

Submit 2 expenses with the same value and merchant:

Expense created in Workspace 1 with some fields filled.
Expense created in Workspace 2.

Steps:

Open the expense created in Workspace 1.
Click the Review Duplicate button.
Select an expense from Workspace 2.
Go to the Confirmation page and Confirm the expense.
Click the "Back button."

Issue:
After clicking the back button, it redirects to a Not Found page.

`main`	This PR
bug_main.mov	bug_48958.mov

Note: The same bug appears on main, so it wasn’t introduced or fixed by this PR. I couldn't find any related issues. I’ll approve the PR since the initial issue was resolved, and I leave it to you, @pecanoro, to decide whether to address this in this PR or to report it separately as a new issue. Everything else looks pretty good to me 🚀

pecanoro · 2024-11-05T16:31:32Z

@brunovjk Could be that one related to this one you reported before? Either way, if it happens on main too, let's report it as a separate bug. With that in mind, can I go ahead a do a final review?

brunovjk · 2024-11-05T17:00:58Z

@brunovjk Could be that one related to this one you reported before? Either way, if it happens on main too, let's report it as a separate bug.

I don't think so, I'll do some more tests later and report what I find on Slack.

With that in mind, can I go ahead a do a final review?

Yes, please 😄

pecanoro · 2024-11-05T18:14:48Z

src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

@@ -294,8 +294,10 @@ function MoneyRequestPreviewContent({

    const navigateToReviewFields = () => {
        const backTo = route.params.backTo;
-        const comparisonResult = TransactionUtils.compareDuplicateTransactionFields(reviewingTransactionID);
-        Transaction.setReviewDuplicatesKey({...comparisonResult.keep, duplicates, transactionID: transaction?.transactionID ?? ''});
+        Transaction.abandonReviewDuplicateTransactions();


I approved, but now that I think about it, can you add a comment before this line explaining why we need to call Transaction.abandonReviewDuplicateTransactions();?

// Clear the draft before selecting a different expense to prevent merging fields from the previous expense // (e.g., category, tag, tax) that may be not enabled/available in the new expense's policy.

comment added.

Signed-off-by: krishna2323 <[email protected]>

pecanoro · 2024-11-06T16:03:11Z

🎉 Merged!! 🎉

OSBotify · 2024-11-06T16:13:20Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-11-07T13:08:23Z

🚀 Deployed to staging by https://github.com/pecanoro in version: 9.0.59-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	success ✅
🍎🔄 iOS HybridApp 🍎🔄	success ✅

IuliiaHerets · 2024-11-07T18:34:08Z

This PR is failing because of issue #52210

github-actions · 2024-11-11T18:13:58Z

🚀 Deployed to production by https://github.com/francoisl in version: 9.0.59-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅
🤖🔄 android HybridApp 🤖🔄	skipped 🚫
🍎🔄 iOS HybridApp 🍎🔄	skipped 🚫

Krishna2323 added 5 commits August 21, 2024 22:13

demo skip dupe tax step.

913a3d9

Signed-off-by: krishna2323 <[email protected]>

Merge branch 'Expensify:main' into krishna2323/issue/47796

19ae510

minor update.

0429b7f

Signed-off-by: krishna2323 <[email protected]>

Merge branch 'Expensify:main' into krishna2323/issue/47796

009faf4

revert changes in ReviewTaxRate page.

9050810

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 requested a review from a team as a code owner September 11, 2024 06:19

melvin-bot bot requested review from brunovjk and removed request for a team September 11, 2024 06:19

Krishna2323 marked this pull request as draft September 11, 2024 06:19

Merge branch 'Expensify:main' into krishna2323/issue/47796

4f6b4db

Krishna2323 added 3 commits September 16, 2024 02:54

revert changes.

b2b979b

Signed-off-by: krishna2323 <[email protected]>

remove tax code from changes if policy doesn't include that.

61ce002

Signed-off-by: krishna2323 <[email protected]>

minor update.

5fff73f

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 added 2 commits September 17, 2024 02:40

only add taxCode in changes if availble in policy.

9801ae8

Signed-off-by: krishna2323 <[email protected]>

Merge branch 'Expensify:main' into krishna2323/issue/47796

033b430

pecanoro self-requested a review September 20, 2024 14:48

Krishna2323 added 2 commits November 2, 2024 02:58

Merge branch 'Expensify:main' into krishna2323/issue/47796

0516d60

fix: category, tag review step not shown.

16008d8

Signed-off-by: krishna2323 <[email protected]>

brunovjk approved these changes Nov 5, 2024

View reviewed changes

pecanoro approved these changes Nov 5, 2024

View reviewed changes

pecanoro requested changes Nov 5, 2024

View reviewed changes

Krishna2323 added 2 commits November 6, 2024 09:08

Merge branch 'Expensify:main' into krishna2323/issue/47796

9a3e1db

added comment/

5c6f079

Signed-off-by: krishna2323 <[email protected]>

pecanoro approved these changes Nov 6, 2024

View reviewed changes

pecanoro merged commit 8a411ff into Expensify:main Nov 6, 2024
17 checks passed

github-actions bot mentioned this pull request Nov 7, 2024

Deploy Checklist: New Expensify 2024-11-07 #52178

Closed

60 tasks

IuliiaHerets mentioned this pull request Nov 7, 2024

Unable to see 'Review Duplicates' button while offline #52210

Closed

8 tasks

IuliiaHerets mentioned this pull request Nov 8, 2024

[HOLD for payment 2024-12-05] [$250] Dupe detection - RBR is not shown when creating dupe expense with a Gmail account #52243

Open

8 tasks

fix: [Dupe detection] Tax code review step shows empty option when workspace does not have tax rate #48958

fix: [Dupe detection] Tax code review step shows empty option when workspace does not have tax rate #48958

Conversation

Krishna2323 commented Sep 11, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 11, 2024

Krishna2323 commented Sep 11, 2024

brunovjk commented Sep 11, 2024

Krishna2323 commented Sep 13, 2024 • edited Loading

Krishna2323 commented Sep 15, 2024

Krishna2323 commented Sep 16, 2024

brunovjk commented Sep 16, 2024

Krishna2323 commented Sep 17, 2024

brunovjk commented Sep 17, 2024

brunovjk commented Sep 20, 2024

brunovjk commented Sep 20, 2024

pecanoro commented Sep 20, 2024

brunovjk commented Sep 20, 2024

brunovjk commented Sep 23, 2024

brunovjk commented Oct 30, 2024

brunovjk commented Nov 1, 2024

Krishna2323 commented Nov 1, 2024

Krishna2323 commented Nov 1, 2024

brunovjk commented Nov 1, 2024 • edited Loading

Krishna2323 commented Nov 4, 2024

brunovjk commented Nov 4, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

pecanoro commented Nov 4, 2024

brunovjk commented Nov 4, 2024

brunovjk commented Nov 4, 2024

Krishna2323 commented Nov 5, 2024

brunovjk left a comment • edited Loading

Choose a reason for hiding this comment

pecanoro commented Nov 5, 2024 • edited Loading

brunovjk commented Nov 5, 2024

pecanoro Nov 5, 2024

Choose a reason for hiding this comment

Krishna2323 Nov 6, 2024

Choose a reason for hiding this comment

pecanoro commented Nov 6, 2024

OSBotify commented Nov 6, 2024

github-actions bot commented Nov 7, 2024

IuliiaHerets commented Nov 7, 2024

github-actions bot commented Nov 11, 2024

Krishna2323 commented Sep 11, 2024 •

edited

Loading

Krishna2323 commented Sep 13, 2024 •

edited

Loading

brunovjk commented Nov 1, 2024 •

edited

Loading

brunovjk commented Nov 4, 2024 •

edited

Loading

brunovjk left a comment •

edited

Loading

pecanoro commented Nov 5, 2024 •

edited

Loading