Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow receipt recovery if the upload fails #29790

Merged
merged 41 commits into from
Nov 22, 2023
Merged

Allow receipt recovery if the upload fails #29790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4fa79fd
Add error message
Gonals Oct 17, 2023
418f5d7
handle htmL
Gonals Oct 17, 2023
ccda700
fix regex
Gonals Oct 17, 2023
ee3fbdb
style
Gonals Oct 17, 2023
6cb7230
use object for error
Gonals Oct 17, 2023
8749857
Use new approach
Gonals Oct 17, 2023
52f5e50
use function
Gonals Oct 17, 2023
d97beb3
remove unneeded helper function
Gonals Oct 17, 2023
ca1443a
spanish
Gonals Oct 17, 2023
0c559d1
lint
Gonals Oct 18, 2023
08d8e86
prettier
Gonals Oct 18, 2023
040c5ed
allow multiple lines
Gonals Oct 18, 2023
c43ff48
Merge branch 'main' into alberto-recoverReceipt
Gonals Oct 19, 2023
17a1c43
add split flow
Gonals Oct 19, 2023
1d95f46
use isEmpty
Gonals Oct 19, 2023
0d010f9
use empty for split
Gonals Oct 19, 2023
7867ece
conflicts
Gonals Oct 24, 2023
b1d6295
conflicts
Gonals Oct 30, 2023
42e4ba3
import update
Gonals Oct 30, 2023
8b0382b
reorder
Gonals Oct 30, 2023
5529aae
Merge branch 'main' into alberto-recoverReceipt
Gonals Oct 30, 2023
f8ce4eb
prettier
Gonals Oct 30, 2023
97e94ae
Move things around a bit
Gonals Nov 3, 2023
3dc3512
docs
Gonals Nov 3, 2023
04b4563
Merge branch 'main' into alberto-recoverReceipt
Gonals Nov 9, 2023
b1e1d13
Correctly display message
Gonals Nov 9, 2023
467fcc3
Merge branch 'main' into alberto-recoverReceipt
Gonals Nov 14, 2023
a87aac4
Avoid warnings
Gonals Nov 14, 2023
b308067
prettier
Gonals Nov 14, 2023
de455c3
conflicts
Gonals Nov 16, 2023
9bf109c
Allow temporary files download in-tab
Gonals Nov 16, 2023
24a1a76
add const
Gonals Nov 16, 2023
cc15c6d
update const
Gonals Nov 16, 2023
b3a6ca9
prettier
Gonals Nov 16, 2023
70d2432
conflicts
Gonals Nov 17, 2023
bb41f23
delete ruby version change
Gonals Nov 17, 2023
e134818
Merge branch 'main' into alberto-recoverReceipt
Gonals Nov 21, 2023
57f1a8d
add helper function
Gonals Nov 21, 2023
4366cd8
move function
Gonals Nov 21, 2023
7021a01
use for everything
Gonals Nov 21, 2023
d52631c
update message
Gonals Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,7 @@ const CONST = {
DOCX: 'docx',
SVG: 'svg',
},
RECEIPT_ERROR: 'receiptError',
},

GROWL: {
Expand Down
55 changes: 46 additions & 9 deletions src/components/DotIndicatorMessage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ import PropTypes from 'prop-types';
import React from 'react';
import {View} from 'react-native';
import _ from 'underscore';
import fileDownload from '@libs/fileDownload';
import * as Localize from '@libs/Localize';
import stylePropTypes from '@styles/stylePropTypes';
import * as StyleUtils from '@styles/StyleUtils';
import useTheme from '@styles/themes/useTheme';
import useThemeStyles from '@styles/useThemeStyles';
import CONST from '@src/CONST';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import PressableWithoutFeedback from './Pressable/PressableWithoutFeedback';
import Text from './Text';

const propTypes = {
Expand All @@ -19,7 +22,9 @@ const propTypes = {
* timestamp: 'message',
* }
*/
messages: PropTypes.objectOf(PropTypes.oneOfType([PropTypes.string, PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.object]))])),
messages: PropTypes.objectOf(
PropTypes.oneOfType([PropTypes.oneOfType([PropTypes.string, PropTypes.object]), PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.object]))]),
),

// The type of message, 'error' shows a red dot, 'success' shows a green dot
type: PropTypes.oneOf(['error', 'success']).isRequired,
Expand All @@ -38,6 +43,19 @@ const defaultProps = {
textStyles: [],
};

/**
* Check if the error includes a receipt.
*
* @param {String} message
* @returns {Boolean}
*/
const isReceiptError = (message) => {
if (_.isString(message)) {
return false;
}
return _.get(message, 'error', '') === CONST.IOU.RECEIPT_ERROR;
Gonals marked this conversation as resolved.
Show resolved Hide resolved
};

function DotIndicatorMessage(props) {
const theme = useTheme();
const styles = useThemeStyles();
Expand Down Expand Up @@ -71,14 +89,33 @@ function DotIndicatorMessage(props) {
/>
</View>
<View style={styles.offlineFeedback.textContainer}>
{_.map(sortedMessages, (message, i) => (
<Text
key={i}
style={[StyleUtils.getDotIndicatorTextStyles(isErrorMessage), ...props.textStyles]}
>
{message}
</Text>
))}
{_.map(sortedMessages, (message, i) =>
isReceiptError(message) ? (
<PressableWithoutFeedback
Copy link
Contributor

@blazejkustra blazejkustra Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gonals I'm working on DotIndicatorMessage TS migration PR and while resolving conflicts I noticed this logic change:

isReceiptError(message) ? (
                        <PressableWithoutFeedback
...

👇 With these lines sortedMessages is an array of strings, so I don't see how isReceiptError will ever return true for the code above, therefore the whole logic with PressableWithoutFeedback is unnecessary in my opinion 🤔

    const sortedMessages = _.chain(props.messages)
        .keys()
        .sortBy()
        .map((key) => props.messages[key])
        // Using uniq here since some fields are wrapped by the same OfflineWithFeedback component (e.g. WorkspaceReimburseView)
        // and can potentially pass the same error.
        .uniq()
        .map((message) => Localize.translateIfPhraseKey(message))
        .value();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to follow the steps from the description but I get a different error:
image

Copy link
Contributor

@blazejkustra blazejkustra Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah actually it works but it is kind of hacky 😅

ReceiptError object is passed to this function as a 'message', the error is thrown inside try block but it is caught in the catch block and the whole object is returned instead of a string as function signature suggests. I think it would makes things more readable if we were to change the logic here/or use a different component for Receipt errors. cc @Gonals @nkuoch @0xmiroslav

/**
 * Return translated string for given error.
 */
function translateIfPhraseKey(message: string | [string, Record<string, unknown> & {isTranslated?: true}] | []): string {
    if (!message || (Array.isArray(message) && message.length === 0)) {
        return '';
    }

    try {
        // check if error message has a variable parameter
        const [phrase, variables] = Array.isArray(message) ? message : [message];

        // This condition checks if the error is already translated. For example, if there are multiple errors per input, we handle translation in ErrorUtils.addErrorMessage due to the inability to concatenate error keys.
        if (variables?.isTranslated) {
            return phrase;
        }

        return translateLocal(phrase as TranslationPaths, variables as never);
    } catch (error) {
        return Array.isArray(message) ? message[0] : message;
    }
}

key={i}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.LINK}
onPress={() => {
fileDownload(message.source, message.filename);
}}
>
<Text
key={i}
style={styles.offlineFeedback.text}
>
<Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{Localize.translateLocal('iou.error.receiptFailureMessage')}</Text>
<Text style={[styles.optionAlternateText, styles.textLabelSupporting, styles.link]}>{Localize.translateLocal('iou.error.saveFileMessage')}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming from this issue #39330, we should use either Pressable component or TextLink here, instead of wrapping the whole text inside a Pressable.

<Text style={[styles.optionAlternateText, styles.textLabelSupporting]}>{Localize.translateLocal('iou.error.loseFileMessage')}</Text>
</Text>
Comment on lines +101 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gonals The error text next to red dot indicators is meant to be red to be consistent with other error messages in the app.

With your current change here, the error text will revert back to light-greyish green.

expensify-new-error-text-bug-introduced

I just worked on an issue fixing this and just luckily stumbled on the regression from the video of another issue I was looking at.

</PressableWithoutFeedback>
) : (
<Text
key={i}
style={[StyleUtils.getDotIndicatorTextStyles(isErrorMessage), ...props.textStyles]}
>
{message}
</Text>
),
)}
</View>
</View>
);
Expand Down
4 changes: 3 additions & 1 deletion src/components/MessagesRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import Tooltip from './Tooltip';

const propTypes = {
/** The messages to display */
messages: PropTypes.objectOf(PropTypes.oneOfType([PropTypes.string, PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.object]))])),
messages: PropTypes.objectOf(
PropTypes.oneOfType([PropTypes.oneOfType([PropTypes.string, PropTypes.object]), PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.object]))]),
),

/** The type of message, 'error' shows a red dot, 'success' shows a green dot */
type: PropTypes.oneOf(['error', 'success']).isRequired,
Expand Down
3 changes: 3 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,9 @@ export default {
invalidSplit: 'Split amounts do not equal total amount',
other: 'Unexpected error, please try again later',
genericCreateFailureMessage: 'Unexpected error requesting money, please try again later',
receiptFailureMessage: "The receipt didn't upload. ",
saveFileMessage: 'Download the file ',
loseFileMessage: 'or dismiss this error and lose it',
genericDeleteFailureMessage: 'Unexpected error deleting the money request, please try again later',
genericEditFailureMessage: 'Unexpected error editing the money request, please try again later',
genericSmartscanFailureMessage: 'Transaction is missing fields',
Expand Down
3 changes: 3 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,9 @@ export default {
invalidSplit: 'La suma de las partes no equivale al monto total',
other: 'Error inesperado, por favor inténtalo más tarde',
genericCreateFailureMessage: 'Error inesperado solicitando dinero, Por favor, inténtalo más tarde',
receiptFailureMessage: 'El recibo no se subió. ',
saveFileMessage: 'Guarda el archivo ',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this word "Guarda" to "Download" as well for consistency.
cc: @marcochavezf for Spanish help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm from Spain! I decided to leave both as they are on purpose 😁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok!

loseFileMessage: 'o descarta este error y piérdelo',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB: I think it sounds a bit better

Suggested change
loseFileMessage: 'o descarta este error y piérdelo',
loseFileMessage: 'o descarta este error y pierde el archivo',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While updating above, please also apply this suggestion 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using "archivo" twice in a row doesn't quite work: "Guarda el archivo o descarta este error y pierde el archivo" is a bit redundant

genericDeleteFailureMessage: 'Error inesperado eliminando la solicitud de dinero. Por favor, inténtalo más tarde',
genericEditFailureMessage: 'Error inesperado al guardar la solicitud de dinero. Por favor, inténtalo más tarde',
genericSmartscanFailureMessage: 'La transacción tiene campos vacíos',
Expand Down
10 changes: 9 additions & 1 deletion src/libs/ErrorUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ function getMicroSecondOnyxError(error: string): Record<number, string> {
return {[DateUtils.getMicroseconds()]: error};
}

/**
* Method used to get an error object with microsecond as the key and an object as the value.
* @param error - error key or message to be saved
*/
function getMicroSecondOnyxErrorObject(error: Record<string, string>): Record<number, Record<string, string>> {
return {[DateUtils.getMicroseconds()]: error};
}

type OnyxDataWithErrors = {
errors?: Errors;
};
Expand Down Expand Up @@ -111,4 +119,4 @@ function addErrorMessage<TKey extends TranslationPaths>(errors: ErrorsList, inpu
}
}

export {getAuthenticateErrorMessage, getMicroSecondOnyxError, getLatestErrorMessage, getLatestErrorField, getEarliestErrorField, addErrorMessage};
export {getAuthenticateErrorMessage, getMicroSecondOnyxError, getMicroSecondOnyxErrorObject, getLatestErrorMessage, getLatestErrorField, getEarliestErrorField, addErrorMessage};
24 changes: 18 additions & 6 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,18 @@ function resetMoneyRequestInfo(id = '') {
});
}

/**
* Helper function to get the receipt error for money requests, or the generic error if there's no receipt
*
* @param {Object} receipt
* @returns {Object}
*/
function getReceiptError(receipt) {
return _.isEmpty(receipt)
? ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage')
: ErrorUtils.getMicroSecondOnyxErrorObject({error: CONST.IOU.RECEIPT_ERROR, source: receipt.source, filename: receipt.filename});
}

function buildOnyxDataForMoneyRequest(
chatReport,
iouReport,
Expand Down Expand Up @@ -344,7 +356,7 @@ function buildOnyxDataForMoneyRequest(
...(isNewChatReport
? {
[chatCreatedAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(transaction.receipt),
},
[reportPreviewAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError(null),
Expand All @@ -353,7 +365,7 @@ function buildOnyxDataForMoneyRequest(
: {
[reportPreviewAction.reportActionID]: {
created: reportPreviewAction.created,
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(transaction.receipt),
},
}),
},
Expand All @@ -365,15 +377,15 @@ function buildOnyxDataForMoneyRequest(
...(isNewIOUReport
? {
[iouCreatedAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(transaction.receipt),
},
[iouAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError(null),
},
}
: {
[iouAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(transaction.receipt),
},
}),
},
Expand Down Expand Up @@ -1433,7 +1445,7 @@ function startSplitBill(participants, currentUserLogin, currentUserAccountID, co
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${splitChatReport.reportID}`,
value: {
[splitIOUReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(receipt),
},
},
});
Expand All @@ -1456,7 +1468,7 @@ function startSplitBill(participants, currentUserLogin, currentUserAccountID, co
errors: ErrorUtils.getMicroSecondOnyxError('report.genericCreateReportFailureMessage'),
},
[splitIOUReportAction.reportActionID]: {
errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericCreateFailureMessage'),
errors: getReceiptError(receipt),
},
},
},
Expand Down
Loading