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

feat: [Held requests] option does not show in the preview overflow menu. #42034

Merged
merged 13 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
93 changes: 85 additions & 8 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2391,6 +2391,14 @@ function isReportFieldOfTypeTitle(reportField: OnyxEntry<PolicyReportField>): bo
return reportField?.type === 'formula' && reportField?.fieldID === CONST.REPORT_FIELD_TITLE_FIELD_ID;
}

/**
* Check if Report has any held expenses
*/
function isHoldCreator(transaction: OnyxEntry<Transaction>, reportID: string): boolean {
const holdReportAction = ReportActionsUtils.getReportAction(reportID, `${transaction?.comment?.hold ?? ''}`);
return isActionCreator(holdReportAction);
}

/**
* Check if report fields are available to use in a report
*/
Expand Down Expand Up @@ -2707,6 +2715,80 @@ function canEditReportAction(reportAction: OnyxEntry<ReportAction>): boolean {
);
}

function getParentReportAction(parentReportActions: OnyxEntry<ReportActions>, parentReportActionID: string | undefined): OnyxEntry<ReportAction> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you mentioned you removed this one?

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 re-added this as the other function is deprecated and we are using this function in ReportScreen also.

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertjchen This is my concern, we should use deprecated function or creating a new one here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we can't use Use Onyx.connect() or withOnyx() instead as recommended by the deprecation comment? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@Krishna2323 Bump on the above question.

I checked ReportUtils, I saw that we're still using ReportActionsUtils.getParentReportAction(report); to get parentReportActions.

Copy link
Contributor

Choose a reason for hiding this comment

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

little bump @Krishna2323 so we can merge this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hungvu193, I think we can leave getParentReportAction as it is for now. It will be removed/replaced in this issue, and we are already using getParentReportAction in multiple places.

function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, parentReportActionID: string | undefined): OnyxEntry<OnyxTypes.ReportAction> {
if (!parentReportActions || !parentReportActionID) {
return null;
}
return parentReportActions[parentReportActionID ?? '0'];
}

@tgolen, can you pls confirm if we are going to replace getParentReportAction or not? I don't see any open issue/PR for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on this, @robertjchen? Since we're already using the deprecated getParentReportAction in ReportUtils, I'm okay with using it until we have a plan to remove it all at once.

Copy link
Contributor

@robertjchen robertjchen Jun 6, 2024

Choose a reason for hiding this comment

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

Sounds good, given the current progress on the migration- we can use the deprecated function for now 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming 😄 .
@Krishna2323 Can you update this please? Thank you 🙇

if (!parentReportActions || !parentReportActionID) {
return null;
}
return parentReportActions[parentReportActionID ?? '0'];
}

function canHoldUnholdReportAction(reportAction: OnyxEntry<ReportAction>): {canHoldRequest: boolean; canUnholdRequest: boolean} {
if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU) {
return {canHoldRequest: false, canUnholdRequest: false};
}

const moneyRequestReportID = reportAction?.originalMessage?.IOUReportID ?? 0;
const moneyRequestReport = getReport(String(moneyRequestReportID));

if (!moneyRequestReportID || !moneyRequestReport) {
return {canHoldRequest: false, canUnholdRequest: false};
}

const isRequestSettled = isSettled(moneyRequestReport?.reportID);
const isApproved = isReportApproved(moneyRequestReport);
const transactionID = moneyRequestReport ? reportAction?.originalMessage?.IOUTransactionID : 0;
const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`] ?? ({} as Transaction);

const parentReportActionsKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${moneyRequestReport.parentReportID}`;
const parentReportActions = allReportActions?.[parentReportActionsKey];
const parentReport = getReport(String(moneyRequestReport.parentReportID));
const parentReportAction = getParentReportAction(parentReportActions ?? {}, moneyRequestReport?.parentReportActionID);

const isRequestIOU = parentReport?.type === 'iou';
const isRequestHoldCreator = isHoldCreator(transaction, moneyRequestReport?.reportID) && isRequestIOU;
const isTrackExpenseMoneyReport = isTrackExpenseReport(moneyRequestReport);
const isActionOwner =
typeof parentReportAction?.actorAccountID === 'number' &&
typeof currentUserPersonalDetails?.accountID === 'number' &&
parentReportAction.actorAccountID === currentUserPersonalDetails?.accountID;
const isApprover = isMoneyRequestReport(moneyRequestReport) && moneyRequestReport?.managerID !== null && currentUserPersonalDetails?.accountID === moneyRequestReport?.managerID;
const isOnHold = TransactionUtils.isOnHold(transaction);
const isScanning = TransactionUtils.hasReceipt(transaction) && TransactionUtils.isReceiptBeingScanned(transaction);

const canModifyStatus = !isTrackExpenseMoneyReport && (isPolicyAdmin || isActionOwner || isApprover);
const isDeletedParentAction = ReportActionsUtils.isDeletedAction(parentReportAction);

const canHoldOrUnholdRequest = !isRequestSettled && !isApproved && !isDeletedParentAction;
const canHoldRequest = canHoldOrUnholdRequest && !isOnHold && (isRequestHoldCreator || (!isRequestIOU && canModifyStatus)) && !isScanning;
const canUnholdRequest = Boolean(canHoldOrUnholdRequest && isOnHold && (isRequestHoldCreator || (!isRequestIOU && canModifyStatus)));

return {canHoldRequest, canUnholdRequest};
}

const changeMoneyRequestHoldStatus = (reportAction: OnyxEntry<ReportAction>): void => {
if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU) {
return;
}
const moneyRequestReportID = reportAction?.originalMessage?.IOUReportID ?? 0;

const moneyRequestReport = getReport(String(moneyRequestReportID));
if (!moneyRequestReportID || !moneyRequestReport) {
return;
}
Comment on lines +2774 to +2782
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge these 2 conditions?

if (reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU || !moneyRequestReportID || !moneyRequestReport) {
return;
}

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 thats fine, reportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.IOU check is required before defining moneyRequestReport or moneyRequestReportID.


const transactionID = reportAction?.originalMessage?.IOUTransactionID ?? '';
const transaction = allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`] ?? ({} as Transaction);
const isOnHold = TransactionUtils.isOnHold(transaction);
const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${moneyRequestReport.policyID}`] ?? null;

if (isOnHold) {
IOU.unholdRequest(transactionID, reportAction.childReportID ?? '');
Copy link
Contributor

Choose a reason for hiding this comment

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

In here, you updated the hold expense with its childReportID, in our BE we didn't send Pusher event to update its parent that's why our replies didn't increase when you held the expense.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems that we will need BE changes here cc @robertjchen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hungvu193, thanks a lot for looking into this, I agree with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so the backend should also push updates for the parent report here, is that correct?

Could you let me know which particular field changes in the parent report that makes the update from the backend necessary? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

childVisibleActionCount is the current field to count the replies.
Since we also added a comment when we held request, I think the pusher event will be exactly the same when we added a new comment inside a thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, we can investigate- can this PR go out ahead of the backend changes or is it blocked until that field is available?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go ahead with this PR first, but let's leave a note for QA team that the issue about counting replies will be fixed once BE PR is deployed.

} else {
const activeRoute = encodeURIComponent(Navigation.getActiveRouteWithoutParams());
Navigation.navigate(ROUTES.MONEY_REQUEST_HOLD_REASON.getRoute(policy?.type ?? CONST.POLICY.TYPE.PERSONAL, transactionID, reportAction.childReportID ?? '', activeRoute));
}
};

/**
* Gets all transactions on an IOU report with a receipt
*/
Expand Down Expand Up @@ -6210,14 +6292,6 @@ function navigateToPrivateNotes(report: OnyxEntry<Report>, session: OnyxEntry<Se
Navigation.navigate(ROUTES.PRIVATE_NOTES_LIST.getRoute(report.reportID));
}

/**
* Check if Report has any held expenses
*/
function isHoldCreator(transaction: OnyxEntry<Transaction>, reportID: string): boolean {
const holdReportAction = ReportActionsUtils.getReportAction(reportID, `${transaction?.comment?.hold ?? ''}`);
return isActionCreator(holdReportAction);
}

/**
* Get all held transactions of a iouReport
*/
Expand Down Expand Up @@ -6721,6 +6795,7 @@ export {
canCreateTaskInReport,
canCurrentUserOpenReport,
canDeleteReportAction,
canHoldUnholdReportAction,
canEditFieldOfMoneyRequest,
canEditMoneyRequest,
canEditPolicyDescription,
Expand Down Expand Up @@ -6782,6 +6857,7 @@ export {
getOriginalReportID,
getOutstandingChildRequest,
getParentNavigationSubtitle,
getParentReportAction,
getParsedComment,
getParticipantAccountIDs,
getParticipants,
Expand Down Expand Up @@ -6940,6 +7016,7 @@ export {
shouldShowMerchantColumn,
isCurrentUserInvoiceReceiver,
isDraftReport,
changeMoneyRequestHoldStatus,
createDraftWorkspaceAndNavigateToConfirmationScreen,
};

Expand Down
13 changes: 3 additions & 10 deletions src/pages/home/ReportScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,6 @@ function isEmpty(report: OnyxTypes.Report): boolean {
return !Object.values(report).some((value) => value !== undefined && value !== '');
}

function getParentReportAction(parentReportActions: OnyxEntry<OnyxTypes.ReportActions>, parentReportActionID: string | undefined): OnyxEntry<OnyxTypes.ReportAction> {
if (!parentReportActions || !parentReportActionID) {
return null;
}
return parentReportActions[parentReportActionID ?? '0'];
}

function ReportScreen({
betas = [],
route,
Expand Down Expand Up @@ -261,7 +254,7 @@ function ReportScreen({
],
);

const parentReportAction = useMemo(() => getParentReportAction(parentReportActions, report?.parentReportActionID), [parentReportActions, report.parentReportActionID]);
const parentReportAction = useMemo(() => ReportUtils.getParentReportAction(parentReportActions, report?.parentReportActionID), [parentReportActions, report.parentReportActionID]);

const prevReport = usePrevious(report);
const prevUserLeavingStatus = usePrevious(userLeavingStatus);
Expand Down Expand Up @@ -813,8 +806,8 @@ export default withCurrentReportID(
},
})(
memo(ReportScreen, (prevProps, nextProps) => {
const prevParentReportAction = getParentReportAction(prevProps.parentReportActions, prevProps.report?.parentReportActionID);
const nextParentReportAction = getParentReportAction(nextProps.parentReportActions, nextProps.report?.parentReportActionID);
const prevParentReportAction = ReportUtils.getParentReportAction(prevProps.parentReportActions, prevProps.report?.parentReportActionID);
const nextParentReportAction = ReportUtils.getParentReportAction(nextProps.parentReportActions, nextProps.report?.parentReportActionID);
return (
prevProps.isSidebarLoaded === nextProps.isSidebarLoaded &&
lodashIsEqual(prevProps.sortedAllReportActions, nextProps.sortedAllReportActions) &&
Expand Down
34 changes: 34 additions & 0 deletions src/pages/home/report/ContextMenu/ContextMenuActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,40 @@ const ContextMenuActions: ContextMenuAction[] = [
},
getDescription: () => {},
},
{
isAnonymousAction: false,
textTranslateKey: 'iou.unholdExpense',
icon: Expensicons.Stopwatch,
shouldShow: (type, reportAction) =>
type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && ReportUtils.canEditReportAction(reportAction) && ReportUtils.canHoldUnholdReportAction(reportAction).canUnholdRequest,
onPress: (closePopover, {reportAction}) => {
if (closePopover) {
hideContextMenu(false, () => ReportUtils.changeMoneyRequestHoldStatus(reportAction));
return;
}

// No popover to hide, call changeMoneyRequestHoldStatus immediately
ReportUtils.changeMoneyRequestHoldStatus(reportAction);
},
getDescription: () => {},
},
{
isAnonymousAction: false,
textTranslateKey: 'iou.hold',
icon: Expensicons.Stopwatch,
shouldShow: (type, reportAction) =>
type === CONST.CONTEXT_MENU_TYPES.REPORT_ACTION && ReportUtils.canEditReportAction(reportAction) && ReportUtils.canHoldUnholdReportAction(reportAction).canHoldRequest,
onPress: (closePopover, {reportAction}) => {
if (closePopover) {
hideContextMenu(false, () => ReportUtils.changeMoneyRequestHoldStatus(reportAction));
return;
}

// No popover to hide, call changeMoneyRequestHoldStatus immediately
ReportUtils.changeMoneyRequestHoldStatus(reportAction);
},
getDescription: () => {},
},
{
isAnonymousAction: false,
textTranslateKey: 'reportActionContextMenu.joinThread',
Expand Down
Loading