-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix: [Dupe detection] Tax code review step shows empty option when workspace does not have tax rate #48958
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
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. |
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 🙏🏻. |
Working on this, hopefully this will be ready for review in 1-2 hours. |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
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 . |
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:
@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 |
@Krishna2323 I'm dealing with some problems with my internet. I'll come back here as soon as I resolve it. Thank you |
I'm back @Krishna2323 :D Now at full steam. I'll review it and let you know. |
@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! |
@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. |
@Krishna2323 Would you mind merging main to update the branch? Thanks. |
@brunovjk All yours again! |
@Krishna2323 Is this expected? I think not, right? can you reproduce it? Thank you. Screen.Recording.2024-09-23.at.20.33.03.mov |
No, that's definitely not the correct behaviour, I'll look into this today. |
@Krishna2323 any updates? |
I need some time for this. I tried to debug the issue but wasn't completely successful, as the flow is a bit complicated. I'll try to finish this over the weekend, and if I can't make good progress, I'll request reassignment. I've been dealing with a slipped disc since last month and I'm only able work 2-3 hours a day. Thanks for your understanding. |
You are doing a great job so far. During the weekend I will back here try to find a solution. Please feel free to share anything you find :D |
@brunovjk, I tried to replicate this issue, but it seems like it's either already fixed somewhere, or we have a new bug or feature where the category is auto-selected if not chosen on the confirmation page. Latest mainMonosnap.screencast.2024-09-30.07-21-57.mp4This PRMonosnap.screencast.2024-09-30.07-30-31.mp4 |
In fact @Krishna2323, I can't reproduce the error we had before. I have to focus on another PR now, tomorrow I'll come back and test it thoroughly, if everything goes well I'll complete the checklist. Before, can you check this typesrcipt error? Thank you very much for the effort :D |
Very good @Krishna2323, the initial issue seems to have been successfully resolved: Screen.Recording.2024-10-01.at.11.34.24.movBut we still have the category and tag issues (which do not exist in workspace B). Should we update it to be the same as Tax? In other words, skip it if it does not exist in workspace B. Maybe we can try to increase the compensation (not guaranteed), if we realize that the scope was larger than expected in the issue. Please let me know what you think. |
@Krishna2323 @brunovjk can we get this moving again now that we decided in slack the path forward? |
Probably will get this done within 2 days. |
Details
Fixed Issues
$ #47796
PROPOSAL: #47796 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop