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

CRITICAL: [Actionable Whispers] [$250] Converting Tracked Expense Hides All Previous ReportActions #40265

Closed
1 of 6 tasks
thienlnam opened this issue Apr 16, 2024 · 25 comments
Closed
1 of 6 tasks
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

Comments

@thienlnam
Copy link
Contributor

thienlnam commented Apr 16, 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: N/A
Reproducible in staging?: N/A
Reproducible in production?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

Action Performed:

Note: This occurs as part of a feature from this PR

Break down in numbered steps

  1. Create a policy expense chat
  2. add messages in that chat
  3. Track an expense to your self-DM
  4. Categorize it (moving it to the workspace)

Expected Result:

You should see the new moneyRequestPreview action, along with the old chats

Actual Result:

You only see the new moneyRequestPreview action, and everything else remains "loading" or is gone from the chat

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Screen.Recording.2024-04-15.at.7.24.58.PM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e87700b7450abadb
  • Upwork Job ID: 1780297096064262144
  • Last Price Increase: 2024-04-16
  • Automatic offers:
    • DylanDylann | Contributor | 0
@thienlnam thienlnam added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 16, 2024
@thienlnam thienlnam self-assigned this Apr 16, 2024
Copy link

melvin-bot bot commented Apr 16, 2024

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

@adelekennedy
Copy link

added to vip-vsb for self dm

@adelekennedy
Copy link

@thienlnam can we make this external?

@thienlnam
Copy link
Contributor Author

Yeah let's do it - we're not entirely sure what causes this yet

@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Apr 16, 2024
@melvin-bot melvin-bot bot changed the title [Track Expense] Converting Tracked Expense Hides All Previous ReportActions [$250] [Track Expense] Converting Tracked Expense Hides All Previous ReportActions Apr 16, 2024
Copy link

melvin-bot bot commented Apr 16, 2024

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

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

melvin-bot bot commented Apr 16, 2024

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

@james-tindal
Copy link

I get no message from Concierge on submitting an expense.

@thienlnam
Copy link
Contributor Author

This is only on main right now - we just merged the actionable whisper PR

@quinthar quinthar moved this to CRITICAL in [#whatsnext] #vip-vsb Apr 18, 2024
@quinthar quinthar changed the title [$250] [Track Expense] Converting Tracked Expense Hides All Previous ReportActions CRITICAL: [Actionable Whispers] [$250] Converting Tracked Expense Hides All Previous ReportActions Apr 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 18, 2024
@adelekennedy
Copy link

@thienlnam since this is critical and we haven't gotten any proposals should we make this internal and get an engineer assigned?

@melvin-bot melvin-bot bot removed the Overdue label Apr 19, 2024
@james-tindal
Copy link

I've been working on it for the past 3 days and closing in on a solution.

@quinthar
Copy link
Contributor

@james-tindal - Can you give an update on your progress?

@melvin-bot melvin-bot bot added the Overdue label Apr 21, 2024
@james-tindal
Copy link

james-tindal commented Apr 22, 2024

I expect to have a solution today.

Root cause

The server sends a list of actions in the report. These are filtered so the remaining actions form a chain where action.previousId === previousAction.Id

When the actionable whisper moves the IOU to a workspace as a ReportPreview, it also adds another ReportPreview which breaks the chain between the converted IOU and previous messages.

@adelekennedy
Copy link

great @james-tindal - @ntdiary will you prioritize reviewing @james-tindal solution today?

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Apr 22, 2024

great @james-tindal - @ntdiary will you prioritize reviewing @james-tindal solution today?

Ah, I should go to bed soon, will request another c+ on slack to tabke over this issue. :)

@ntdiary
Copy link
Contributor

ntdiary commented Apr 22, 2024

@DylanDylann will take over it. 😄

@DylanDylann
Copy link
Contributor

@james-tindal Could you update your proposal following the guideline

@james-tindal
Copy link

I didn't mean to imply I had a working solution. I have a hack fix only. Pretty sure I'll get it finished within 24 hours, then I'll update.

@quinthar
Copy link
Contributor

Great, thanks!

@adelekennedy adelekennedy assigned DylanDylann and unassigned ntdiary Apr 23, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@james-tindal
Copy link

james-tindal commented Apr 23, 2024

There's a comment about a relevant conversation. Can someone share what has been said?
https://expensify.slack.com/archives/C035J5C9FAP/p1705417778466539?thread_ts=1705035404.136629&cid=C035J5C9FAP

@james-tindal
Copy link

james-tindal commented Apr 24, 2024

Proposal

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

All previous report actions are replaced by a loading skeleton when a tracked expense is moved to a workspace.

What is the root cause of that problem?

The displayed actions must form a chain linked by action.previousActionId === previousAction.id

New action A is appended to the report optimistically, before the server responds.
It has no previousActionId, so it breaks the chain.

{ id: A, previousActionId: undefined }

The server responds with an identical action B, only with a different id and the correct previousActionId.
If A and B had the same id, A would be replaced. Instead, they are both at the top of the list.
Now reportActions looks like this:

{ id: B, previousActionId: P }
{ id: A, previousActionId: undefined }
{ id: P, previousActionId: Q }
{ id: Q, previousActionId: R }
{ id: R, previousActionId: "0" }

So the chain is still broken after the server responds.

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

The simplest solution to the problem of all previous reportActions being hidden, is to remove the optimistic data for this API request, until it is fixed.

What alternative solutions did you explore?

Keep optimistic UI and fix it.

I have written code to add the correct previousActionId to the optimistic data.
But, I have not yet figured out how to get the optimistic reportAction to delete when the server response arrives.
This results in reportActions looking like this:

{ id: B, previousActionId: P }
{ id: A, previousActionId: P }
{ id: P, previousActionId: Q }
{ id: Q, previousActionId: R }
{ id: R, previousActionId: "0" }

The chain is still broken after the first action.

If the server returned an action with the same ID as the optimistic one, it would replace it, but because it has a different ID, it is appending instead.

I will keep working on finding a way to delete the optimistic reportAction.

Fixing already broken reports

  1. Delete the reportActions and report from IndexedDB
  2. Refresh the page
  3. App refetches the uncorrupted data from the server.

@thienlnam
Copy link
Contributor Author

I'm curious if this is resolved now - we fixed a problem where the optimistic reportPreview reportAction was not stored in the database so we essentially had

  • an optimistic reportAction
  • a different real reportAction

So it kept searching for a reportAction that is not there

@james-tindal
Copy link

I can no longer reproduce the bug. Can you link the pull request that fixed it?

@quinthar
Copy link
Contributor

Neat, can we close this?

@thienlnam
Copy link
Contributor Author

I can no longer reproduce the bug. Can you link the pull request that fixed it?

This was a back-end fix

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
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

6 participants