-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Display Smartscan errors #26155
Display Smartscan errors #26155
Changes from all commits
03f6abe
1f42544
a9baa69
a83017a
6bbf2e1
6013eb8
907f084
d1d78c8
a8dac54
e20f91d
fb97405
f0d2fc3
b463346
fa60dd7
afc5d1d
17dfe90
46835eb
aaa345b
4654843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,8 +86,10 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans | |
|
||
const hasReceipt = TransactionUtils.hasReceipt(transaction); | ||
let receiptURIs; | ||
let hasErrors = false; | ||
if (hasReceipt) { | ||
receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(transaction.receipt.source, transaction.filename); | ||
hasErrors = TransactionUtils.hasMissingSmartscanFields(transaction); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only needed to show the error if the user was able to edit this report. This later caused #27333 |
||
} | ||
|
||
const isDistanceRequest = TransactionUtils.isDistanceRequest(transaction); | ||
|
@@ -120,6 +122,9 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans | |
interactive={canEdit} | ||
shouldShowRightIcon={canEdit} | ||
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.AMOUNT))} | ||
brickRoadIndicator={hasErrors && transactionAmount === 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} | ||
subtitle={hasErrors && transactionAmount === 0 ? translate('common.error.enterAmount') : ''} | ||
subtitleTextStyle={styles.textLabelError} | ||
/> | ||
</OfflineWithFeedback> | ||
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.comment') || lodashGet(transaction, 'pendingAction')}> | ||
|
@@ -140,6 +145,9 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans | |
shouldShowRightIcon={canEdit} | ||
titleStyle={styles.flex1} | ||
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))} | ||
brickRoadIndicator={hasErrors && transactionDate === '' ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} | ||
subtitle={hasErrors && transactionDate === '' ? translate('common.error.enterDate') : ''} | ||
subtitleTextStyle={styles.textLabelError} | ||
/> | ||
</OfflineWithFeedback> | ||
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.merchant') || lodashGet(transaction, 'pendingAction')}> | ||
|
@@ -150,6 +158,9 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, trans | |
shouldShowRightIcon={canEdit} | ||
titleStyle={styles.flex1} | ||
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.MERCHANT))} | ||
brickRoadIndicator={hasErrors && transactionMerchant === CONST.TRANSACTION.UNKNOWN_MERCHANT ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''} | ||
subtitle={hasErrors && transactionMerchant === CONST.TRANSACTION.UNKNOWN_MERCHANT ? translate('common.error.enterMerchant') : ''} | ||
subtitleTextStyle={styles.textLabelError} | ||
/> | ||
</OfflineWithFeedback> | ||
{shouldShowHorizontalRule && <View style={styles.reportHorizontalRule} />} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ import * as ReceiptUtils from '../../libs/ReceiptUtils'; | |
import * as ReportActionUtils from '../../libs/ReportActionsUtils'; | ||
import * as TransactionUtils from '../../libs/TransactionUtils'; | ||
import ReportActionItemImages from './ReportActionItemImages'; | ||
import colors from '../../styles/colors'; | ||
|
||
const propTypes = { | ||
/** All the data of the action */ | ||
|
@@ -115,11 +116,12 @@ function ReportPreview(props) { | |
const numberOfScanningReceipts = _.filter(transactionsWithReceipts, (transaction) => TransactionUtils.isReceiptBeingScanned(transaction)).length; | ||
const hasReceipts = transactionsWithReceipts.length > 0; | ||
const isScanning = hasReceipts && ReportUtils.areAllRequestsBeingSmartScanned(props.iouReportID, props.action); | ||
const hasErrors = hasReceipts && ReportUtils.hasMissingSmartscanFields(props.iouReportID); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This component hasn't subscribed to Onyx transactions collection, so if there is any update in related Onyx transactions collection, it won't cause re-render here. Causing this bug: #32256 |
||
const lastThreeTransactionsWithReceipts = ReportUtils.getReportPreviewDisplayTransactions(props.action); | ||
|
||
const hasOnlyOneReceiptRequest = numberOfRequests === 1 && hasReceipts; | ||
const previewSubtitle = hasOnlyOneReceiptRequest | ||
? transactionsWithReceipts[0].merchant | ||
? TransactionUtils.getMerchant(transactionsWithReceipts[0]) | ||
: props.translate('iou.requestCount', { | ||
count: numberOfRequests, | ||
scanningReceipts: numberOfScanningReceipts, | ||
|
@@ -188,6 +190,12 @@ function ReportPreview(props) { | |
<View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter]}> | ||
<Text style={[styles.textLabelSupporting, styles.mb1, styles.lh16]}>{getPreviewMessage()}</Text> | ||
</View> | ||
{hasErrors && ( | ||
<Icon | ||
src={Expensicons.DotIndicator} | ||
fill={colors.red} | ||
/> | ||
)} | ||
<Icon | ||
fill={StyleUtils.getIconFillColor(getButtonState(props.isHovered))} | ||
src={Expensicons.ArrowRight} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -85,6 +85,14 @@ function hasReceipt(transaction) { | |||||||||
return lodashGet(transaction, 'receipt.state', '') !== ''; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* @param {Object} transaction | ||||||||||
* @returns {Boolean} | ||||||||||
*/ | ||||||||||
function areModifiedFieldsPopulated(transaction) { | ||||||||||
return transaction.modifiedMerchant !== CONST.TRANSACTION.UNKNOWN_MERCHANT && transaction.modifiedAmount !== 0 && transaction.modifiedCreated !== ''; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Given the edit made to the money request, return an updated transaction object. | ||||||||||
* | ||||||||||
|
@@ -126,6 +134,7 @@ function getUpdatedTransaction(transaction, transactionChanges, isFromExpenseRep | |||||||||
if (shouldStopSmartscan && _.has(transaction, 'receipt') && !_.isEmpty(transaction.receipt) && lodashGet(transaction, 'receipt.state') !== CONST.IOU.RECEIPT_STATE.OPEN) { | ||||||||||
updatedTransaction.receipt.state = CONST.IOU.RECEIPT_STATE.OPEN; | ||||||||||
} | ||||||||||
|
||||||||||
updatedTransaction.pendingFields = { | ||||||||||
...(_.has(transactionChanges, 'comment') && {comment: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}), | ||||||||||
...(_.has(transactionChanges, 'created') && {created: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}), | ||||||||||
|
@@ -228,6 +237,20 @@ function getCreated(transaction) { | |||||||||
return ''; | ||||||||||
} | ||||||||||
|
||||||||||
function isReceiptBeingScanned(transaction) { | ||||||||||
return _.contains([CONST.IOU.RECEIPT_STATE.SCANREADY, CONST.IOU.RECEIPT_STATE.SCANNING], transaction.receipt.state); | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Check if the transaction has a non-smartscanning receipt and is missing required fields | ||||||||||
* | ||||||||||
* @param {Object} transaction | ||||||||||
* @returns {Boolean} | ||||||||||
*/ | ||||||||||
function hasMissingSmartscanFields(transaction) { | ||||||||||
return hasReceipt(transaction) && !isReceiptBeingScanned(transaction) && !areModifiedFieldsPopulated(transaction); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you clarify this? isReceiptBeingScanned checks for SCANFAILED and SCANCOMPLETE App/src/libs/TransactionUtils.js Lines 254 to 257 in ebcc8d0
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to this function, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not saying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I am concerned is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thanks for clarifying! So I think if we have any missing fields (even if the receipt is in the OPEN state), we would want to show the RBR to highlight any missing fields to the user |
||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Get the transactions related to a report preview with receipts | ||||||||||
* Get the details linked to the IOU reportAction | ||||||||||
|
@@ -244,10 +267,6 @@ function getAllReportTransactions(reportID) { | |||||||||
return _.filter(allTransactions, (transaction) => transaction.reportID === reportID); | ||||||||||
} | ||||||||||
|
||||||||||
function isReceiptBeingScanned(transaction) { | ||||||||||
return transaction.receipt.state === CONST.IOU.RECEIPT_STATE.SCANREADY || transaction.receipt.state === CONST.IOU.RECEIPT_STATE.SCANNING; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
* Verifies that the provided waypoints are valid | ||||||||||
* @param {Object} waypoints | ||||||||||
|
@@ -308,4 +327,5 @@ export { | |||||||||
isReceiptBeingScanned, | ||||||||||
validateWaypoints, | ||||||||||
isDistanceRequest, | ||||||||||
hasMissingSmartscanFields, | ||||||||||
}; |
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.
@Gonals The
hasReceipt
aslo returntrue
in case of manual request with attached receipt. So we want to show the error if the request is manual request with the attached receipt as well, right?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 don't think we smartscan in those cases
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.
So rather than using
hasReceipt
(line 90), we should use something likeisScanRequest
(a way to differentiate manual requests with an attached receipt from a smartscan request), right?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.
Because using
hasReceipt
leads to the bug #34997There 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 seems reasonable, yes.
I think this was added before we implemented the functionality to attach receipts to manual requests, so that wasn't a problem at the time.
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 don't think I would return anything new from the server (especially since
isScanRequest
isn't something stored in the database). The proper way to know if a receipt was smartscanned is if the receipt has one of theCONST.IOU.RECEIPT_STATE
states. I thinkTransactionUtils.hasReceipt(transaction);
should be updated to look at that instead. Can you look into doing that?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.
Also I am thinking that we maybe need two functions...
TransactionUtils.hasAttachment()
TransactionUtils.hasReceipt()
This would cover both cases and be more explicit for cases like this which are only meant to handle one case.
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.
@tgolen
Perhaps you misunderstood my point. I mean that we need to differentiate manual requests with an attached receipt from a smartscan request as we did in FE side:
App/src/libs/TransactionUtils.ts
Lines 51 to 52 in c675857
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.
Hm, the backend already has several methods that are similar to that. I still don't know that I am following this discussion completely but from the way I see it, the FE code for
isScanRequest()
needs to be refactored to account for the difference between a smartscan request and a manual request with attachment.What does the backend have to do with it?
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.
The check for errors is done only when there is receipt and that caused #34997