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

Fix couple of manual request bugs #19194

Merged
merged 9 commits into from
May 19, 2023
Merged

Fix couple of manual request bugs #19194

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6761446
Make sure to use the chat reportID
mountiny May 18, 2023
a29f4bb
Remove the optimistic update of report preview
mountiny May 18, 2023
387a176
Hide the pay button on the transaction view
mountiny May 18, 2023
c4df4b3
Fix styles
mountiny May 18, 2023
d81c6d8
Remove the report preview action from failure data
mountiny May 18, 2023
4201994
Fix pay money request optimistic data
mountiny May 18, 2023
4d41619
Make sure there is no settled 0 messages showing
mountiny May 18, 2023
326e0e4
Refactor code for cleaner condition
mountiny May 18, 2023
f21c9bc
Merge branch 'main' into vit-fixHeaderPaymentButton
mountiny May 19, 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
6 changes: 4 additions & 2 deletions src/components/MoneyRequestHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ const MoneyRequestHeader = (props) => {
: ReportUtils.getAvatar(lodashGet(props.personalDetails, [moneyRequestReport.managerEmail, 'avatar']), moneyRequestReport.managerEmail);
const policy = props.policies[`${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}`];
const shouldShowSettlementButton =
!isSettled && (Policy.isAdminOfFreePolicy([policy]) || (ReportUtils.isMoneyRequestReport(props.report) && lodashGet(props.session, 'email', null) === props.report.managerEmail));
!isSettled &&
!props.isSingleTransactionView &&
(Policy.isAdminOfFreePolicy([policy]) || (ReportUtils.isMoneyRequestReport(props.report) && lodashGet(props.session, 'email', null) === props.report.managerEmail));
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a whole lotta logic. Maybe consider a helper or separate condition within the component for the final line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

split it up a bit!

return (
<View style={[{backgroundColor: themeColors.highlightBG}, styles.pl0]}>
<HeaderWithCloseButton
Expand Down Expand Up @@ -149,7 +151,7 @@ const MoneyRequestHeader = (props) => {
currency={props.report.currency}
policyID={props.report.policyID}
shouldShowPaypal={Boolean(lodashGet(props.personalDetails, [moneyRequestReport.managerEmail, 'payPalMeAddress']))}
chatReportID={props.report.chatReportID}
chatReportID={props.chatReport.reportID}
iouReport={props.report}
onPress={(paymentType) => IOU.payMoneyRequest(paymentType, props.chatReport, props.report)}
enablePaymentsRoute={ROUTES.BANK_ACCOUNT_NEW}
Expand Down
8 changes: 5 additions & 3 deletions src/components/ReportActionItem/ReportPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ const ReportPreview = (props) => {
const isCurrentUserManager = managerEmail === lodashGet(props.session, 'email', null);
return (
<View style={[styles.chatItemMessage, styles.mt4]}>
{_.map(props.action.message, (index) => (
{_.map(props.action.message, (message, index) => (
<Pressable
key={`ReportPreview-${props.action.reportActionID}-${index}`}
onPress={() => {
Expand All @@ -114,10 +114,12 @@ const ReportPreview = (props) => {
>
<View style={[styles.flexShrink1]}>
{props.iouReport.hasOutstandingIOU ? (
<Text style={[styles.chatItemMessage, styles.cursorPointer]}>{props.translate('iou.payerOwesAmount', {payer: managerName, amount: reportAmount})}</Text>
<Text style={[styles.chatItemMessage, styles.cursorPointer]}>
{lodashGet(message, 'html', props.translate('iou.payerOwesAmount', {payer: managerName, amount: reportAmount}))}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah one small issue with using the HTML is that the 'settled up' String should be lower case. But we can fix that here later 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handling it here https://github.com/Expensify/Web-Expensify/pull/37494/files#r1198024625

Copy link
Contributor

Choose a reason for hiding this comment

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

The html message is not translated. Should we translate the default message in this case?

</Text>
) : (
<View style={[styles.flexRow]}>
<Text style={[styles.chatItemMessage, styles.cursorPointer]}>{props.translate('iou.payerSettled', {amount: reportAmount})}</Text>
<Text style={[styles.chatItemMessage, styles.cursorPointer]}>{lodashGet(message, 'html', props.translate('iou.payerSettled', {amount: reportAmount}))}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

same 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.

Lets just translate it, I dont think it causes any harm. Again, we should not get to the point we will need the default there

{!props.iouReport.hasOutstandingIOU && (
<Icon
style={[styles.ml10]}
Expand Down
5 changes: 3 additions & 2 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1383,9 +1383,10 @@ function getIOUReportActionMessage(type, total, comment, currency, paymentType =
* @param {String} [paymentType] - Only required if the IOUReportAction type is 'pay'. Can be oneOf(elsewhere, payPal, Expensify).
* @param {String} [iouReportID] - Only required if the IOUReportActions type is oneOf(decline, cancel, pay). Generates a randomID as default.
* @param {Boolean} [isSettlingUp] - Whether we are settling up an IOU.
* @param {Boolean} [isSendMoneyFlow] - Whether this is send money flow
* @returns {Object}
*/
function buildOptimisticIOUReportAction(type, amount, currency, comment, participants, transactionID, paymentType = '', iouReportID = '', isSettlingUp = false) {
function buildOptimisticIOUReportAction(type, amount, currency, comment, participants, transactionID, paymentType = '', iouReportID = '', isSettlingUp = false, isSendMoneyFlow = false) {
const IOUReportID = iouReportID || generateReportID();
const parser = new ExpensiMark();
const commentText = getParsedComment(comment);
Expand All @@ -1401,7 +1402,7 @@ function buildOptimisticIOUReportAction(type, amount, currency, comment, partici
};

// We store amount, comment, currency in IOUDetails when type = pay
if (type === CONST.IOU.REPORT_ACTION_TYPE.PAY) {
if (type === CONST.IOU.REPORT_ACTION_TYPE.PAY && isSendMoneyFlow) {
_.each(['amount', 'comment', 'currency'], (key) => {
delete originalMessage[key];
});
Expand Down
21 changes: 2 additions & 19 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,8 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
optimisticTransaction.transactionID,
paymentMethodType,
optimisticIOUReport.reportID,
false,
true,
);

// First, add data that will be used in all cases
Expand Down Expand Up @@ -1014,7 +1016,6 @@ function getSendMoneyParams(report, amount, currency, comment, paymentMethodType
* @returns {Object}
*/
function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMethodType) {
const reportPreviewAction = ReportActionsUtils.getReportPreviewAction(chatReport.reportID, iouReport.reportID);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing the optimistic action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what was in the end causing the bugs with payment. if you sign in and navigate dirrectly to the IOU report and click pay, we dont have the report preview action as it was not treated as the parent report action -> hence the auth change. So the app crashed here/ you could not pay.

const optimisticTransaction = TransactionUtils.buildOptimisticTransaction(iouReport.total, iouReport.currency, iouReport.reportID);
const optimisticIOUReportAction = ReportUtils.buildOptimisticIOUReportAction(
CONST.IOU.REPORT_ACTION_TYPE.PAY,
Expand Down Expand Up @@ -1042,15 +1043,6 @@ function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMetho
lastMessageHtml: optimisticIOUReportAction.message[0].html,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[reportPreviewAction.reportActionID]: {
created: DateUtils.getDBTime(),
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReport.reportID}`,
Expand Down Expand Up @@ -1104,15 +1096,6 @@ function getPayMoneyRequestParams(chatReport, iouReport, recipient, paymentMetho
];

const failureData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
value: {
[reportPreviewAction.reportActionID]: {
created: reportPreviewAction.created,
},
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReport.reportID}`,
Expand Down
3 changes: 1 addition & 2 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,10 @@ class ReportActionItem extends Component {
originalMessage &&
(originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.CREATE ||
originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.SPLIT ||
(originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && originalMessage.IOUDetails))
(originalMessage.type === CONST.IOU.REPORT_ACTION_TYPE.PAY && _.has(originalMessage, 'IOUDetails')))
) {
// There is no single iouReport for bill splits, so only 1:1 requests require an iouReportID
const iouReportID = originalMessage.IOUReportID ? originalMessage.IOUReportID.toString() : '0';

children = (
<MoneyRequestAction
chatReportID={this.props.report.reportID}
Expand Down