Ensure we call ReconnectApp when the passed clientUpdateID is not in the DB

[iwiznia]

Context https://expensify.slack.com/archives/C03TQ48KC/p1723832372007299?thread_ts=1723826918.927409&cid=C03TQ48KC

Problem

Right now when the clientUpdateID is not in the DB because it got deleted already, we are just assuming you are up to date, but we really can't know that, there might be updates that you missed.

Solution

If we don't have the clientUpdateID in the DB we should instruct the client to call ReconnectApp to ensure they get all the data they might have missed

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[iwiznia]

Need to update all overdue issues and then will work on this

[iwiznia]

ok writing down the solution here:

1. In auth, we will pass clientUpdateID to getPreviousUpdateID here
2. At the beginning of getPreviousUpdateID we will check if clientUpdateID is in the DB, if it is not we will return -1
3. In Auth class here if the response has accounts.X.lastUpdateID and previousUpdateID is -1, we will call AppInit::reconnectApp without $updateIDFrom
4. This is the part that's unclear... we basically need to add them to the response, but not send them via pusher nor save them in the DB (so we don't want to call Push_API::queue) we basically want to include them in Onyx::getAccountOnyxUpdates so that they get added to the response here, so maybe we can convert the data to events and add them directly to Pusher_API::$sentEvents (which is what Onyx::getAccountOnyxUpdates uses). I think this should work, even if it is a little unconventional (because we did not actually sent the events)
5. We would also save the fact we called ReconnectApp in a class variable in the Auth class and skip the behavior on step 3 by checking this variable

Thoughts? @danieldoglas

[danieldoglas]

I'm OK with this logic, we only want to add the ReconnectApp for the caller.

IF we're doing it to async onyx updates, then I think we might need to send something that says "you should call reconnectApp from scratch"

Maybe it's time to introduce new custom actions we can send to the app?

[iwiznia]

IF we're doing it to async onyx updates,

We are not, since we don't have a clientUpdateID there

[danieldoglas]

how are we dealing with cases where we can't find a previousUpdateID (so no updates at all for them) for an account that is receiving an async update?

[iwiznia]

Yesterday I thought about it and thought it was a non issue, but I don't know how I convinced myself of that 😂

I think there's 2 scenarios, both of them happen when we are in the GetMissingOnyxMessages flow:

1. We find a previousUpdateID for you. I think there's no issue here. When you receive it either you apply it or if you don't have the previousUpdateID, call GetMissingOnyxMessages which will either download the missing updates or do a full reconnect if we don't have them
2. We don't find a previousUpdateID and thus return -1. In this case we would have to run the same logic described in step 4 but with the main difference that we would need to track if we run that logic for each user in that same class. We would do that here

Does that make sense?

[danieldoglas]

I don't think that's the best solution. That means we would send possibly sereal megabytes of updates through pusher for those users that do not have the previous updateID

I'd say we'd need to create a new type of onyx operation, something like reload, and send that operation with the update that is being sent. If they receive that operation, then we do a OpenApp in the app to reload everything.

[iwiznia]

Good point! That makes total sense, a ReconnectApp call can be huge and the users might not even have the app/web open.

But if we do that, wouldn't it make more sense to use that same onyx operation for the current user and simplify this whole thing? I think it would, because that way we would only have one way of handling this for both cases. Also while we could do what I described above for the current user and avoid the extra network roundtrip, this flow is basically the same as you are missing updates and need to do another call that we already have.

[iwiznia]

I just realized that a new onyx operation is not the right call. Calling an API command to do a full reconnect is not an onyx operation at all. Instead, we can use a new pusher event. Summarizing the updated solution that solves the problem for both current user and others:

1. In auth, we will pass clientUpdateID to getPreviousUpdateID here
2. At the beginning of getPreviousUpdateID we will check if clientUpdateID is in the DB, if it is not we will return 0 (same as we do when we find no updates)
3. In Auth class here if the response has accounts.X.lastUpdateID and previousUpdateID is 0, we will queue pusher event appReload ensuring we set the event's pusherSocketID to 0 (this is so that the event is delivered to the user making the request and is not instead included in the response)
4. We would also save the fact we queued this update in a class variable in the Auth class so we can avoid sending 2 events to the same user
5. In app, we will subscribe to this new event like we do here
6. And call App action reconnectApp

[danieldoglas]

I think this makes sense. I'm just not fully sold on adding a new event/event subscription.

[danieldoglas]

After talking to ioni, I agree we can create a new event.

[melvin-bot]

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

[iwiznia]

Working on this. I have the PRs ready above. I need to test the other users case.

[iwiznia]

Sent all PRs for review

[melvin-bot]

This issue has not been updated in over 15 days. @iwiznia, @danieldoglas eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[iwiznia]

This was done way back