-
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 15 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,6 +116,7 @@ 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; | ||
|
@@ -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,21 @@ function hasReceipt(transaction) { | |||||
return lodashGet(transaction, 'receipt.state', '') !== ''; | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {Object} transaction | ||||||
* @returns {Boolean} | ||||||
*/ | ||||||
function areRequiredFieldsPopulated(transaction) { | ||||||
return ( | ||||||
transaction.merchant !== CONST.TRANSACTION.DEFAULT_MERCHANT && | ||||||
transaction.modifiedMerchant !== CONST.TRANSACTION.DEFAULT_MERCHANT && | ||||||
transaction.amount !== 0 && | ||||||
transaction.modifiedAmount !== 0 && | ||||||
transaction.created !== '' && | ||||||
transaction.modifiedCreated !== '' | ||||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Given the edit made to the money request, return an updated transaction object. | ||||||
* | ||||||
|
@@ -126,6 +141,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 +244,20 @@ function getCreated(transaction) { | |||||
return ''; | ||||||
} | ||||||
|
||||||
function isReceiptBeingScanned(transaction) { | ||||||
return transaction.receipt.state === CONST.IOU.RECEIPT_STATE.SCANREADY || transaction.receipt.state === CONST.IOU.RECEIPT_STATE.SCANNING; | ||||||
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.
Suggested change
NAB |
||||||
} | ||||||
|
||||||
/** | ||||||
* 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) && !areRequiredFieldsPopulated(transaction); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Get the transactions related to a report preview with receipts | ||||||
* Get the details linked to the IOU reportAction | ||||||
|
@@ -244,10 +274,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 +334,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