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

[HOLD for payment 2025-01-22] [$500] Expense - Submit button appears for archived workspace chat if delayed submission is enabled #49169

Closed
3 of 6 tasks
IuliiaHerets opened this issue Sep 13, 2024 · 88 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Sep 13, 2024

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: 9.0.33-4
Reproducible in staging?: Y
**Reproducible in production?:**Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Sign up with a new Gmail account
  3. Create two workspaces
  4. Invite a member on the second workspace
  5. Enable workflows
  6. Enable delayed submission and make the frequency "Manually"
  7. Create an expense as the employee
  8. Delete the workspace as the admin

Expected Result:

Submit button doesn't show on the archived workspace chat on the expense

Actual Result:

Submit button shows on both the employee's and admin's archived workspace chat and clicking on the button results in an error

Additional Expected Behavior:

  1. When a workspace is deleted, no expense report should show Submit, Approve or Pay action buttons.
  2. Additionally Submit expense & Track expense options should be hidden from the expense report’s create menu (i.e the menu where Add attachment etc lives).

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6602198_1726224396008.bandicam_2024-09-13_13-35-07-112.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836147581047135142
  • Upwork Job ID: 1836147581047135142
  • Last Price Increase: 2024-10-24
  • Automatic offers:
    • FitseTLT | Contributor | 104580833
Issue OwnerCurrent Issue Owner: @johncschuster
@IuliiaHerets IuliiaHerets added Daily KSv2 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 @johncschuster (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.

@IuliiaHerets
Copy link
Author

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@abzokhattab
Copy link
Contributor

abzokhattab commented Sep 13, 2024

Edited by proposal-police: This proposal was edited at 2024-09-13 12:03:09 UTC.

Proposal

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

Submit button appears for archived workspace chat if delayed submission is en bled

What is the root cause of that problem?

we dont disable the submit button if the workspce is archived here

const shouldShowSubmitButton = isDraft && reimbursableSpend !== 0 && !allHavePendingRTERViolation;

const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage;

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

we should add another condition that the workspace is not deleted above using the isArchivedReport var

or it could be added to the isOpenExpenseReport directly (to follow the same concept as in the isOpenTaskReport function) :

    return isExpenseReport(report) && !isArchivedRoomWithID(report?.reportID) && report?.stateNum === CONST.REPORT.STATE_NUM.OPEN && report?.statusNum === CONST.REPORT.STATUS_NUM.OPEN;

POC

Screen.Recording.2024-09-13.at.14.21.13.mov

What alternative solutions did you explore? (Optional)

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Sep 13, 2024

Edited by proposal-police: This proposal was edited at 2024-10-17 10:27:58 UTC.

Proposal

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

Expense - Submit button appears for archived workspace chat if delayed submission is enabled

What is the root cause of that problem?

  1. For submit button
    We display the button here even if the report is archived
    here

    const shouldShowSubmitButton = isDraft && reimbursableSpend !== 0 && !hasAllPendingRTERViolations;

    And here
    const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage;

  2. For Approve button we are already hiding the button when workspace is archived here
    Here we check if the workspace is archived

    const isArchivedReport = ReportUtils.isArchivedRoom(iouReport, reportNameValuePairs);

Then we make shouldShowApproveButton button false if it is archived here

const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(iouReport, policy), [iouReport, policy]);

3. For Pay button we are already hiding the button if the workspace is archived
Here we check if workspace is achieved
const isChatReportArchived = ReportUtils.isArchivedRoom(chatReport, reportNameValuePairs);

And we make canIOUBePaid false if the workspace is archived
const canIOUBePaid = useMemo(() => getCanIOUBePaid(), [getCanIOUBePaid]);
const onlyShowPayElsewhere = useMemo(() => !canIOUBePaid && getCanIOUBePaid(true), [canIOUBePaid, getCanIOUBePaid]);
const shouldShowPayButton = isPaidAnimationRunning || canIOUBePaid || onlyShowPayElsewhere;
const shouldShowApproveButton = useMemo(() => IOU.canApproveIOU(iouReport, policy), [iouReport, policy]);
const shouldDisableApproveButton = shouldShowApproveButton && !ReportUtils.isAllowedToApproveExpenseReport(iouReport);
const shouldShowSettlementButton = (shouldShowPayButton || shouldShowApproveButton) && !showRTERViolationMessage && !shouldShowBrokenConnectionViolation;

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

  1. For submit button We should hide the button when report is archived
    here
    const shouldShowSubmitButton = isDraft && reimbursableSpend !== 0 && !hasAllPendingRTERViolations && !isArchivedReport;

And here

const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage && !isArchivedReport;

When a workspace is deleted, no expense report should show Submit, Approve or Pay action buttons.

For approve and pay button we don't have to make any change since we already hiding the buttons when the workspace is archived.

Additionally Submit expense & Track expense options should be hidden from the expense report’s create menu (i.e the menu where Add attachment etc lives).

Currently the composer with all the submit, track expense option is closed when workspace chat is archived

What alternative solutions did you explore? (Optional)

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Sep 13, 2024

Note

A kind reminder for C+:

The fist section of the first proposal's permalink edit that points to shouldShowSubmitButton comes after my proposal. Prior to that it pointed to shouldDisableApproveButton.

Screenshot of time of the first propsal Screenshot 2024-09-13 at 3 33 01 in the afternoon
Screenshot of time my proposal Screenshot 2024-09-13 at 3 17 27 in the afternoon

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Sep 13, 2024

Hi @abzokhattab. Writing few details takes time and make a difference in proposal submission time, and there seems to be a little difference between what you have suggested at first which is disabling the button (we dont disable the submit button ) by pointing a permalink to shouldDisableApproveButton, and what you suggested after the edit. That was why I raised the question. I have no ill intention towards you in saying that though🙏.

Thanks!

@dominictb
Copy link
Contributor

Proposal

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

  • Submit button shows on both the employee's and admin's archived workspace chat and clicking on the button results in an error

What is the root cause of that problem?

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

  • In general, we shouldn't show the submit button if the report is archived, pending deletion or encountered any error.

All of these conditions can be handled in:

function canUserPerformWriteAction(report: OnyxEntry<Report>) {

    const shouldShowSubmitButton = isDraft && reimbursableSpend !== 0 && !hasAllPendingRTERViolations && ReportUtils.canUserPerformWriteAction(moneyRequestReport);

and

const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage;

    const shouldShowSubmitButton = isOpenExpenseReport && reimbursableSpend !== 0 && !showRTERViolationMessage && ReportUtils.canUserPerformWriteAction(iouReport);

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick!

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Sep 17, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021836147581047135142

@melvin-bot melvin-bot bot changed the title Expense - Submit button appears for archived workspace chat if delayed submission is enabled [$250] Expense - Submit button appears for archived workspace chat if delayed submission is enabled Sep 17, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External)

@melvin-bot melvin-bot bot removed the Overdue label Sep 17, 2024
@Ollyws
Copy link
Contributor

Ollyws commented Sep 18, 2024

@etCoderDysto's proposal LGTM.
@dominictb Thanks for the proposal but I don't think it's substantially different enough to @etCoderDysto's to be fair to pick it over theirs.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 18, 2024

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Sep 18, 2024

Hi @abzokhattab. Writing few details takes time and make a difference in proposal submission time, and there seems to be a little difference between what you have suggested at first which is disabling the button (we dont disable the submit button ) by pointing a permalink to shouldDisableApproveButton, and what you suggested after the edit. That was why I raised the question. I have no ill intention towards you in saying that though🙏.

Thanks!

Before edit, it seems that you were suggesting disabling the button as I have commented here.

@trjExpensify
Copy link
Contributor

Hm, wait. Why is the expense report being archived in the OP video, @srikarparsi? 🤔 We should have stopped doing that, shouldn't we?

@trjExpensify
Copy link
Contributor

As for how to handle this case of an open report on a deleted workspace, let's tread carefully. On OldDot I believe we automatically move it to the submitter's "new" primary policy or something like that. The report action buttons shown are then applicable to whichever workspace it was moved to.

I think to handle this properly and holistically for all report states and setup scenarios, we're going to need better planning and probably the inclusion of the feature to let people manually move a report to a different workspace.

@srikarparsi
Copy link
Contributor

Why is the expense report being archived in the OP video, @srikarparsi? 🤔 We should have stopped doing that, shouldn't we?

Hmm, I think we wanted to stop closing the expense report when the workspace is deleted, but continue archiving it. If it wasn't archived, then transactions could be continued to be submitted on the report and people could continue to chat on the report (which is what I thought we didn't want?). What do you think?

I think to handle this properly and holistically for all report states and setup scenarios, we're going to need better planning and probably the inclusion of the feature to let people manually move a report to a different workspace.

Yeah I think I agree

@trjExpensify
Copy link
Contributor

Okay, I can see why we kept doing that for now because of otherwise having the ability to add expenses to the report, but we want to continue to allow people to chat on an expense report to discuss what to do about outstanding reports - and more importantly, we need to stop the actions of submit/approve/pay which all will fail - so I don't think we should archive them. Thinking through how we should handle this broadly and consistently:

When a workspace is deleted

  1. All of Submit, Approve and Pay action buttons are hidden for all participants of the report, as those actions are no longer available on this now deleted workspace.
  2. The expense report is not archived to retain the ability to chat on it, but we remove the Submit expense & Track expense options from the expense report's create menu (i.e the menu where Add attachment etc lives).
  3. In the report's details page, we add an option row to Move report
    a. Submitter: The option row is visible on open and processing reports.
    b. Admin: The option row is visible on open, processing, approved and closed reports. (reimbursed reports can't be moved because they are a record of payment made from this company workspace).
    c. Everyone: You should not be able to move a report that has been previously exported to a connected accounting system, so the option row shouldn't show on exported reports.
  4. When Move report is pressed it opens a filtered participant selector:
    a. Submitter: all workspaces they have access to + their selfDM*(note on this below)
    b. Admins: all workspaces they are an admin of. If they choose a workspace that the submitter is not a member of, we invite the submitter to the workspace and do the following:
    • Set that workspace owner as the member's approver if it's a workspace with "Advanced approvals", otherwise whoever is the approver set for "Submit & close" and "Submit & approve".
    • Move the report to the open state if the workspace has delayed submission enabled. A Submit button is now available on the report.
    • Move the report to the processing state if the workspace has delayed submission disabled (A.k.a "instant submit"). An Approve || Pay button is now visible depending on the workspace settings.
  5. When the expense report is moved to somewhere else, we add a system message from the actor who moved the expense report to capture the action in the audit history that reads:
    • "moved this report to the $workspaceChatReportLink workspace" (code, it exists for the submit > pay with biz bank account flow)
    • "moved this report to the $selfDMChatReportLink" (doesn't exist yet)

*Q: If the submitter chooses their selfDM here in 4.a how should we handle it? All expenses in the selfDM are unreported as it stands I believe, so would we be effectively deleting the expense report to unreport the expenses and show them in the selfDM where they can then be submitted on to wherever they want to put them via the actionable whisper message?

How does that sound? Let's align, and then I can create mocks for the relevant parts above. CC: @JmillsExpensify

@srikarparsi
Copy link
Contributor

The expense report is not archived to retain the ability to chat on it

Will reimbursed expenses not be archived as well? I feel like this distinction would be kind of weird, since you would no longer be able to chat on chat threads or task threads but would be able to continue chatting on expense threads after a workspace is deleted. I'm also not sure if allowing users to chat on these expense threads is useful since they have to be moved to a different report to be paid anyway (where they can also chat).

But I agree with everything else!

@trjExpensify
Copy link
Contributor

Right, if the expense report isn't archived neither are the expenses on them. People retain the ability to comment on expense reports after a workspace is deleted today on OldDot because ultimately it can be moved to another workspace to progress to resolution, and for example, if the workspace was deleted and they still need to be processed to paid, the conversation can be had. So the suggestion is to keep that behaviour intact.

@FitseTLT
Copy link
Contributor

There is a pending BE pr. As soon as it is merged we will have a final test and record snapshots 👍

@johncschuster
Copy link
Contributor

That's great! Thanks, @FitseTLT!

@johncschuster
Copy link
Contributor

@FitseTLT and @Ollyws, I will be OOO starting December 23 and will be returning January 6th. A handful of folks on the BZ team will be online for a few days in between the 25th and the 1st, but we'll be operating with a skeleton crew. I will be issuing my payments when I return on January 6th. If the PR linked passes the regression threshold before I return and you would like the payment issued sooner, please post this issue in #expensify-open-source and someone on the team will jump in.

Thank you!

@johncschuster
Copy link
Contributor

Hey everyone! I'm back from break and will continue to push this forward 👋

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 6, 2025
@johncschuster
Copy link
Contributor

Ok! It looks like the BE PR from above was merged into Auth about two weeks ago. That means we're waiting on tests and snapshots, right @FitseTLT?

@FitseTLT
Copy link
Contributor

FitseTLT commented Jan 7, 2025

Ok! It looks like the BE PR from above was merged into Auth about two weeks ago. That means we're waiting on tests and snapshots, right @FitseTLT?

Yep correct on C+ review stage

@johncschuster
Copy link
Contributor

Great! I'll leave it to you then. Thanks, @FitseTLT!

@johncschuster
Copy link
Contributor

Looks like this #52183 is almost finished, which, I think is our only blocker.

@johncschuster
Copy link
Contributor

#52183 has been deployed to staging 🥳

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 15, 2025
@melvin-bot melvin-bot bot changed the title [$500] Expense - Submit button appears for archived workspace chat if delayed submission is enabled [HOLD for payment 2025-01-22] [$500] Expense - Submit button appears for archived workspace chat if delayed submission is enabled Jan 15, 2025
Copy link

melvin-bot bot commented Jan 15, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 15, 2025
Copy link

melvin-bot bot commented Jan 15, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.85-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-01-22. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jan 15, 2025

@Ollyws @johncschuster @Ollyws The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

@johncschuster
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment:

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

@johncschuster
Copy link
Contributor

Payment Summary

Contributor: @FitseTLT paid $500 via Upwork
Contributor+: @Ollyws owed $500 via NewDot

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 22, 2025
@johncschuster
Copy link
Contributor

@Ollyws can you complete the BZ Checklist above?

@johncschuster
Copy link
Contributor

Payment has been issued to @FitseTLT. Once the checklist is complete, please go ahead and request payment via ND, @Ollyws. Thank you!

@Ollyws
Copy link
Contributor

Ollyws commented Jan 22, 2025

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on both staging and production
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment:

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

1. Create a workspace from User A and enable workflow and manual submission
2. Invite User B
3. Submit an expense report from User B's account
4. Remove User B from the workspace using User A's account
5. From User B side open the archive workspace chat
6. Open the expense report you submitted in (3)
7. Verify that composer is visible (not hidden) and you can comment, all context menu work correctly like reply in thread, react.
8. Verify that you can edit the expense fields like date, receipt, description, merchant, amount.
9. Verify that you cannot submit (submit button doesn't appear)
10. Verify that you cannot create expense on the archived expense report.

Do we agree 👍 or 👎

@Ollyws
Copy link
Contributor

Ollyws commented Jan 22, 2025

Requested in ND.

@johncschuster
Copy link
Contributor

Thanks, @Ollyws! We're good to close this up!

@garrettmknight
Copy link
Contributor

$500 approved for @Ollyws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants