Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$250] Expense - Submit button appears for archived workspace chat if delayed submission is enabled #49169

Open IuliiaHerets opened 6 days ago

IuliiaHerets commented 6 days ago

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.33-4 Reproducible in staging?: Y Reproducible in production?:Y Email or phone of affected tester (no customers): nathanmulugetatesting+1469@gmail.com 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

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/bca2d8f4-6b61-49d7-a214-ae98828881ed

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-09-17
Issue OwnerCurrent Issue Owner: @Ollyws
melvin-bot[bot] commented 6 days ago

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 commented 6 days ago

@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 commented 6 days ago

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

https://github.com/Expensify/App/blob/eb5a569942af5f4117a548d9ed683046e11f9012/src/components/MoneyReportHeader.tsx#L126 https://github.com/Expensify/App/blob/4817f9dada5f97392df81037097502039704ca5e/src/components/ReportActionItem/ReportPreview.tsx#L185

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

https://github.com/user-attachments/assets/59ac5207-295f-4c58-9846-e6dc98f0e09b

What alternative solutions did you explore? (Optional)

etCoderDysto commented 6 days ago

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?

We display when report is archived here https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/components/MoneyReportHeader.tsx#L126 And here https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/components/ReportActionItem/ReportPreview.tsx#L187

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

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;

What alternative solutions did you explore? (Optional)

etCoderDysto commented 6 days ago

[!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
abzokhattab commented 6 days ago

Thanks for pointing it out @etCoderDysto. I believe the current proposal is the same as the first one since I didn’t introduce any new solutions or approaches—just code implementation and POC, which isn't part of the proposal requirements to be accepted. That’s why I didn’t add the proposal updated comment.

however regardless, your proposal is only similar to my first solution, the second one is diff.

etCoderDysto commented 6 days ago

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 commented 5 days ago

Proposal

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

What is the root cause of that problem?

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

All of these conditions can be handled in:

https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/ReportUtils.ts#L6693

and

https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/components/ReportActionItem/ReportPreview.tsx#L187

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

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 1 day ago

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

melvin-bot[bot] commented 1 day ago

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

Ollyws commented 1 day ago

@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

melvin-bot[bot] commented 1 day ago

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

abzokhattab commented 1 day ago
  1. I suggest adding this to the isOpenExpenseReport. I believe this would be a better solution and more consistent with the isOpenTaskReport function. It would also help reduce code duplication. The report shouldn't be marked as open if the parent room is archived, similar to how it's handled in isOpenTaskReport, where we check if the parent action is deleted. The same concept applies here.
  2. I see that the selected proposal is the same as the first proposal as far as I know adding code details is not necessary to be selected.. that is why i added the first version without the code changes. image

Looking forward to hearing your thoughts!

etCoderDysto commented 1 day ago

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.

abzokhattab commented 1 day ago

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

Thanks for your input, by disabling I mean not to make it shown ... meaning making the shouldShowSubmitButton as false.

The first link was linked by mistake to line L124 (which is shouldDisableApproveButton which is related to the approve not the submit button) instead of L126 ... whereas the second link was correct.

so you can see its a reference mistake

trjExpensify commented 1 day ago

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

trjExpensify commented 1 day ago

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 commented 16 hours ago

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 commented 4 hours ago

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