Edit command is slow

[iwiznia]

Context https://expensify.slack.com/archives/C03TQ48KC/p1671566294965399

Problem

UpdateComment and really Report_EditComment too (they use the same code) are slow.
Mostly, it is caused by fully loading and instantiating all the report history.

Solution

Instead of loading the whole history, to then call ReportUtils::getMostRecentVisibleAction and ReportUtils::getMostRecentVisibleUnreadAction, we should have auth return those values by querying them directly in the DB and only returning the data we need.

BTW this will not be fixed by the deprecate sequence numbers project, so it needs to be done separately.

cc @chiragsalian

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016ec925fa79763af8
• Upwork Job ID: 1605317751109066752
• Last Price Increase: 2022-12-20

[luacmartins]

So it seems like we want to create a new Auth command, e.g. getMostRecentVisibleAction that will return the most recent visible action or the most recent visible unread action to PHP so we can stop calling $report->getHistory(). Does that sound right @iwiznia?

[iwiznia]

Yeah maybe... I am not that familiar with this flow, like why do we care about the most recent visible action?

[luacmartins]

From what I can tell, it seems to be used to:

1. Update the last message text in the LHN if we edited the most recent message
   

2. Update the unread count if we deleted the last unread message

[luacmartins]

Maybe we don't need a new command and we can move that logic to UpdateReportComment in Auth. I'll explore that first since it would most align with our 1:1:1 goal.

[iwiznia]

The last 1 of 1:1:1 was not decided and I expressed my concern about it many times before: adding reads to a write makes the command more likely to conflict, so I am not sure if it's best to add it in that command or have a separate one.

[luacmartins]

Makes sense, I think in that case a new command is better.

[luacmartins]

I'm going ooo until Jan 3, so I'll unassign myself in case someone else wants to work on this while I'm away.

I have some draft PRs (Web-E and Auth), but they are far from being ready. If anyone picks this up, feel free to use/discard my draft PRs. If nobody picks this up, I'll come back to it in January!

[JmillsExpensify]

Thanks for the heads up! I'm trying to wrap my head around the importance of this. My initial sense is that it's importance for Expensify employees but not particularly important for WAQ. So we should solve it but it's not critically pressing. Does that sound right to others on this issue?

[JmillsExpensify]

Re-added the bug label so that we can get someone else on this over the holidays. And....that didn't work. Going with the engineering label instead.

[JmillsExpensify]

@sketchydroide Do you think you'll have time to help move this forward this week? Carlos has two draft PRs up as a starting point.

[sketchydroide]

Yeah, I can help with this, if I get stuck, I'll be sure to ask for help, but for now I think I understand the core of it

[JmillsExpensify]

Awesome, thanks!

[sketchydroide]

I've started work on it, kept the changes of Carlos, and working on top of the original PRs
I think I have a good grasp of what is needed, I think most of the changes wil be on Auth and then Web is mostly cleanup, although I think I need to create a command for getMostRecentVisibleUnreadAction both on Auth and Web.

This is the one that used the lastReadSequenceNumber that I will not use, will use the timeStamp instead as per the new deprecate sequenceNumber doc.

[sketchydroide]

Thanks @iwiznia for the comments on the PR, will go through them today

[sketchydroide]

Auth PR is done, and I will put it for review tomorrow
Web should be very close, it's much smaller, it might need some work on the App

[sketchydroide]

ok Auth PR is now ready for review

[JmillsExpensify]

Looks like we're still working through the linked Auth PR.

[sketchydroide]

It's under review, and there has been some comments on what can be improved, and some doubts poping up
I'll get them sorted today, and maybe get a convo happening if it becomes to complex for a PR.

[sketchydroide]

Auth PR seems to be close to being done with the comments

[JmillsExpensify]

Assigning Ionatan on this as he takes over the linked PR.

[MelvinBot]

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

[iwiznia]

Waiting for reviews

[MelvinBot]

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

[MelvinBot]

@iwiznia, @JmillsExpensify Still overdue 6 days?! Let's take care of this!

[MelvinBot]

@iwiznia, @JmillsExpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[iwiznia]

Working on it, conflicts and people reusing a method I had killed delayed this a while, plus it will require waiting for an auth deploy.

[MelvinBot]

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

[MelvinBot]

@iwiznia, @JmillsExpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MelvinBot]

@iwiznia, @JmillsExpensify Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[MelvinBot]

@iwiznia, @JmillsExpensify 10 days overdue. I'm getting more depressed than Marvin.

[JmillsExpensify]

Still in progress. Thanks Melvin

[MelvinBot]

@iwiznia, @JmillsExpensify Huh... This is 4 days overdue. Who can take care of this?

[MelvinBot]

@iwiznia, @JmillsExpensify Still overdue 6 days?! Let's take care of this!

[iwiznia]

This is done. I did a few tests and it seems it's fast now.