Skip to content

Expense - Hold banner not dismissed for employee after admin acknowledges #58248

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

Closed
3 of 8 tasks
vincdargento opened this issue Mar 11, 2025 · 8 comments
Closed
3 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@vincdargento
Copy link

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: V9.1.11-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
If this was caught during regression testing, add the test name, ID and link from TestRail: N
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Redminote 10s android 13
App Component: Money Requests

Action Performed:

  1. Launch app in mweb
  2. Create a workspace - invite a member
  3. Login in android app as member
  4. Open workspace chat, create an expense
  5. In mweb, as admin opening the expense
  6. Open the expense details page , tap header -hold
  7. After a reason entered - note a hold banner is shown
  8. Now in android, open expense details page
  9. Note hold banner is shown
  10. In mweb , tap got it in hold banner
  11. Note for employee banner is still displayed

Expected Result:

If the hold banner is supposed to be shown for the employee when the admin places the expense on hold, then after the admin taps "Got it" in MWeb, the banner should be dismissed for the employee in Android as well.

Actual Result:

  1. When the admin places the expense on hold, the hold banner is shown for the employee.

  2. After the admin taps "Got it" on the hold banner in MWeb, the banner remains displayed for the employee in Android.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

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

melvin-bot bot commented Mar 11, 2025

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Mar 11, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-03-11 19:47:54 UTC.

Proposal

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

Expense - Hold banner not dismissed for employee after admin acknowledges

What is the root cause of that problem?

We have no implementation to dismiss the modal when dismissedHoldUseExplanation value changes in ProcessMoneyRequestHoldPage

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

We can add an effect that dismisses the modal when dismissedHoldUseExplanation becomes true

    const [dismissedHoldUseExplanation] = useOnyx(ONYXKEYS.NVP_DISMISSED_HOLD_USE_EXPLANATION, {initialValue: false});

    useEffect(() => {
        if (!dismissedHoldUseExplanation) {
            return;
        }
        Navigation.goBack();
    }, [dismissedHoldUseExplanation]);

We can also optionally give the route's backTo as fallbackt to the goBack function.

We call the dismissHoldUseExplanation via onClose here beforeRemove on navigating away but we don't need to dismissHoldUseExplanation in this case as it is already dismissed so we can avoid it by setting some ref before navigation on our new effect and avoid dismissing in onClose callback when the ref is set.

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

If needed we can render ProcessMoneyRequestHoldPage and assert we navigate back if dismissedHoldUseExplanation changes to true

What alternative solutions did you explore? (Optional)

@Julesssss Julesssss self-assigned this Mar 11, 2025
@thelullabyy
Copy link
Contributor

Proposal

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

  • When the admin places the expense on hold, the hold banner is shown for the employee.

  • After the admin taps "Got it" on the hold banner in MWeb, the banner remains displayed for the employee in Android.

What is the root cause of that problem?

  • When user A (employee) requests money from user B (admin), and B places the request on hold, the hold banner appears for both users due to this logic:
    useEffect(() => {
    // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
    if (isLoadingHoldUseExplained || dismissedHoldUseExplanation || !isOnHold) {
    return;
    }
    Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD.getRoute(Navigation.getReportRHPActiveRoute()));
    }, [dismissedHoldUseExplanation, isLoadingHoldUseExplained, isOnHold]);

(This assumes both users have nvp_dismissedHoldUseExplanation set to false.)

  • If user B clicks "Got it," the nvp_dismissedHoldUseExplanation is only updated to true on B’s side — user A’s value remains false.

  • As a result, the hold banner continues to display for user A, which matches the behavior described in this issue.

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

  • The hold banner modal is meant to educate users about the hold state. So, even if user B clicks the "Got it" button, it shouldn’t dismiss the banner for user A.

  • Additionally, even if we wanted to dismiss the hold banner for user A, we couldn’t — the nvp_dismissedHoldUseExplanation value is unique to each account and only updates on the user who interacts with the button.

  • There’s one area that could be improved: when the hold banner appears for both users and user B releases the hold, the banner still shows on user A’s side, incorrectly stating, "This request is on hold." In reality, the request is no longer on hold — it was held previously but isn’t anymore.

To fix this, we could consider one of these options:

  1. Update the banner text — Change the title from "This request is on hold" to something more general that educates users about the hold status without implying the request is still on hold.

  2. Auto-dismiss the banner — Introduce logic to automatically dismiss the hold banner when the status changes from "hold" to "unhold."

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

  • UI improvement, don't need the test here.

What alternative solutions did you explore? (Optional)

@thelullabyy
Copy link
Contributor

@Expensify/design

In the expected result in this issue is after the admin taps "Got it" in MWeb, the banner should be dismissed for the employee in Android as well.

But, I think the hold banner modal is meant to educate users about the hold state. So, even if user B clicks the "Got it" button, it shouldn’t dismiss the banner for user A. What do you think about this point?

@VictoriaExpensify
Copy link
Contributor

But, I think the hold banner modal is meant to educate users about the hold state. So, even if user B clicks the "Got it" button, it shouldn’t dismiss the banner for user A. What do you think about this point?

Oh yeah, I agree with you @thelullabyy - the banner should inform both parties so I don't think the admin tapping "Got It" should remove the message for the submitter.

In saying that, I don't this is the most ideal message for the submitter to see:

Image

The explanation around what "Hold" means is very much from an Admin perspective. If would be good if the submitter saw something more tailored to them.

Let's wait and see what the design team think, though

@melvin-bot melvin-bot bot added the Overdue label Mar 17, 2025
@VictoriaExpensify
Copy link
Contributor

Taken it to Slack for discussion - https://expensify.slack.com/archives/C01SKUP7QR0/p1742272889490259

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 18, 2025
@VictoriaExpensify
Copy link
Contributor

Ok we've had a bit of a discussion around this and agreed that the expected behaviour is for the banner to remain visible to each party until they submit it, so this is working as expected.

But we did agree the language could do with some tweaking to make it less focussed on the approver. @jamesdeanexpensify is taking care of that here - thanks James!

@VictoriaExpensify
Copy link
Contributor

And thanks for flagging that this was likely expected behaviour @thelullabyy !

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. Daily KSv2
Projects
None yet
Development

No branches or pull requests

5 participants