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

[No QA] Bootstrap secondary actions getter #57678

13 changes: 13 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1116,6 +1116,19 @@ const CONST = {
MIN_INITIAL_REPORT_ACTION_COUNT: 15,
UNREPORTED_REPORTID: '0',
SPLIT_REPORTID: '-2',
SECONDARY_ACTIONS: {
SUBMIT: 'submit',
APPROVE: 'approve',
UNAPPROVE: 'unapprove',
CANCEL_PAYMENT: 'cancelPayment',
EXPORT_TO_ACCOUNTING: 'exportToAccounting',
MARK_AS_EXPORTED: 'markAsExported',
HOLD: 'hold',
DOWNLOAD: 'download',
CHANGE_WORKSPACE: 'changeWorkspace',
VIEW_DETAILS: 'viewDetails',
DELETE: 'delete',
},
PRIMARY_ACTIONS: {
SUBMIT: 'submit',
APPROVE: 'approve',
Expand Down
5 changes: 5 additions & 0 deletions src/libs/ReportActionsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2182,6 +2182,10 @@ function getReportActionsLength() {
return Object.keys(allReportActions ?? {}).length;
}

function getReportActions(report: Report) {
return allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.reportID}`];
}

function wasActionCreatedWhileOffline(action: ReportAction, isOffline: boolean, lastOfflineAt: Date | undefined, lastOnlineAt: Date | undefined, locale: Locale): boolean {
// The user has never gone offline or never come back online
if (!lastOfflineAt || !lastOnlineAt) {
Expand Down Expand Up @@ -2333,6 +2337,7 @@ export {
getWorkspaceTagUpdateMessage,
getWorkspaceReportFieldUpdateMessage,
getWorkspaceReportFieldDeleteMessage,
getReportActions,
};

export type {LastVisibleMessage};
332 changes: 332 additions & 0 deletions src/libs/ReportSecondaryActionUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,332 @@
import type {OnyxCollection} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import CONST from '@src/CONST';
import type {Policy, Report, Transaction, TransactionViolation} from '@src/types/onyx';
import {isApprover as isApprovedMember} from './actions/Policy/Member';
import {getCurrentUserAccountID} from './actions/Report';
import {arePaymentsEnabled, getCorrectedAutoReportingFrequency, hasAccountingConnections, hasNoPolicyOtherThanPersonalType, isAutoSyncEnabled, isPrefferedExporter} from './PolicyUtils';
import {getReportActions} from './ReportActionsUtils';
import {
isClosedReport,
isCurrentUserSubmitter,
isExpenseReport,
isExported,
isInvoiceReport,
isIOUReport,
isOpenReport,
isPayer,
isProcessingReport,
isReportApproved,
isReportManager,
isSettled,
} from './ReportUtils';
import {getSession} from './SessionUtils';
import {allHavePendingRTERViolation, isDuplicate, isOnHold as isOnHoldTransactionUtils, shouldShowBrokenConnectionViolationForMultipleTransactions} from './TransactionUtils';

function isSubmitAction(report: Report, policy: Policy): boolean {
const isExpense = isExpenseReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const isExpense = isExpenseReport(report);
const isExpenseReport = isExpenseReportReportUtils(report);

Please have a look at this slack post for details about when variable and util names are the same


if (!isExpense) {
return false;
}

const isSubmitter = isCurrentUserSubmitter(report.reportID);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

const isApprover = isApprovedMember(policy, getCurrentUserAccountID());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use existing isApprover util 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.

I'm using existing isApprover from libs/actions/Policy/Member


if (!isSubmitter && !isApprover) {
return false;
}

const isOpen = isOpenReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^, please name the report as isOpenReport and adjust the util accordingly


if (!isOpen) {
return false;
}

const autoReportingFrequency = getCorrectedAutoReportingFrequency(policy);

const isScheduledSubmitEnabled = policy?.harvesting?.enabled && autoReportingFrequency !== CONST.POLICY.AUTO_REPORTING_FREQUENCIES.MANUAL;

return !!isScheduledSubmitEnabled;
}

function isApproveAction(report: Report, policy: Policy, reportTransactions: Transaction[], violations: OnyxCollection<TransactionViolation[]>): boolean {
const isExpense = isExpenseReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

const isApprover = isApprovedMember(policy, getCurrentUserAccountID());
const isProcessing = isProcessingReport(report);
const hasDuplicates = reportTransactions.some((transaction) => isDuplicate(transaction.transactionID));

if (isExpense && isApprover && isProcessing && hasDuplicates) {
return true;
}

const transactionIDs = reportTransactions.map((t) => t.transactionID);

const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs, violations);

if (hasAllPendingRTERViolations) {
return true;
}

const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be compared against the current user right? , @luacmartins is this correct? i'm not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

policy.role is specific to the current user, so this is comparing against the current user


const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationForMultipleTransactions(transactionIDs, report, policy, violations);

const userControllsReport = isApprover || isAdmin;
Copy link
Contributor

Choose a reason for hiding this comment

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

userControllsReport can you think of any better name for this? c.c. @luacmartins

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 this name is fine and we're already using it in the primary actions. Let's just fix the typo userControlsReport

return userControllsReport && shouldShowBrokenConnectionViolation;
}

function isUnapproveAction(report: Report, policy: Policy): boolean {
const isExpense = isExpenseReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

const isApprover = isApprovedMember(policy, getCurrentUserAccountID());
const isApproved = isReportApproved({report});
Copy link
Contributor

Choose a reason for hiding this comment

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

please name this as isReportApproved, and rename util accordingly


return isExpense && isApprover && isApproved;
}

function isCancelPaymentAction(report: Report): boolean {
const isExpense = isExpenseReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^


if (!isExpense) {
return false;
}

const isPaidElsewhere = report.stateNum === CONST.REPORT.STATE_NUM.APPROVED && report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED;
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename this as isReportPaidElsewhere


if (isPaidElsewhere) {
return true;
}

const isPaymentProcessing = true; // TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luacmartins I'm not sure how to determine this value

Copy link
Contributor

Choose a reason for hiding this comment

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

We can check if isSettled === true here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@luacmartins but the payment is processing and not settled ? do we assume processing payments as settled ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luacmartins current condition for showing cancel payment button is this:

const shouldShowCancelPaymentButton = caseID === CASES.MONEY_REPORT && canCancelPayment(moneyRequestReport, session);

And canCancelPayment implementation looks like this:

App/src/libs/actions/IOU.ts

Lines 8287 to 8289 in 237759d

function canCancelPayment(iouReport: OnyxEntry<OnyxTypes.Report>, session: OnyxEntry<OnyxTypes.Session>) {
return isPayerReportUtils(session, iouReport) && (isSettled(iouReport) || iouReport?.isWaitingOnBankAccount) && isExpenseReport(iouReport);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@luacmartins but the payment is processing and not settled ? do we assume processing payments as settled ?

Well, in this case the processing state is given by the combination of isSettled and is past the NACHA cutoff (at which point we can't cancel it anymore. If we paid the report and we're within the nacha cutoff window, we can still cancel it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jnowakow yes, we use isSettled in the current condition. I think the isWaitingOnBankAccount condition is redundant since isSettled also takes that into account. The new change here is adding the nacha cutoff.

const hasDailyNachaCutoffPassed = false; // TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luacmartins I'm not sure how to determine this value

Copy link
Contributor

@luacmartins luacmartins Mar 4, 2025

Choose a reason for hiding this comment

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

The cutoff is 23:45 PM UTC the day the payment was made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm so here I have to find payment transaction and get it's creation date and check if it's after 23:45 on that day?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can probably use getIOUActionForReportID to get the IOU action and check if it's a pay action and then use the reportAction date

return isPaymentProcessing && !hasDailyNachaCutoffPassed;
}

function isExportAction(report: Report, policy: Policy): boolean {
const isInvoice = isInvoiceReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

const isSender = isCurrentUserSubmitter(report.reportID);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^


if (isInvoice && isSender) {
return true;
}

const isExpense = isExpenseReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^


const hasAccountingConnection = hasAccountingConnections(policy);

if (!isExpense || !hasAccountingConnection) {
return false;
}

const isApproved = isReportApproved({report});
const isReportPayer = isPayer(getSession(), report, false, policy);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as ^

const isPaymentsEnabled = arePaymentsEnabled(policy);
const isClosed = isClosedReport(report);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be names as isReportClosed


if (isReportPayer && isPaymentsEnabled && (isApproved || isClosed)) {
return true;
}

const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this check is correct, same @luacmartins we should check this with the current user right?

Copy link
Contributor

Choose a reason for hiding this comment

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

policy.role is specific to the current user, so this is comparing against the current user

const isReimbursed = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED;
Copy link
Contributor

Choose a reason for hiding this comment

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

isReportReimbursed please

const syncEnabled = isAutoSyncEnabled(policy);
const isReportExported = isExported(getReportActions(report));
const isFinished = isApproved || isReimbursed || isClosed;
Copy link
Contributor

Choose a reason for hiding this comment

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

isFinished doesn't really explain what this is used for, maybe something more meaningful?


return isAdmin && isFinished && syncEnabled && !isReportExported;
}

function isMarkAsExportedAction(report: Report, policy: Policy): boolean {
const isInvoice = isInvoiceReport(report);
const isSender = isCurrentUserSubmitter(report.reportID);

if (isInvoice && isSender) {
return true;
}

const isExpense = isExpenseReport(report);

if (!isExpense) {
return false;
}

const isReportPayer = isPayer(getSession(), report, false, policy);
const isPaymentsEnabled = arePaymentsEnabled(policy);
const isApproved = isReportApproved({report});
const isClosed = isClosedReport(report);
const isClosedOrApproved = isClosed || isApproved;

if (isReportPayer && isPaymentsEnabled && isClosedOrApproved) {
return true;
}

const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
const isReimbursed = isSettled(report);
const hasAccountingConnection = hasAccountingConnections(policy);
const syncEnabled = isAutoSyncEnabled(policy);
const isFinished = isClosedOrApproved || isReimbursed;

if (isAdmin && isFinished && hasAccountingConnection && syncEnabled) {
return true;
}

const isExporter = isPrefferedExporter(policy);

if (isExporter && isFinished && hasAccountingConnection && !syncEnabled) {
return true;
}

return false;
}

function isHoldAction(report: Report, reportTransactions: Transaction[]): boolean {
const isExpense = isExpenseReport(report);

if (!isExpense) {
return false;
}

const isOnHold = reportTransactions.some(isOnHoldTransactionUtils);

if (isOnHold) {
return false;
}

const isOpen = isOpenReport(report);
const isProcessing = isProcessingReport(report);
const isApproved = isReportApproved({report});

return isOpen || isProcessing || isApproved;
}

function isChangeWorkspaceAction(report: Report, policy: Policy, reportTransactions: Transaction[], violations: OnyxCollection<TransactionViolation[]>): boolean {
const isExpense = isExpenseReport(report);
const isSubmitter = isCurrentUserSubmitter(report.reportID);
const areWorkflowsEnabled = policy.areWorkflowsEnabled;
const isClosed = isClosedReport(report);

if (isExpense && isSubmitter && !areWorkflowsEnabled && isClosed) {
return true;
}

const isOpen = isOpenReport(report);
const isProcessing = isProcessingReport(report);

if (isSubmitter && (isOpen || isProcessing)) {
return true;
}

const isApprover = isApprovedMember(policy, getCurrentUserAccountID());

if (isApprover && isProcessing) {
return true;
}

const isReportPayer = isPayer(getSession(), report, false, policy);
const isApproved = isReportApproved({report});

if (isReportPayer && (isApproved || isClosed)) {
return true;
}

const isAdmin = policy?.role === CONST.POLICY.ROLE.ADMIN;
const isReimbursed = isSettled(report);
const transactionIDs = reportTransactions.map((t) => t.transactionID);
const hasAllPendingRTERViolations = allHavePendingRTERViolation(transactionIDs, violations);

const shouldShowBrokenConnectionViolation = shouldShowBrokenConnectionViolationForMultipleTransactions(transactionIDs, report, policy, violations);

const userControlsReport = isSubmitter || isApprover || isAdmin;
const hasReceiptMatchViolation = hasAllPendingRTERViolations || (userControlsReport && shouldShowBrokenConnectionViolation);
const isReportExported = isExported(getReportActions(report));

if (isAdmin && ((!isReportExported && (isApproved || isReimbursed || isClosed)) || hasReceiptMatchViolation)) {
return true;
}

const isIOU = isIOUReport(report);
const hasOnlyPersonalWorkspace = hasNoPolicyOtherThanPersonalType();
const isReceiver = isReportManager(report);
if (isIOU && !hasOnlyPersonalWorkspace && isReceiver && isReimbursed) {
return true;
}

const isSender = isCurrentUserSubmitter(report.reportID);
// it's already satisified in line 215
if (isSender && isProcessing) {
return true;
}

return false;
}

function isDeleteAction(report: Report): boolean {
const isExpense = isExpenseReport(report);

if (!isExpense) {
return false;
}

const isSubmitter = isCurrentUserSubmitter(report.reportID);

if (!isSubmitter) {
return false;
}

const isOpen = isOpenReport(report);
const isProcessing = isProcessingReport(report);
const isApproved = isReportApproved({report});

return isOpen || isProcessing || isApproved;
}

function getSecondaryAction(
Copy link
Contributor

Choose a reason for hiding this comment

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

@jnowakow shouldn't we just return if one of the conditions below are true? will there be a case where report will have many actions ? checking for each action isn't optimal right, unless i'm missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allgandalf assumption is that there will be only one primary action and it was already implemented here. There can be many secondary actions so we're creating array of them as stated in design doc.
It will look like this:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. We can have many secondary actions, which is what we're implementing in this PR.

report: Report,
policy: Policy,
reportTransactions: Transaction[],
violations: OnyxCollection<TransactionViolation[]>,
): Array<ValueOf<typeof CONST.REPORT.SECONDARY_ACTIONS>> {
const options: Array<ValueOf<typeof CONST.REPORT.SECONDARY_ACTIONS>> = [];
options.push(CONST.REPORT.SECONDARY_ACTIONS.DOWNLOAD);
options.push(CONST.REPORT.SECONDARY_ACTIONS.VIEW_DETAILS);

if (isSubmitAction(report, policy)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.SUBMIT);
}

if (isApproveAction(report, policy, reportTransactions, violations)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.APPROVE);
}

if (isUnapproveAction(report, policy)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.UNAPPROVE);
}

if (isCancelPaymentAction(report)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.CANCEL_PAYMENT);
}

if (isExportAction(report, policy)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.EXPORT_TO_ACCOUNTING);
}

if (isMarkAsExportedAction(report, policy)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.MARK_AS_EXPORTED);
}

if (isHoldAction(report, reportTransactions)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.HOLD);
}

if (isChangeWorkspaceAction(report, policy, reportTransactions, violations)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.CHANGE_WORKSPACE);
}

if (isDeleteAction(report)) {
options.push(CONST.REPORT.SECONDARY_ACTIONS.DELETE);
}

return options;
}

export default getSecondaryAction;
2 changes: 1 addition & 1 deletion src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,7 @@ function isSettled(reportOrID: OnyxInputOrEntry<Report> | SearchReport | string
return false;
}

if (isEmptyObject(report) || report.isWaitingOnBankAccount) {
if (isEmptyObject(report)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jnowakow i don't understand the change here? why do we need to remove this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is contradiction in this function. Discussion is here

return false;
}

Expand Down
Loading
Loading