Skip to content

Followup/23961 edit request headers #25399

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

Merged
mountiny merged 7 commits into from
Aug 21, 2023
Merged

Followup/23961 edit request headers #25399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
206eb2b
feat: grey out the headers as soon as the user goes offline
koko57 Aug 17, 2023
2d25488
feat: create a function for getting a proper message schema
koko57 Aug 17, 2023
03b3836
Merge branch 'main' into followup/23961-edit-request-headers
koko57 Aug 17, 2023
f776dc5
fix: remove request word from the message
koko57 Aug 17, 2023
782ae85
feat: only grey out updated fields
BeeMargarida Aug 18, 2023
54cb2c4
fix: grey out when pendingAction is ADD
BeeMargarida Aug 18, 2023
860e2f4
Merge branch 'main' into followup/23961-edit-request-headers
BeeMargarida Aug 18, 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: 3 additions & 3 deletions src/components/OfflineWithFeedback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ function OfflineWithFeedback(props) {
const errorMessages = _.omit(props.errors, (e) => e === null);
const hasErrorMessages = !_.isEmpty(errorMessages);
const isOfflinePendingAction = props.network.isOffline && props.pendingAction;
const isUpdateOrDeleteError = hasErrors && (props.pendingAction === 'delete' || props.pendingAction === 'update');
const isAddError = hasErrors && props.pendingAction === 'add';
const isUpdateOrDeleteError = hasErrors && (props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE);
const isAddError = hasErrors && props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD;
const needsOpacity = (isOfflinePendingAction && !isUpdateOrDeleteError) || isAddError;
const needsStrikeThrough = props.network.isOffline && props.pendingAction === 'delete';
const needsStrikeThrough = props.network.isOffline && props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
const hideChildren = props.shouldHideOnDelete && !props.network.isOffline && props.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && !hasErrors;
let children = props.children;

Expand Down
55 changes: 31 additions & 24 deletions src/components/ReportActionItem/MoneyRequestView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import * as CurrencyUtils from '../../libs/CurrencyUtils';
import EmptyStateBackgroundImage from '../../../assets/images/empty-state_background-fade.png';
import useLocalize from '../../hooks/useLocalize';
import useWindowDimensions from '../../hooks/useWindowDimensions';
import OfflineWithFeedback from '../OfflineWithFeedback';

const propTypes = {
/** The report currently being looked at */
Expand Down Expand Up @@ -97,30 +98,36 @@ function MoneyRequestView({report, parentReport, shouldShowHorizontalRule, polic
style={[StyleUtils.getReportWelcomeBackgroundImageStyle(true)]}
/>
</View>
<MenuItemWithTopDescription
title={formattedTransactionAmount ? formattedTransactionAmount.toString() : ''}
shouldShowTitleIcon={isSettled}
titleIcon={Expensicons.Checkmark}
description={description}
titleStyle={styles.newKansasLarge}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.AMOUNT))}
/>
<MenuItemWithTopDescription
description={translate('common.description')}
title={transactionDescription}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION))}
/>
<MenuItemWithTopDescription
description={translate('common.date')}
title={transactionDate}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))}
/>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.amount') || lodashGet(transaction, 'pendingAction')}>

@jakub-trzebiatowski jakub-trzebiatowski Oct 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Coming from #26235

When adding this, we didn't take into consideration that MoneyRequestView itself is wrapped in <OfflineWithFeedback>, which can lead to a cumulated grey-out effect.

@koko57 koko57 Oct 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but in the same commit wrapping the whole component was removed. So it looks like it was restored later and caused the issue

@jakub-trzebiatowski jakub-trzebiatowski Oct 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed that! Thank you, I'll update my checklist.

@jakub-trzebiatowski jakub-trzebiatowski Oct 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Found it: https://github.com/Expensify/App/pull/24235/files#r1348589086

<MenuItemWithTopDescription
title={formattedTransactionAmount ? formattedTransactionAmount.toString() : ''}
shouldShowTitleIcon={isSettled}
titleIcon={Expensicons.Checkmark}
description={description}
titleStyle={styles.newKansasLarge}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.AMOUNT))}
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.comment') || lodashGet(transaction, 'pendingAction')}>
<MenuItemWithTopDescription
description={translate('common.description')}
title={transactionDescription}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION))}
/>
</OfflineWithFeedback>
<OfflineWithFeedback pendingAction={lodashGet(transaction, 'pendingFields.created') || lodashGet(transaction, 'pendingAction')}>
<MenuItemWithTopDescription
description={translate('common.date')}
title={transactionDate}
disabled={isSettled || !canEdit}
shouldShowRightIcon={canEdit}
onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))}
/>
</OfflineWithFeedback>
{shouldShowHorizontalRule && <View style={styles.reportHorizontalRule} />}
</View>
);
Expand Down
36 changes: 31 additions & 5 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,7 +1206,10 @@ function getPolicyExpenseChatName(report, policy = undefined) {
function getMoneyRequestReportName(report, policy = undefined) {
const formattedAmount = CurrencyUtils.convertToDisplayString(getMoneyRequestTotal(report), report.currency);
const payerName = isExpenseReport(report) ? getPolicyName(report, false, policy) : getDisplayNameForParticipant(report.managerID);
const payerPaidAmountMesssage = Localize.translateLocal('iou.payerPaidAmount', {payer: payerName, amount: formattedAmount});
const payerPaidAmountMesssage = Localize.translateLocal('iou.payerPaidAmount', {
payer: payerName,
amount: formattedAmount,
});

if (report.isWaitingOnBankAccount) {
return `${payerPaidAmountMesssage} • ${Localize.translateLocal('iou.pending')}`;
Expand Down Expand Up @@ -1304,6 +1307,29 @@ function getReportPreviewMessage(report, reportAction = {}) {
return Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount});
}

/**
* Get the proper message schema for modified expense message.
*
* @param {String} newValue
* @param {String} oldValue
* @param {String} valueName
* @param {Boolean} valueInQuotes
* @returns {String}
*/

function getProperSchemaForModifiedExpenseMessage(newValue, oldValue, valueName, valueInQuotes) {
const newValueToDisplay = valueInQuotes ? `"${newValue}"` : newValue;
const oldValueToDisplay = valueInQuotes ? `"${oldValue}"` : oldValue;

if (!oldValue) {
return `set the ${valueName} to ${newValueToDisplay}`;
}
if (!newValue) {
return `removed the ${valueName} (previously ${oldValueToDisplay})`;
}
return `changed the ${valueName} to ${newValueToDisplay} (previously ${oldValueToDisplay})`;
Comment thread
mountiny marked this conversation as resolved.
}

/**
* Get the report action message when expense has been modified.
*
Expand All @@ -1328,25 +1354,25 @@ function getModifiedExpenseMessage(reportAction) {
const currency = reportActionOriginalMessage.currency;
const amount = CurrencyUtils.convertToDisplayString(reportActionOriginalMessage.amount, currency);

return `changed the request to ${amount} (previously ${oldAmount})`;
return getProperSchemaForModifiedExpenseMessage(amount, oldAmount, 'amount', false);
}

const hasModifiedComment = _.has(reportActionOriginalMessage, 'oldComment') && _.has(reportActionOriginalMessage, 'newComment');
if (hasModifiedComment) {
return `changed the request description to "${reportActionOriginalMessage.newComment}" (previously "${reportActionOriginalMessage.oldComment}")`;
return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage.newComment, reportActionOriginalMessage.oldComment, 'description', true);
}

const hasModifiedMerchant = _.has(reportActionOriginalMessage, 'oldMerchant') && _.has(reportActionOriginalMessage, 'merchant');
if (hasModifiedMerchant) {
return `changed the request merchant to "${reportActionOriginalMessage.merchant}" (previously "${reportActionOriginalMessage.oldMerchant}")`;
return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage.merchant, reportActionOriginalMessage.oldMerchant, 'merchant', true);
}

const hasModifiedCreated = _.has(reportActionOriginalMessage, 'oldCreated') && _.has(reportActionOriginalMessage, 'created');
if (hasModifiedCreated) {
// Take only the YYYY-MM-DD value as the original date includes timestamp
let formattedOldCreated = new Date(reportActionOriginalMessage.oldCreated);
formattedOldCreated = format(formattedOldCreated, CONST.DATE.FNS_FORMAT_STRING);
return `changed the request date to ${reportActionOriginalMessage.created} (previously ${formattedOldCreated})`;
return getProperSchemaForModifiedExpenseMessage(reportActionOriginalMessage.created, formattedOldCreated, 'date', false);
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/libs/TransactionUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,12 @@ function getUpdatedTransaction(transaction, transactionChanges, isFromExpenseRep
if (_.has(transactionChanges, 'currency')) {
updatedTransaction.modifiedCurrency = transactionChanges.currency;
}
updatedTransaction.pendingAction = CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE;
updatedTransaction.pendingFields = {
...(_.has(transactionChanges, 'comment') && {comment: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
...(_.has(transactionChanges, 'created') && {created: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
...(_.has(transactionChanges, 'amount') && {amount: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
...(_.has(transactionChanges, 'currency') && {currency: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE}),
};
Comment on lines +100 to +105

@s77rt s77rt Nov 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't overwrite the pendingFields as is. Instead we should merge it because the existing pendingFields are expected to be still there.


return updatedTransaction;
}
Expand Down
9 changes: 8 additions & 1 deletion src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,14 @@ function editMoneyRequest(transactionID, transactionThreadReportID, transactionC
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
value: {pendingAction: null},
value: {
pendingFields: {
comment: null,
amount: null,
created: null,
currency: null,
},
},
},
];

Expand Down
10 changes: 4 additions & 6 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,12 +468,10 @@ function ReportActionItem(props) {
const parentReportAction = ReportActionsUtils.getParentReportAction(props.report);
if (ReportActionsUtils.isTransactionThread(parentReportAction)) {
return (
<OfflineWithFeedback pendingAction={props.action.pendingAction}>
<MoneyRequestView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
</OfflineWithFeedback>
<MoneyRequestView
report={props.report}
shouldShowHorizontalRule={!props.shouldHideThreadDividerLine}
/>
);
}
if (ReportUtils.isTaskReport(props.report)) {
Expand Down