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

[Held requests] [$250] Expense on hold is displayed as single expense when employee hold it second time #45276

Closed
2 of 6 tasks
lanitochka17 opened this issue Jul 11, 2024 · 81 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 11, 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.6-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/4706804&group_by=cases:section_id&group_order=asc&group_id=309128
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Preconditions:

  1. Login as the owner of the workspace
  2. Create a workspace
  3. Invite the approver and employee
  4. Navigate to more features
  5. Enable "workflows"
  6. On the "Workflow" editor - enable "Add Approvals"
  7. Set the Approver account as the Approver
    Steps:
  8. As Employee submit 2 expenses to Workspace chat
  9. As Employee right click on first expense> Hold> Finish the hold process
  10. As Employee right click on first expense> Unhold
  11. Navigate to transaction thread of first expense
  12. Navigate back to expenses page> right click on first expense> Hold> Finish the hold process

Expected Result:

User should remain on expenses page and both expenses (expense on hold and other expense) should be visible

Actual Result:

Expense on hold is displayed as single expense when employee hold it second time

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

Bug6538072_1720627498474.Recording__3473.mp4
Bug6538072_1720650477562.Recording__3486.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0126f16401c1dcd30e
  • Upwork Job ID: 1811439245231442766
  • Last Price Increase: 2024-09-02
  • Automatic offers:
    • hoangzinh | Contributor | 103359561
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

Triggered auto assignment to @sakluger (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
Copy link
Author

@sakluger 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
Copy link
Author

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

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Jul 11, 2024
@melvin-bot melvin-bot bot changed the title Hold - Expense on hold is displayed as single expense when employee hold it second time [$250] Hold - Expense on hold is displayed as single expense when employee hold it second time Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@kabeer95
Copy link

Issue Report: Expense on Hold is Displayed as a Single Expense when Employee Holds it a Second Time on Expensify

Summary:

When an employee puts an expense on hold for the second time, it is displayed as a single expense on Expensify, rather than showing the hold history. This can lead to confusion and make it difficult to track the expense's status.

Steps to Reproduce:

An employee submits an expense report with an expense.
The expense is put on hold by the manager or administrator.
The employee makes changes to the expense and resubmits it.
The manager or administrator puts the expense on hold again.
Expected Result:

The expense should display a hold history, showing that it was previously held and the reasons for the hold.

Actual Result:

The expense is displayed as a single expense, with no indication that it was previously held.

Impact:

This issue can cause confusion and make it difficult to track the status of expenses. It may also lead to delays in expense approval and reimbursement.

Proposed Solution:

To resolve this issue, we recommend updating the Expensify platform to display the hold history for expenses that have been put on hold multiple times. This could be achieved by:

Adding a "Hold History" section to the expense details page, which displays a list of all holds, including the date, reason, and user who initiated the hold.
Updating the expense list view to display a "Hold" indicator, which shows the number of times the expense has been held.
Estimated Development Time:

We estimate X hours to implement this solution and update the Expensify platform to display hold history for expenses.

Priority:

We consider this issue a medium priority, as it affects the usability and functionality of the Expensify platform.

Please let us know if you would like to discuss this issue further or proceed with implementation.

@hoangzinh
Copy link
Contributor

Hi @kabeer95, can you follow the PROPOSAL_TEMPLATE and also provide a root cause of this issue? Thank you.

@kabeer95
Copy link

Proposal: Fix Expense on Hold Display Issue

Problem Statement:
When an employee puts an expense on hold for the second time, it is displayed as a single expense instead of showing the correct hold status.

Root Cause:
The current implementation of the hold feature does not correctly update the expense status when an employee puts an expense on hold multiple times.

Changes to Solve the Problem:

Update updateExpenses Method

Update the updateExpenses method to handle the case where an expense is already on hold. Specifically, add a check to ensure that the expense is not already on hold before updating it.

const updateExpenses = (expenses, updatedExpense) => {
  setExpenses(expenses.map((expense) => {
    if (expense.id === updatedExpense.id && !expense.isHold) {
      return updatedExpense;
    }
    return expense;
  }));
};

Update Expense Status

Update the expense status to reflect the correct hold status when an employee puts an expense on hold multiple times.

  if (expense.isOnHold) {
    expense.holdCount = (expense.holdCount || 0) + 1;
  } else {
    expense.holdCount = 0;
  }
  return expense;
};

Display Accurate Hold Information

Display accurate hold information, including the number of times an expense has been put on hold, in the expense list.

  if (expense.holdCount > 1) {
    return On hold (${expense.holdCount} times)`;
 } else if (expense.holdCount === 1) {
    return 'On hold';
  } else {`
    return 'Not on hold';
  }
};

Alternative Solutions Explored:

Use a Separate Hold Table: Instead of updating the expense status, we could use a separate hold table to store hold information, including the number of times an expense has been put on hold. This approach may be more scalable and flexible.
Implement a Hold History: We could implement a hold history feature that displays a list of all hold actions taken on an expense, including the date and time of each hold action. This approach may provide more transparency and accountability.

@kabeer95
Copy link

@hoangzinh it's time first time trying to contribute to the Expensify issues , kindly avoid the basic mistakes

@hoangzinh
Copy link
Contributor

Sure @kabeer95. Can you follow the proposal format here again, please https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md? This is an example of a proposal that follows the format look like #44153 (comment).

The current implementation of the hold feature does not correctly update the expense status when an employee puts an expense on hold multiple times.

Moreover, can you elaborate on what is not correct about current implementation of hold features? (link to existing LOC/method and explain which lines or points are not correct).

Update the updateExpenses method

I couldn't find out the existing method name updateExpenses in codebase, can you link me to that method? Thank you.

@amunim
Copy link

amunim commented Jul 14, 2024

Unable to reproduce

Copy link

melvin-bot bot commented Jul 17, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Jul 17, 2024
@hoangzinh
Copy link
Contributor

awaiting for proposals

@melvin-bot melvin-bot bot removed the Overdue label Jul 18, 2024
Copy link

melvin-bot bot commented Jul 18, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Jul 23, 2024

@sakluger, @hoangzinh Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Jul 23, 2024
@sakluger
Copy link
Contributor

We're still waiting for more proposals.

@melvin-bot melvin-bot bot added the Overdue label Aug 26, 2024
@hoangzinh
Copy link
Contributor

Not overdue. It's still waiting for new proposals and updates from @Zakpak0 as well on his proposal

@melvin-bot melvin-bot bot removed the Overdue label Aug 26, 2024
@trjExpensify trjExpensify added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 26, 2024
@trjExpensify
Copy link
Contributor

Adding the Help wanted label back to this issue in the meantime. Melvin kicked it off when @hoangzinh was re-assigned about a month ago. Just a quick reminder @sakluger to check for the label - the issue isn't discoverable as being available otherwise.

Copy link

melvin-bot bot commented Aug 26, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@dylanexpensify
Copy link
Contributor

@Zakpak0 any updates please?

@Zakpak0
Copy link
Contributor

Zakpak0 commented Aug 30, 2024

@dylanexpensify will update today

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2024
@sakluger
Copy link
Contributor

I'm back! Thanks @dylanexpensify for monitoring while I was gone, and thanks @trjExpensify for fixing the labels.

@Zakpak0
Copy link
Contributor

Zakpak0 commented Sep 1, 2024

Proposal

Updated
I believe I've found the true root cause.
But I'm unclear on this comment.
Could @perunt elaborate on why it's necessary to set this to 1?

@perunt
Copy link
Contributor

perunt commented Sep 2, 2024

Hey there!
I used it for a very specific reason related to comment linking. Here's the deal:

  1. Comment linking: We implemented a feature to navigate to specific messages. This means we need to position these linked messages properly when a user jumps to them.
  2. The problem with new items: In our inverted list, when do comment linking the new items also can be added at the bottom (index 0). Without this setting, these new items could potentially slide our linked message out of position.

The result: This keeps our linked message stable in its position, even when new content is loaded above/below it. It prevents that annoying "jump" effect where the message you're trying to show suddenly moves.

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

melvin-bot bot commented Sep 2, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@hoangzinh
Copy link
Contributor

Not overdue, still review proposals

@melvin-bot melvin-bot bot removed the Overdue label Sep 2, 2024
@hoangzinh
Copy link
Contributor

Hi @Zakpak0. Thanks for the updates. Can you confirm if your main solution works? It didn't work for me. Besides that, can you explain a little bit about your main solution? What is purpose of "typeof loading == "boolean" ? loading ? 1 : 0 : 1"?

Screen.Recording.2024-09-03.at.22.17.38.mov

@Zakpak0
Copy link
Contributor

Zakpak0 commented Sep 3, 2024

@hoangzinh pretty positive, it's working here on my end.
I'll post a more substantial RCA/Solution now given the new insight from @perunt.
But to answer your question, I used the typeof function in cases where someone is using the BaseInvertedFlatList component but did not pass down the loading boolean as a prop. Simply to guard against undefined errors.
This will now be removed in my final solution and actually causes a regression. I only added it because the comment here stated that this is to guard against

"loading views as anchors"

So I made an assumption that this value is safe to change after loading is complete.

The result: This keeps our linked message stable in its position, even when new content is loaded above/below it. It prevents that annoying "jump" effect where the message you're trying to show suddenly moves.

This information clarified that this was an incorrect assumption.
So I will be changing my solution to go in another direction, most likely based on the length of the list vs whether loading is true or false.

Lastly, I believe on your branch the solution many not be working because the loading prop is not being passed down.
I added that and passed it down the component tree on my branch.
My hint for that is in your video the loading prop is giving a typescript error, while on my branch it does not because I extended the BaseInvertedFlatList type to include the loading boolean.

Currently I'm thinking that I will make the clause

reportActions.length > 3 ? 1 : 0

or something of that nature.
Because in this case it shouldn't matter about the position of the list "jumping", it will be too short to fill height of the screen anyways.

@Zakpak0
Copy link
Contributor

Zakpak0 commented Sep 4, 2024

Proposal

Updated

@trjExpensify
Copy link
Contributor

@hoangzinh thoughts? Keen to keep this moving!

@hoangzinh
Copy link
Contributor

Yep, I'm still reviewing @Zakpak0's proposal. It's quite tricky so I have to take time to review it carefully. Btw, I'm unable to reproduce this issue in staging at the moment.

Screen.Recording.2024-09-04.at.22.37.15.mov

Is there anyone still able to reproduce this issue?

@trjExpensify
Copy link
Contributor

Hm, I don't think I can actually. 🤔 Maybe @Zakpak0 can share some up to date steps if the ones in the OP aren't right or clear.

@sakluger
Copy link
Contributor

sakluger commented Sep 4, 2024

I'm also unable to reproduce in Staging using the reproduction steps in the OP.

@Zakpak0
Copy link
Contributor

Zakpak0 commented Sep 6, 2024

The bug isn't reproducible anymore before of this.
This is similar to the solution I offered in my previous solution which is now my alternative.
We didn't go ahead with it because I couldn't explanation why the problem only occurs when there were two report actions.
And as it turned out the problem was in the BaseInvertedFlatlist and my previous solution was just a downstream workaround.

@hoangzinh
Copy link
Contributor

Thanks for your insight and effort in this issue. Yeah, it's unlucky that this issue was fixed there before we can finalize our solution here.

@robertjchen
Copy link
Contributor

Appreciate all the discussion and efforts here @Zakpak0 @hoangzinh 🙇 Definitely unfortunate given the circumstances- we'll get the next one for sure! 👍

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 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
No open projects
Status: Done
Development

No branches or pull requests

10 participants