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

Expense - Error when deleting expense when Delay submissions is enabled #49157

Closed
6 tasks done
IuliiaHerets opened this issue Sep 13, 2024 · 19 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.34-2
Reproducible in staging?: Y
Reproducible in production?: N/A - no delete option in Admin side
Issue was found when executing this PR: #48998
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  • Admin and employee are in the same workspace.
  • Delay submissions is enabled in Worflows page.
  1. Go to staging.new.expensify.com
  2. [Employee] Submit an expense in the workspace chat, but do not click the Submit button.
  3. [Admin] Go to transaction thread in the workspace chat.
  4. [Admin] Click on the report header.
  5. [Admin] Click Delete expense.
  6. [Admin] Delete the expense.

Expected Result:

The expense should be deleted because delete option is present.
On prod, delete option is not available.

Actual Result:

The expense is not deleted and error shows up.
Also, when selecting the same expense in Search, there is no delete option from the dropdown but it is present in the transaction thread.

Workaround:

Unknown

Platforms:

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6601953_1726210847476.20240913_145443.mp4

View all open jobs on GitHub

@IuliiaHerets IuliiaHerets added DeployBlockerCash This issue or pull request should block deployment Bug Something is broken. Auto assigns a BugZero manager. labels Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

Triggered auto assignment to @flodnv (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Sep 13, 2024

Triggered auto assignment to @trjExpensify (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.

@melvin-bot melvin-bot bot added the Daily KSv2 label Sep 13, 2024
@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Sep 13, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@daledah
Copy link
Contributor

daledah commented Sep 13, 2024

Proposal

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

The expense is not deleted and error shows up.
Also, when selecting the same expense in Search, there is no delete option from the dropdown but it is present in the transaction thread.

What is the root cause of that problem?

The expense is not deleted and error shows up.

In

App/src/libs/ReportUtils.ts

Lines 1668 to 1683 in 00bc167

function canAddOrDeleteTransactions(moneyRequestReport: OnyxEntry<Report>): boolean {
if (!isMoneyRequestReport(moneyRequestReport)) {
return false;
}
const policy = getPolicy(moneyRequestReport?.policyID);
if (PolicyUtils.isInstantSubmitEnabled(policy) && PolicyUtils.isSubmitAndClose(policy) && hasOnlyNonReimbursableTransactions(moneyRequestReport?.reportID)) {
return false;
}
if (isReportApproved(moneyRequestReport) || isSettled(moneyRequestReport?.reportID)) {
return false;
}
return true;
}

We see that when WS has Delayed submission, PolicyUtils.isIntantSubmitEnabled is false, the transaction is not approved or settled, so ReportUtils.canDeleteTransaction returns true, results in

const canDeleteRequest = (isActionOwner || isPolicyAdmin) && (ReportUtils.canDeleteTransaction(moneyRequestReport) || isSelfDMTrackExpenseReport) && !isDeletedParentAction;

becomes true, the Delete Button is shown.

Also, when selecting the same expense in Search, there is no delete option from the dropdown but it is present in the transaction thread.

This is expected, as in

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

transaction doesn't have canDelete in Onyx, so shouldShowDeleteOption is false

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

Update

if (PolicyUtils.isInstantSubmitEnabled(policy) && PolicyUtils.isSubmitAndClose(policy) && hasOnlyNonReimbursableTransactions(moneyRequestReport?.reportID)) {

to

if (!PolicyUtils.isInstantSubmitEnabled(policy) && PolicyUtils.isSubmitAndClose(policy) && hasOnlyNonReimbursableTransactions(moneyRequestReport?.reportID)) {

So that the Delete options is hidden when WS has delayed submission

What alternative solutions did you explore? (Optional)

@flodnv
Copy link
Contributor

flodnv commented Sep 13, 2024

Hmmm, this solution implies that the owner also won't be able to delete the transaction, which I think is wrong?

@daledah
Copy link
Contributor

daledah commented Sep 13, 2024

@flodnv I don't really understand the BE logic, in #46762 (comment) I see that admin can delete employee's expenses, but not when the WS has delayed submission? What's the exact behavior here? I will update my proposal when I have the correct behavior.

@luacmartins
Copy link
Contributor

Logs FYI the email provided in the OP seems incorrect, the correct one should be applausetester+kh11090001@applause.expensifail.com

Throw ExpException - 8478b963e0cf0e66800a56c0d4ea36bc ~~ message: '401 Unauthorized' exceptionMessage: 'Auth DeleteMoneyRequest returned an error' exceptionFile: '/git/releases/expensify.com/a07a885/lib/Auth.php' exceptionLine: '128' exceptionCode: '401'

So this means we prevent admins from deleted unsubmitted expenses, which I think is correct. So we should update the logic to hide the button for the admin as we have in prod.

@Pujan92
Copy link
Contributor

Pujan92 commented Sep 13, 2024

@daledah I think we need to add an extra condition of report is in the open state or not with isPolicyAdmin.

const canDeleteRequest = (isActionOwner || isPolicyAdmin) && (ReportUtils.canDeleteTransaction(moneyRequestReport) || isSelfDMTrackExpenseReport) && !isDeletedParentAction;

(isPolicyAdmin && !ReportUtils.isOpenExpenseReport(moneyRequestReport))

@trjExpensify
Copy link
Contributor

Following the blame, pulling this one out to #wave-control for a second: https://expensify.slack.com/archives/C06ML6X0W9L/p1726235265572009

@trjExpensify
Copy link
Contributor

So I'm not sure where this came from, wires must have been crossed, but to confirm on the expected behaviour...

Admins should not be able to delete other people's expenses in NewDot. They can delete their own, but for others, they can put them on hold, not delete them. So please can we revert whatever made that possible in all places;

  • Search page
  • Request preview component action menu
  • RHP details of an expense
  • Anywhere else I've forgotten, but I think that's it! 😅

Thanks!

@luacmartins
Copy link
Contributor

Probably this PR? #48998

@luacmartins
Copy link
Contributor

I think we should revert #48998 and then work on a backend fix to prevent admins from being able to delete someone else's expenses.

@luacmartins
Copy link
Contributor

Revert here #49178

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Sep 13, 2024
@Pujan92
Copy link
Contributor

Pujan92 commented Sep 13, 2024

Probably this PR? #48998

Yes @luacmartins, bcoz as per the confirmation by @dylanexpensify we allowed admin to delete the expenses. But as it seems to be wrong behavior we need to revert the PR as you suggested.

@luacmartins
Copy link
Contributor

luacmartins commented Sep 13, 2024

I looked at the backend logic and I'm not sure why the search page would return true in that case, since the DeleteMoneyRequest command uses the exact same predicate to check eligibility.

@luacmartins
Copy link
Contributor

Regardless, once the CP is done this is no longer an App blocker since the backend logic hasn't changed in a while

@flodnv
Copy link
Contributor

flodnv commented Sep 13, 2024

Hmmm IIRC, admins can delete (remove) expenses from reports in order to not approve them -- of course, that was before we implemented "hold".

@trjExpensify
Copy link
Contributor

Yeah, we don't have that concept in NewDot.

@luacmartins
Copy link
Contributor

This is fixed on staging. The Search part is still an issue, so I'll keep the original issue open since we still need to address that.

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Sep 13, 2024
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. Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants