Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: [Violations] Distance - Incorrect error message when distance amount is changed to smaller amount. #41649
fix: [Violations] Distance - Incorrect error message when distance amount is changed to smaller amount. #41649
Changes from 30 commits
2f4d561
bac54f1
d399453
c7ec1df
34d5e2e
0549056
ef8e655
f039038
be4f9be
c9fd9aa
b801997
4240c2f
1043d16
3c71e67
12f0433
407e552
7611122
4ebb250
03a9345
d693489
245d667
efd639c
4e8f365
9e452c4
49954fb
d671670
e2851c4
7ae2d6b
e5420c1
7c38e5c
1595f33
2589ba6
6e63f57
9fec7f9
4fe1913
68ebaa7
2a76377
c5d5a3c
995eaed
e3b692e
0a92c99
1c4d2a1
7cdc36c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block needs a comment explaining why we're doing what we're doing here, or as I mentioned in another comment, we need to document shouldShowAuditSuccess and shouldShowAuditFailure to make this clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sobitneupane @cead22, I mistakenly added
modified amount
violation here while resolving conflict, thats why the violation was shown under receipt. I have removed that and alsoCONST.VIOLATIONS.MODIFIED_DATE
because I believe the date violation should also be shown under date field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been looking at this for a minute, and it's not obvious to me why we now need shouldShowAuditSuccess and shouldShowAuditFailure, instead of using notes.length to determine what to show.
What cases would we miss if we went back to relying only on notes.length? If you can update the docs for these params to make it clear, please do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously we were relying only on notes.length but if I recall correctly there were cases where success/failure text was shown before the receipt is scanned and even if there were no receipt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm, and if you do, add a manual tests that would reproduce the issue, and confirm your change solves it. If you can't confirm, let's revert to what we had previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22, pls check the 2nd issue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krishna2323 sorry if this is obvious, but can you explain why we can't use a single prop? Both success and failure need to be shown only if there's a receipt, and the issue you linked shows an error when there's no receipt. What am I missing? Can you share test steps to reproduce the issue when we use only 1 prop, that doesn't happen with this solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, this is bit confusing. Above condition won't work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22, I think we need
shouldShowAuditSuccess
andshouldShowAuditFailure
or a single prop to check if the receipt is being scanned. Relying on the notes length will showVerified
while the receipt is being scanned, as initially, notes will be an empty array.We can remove
shouldShowAuditSuccess
andshouldShowAuditFailure
and introduce a new prop,isScanning
. This way, we only display thefailure/verified
message when the receipt is not being scanned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, sorry for the late reply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krishna2323 will you be able to update this today and get the PR ready for review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cead22, on it, will be ready in few minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why did this condition change? Why don't we need to check
!isReceiptBeingScanned
andReportUtils.isPaidGroupPolicy(report);
anymore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, maybe I removed it in confusion, sorry for that, added that back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change must have broken something that isn't being tested in the manual tests of this PR (otherwise we would've noticed it broke something). Given that, consider adding a manual test and confirming your changes/code solve for the case in question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make
isPaidGroupPolicy
mandatory, and use a better name, since we're passing this as the value for this param!isReceiptBeingScanned && ReportUtils.isPaidGroupPolicy(report)
. PerhapsshouldShowOnlyViolations
, and in the docs we can describe it as "Whether we should only show violations of type 'violation'"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#41401 (comment)
displayPercentVariance
is present in type 'smartscan' not 'card'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 👀