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.

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

Improve cancelling money in a different currency offline #13329

Merged
merged 37 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
157184d
Display a pending message offline when currency request or cancelled …
youssef-lr Dec 5, 2022
099cca6
Improve cancel money request flow when dealing with different currencies
youssef-lr Dec 5, 2022
ed02180
Cleanup
youssef-lr Dec 5, 2022
cf638e5
Set iouReport total to 0 if all requests have been cancelled
youssef-lr Dec 5, 2022
7038110
Fix logic in isIOUReportPendingCurrencyConversion
youssef-lr Dec 5, 2022
d531cec
Cleanup
youssef-lr Dec 5, 2022
b7c2ac9
Improve comments
youssef-lr Dec 5, 2022
c4492d2
Fix comments, propTypes, and JSDocs
youssef-lr Dec 5, 2022
37e457c
Apply suggestions from code review
youssef-lr Dec 5, 2022
b1442f8
use CONST for report action type
youssef-lr Dec 5, 2022
18ec67e
Add more context to function comment
youssef-lr Dec 5, 2022
ce98c1e
Simplify non trivial logic for deciding if an IOU is pending currency…
youssef-lr Dec 6, 2022
2ade174
Fix logic and clarify comments
youssef-lr Dec 6, 2022
865c684
Revert back to using previous logic as it's reliable
youssef-lr Dec 6, 2022
85f17d3
Remove bug fix to be fixed in a separate issue
youssef-lr Dec 6, 2022
966e018
Simply logic once more
youssef-lr Dec 6, 2022
5f030c6
Fix CSS styling and add converting message
youssef-lr Dec 6, 2022
fb6a160
Small code improvement
youssef-lr Dec 6, 2022
db6a7bd
Add initial tests
youssef-lr Dec 6, 2022
9e94108
Simplify logic
youssef-lr Dec 6, 2022
c069432
Small improvement
youssef-lr Dec 6, 2022
3602b13
Cleanup
youssef-lr Dec 6, 2022
b20b524
Remove 'converting' message
youssef-lr Dec 6, 2022
5f9eb76
Cleanup
youssef-lr Dec 6, 2022
4279965
Improvement
youssef-lr Dec 6, 2022
503ffc4
Change param name
youssef-lr Dec 7, 2022
88531ae
Merge branch 'main' into youssef_iou_pending_currency_conversion
youssef-lr Dec 7, 2022
7cab87d
Add some more tests and comments
youssef-lr Dec 8, 2022
b792cb6
Merge branch 'main' into youssef_iou_pending_currency_conversion
youssef-lr Dec 11, 2022
40b79f9
Add another test
youssef-lr Dec 12, 2022
7661434
Merge branch 'main' into youssef_iou_pending_currency_conversion
youssef-lr Dec 22, 2022
f8c0f43
Merge branch 'main' into youssef_iou_pending_currency_conversion
youssef-lr Dec 23, 2022
a4672eb
Fix indentation
youssef-lr Dec 23, 2022
16e3940
Remove fix to be included in a separate issue
youssef-lr Dec 23, 2022
fb811c5
Add translation
youssef-lr Dec 23, 2022
8a683dd
Apply suggestions from code review
youssef-lr Dec 29, 2022
ba2ab0b
Apply suggestions from code review
youssef-lr Jan 2, 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
45 changes: 40 additions & 5 deletions src/components/ReportActionItem/IOUAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,18 @@ import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';
import {withNetwork} from '../OnyxProvider';
import compose from '../../libs/compose';
import IOUQuote from './IOUQuote';
import reportActionPropTypes from '../../pages/home/report/reportActionPropTypes';
import networkPropTypes from '../networkPropTypes';
import iouReportPropTypes from '../../pages/iouReportPropTypes';
import IOUPreview from './IOUPreview';
import Navigation from '../../libs/Navigation/Navigation';
import ROUTES from '../../ROUTES';
import styles from '../../styles/styles';
import * as IOUUtils from '../../libs/IOUUtils';

const propTypes = {
/** All the data of the action */
Expand All @@ -30,9 +36,16 @@ const propTypes = {
hasOutstandingIOU: PropTypes.bool.isRequired,
}),

/** IOU report data object */
iouReport: iouReportPropTypes.isRequired,

/** Array of report actions for this report */
reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)).isRequired,

/** Whether the IOU is hovered so we can modify its style */
isHovered: PropTypes.bool,

network: networkPropTypes.isRequired,
};

const defaultProps = {
Expand All @@ -52,6 +65,17 @@ const IOUAction = (props) => {
&& Boolean(props.action.originalMessage.IOUReportID)
&& props.chatReport.hasOutstandingIOU) || props.action.originalMessage.type === 'pay';

let shouldShowPendingConversionMessage = false;
Copy link
Contributor

@techievivek techievivek Dec 7, 2022

Choose a reason for hiding this comment

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

Can't we just do props.network.isOffline && IOUUtils.isIOUReportPendingCurrencyConversion(props.reportActions, props.iouReport) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also need to check if we are in the IOUPreview action so that we avoid having to run this code unnecessarily in other actions.

Copy link
Contributor

@techievivek techievivek Dec 28, 2022

Choose a reason for hiding this comment

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

I am thinking if we ever show a pending message then it has to be for the IOUPreview, right? So can't we just do

shouldShowPendingConversionMessage={IOUUtils.isIOUReportPendingCurrencyConversion(props.reportActions, props.iouReport);}

at line no 94.

Because only when IOUPreview will be rendered this expression will get evaluated and I can also see in the isIOUReportPendingCurrencyConversion we are already checking for pending actions to the if block seems unnecessary to me. I am pretty sure we will need to test it out to be sure but this is my guess by looking at the code.

What do you think about it?

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 not sure. This will make isIOUReportPendingCurrencyConversion even when we're online, and even when we're showing an IOUPreview of an action of type pay (the one with a message You paid user@email.com. I added that check so that we only that function if it's the last IOUPreview of a chat that has an outstanding IOU and we're offline. Let me know what you think.

if (
props.iouReport
&& props.chatReport.hasOutstandingIOU
&& props.isMostRecentIOUReportAction
&& props.action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD
&& props.network.isOffline
) {
shouldShowPendingConversionMessage = IOUUtils.isIOUReportPendingCurrencyConversion(props.reportActions, props.iouReport);
}

return (
<>
<IOUQuote
Expand All @@ -64,6 +88,7 @@ const IOUAction = (props) => {
pendingAction={lodashGet(props.action, 'pendingAction', null)}
iouReportID={props.action.originalMessage.IOUReportID.toString()}
chatReportID={props.chatReportID}
shouldShowPendingConversionMessage={shouldShowPendingConversionMessage}
onPayButtonPressed={launchDetailsModal}
onPreviewPressed={launchDetailsModal}
containerStyles={[
Expand All @@ -83,8 +108,18 @@ IOUAction.propTypes = propTypes;
IOUAction.defaultProps = defaultProps;
IOUAction.displayName = 'IOUAction';

export default withOnyx({
chatReport: {
key: ({chatReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`,
},
})(IOUAction);
export default compose(
withOnyx({
chatReport: {
key: ({chatReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${chatReportID}`,
},
iouReport: {
key: ({iouReportID}) => `${ONYXKEYS.COLLECTION.REPORT}${iouReportID}`,
},
reportActions: {
key: ({chatReportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReportID}`,
canEvict: false,
},
}),
withNetwork(),
)(IOUAction);
17 changes: 12 additions & 5 deletions src/components/ReportActionItem/IOUPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,18 @@ const IOUPreview = (props) => {
</Text>
)
: (
<Text>
{props.iouReport.hasOutstandingIOU
? props.translate('iou.owesyou', {manager: managerName})
: props.translate('iou.paidyou', {manager: managerName})}
</Text>
<>
<Text>
{props.iouReport.hasOutstandingIOU
? props.translate('iou.owesyou', {manager: managerName})
: props.translate('iou.paidyou', {manager: managerName})}
</Text>
{props.shouldShowPendingConversionMessage && (
<Text style={[styles.textLabel, styles.colorMuted]}>
{props.translate('iou.pendingConversionMessage')}
</Text>
)}
</>
)}
{(isCurrentUserManager
&& !props.shouldHidePayButton
Expand Down
1 change: 1 addition & 0 deletions src/languages/en.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ export default {
split: ({amount}) => `Split ${amount}`,
send: ({amount}) => `Send ${amount}`,
noReimbursableExpenses: 'This report has an invalid amount',
pendingConversionMessage: 'Total will update when you\'re back online',
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
error: {
invalidSplit: 'Split amounts do not equal total amount',
other: 'Unexpected error, please try again later',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ export default {
split: ({amount}) => `Dividir ${amount}`,
send: ({amount}) => `Enviar ${amount}`,
noReimbursableExpenses: 'El monto de este informe es inválido',
pendingConversionMessage: 'El total se actualizará cuando estés online',
error: {
invalidSplit: 'La suma de las partes no equivale al monto total',
other: 'Error inesperado, por favor inténtalo más tarde',
Expand Down
71 changes: 71 additions & 0 deletions src/libs/IOUUtils.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import _ from 'underscore';
import CONST from '../CONST';

/**
Expand Down Expand Up @@ -65,7 +66,77 @@ function updateIOUOwnerAndTotal(iouReport, actorEmail, amount, currency, type =
return iouReportUpdate;
}

/**
* Returns the list of IOU actions depending on the type and whether or not they are pending.
* Used below so that we can decide if an IOU report is pending currency conversion.
*
* @param {Array} reportActions
* @param {Object} iouReport
* @param {String} type - iouReportAction type. Can be oneOf(create, decline, cancel, pay, split)
* @param {String} pendingAction
* @param {Boolean} filterRequestsInDifferentCurrency
*
* @returns {Array}
*/
function getIOUReportActions(reportActions, iouReport, type = '', pendingAction = '', filterRequestsInDifferentCurrency = false) {
return _.chain(reportActions)
.filter(action => action.originalMessage
&& action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU
&& action.originalMessage.IOUReportID.toString() === iouReport.reportID.toString())
.filter(action => (!_.isEmpty(type) ? action.originalMessage.type === type : true))
.filter(action => (!_.isEmpty(pendingAction) ? action.pendingAction === pendingAction : true))
.filter(action => (filterRequestsInDifferentCurrency ? action.originalMessage.currency !== iouReport.currency : true))
.value();
}

/**
* Returns whether or not an IOU report contains money requests in a different currency
* that are either created or cancelled offline, and thus haven't been converted to the report's currency yet
*
* @param {Array} reportActions
* @param {Object} iouReport
*
* @returns {Boolean}
*/
function isIOUReportPendingCurrencyConversion(reportActions, iouReport) {
techievivek marked this conversation as resolved.
Show resolved Hide resolved
// Pending money requests that are in a different currency
const pendingRequestsInDifferentCurrency = _.chain(getIOUReportActions(
reportActions,
iouReport,
CONST.IOU.REPORT_ACTION_TYPE.CREATE,
CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
true,
)).map(action => action.originalMessage.IOUTransactionID)
.sort()
.value();

// Pending cancelled money requests that are in a different currency
const pendingCancelledRequestsInDifferentCurrency = _.chain(getIOUReportActions(
reportActions,
iouReport,
CONST.IOU.REPORT_ACTION_TYPE.CANCEL,
CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
true,
)).map(action => action.originalMessage.IOUTransactionID)
.sort()
.value();

const hasPendingRequests = Boolean(pendingRequestsInDifferentCurrency.length || pendingCancelledRequestsInDifferentCurrency.length);

// If we have pending money requests made offline, check if all of them have been cancelled offline
// In order to do that, we can grab transactionIDs of all the created and cancelled money requests and check if they're identical
if (hasPendingRequests && _.isEqual(pendingRequestsInDifferentCurrency, pendingCancelledRequestsInDifferentCurrency)) {
return false;
}

// Not all requests made offline had been cancelled,
// simply return if we have any pending created or cancelled requests
return hasPendingRequests;
}

export {
calculateAmount,
updateIOUOwnerAndTotal,
getIOUReportActions,
isIOUReportPendingCurrencyConversion,
};
1 change: 1 addition & 0 deletions src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ class ReportActionItem extends Component {
children = (
<IOUAction
chatReportID={this.props.report.reportID}
iouReportID={this.props.report.iouReportID}
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
action={this.props.action}
isMostRecentIOUReportAction={this.props.isMostRecentIOUReportAction}
isHovered={hovered}
Expand Down
1 change: 1 addition & 0 deletions src/pages/iou/IOUTransactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class IOUTransactions extends Component {
<ReportTransaction
chatReportID={this.props.chatReportID}
iouReportID={this.props.iouReportID}
reportActions={this.props.reportActions}
youssef-lr marked this conversation as resolved.
Show resolved Hide resolved
action={reportAction}
key={reportAction.reportActionID}
canBeRejected={canBeRejected}
Expand Down
134 changes: 134 additions & 0 deletions tests/unit/IOUUtilsTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import * as IOUUtils from '../../src/libs/IOUUtils';
import * as ReportUtils from '../../src/libs/ReportUtils';

let iouReport;
let reportActions;
const ownerEmail = 'owner@iou.com';
const managerEmail = 'manager@iou.com';

function createIOUReportAction(type, amount, currency, {IOUTransactionID, isOnline} = {}) {
const moneyRequestAction = ReportUtils.buildOptimisticIOUReportAction(
1,
type,
amount,
currency,
'Test comment',
[managerEmail],
'',
IOUTransactionID,
iouReport.reportID,
);

// Default is to create requests offline, if this is specified then we need to remove the pendingAction
moneyRequestAction.pendingAction = isOnline ? null : 'add';

reportActions.push(moneyRequestAction);
return moneyRequestAction;
}

function cancelMoneyRequest(moneyRequestAction, {isOnline} = {}) {
createIOUReportAction(
'cancel',
moneyRequestAction.originalMessage.amount,
moneyRequestAction.originalMessage.currency,
{
IOUTransactionID: moneyRequestAction.originalMessage.IOUTransactionID,
isOnline,
},
);
}

beforeEach(() => {
reportActions = [];
const chatReportID = ReportUtils.generateReportID();
const amount = 1000;
const currency = 'USD';

iouReport = ReportUtils.buildOptimisticIOUReport(
ownerEmail,
managerEmail,
amount,
chatReportID,
currency,
'en',
);

// The starting point of all tests is the IOUReport containing a single non-pending transaction in USD
// All requests in the tests are assumed to be offline, unless isOnline is specified
createIOUReportAction('create', amount, currency, {IOUTransactionID: '', isOnline: true});
});

describe('isIOUReportPendingCurrencyConversion', () => {
test('Requesting money offline in a different currency will show the pending conversion message', () => {
// Request money offline in AED
createIOUReportAction('create', 100, 'AED');

// We requested money offline in a different currency, we don't know the total of the iouReport until we're back online
expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(true);
});

test('IOUReport is not pending conversion when all requests made offline have been cancelled', () => {
// Create two requests offline
const moneyRequestA = createIOUReportAction('create', 1000, 'AED');
const moneyRequestB = createIOUReportAction('create', 1000, 'AED');

// Cancel both requests
cancelMoneyRequest(moneyRequestA);
cancelMoneyRequest(moneyRequestB);

// Both requests made offline have been cancelled, total won't update so no need to show a pending conversion message
expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(false);
});

test('Cancelling a request made online shows the preview', () => {
// Request money online in AED
const moneyRequest = createIOUReportAction('create', 1000, 'AED', {isOnline: true});

// Cancel it offline
cancelMoneyRequest(moneyRequest);

// We don't know what the total is because we need to subtract the converted amount of the offline request from the total
expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(true);
});

test('Cancelling a request made offline while there\'s a previous one made online will not show the pending conversion message', () => {
// Request money online in AED
createIOUReportAction('create', 1000, 'AED', {isOnline: true});

// Another request offline
const moneyRequestOffline = createIOUReportAction('create', 1000, 'AED');

// Cancel the request made offline
cancelMoneyRequest(moneyRequestOffline);

expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(false);
});

test('Cancelling a request made online while we have one made offline will show the pending conversion message', () => {
// Request money online in AED
const moneyRequestOnline = createIOUReportAction('create', 1000, 'AED', {isOnline: true});

// Requet money again but offline
createIOUReportAction('create', 1000, 'AED');

// Cancel the request made online
cancelMoneyRequest(moneyRequestOnline);

// We don't know what the total is because we need to subtract the converted amount of the offline request from the total
expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(true);
});

test('Cancelling a request offline in the report\'s currency when we have requests in a different currency does not show the pending conversion message', () => {
// Request money in the report's curreny (USD)
const onlineMoneyRequestInUSD = createIOUReportAction('create', 1000, 'USD', {isOnline: true});

// Request money online in a different currency
createIOUReportAction('create', 2000, 'AED', {isOnline: true});

// Cancel the USD request offline
cancelMoneyRequest(onlineMoneyRequestInUSD);

expect(IOUUtils.isIOUReportPendingCurrencyConversion(reportActions, iouReport)).toBe(false);
});
});

techievivek marked this conversation as resolved.
Show resolved Hide resolved