Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

fix: [Violations] Distance - Incorrect error message when distance amount is changed to smaller amount. #41649

Open Krishna2323 opened 2 weeks ago

Krishna2323 commented 2 weeks ago

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/41401 PROPOSAL: https://github.com/Expensify/App/issues/41401#issuecomment-2091932028

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
melvin-bot[bot] commented 2 weeks ago

@sobitneupane 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 commented 2 weeks ago

@JmillsExpensify @sobitneupane, I found one more issue in this PR. We only show the Receipt Verified audit, but when we have violation notices, we don't show the 'Issue/Issues found' text. The reason for that is I misunderstood this comment and hid the 'Issue/Issues found' message along with Receipt Verified message until the receipt status is SCANCOMPLETE but in case of issue while scanning, the receipt status is to OPEN.

I have fixed the above issue, now it will show Issue/Issues found if we have receipt violations and receipt is present.

Yes we shouldn't show the taxRateChanged note on NewDot, as it is being deprecated given that we have implemented that feature differently vs OldDot.

One more question on the above comment: do we also need to remove the taxAmountChanged notice, along with taxRateChanged, from NewDot? https://github.com/Expensify/App/blob/550e83b2f55f0998feece27b1768ad7148d03398/src/CONST.ts#L3622 https://github.com/Expensify/App/blob/550e83b2f55f0998feece27b1768ad7148d03398/src/CONST.ts#L3624

Bug

Monosnap (149) New Expensify 2024-05-05 05-09-31
sobitneupane commented 1 week ago

@Krishna2323 Have we considered different types as mentioned here? Can you please complete the PR Author checklist. Please include the test cases for different types.

Krishna2323 commented 1 week ago

One more question on the above comment: do we also need to remove the taxAmountChanged notice, along with taxRateChanged, from NewDot?

@JmillsExpensify bump on the question.

Krishna2323 commented 1 week ago

Have we considered different types as mentioned https://github.com/Expensify/App/issues/41401#issuecomment-2091898713?

Code has been updated according to this, will update test cases and recordings after getting clarification on the question above.

JmillsExpensify commented 4 days ago

Whoops, sorry missed this.

One more question on the above comment: do we also need to remove the taxAmountChanged notice, along with taxRateChanged, from NewDot?

Yes, that's correct. We aren't going to show these on NewDot, as both will generate system messages when changed. We also show what tax rate is "default" in a parallel initiative that's currently in implementation for track tracking in New Expensify.

JmillsExpensify commented 3 days ago

Just confirming that the above response makes sense?

Krishna2323 commented 1 day ago

@JmillsExpensify, yup thats clear to me, updating the PR in few moments.

Krishna2323 commented 1 day ago

@sobitneupane, you can review the code changes now. However, I'm not sure what to add in the test cases. Could you provide some guidance? Several types of amount notices are there.