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

Ensure the Reports Bulk action correctly does not allow deleting CC card transactions #59264

Open
8 tasks done
mountiny opened this issue Mar 27, 2025 · 7 comments
Open
8 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Reviewing Has a PR in review Weekly KSv2

Comments

@mountiny
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number:
Reproducible in staging?:
Reproducible in production?:
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @JmillsExpensify
Slack conversation (hyperlinked to channel name):

Action Performed:

Break down in numbered steps

  1. Import CC card transactions so users are not allowed to delete them
  2. Go to Reports page
  3. Bulk select also the transactions that cannot be deleted

Expected Result:

Describe what you think should've happened

The Delete option should not be visible if one of the selected transactions cannot be deleted

Actual Result:

Describe what actually happened

Even if you select all or just one cc transaction that does not have the CONST.TRANSACTION.LIABILITY_TYPE.ALLOW liability type, we show the option to delete the transaction (but it cannot be deleted in the backend)

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

@mountiny mountiny added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Mar 27, 2025
Copy link

melvin-bot bot commented Mar 27, 2025

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@Shahidullah-Muffakir
Copy link
Contributor

Shahidullah-Muffakir commented Mar 27, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-03-27 22:44:33 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

delete btn is showing for CC transaction with no deletion right

What is the root cause of that problem?

we are not checking the deletion right here based on Liability Type:

const shouldShowDeleteOption = !isOffline && selectedTransactionsKeys.every((id) => selectedTransactions[id].canDelete);

Like we check in the report details page:
const isCardTransactionCanBeDeleted = canDeleteCardTransactionByLiabilityType(iouTransactionID);

App/src/libs/ReportUtils.ts

Lines 2260 to 2267 in 845811a

function canDeleteCardTransactionByLiabilityType(iouTransactionID?: string): boolean {
const transaction = getTransaction(iouTransactionID ?? CONST.DEFAULT_NUMBER_ID);
const isCardTransaction = isCardTransactionTransactionUtils(transaction);
if (!isCardTransaction) {
return true;
}
return transaction?.comment?.liabilityType === CONST.TRANSACTION.LIABILITY_TYPE.ALLOW;
}

What changes do you think we should make in order to solve the problem?

Update that condition in the SearchPage , to check for the Liability type as:

const shouldShowDeleteOption = !isOffline && selectedTransactionsKeys.every((id) => selectedTransactions[id].canDelete);

        const shouldShowDeleteOption = !isOffline && selectedTransactionsKeys.every((id) => selectedTransactions[id].canDelete && canDeleteCardTransactionByLiabilityType(id));

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

@mountiny
Copy link
Contributor Author

@Shahidullah-Muffakir Does the selectedTransactions have the liability type information?

@Shahidullah-Muffakir
Copy link
Contributor

@Shahidullah-Muffakir Does the selectedTransactions have the liability type information?

I think we only need the transactionID, as we get the transaction details inside the canDeleteCardTransactionByLiabilityType from onyx

@mountiny
Copy link
Contributor Author

I think we only need the transactionID, as we get the transaction details inside the canDeleteCardTransactionByLiabilityType from onyx

Yes, but what if the transaction is not in onyx, we need to rely on what the Search command returned

@Shahidullah-Muffakir
Copy link
Contributor

Shahidullah-Muffakir commented Mar 27, 2025

Thank you, then it seems directly updating this condition would be better, as it has the transaction data from the search results:

canDelete: transaction.canDelete,

canDelete: transaction.canDelete,

canDelete: item.canDelete,

and we will map the comment property here:

function mapTransactionItemToSelectedEntry(item: TransactionListItemType): [string, SelectedTransactionInfo] {
return [
item.keyForList,
{
isSelected: true,
canDelete: item.canDelete,
canHold: item.canHold,
isHeld: isOnHold(item),

@allgandalf
Copy link
Contributor

we found an odd case, working on it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

5 participants