Open iwiznia opened 1 month ago
Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@iwiznia Whoops! This issue is 2 days overdue. Let's get this updated quick!
@iwiznia 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
Need to update all overdue issues and then will work on this
ok writing down the solution here:
clientUpdateID
to getPreviousUpdateID
heregetPreviousUpdateID
we will check if clientUpdateID
is in the DB, if it is not we will return -1
accounts.X.lastUpdateID
and previousUpdateID
is -1
, we will call AppInit::reconnectApp
without $updateIDFrom
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)Thoughts? @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?
IF we're doing it to async onyx updates,
We are not, since we don't have a clientUpdateID there
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?
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:
GetMissingOnyxMessages
which will either download the missing updates or do a full reconnect if we don't have themDoes that make sense?
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.
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.
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:
clientUpdateID
to getPreviousUpdateID
heregetPreviousUpdateID
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)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)reconnectApp
I think this makes sense. I'm just not fully sold on adding a new event/event subscription.
After talking to ioni, I agree we can create a new event.
@iwiznia, @danieldoglas Whoops! This issue is 2 days overdue. Let's get this updated quick!
Working on this. I have the PRs ready above. I need to test the other users case.
Sent all PRs for review
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