[Search v1.2] - Unsubmitted expense can be held without error in Search

[lanitochka17]

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.16-0
Reproducible in staging?: Y
**Reproducible in production?:*Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4804456
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• Delayed submission is enabled on Workflows
• Workspace has an approver (admin)

1. Go to staging.new.expensify.com
2. [Employee] Go to workspace chat
3. [Employee] Create two expenses and do not click Submit button
4. [Admin] Go to transaction thread of any submitted expense from employee
5. [Admin] Click on the report header > Hold
6. [Admin] Enter reason and save it
7. [Admin] Note that it shows "This request is being modified by another member." and the expense cannot be held
8. [Admin] Go to Search
9. [Admin] Select the expense from Step 4 via checkbox
10. [Admin] Click on the dropdown > Hold
11. [Admin] Enter reason and save it
12. [Admin] Note that admin can hold the request from Search and there is no error message

Expected Result:

Admin should be prevented from holding expenses that are not submitted yet in Search

Actual Result:

In Step 7, admin is not allowed to hold unsubmitted expense when holding via transaction thread
In Step 12, admin can hold unsubmitted expense via dropdown button in Search

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6560305_1722614731549.20240802_235819.mp4

View all open jobs on GitHub

[melvin-bot]

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.

[lanitochka17]

@trjExpensify 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

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[FitseTLT]

BE

[melvin-bot]

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

[trjExpensify]

@cdOut is your PR going to take care of this, actually?

[cdOut]

@trjExpensify Hm, looking at the video it looks a little sketchy, it might be using different logic there. I can test it on the hold cleanup PR and if it doesn't fix it I can work on this one after.

[trjExpensify]

Okay, sounds good. Let me know!

[trjExpensify]

Any luck with testing that @cdOut?

[cdOut]

@trjExpensify I've tested it on my hold cleanup PR and it did not help with this bug. I'll create a separate PR with a fix for this one early next week.

[trjExpensify]

Okay, I'll assign you. :)

[melvin-bot]

@trjExpensify, @cdOut Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[cdOut]

Will raise a PR for it tomorrow.

[cdOut]

@trjExpensify I've raised a PR and moved to review, looks like someone missed a conditional for checking whether the selected requests have been submitted yet by the author.

I've also went and added a revised error message for this case, since the one used for singular requests wasn't fit for this one. So for singular request it's This request is being modified by another member. and for the search feature hold I've went with 'Some of the selected requests are being modified by another member.' which is more descriptive. Let me know if that's okay.

[trjExpensify]

Thanks, Tymo! I actually find these error messages pretty strange for the scenario they were introduced. I see the blame coming from this PR: #37477

If I understand the reportModified one correctly, the case is:

1. Member submits an expense
2. Opens the hold modal to enter a reason
3. during that time the approver approves/pays the report
4. The submitter tries to hold the expense
5. We display an error that says: "This request is being modified by another member."

@jamesdeanexpensify I see you approved that copy, but I think it's incorrect, and we should change it to something like: "This expense can't be held because it is approved or paid."

Moving on to this case Tymo is addressing on the search page:

1. Member creates an expense, puts it on a draft open report
2. Admin selects the expense (or multiple) on the open report
3. They see the Hold action button in the bulk select dropdown
4. Provide a holdReason > confirm
5. We show an error, a discerning one would be: "You can't hold expenses that haven't been submitted yet."

But, I question why in step 3 we're even showing the Hold action button if the selected expenses can't be held, and then we'd avoid this error case altogether. CC: @JmillsExpensify @luacmartins as I'm sure we aren't showing other action buttons that aren't applicable in search (e.g Delete when an expense selected is approved or paid).

[cdOut]

@trjExpensify I just wanted to share my thoughts on the cases you've listed above.

I agree that the current error messages are vague and don't clearly explain the issue. We should definitely revise them and I support the ones you've provided. However, I don't think removing the "Hold" button altogether from displaying here is the best solution.

For cases where the user will never be able to hold a request, I suggest we continue not displaying the hold button at all.

But for reports that are usually holdable but are currently blocked for some reason, I think we should either keep using error messages or find another way to let the user know why they can't hold the report right now. Removing the button in these cases would just cause confusion, but providing an explanation helps users understand what's going on.

What do you think?

[trjExpensify]

Yeah, the reason I suggested this for the search case is because we already don't show action buttons in Search that aren't applicable to the selection made:

But, I question why in step 3 we're even showing the Hold action button if the selected expenses can't be held, and then we'd avoid this error case altogether.

So it seems inconsistent to not include hiding Hold in that.

[cdOut]

Ah, okay. I was missing some context on how the search flow handles such cases. Thanks for clearing that up. In that case, I do agree with hiding the option from the menu. The code I wrote is also applicable in that scenario.

This is definitely out of scope, but I do wonder if maybe always showing the possible options would be a better solution, and when one of the selected requests would block an option, we could then show a warning indicator next to the option in the menu. We could then also highlight which of the selected requests are causing the warning to appear.

[trjExpensify]

Ah, okay. I was missing some context on how the search flow handles such cases. Thanks for clearing that up. In that case, I do agree with hiding the option from the menu. The code I wrote is also applicable in that scenario.

Awesome, apols if that wasn't clear! 👍

This is definitely out of scope, but I do wonder if maybe always showing the possible options would be a better solution, and when one of the selected requests would block an option, we could then show a warning indicator next to the option in the menu. We could then also highlight which of the selected requests are causing the warning to appear.

Yeah, potentially, you could be right. I think we'll hear soon enough if the current approach is confusing via the support channels as we open the flood gates to NewDot with new sign-ups.

[luacmartins]

I put up a PR to fix the hold option showing up in the Search page if the report is Open and the user is not the owner of the request.

[cdOut]

@luacmartins could you share a link to that PR you've mentioned?

[luacmartins]

Yea, this is the PR but it's internal only

[luacmartins]

It was deployed to production ~1h ago

[cdOut]

@trjExpensify since we’ve settled on not showing the hold option if selected requests are unsubmitted I also wanted to clear out how we handle one of the cases for it.

What should happen when the user selects a few requests, some of which are submitted and the other are unsubmitted. Right now I also disable the hold option here from being displayed in the search menu. Is that the correct behavior here?

[trjExpensify]

Sorry for the tardy response @cdOut, I've been OoO.

What should happen when the user selects a few requests, some of which are submitted and the other are unsubmitted. Right now I also disable the hold option here from being displayed in the search menu. Is that the correct behavior here?

Yep, that's the correct behaviour. 👍

[cdOut]

Great, then the PR is ready and I'm just waiting for the C+ checklist 👍

[melvin-bot]

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

[trjExpensify]

Assigning @eh2077 for the PR review: #47413

[luacmartins]

We closed the App PR since the issue was fixed in the backend. However, given that a considerable amount of effort was put into it by @eh2077, I think we should still compensate them for the work. cc @trjExpensify

[luacmartins]

We can close this issue once payment is processed.

[trjExpensify]

Sounds good! Payment summary as follows:

• $250 to @eh2077 for the PR

Offer sent!

[trjExpensify]

Paid!