-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Changes from 6 commits
be5bcfa
2e36d00
d8fc20b
9f330e4
0fac245
3b2e151
ed98b40
051180b
7946f0c
f33575d
2df4f55
721e407
41276b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
*/ | ||
|
@@ -2707,6 +2715,80 @@ function canEditReportAction(reportAction: OnyxEntry<ReportAction>): boolean { | |
); | ||
} | ||
|
||
function getParentReportAction(parentReportActions: OnyxEntry<ReportActions>, parentReportActionID: string | undefined): OnyxEntry<ReportAction> { | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think thats fine, |
||
|
||
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 ?? ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In here, you updated the hold expense with its There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems that we will need BE changes here cc @robertjchen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hungvu193, thanks a lot for looking into this, I agree with you. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
*/ | ||
|
@@ -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 | ||
*/ | ||
|
@@ -6721,6 +6795,7 @@ export { | |
canCreateTaskInReport, | ||
canCurrentUserOpenReport, | ||
canDeleteReportAction, | ||
canHoldUnholdReportAction, | ||
canEditFieldOfMoneyRequest, | ||
canEditMoneyRequest, | ||
canEditPolicyDescription, | ||
|
@@ -6782,6 +6857,7 @@ export { | |
getOriginalReportID, | ||
getOutstandingChildRequest, | ||
getParentNavigationSubtitle, | ||
getParentReportAction, | ||
getParsedComment, | ||
getParticipantAccountIDs, | ||
getParticipants, | ||
|
@@ -6940,6 +7016,7 @@ export { | |
shouldShowMerchantColumn, | ||
isCurrentUserInvoiceReceiver, | ||
isDraftReport, | ||
changeMoneyRequestHoldStatus, | ||
createDraftWorkspaceAndNavigateToConfirmationScreen, | ||
}; | ||
|
||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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? 🤔There was a problem hiding this comment.
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 usingReportActionsUtils.getParentReportAction(report);
to getparentReportActions
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.App/src/pages/home/ReportScreen.tsx
Lines 105 to 111 in 4e4715f
@tgolen, can you pls confirm if we are going to replace
getParentReportAction
or not? I don't see any open issue/PR for that.There was a problem hiding this comment.
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
inReportUtils
, I'm okay with using it until we have a plan to remove it all at once.There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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 🙇