feat: Receipt Audit Feature / Note type violations. #37813
Conversation
Signed-off-by: Krishna Gupta <[email protected]>
melvin-bot
bot
requested review from
sobitneupane
and removed request for
a team
|
@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] |
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
|
@JmillsExpensify @sobitneupane, the PR is nearly ready for review. However, before that, I have a few things left to implement based on the responses to the following questions.
|
I would think so as we do show the scanning info.
If we display
For this, once you add the screen recording and screenshots, we can ask the design team to verify the design and icons. |
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
|
Let's make sure we get a design team member on this PR before it's merged! |
|
@Krishna2323 Can you please add Tests and QA steps. You also need to fill PR Author checklist. |
|
@sobitneupane, I don't know how to generate note type violations, do you have any clue? Test steps will be based on that. |
src/languages/es.ts
Outdated
| receiptAudit: 'Auditoría de recibos', | ||
| receiptVerified: 'Recibo verificado', | ||
| receiptNoIssuesFound: 'No se encontraron problemas', | ||
| receiptIssuesFound: (count: number) => `Se encontró ${count} problema(s)`, |
There was a problem hiding this comment.
@Krishna2323 Did we verify if it is the correct translation? We can ask for the correct translations in slack channel.
| @@ -148,6 +151,8 @@ function MoneyRequestView({ | |||
| (field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0, | |||
| [canUseViolations, getViolationsForField], | |||
| ); | |||
| const noteTypeViolations = transactionViolations?.filter((violation) => violation.type === 'note').map((v) => ViolationsUtils.getViolationTranslation(v, translate)); | |||
There was a problem hiding this comment.
@Krishna2323 Is the violation type 'note' for such violations? What type do we use for violation messages? Can we reuse the existing method for violation messages? If you have any confusion, please do raise it on the issue.
There was a problem hiding this comment.
@sobitneupane, I guess we need to pass array of strings to ReceiptAudit component so we need to extract like this. The type for violation messages is violation.
App/src/libs/TransactionUtils.ts
Line 593 in b7a230e
@JmillsExpensify can you pls confirm if the type for note type violation is note?
There was a problem hiding this comment.
@Krishna2323 type for such violation is "notice"
[
{
"type": "notice",
"name": "modifiedAmount",
"data": null
},
{
"type": "notice",
"name": "modifiedDate",
"data": null
}
]
Signed-off-by: Krishna Gupta <[email protected]>
src/languages/en.ts
Outdated
| receiptAudit: 'Receipt Audit', | ||
| receiptVerified: 'Receipt Verified', | ||
| receiptNoIssuesFound: 'No issues Found', | ||
| receiptIssuesFound: (count: number) => `${count} ${count === 1 ? 'Issue' : 'Issues'} Found`, |
There was a problem hiding this comment.
| receiptAudit: 'Receipt Audit', | |
| receiptVerified: 'Receipt Verified', | |
| receiptNoIssuesFound: 'No issues Found', | |
| receiptIssuesFound: (count: number) => `${count} ${count === 1 ? 'Issue' : 'Issues'} Found`, | |
| receiptAudit: 'Receipt Audit', | |
| receiptVerified: 'Receipt Verified', | |
| receiptNoIssuesFound: 'No issues found', | |
| receiptIssuesFound: (count: number) => `${count} ${count === 1 ? 'issue' : 'issues'} found`, |
There was a problem hiding this comment.
Good 👀, thanks for that. Updated.
|
@Krishna2323 If there are any blockers or if you have any confusion, please don't hesitate to raise those concerns. Were you able to test the issue in your end? You need to be in Violations beta to receive those violations from BE. |
|
Just a heads up that we're going to have a few design changes to this before it gets merged - nothing major, just updating the badge style to repurpose our existing badge component. cc @JmillsExpensify @Expensify/design - do we have a final consensus on the updated design yet? |
| @@ -370,7 +384,9 @@ function MoneyRequestView({ | |||
| } | |||
| /> | |||
| )} | |||
| {canUseViolations && <ViolationMessages violations={getViolationsForField('receipt')} />} | |||
| {!(!hasReceipt && (canEditReceipt || isAdmin || isApprover)) && !(showMapAsImage || hasReceipt) && <View style={{marginVertical: 6}} />} | |||
There was a problem hiding this comment.
This is impossible to read, please intermediate variables with self-explanatory names to simplify this
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
Signed-off-by: Krishna Gupta <[email protected]>
…23/App into krishna2323/feat/36288
Signed-off-by: Krishna Gupta <[email protected]>
|
@Krishna2323 When you're back online, let's try to get these comments addressed and then get this merged. We're super close, I think we're in a position to cross this PR off the list in the next 24 hours. |
|
Ah shoot! Sorry I missed the updated. Going to reach back out to @cead22 for a re-review now. Thank you for the quick work! |
| // TODO: remove the !isTrackExpense from this condition after this fix: https://github.com/Expensify/Expensify/issues/382786 | ||
| const canEditDistance = ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.DISTANCE) && !isTrackExpense; | ||
|
|
||
| const isAdmin = policy?.role === 'admin'; | ||
| const isApprover = ReportUtils.isMoneyRequestReport(moneyRequestReport) && (session?.accountID ?? null) === moneyRequestReport?.managerID; |
There was a problem hiding this comment.
The second condition looks wrong, or at least it doesn't seem like we need the ?? null fallback. But also, don't we need to make sure moneyRequestReport?.managerID; isn't null? I think this should be
| const isApprover = ReportUtils.isMoneyRequestReport(moneyRequestReport) && (session?.accountID ?? null) === moneyRequestReport?.managerID; | |
| const isApprover = ReportUtils.isMoneyRequestReport(moneyRequestReport) && moneyRequestReport?.managerID !== null && session?.accountID === moneyRequestReport?.managerID; |
There was a problem hiding this comment.
I took that condition from MoneyRequestHeader, but the suggested changes looks more accurate to me so I updated both instances.
App/src/components/MoneyRequestHeader.tsx
Line 81 in 377121a
| @@ -324,12 +333,20 @@ function MoneyRequestView({ | |||
| </OfflineWithFeedback> | |||
| ); | |||
|
|
|||
| const shouldShowReceiptEmptyState = !hasReceipt && (canEditReceipt || isAdmin || isApprover); | |||
| /* eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing */ | |||
| const shouldShowMapOrReceipt = showMapAsImage || hasReceipt; | |||
There was a problem hiding this comment.
- Let's move
const showMapAsImage = isDistanceRequest && hasPendingWaypoints;above this line - Let's move
const hasPendingWaypoints = transaction?.pendingFields?.waypoints;above that ^ line and make itconst hasPendingWaypoints = Boolean(transaction?.pendingFields?.waypoints;)- That way
hasPendingWaypointsis a bool - And we can remove the eslint rule suppression
- That way
There was a problem hiding this comment.
can you pls check again.
Signed-off-by: Krishna Gupta <[email protected]>
|
@Krishna2323 changes look good, can you give this another round of testing and we can merge? |
|
@cead22, tested, works well on my end. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Great work on this PR @Krishna2323! |
|
@JmillsExpensify, thanks a lot for the acknowledgment! I'm glad you liked the work on the PR. Thanks to everyone for clearing all my doubts during the PR and specially @sobitneupane & @cead22 for their great review on the code changes. Excited to contribute more to Expensify! |
|
🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.69-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.69-2 🚀
|
Details
Fixed Issues
$ #36288
PROPOSAL: #36288 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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_v.mp4
MacOS: Desktop
desktop_app.mp4