Skip to content

Update database with feature level etc. on each register #458

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

Closed
gnprice opened this issue Dec 21, 2023 · 3 comments · Fixed by #702
Closed

Update database with feature level etc. on each register #458

gnprice opened this issue Dec 21, 2023 · 3 comments · Fixed by #702
Assignees
Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.)

Comments

@gnprice
Copy link
Member

gnprice commented Dec 21, 2023

On the Account class, corresponding to a row in our local database, we have several fields for values that may in reality change over time: most notably zulipFeatureLevel, also zulipVersion and zulipMergeBase.

Currently we take the values of those fields when first logging in, and don't ever update them later. We should do so. In particular when we have compatibility logic that conditions on zulipFeatureLevel, that will allow that logic to correctly use the server's current version rather than the version it had when we first logged in (perhaps months ago). Discussion here:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/zulipFeatureLevel.20out.20of.20sync/near/1708193

Specifically this issue is for updating these values using the registerQueue response. We should also do so on restart events; that's tracked as part of #135.

I have a draft branch for this which I wrote one morning a few months ago and then didn't finish. (That branch handled restart events, which aren't so much a priority in themselves, but most of the work involved was for the ability to update account records in the database at all.)

@gnprice gnprice added a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.) labels Dec 21, 2023
@gnprice gnprice added this to the Beta 2 milestone Dec 21, 2023
@gnprice
Copy link
Member Author

gnprice commented Jan 5, 2024

Another effect of this is that it will serve as an early small trial run of the sort of code changes we'll need for #477, because it'll involve writing changes into the database in tandem with making them in our Dart data structures.

@gnprice
Copy link
Member Author

gnprice commented Feb 21, 2024

I have a draft branch for this which I wrote one morning a few months ago and then didn't finish

I've just sent #526, built around one commit from that branch but then with 14 other commits of followups it led to.

I've also rebased the branch as a whole and updated the rest of it a bit, and have it still as a draft branch.

chrisbobbe pushed a commit that referenced this issue May 22, 2024
This is the first in a series of steps toward making
[PerAccountStore.handleEvent] an async method.  We'll need that
in turn in order to let it start saving changes to disk when
applicable, like for #458, and having both it and its caller,
[UpdateMachine.poll], wait for the result rather than barreling
ahead to make other changes concurrently.

This change comes first because when we make `handleEvent` async,
we'll want to make sure all its call sites properly wait for it
to finish; and when that requires changing the caller to an async
method, we'll want to do the same for *its* call sites, recursively.
To keep the changes manageable to read and review, we'll make
separate commits for each method, except local helpers with only a
handful of call sites.  The callers have to go before the callees
in order to keep the commits coherent, so we start with this
transitive caller.
chrisbobbe pushed a commit that referenced this issue May 22, 2024
With #458, we'll want this method to write changes to the database
on certain events, to update the metadata we keep about the user's
Zulip servers.

When it does that, we'll want it to wait for that write to finish
before it moves on to make other changes, and for its caller
[UpdateMachine.poll] to similarly wait.  That saves us from having
to worry about potential races if some later event also wants to
make database changes while the first changes haven't finished yet.
It also gives us a good foundation for generally not showing the
user a confirmation that some change has been made until we've
actually saved the change.

So to do that, we'll need the method to be async, and to return a
Future which its call sites should wait for.  That's this commit.
It completes a series where we did the same thing for its callers,
recursively.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue May 23, 2024
@gnprice
Copy link
Member Author

gnprice commented Feb 16, 2025

In baea6d7 / #701 I wrote that with this issue #458 we'd want PerAccountStore.handleEvent to write changes to the database on certain events, and so that method needed to become async.

That wasn't quite right (or was imprecise, I guess, depending how one reads "with"): this issue only needed us to write such changes on register, not on any events. Hence this issue was completed last year, while we don't yet actually write any database changes caused by events (cf #1327 (comment)).

The part that will call for database changes in handling events is when we handle restart events:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-api Implementing specific parts of the Zulip server API a-model Implementing our data model (PerAccountStore, etc.)
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant