Migrate data from legacy app #1588
Conversation
|
I probably haven't set this up properly. On my Android debug build, |
lib/model/legacy_app_data.dart
Outdated
| const _LegacyAppJsonConverter(); | ||
|
|
||
| static const List<_LegacyAppJsonConverter<Object?>> converters = [ | ||
| _LegacyAppUrlJsonConverter(), |
There was a problem hiding this comment.
I can see why _LegacyAppZulipVersionJsonConverter got left out here, since that converter has T as String. A comment explaining that this is intentional might be helpful.
(Or perhaps just inline the _LegacyAppUrlJsonConverter annotation when needed?)
There was a problem hiding this comment.
Yeah, this could be clearer. I started with this one; then got to where ZulipVersion is needed and the same approach doesn't apply.
Given that there ended up being just one place _LegacyAppUrlJsonConverter comes into effect (for LegacyAppAccount.realm), and that this set of types is basically complete now and isn't going to expand in the future, I think I'll just apply the converter explicitly in that one spot and drop this list.
| await db.update(db.globalSettings).write(GlobalSettingsCompanion( | ||
| // TODO(#1139) apply settings.language | ||
| themeSetting: switch (settings.theme) { | ||
| LegacyAppThemeSetting.default_ => drift.Value.absent(), |
There was a problem hiding this comment.
I think the "default" in the legacy app is light mode.
There was a migration to rename "default" to "light" (zulip/zulip-mobile@43acc41). Oh, but that was ultimately for zulip/zulip-mobile#5533 (Automatically follow user's OS-level dark/light preference). So I think, after all, this completes:
On existing installs where the user has stuck with the default of light mode, we'll give them the new default.
There was a problem hiding this comment.
Yeah, this is a good question to consider explicitly.
The legacy app has just two values: always light and always dark. In the app's data store, and so in the database, these are called "default" and "night" respectively (that's the ThemeSetting enum cited in a comment here). And the default is "default" (i.e. thankfully the name isn't misleading on that point).
The new app has values light, dark, and null. Null means to use the system-wide setting, and is the default.
So if the value in the legacy app's database is "night", clearly we should translate that to dark. The user explicitly chose a dark theme.
If the value in the legacy app's database is "default", though, it's less clear. That's the value it will have if the user didn't make any choice.
So my thinking is that if the value is "default", then we should translate that to the new default, which is to use the system-wide setting. If the system-wide setting is on a daily cycle, then the user will almost certainly experience that as an upgrade: at last Zulip follows the daily cycle the rest of the device follows. If the system-wide setting is light, there'll be no change. And if the system-wide setting is dark, but the user really does prefer Zulip specifically to be in a light theme… well, it's easy for them to change it.
There was a problem hiding this comment.
Ah, and GitHub was showing me a stale page that didn't have your edit. Yeah, exactly, this effectively completes that issue zulip/zulip-mobile#5533 .
|
Thanks for the review! For investigating the file paths, I started a chat thread: #mobile-team > migrating legacy data @ 💬. Let's continue there for debugging. (I'll also reply on the other subthreads above, but that one is the most critical since it could affect whether the migration works at all for a given user.) |
| realmUrl: account.realm, | ||
| userId: account.userId!, | ||
| email: account.email, | ||
| apiKey: account.apiKey, |
There was a problem hiding this comment.
Hmm, what happens if apiKey is an empty string (possible for logged-out users)?
I think the converted Account will show up on the choose-account page, and will get handled by the code responsible for invalid API keys. The Flutter app doesn't remember logged-out accounts right now, similar to how language setting is not yet supported. Perhaps this can be a TODO?
There was a problem hiding this comment.
Hmm, yeah, good find.
The new app doesn't have a concept of an account that's in the list and yet logged out.
I'm not sure it was all that useful in the legacy app either — it's basically a relic of the fact that, when you were a screen or two into the login flow, the legacy app didn't originally have a better place to keep that information (with the realm URL, and another screen later the email and password, that you'd already entered) than in its global persistent list of accounts. We eventually fixed that, making that information part of the relevant pages' local state instead, and the new app does the same thing.
Since the new app doesn't have this concept, I'll have it drop such records if they appear.
gnprice
force-pushed
the
pr-legacy-db
branch
2 times, most recently
from
5e42644 to
158fbaa
Compare
|
Revision pushed. Thanks @PIG208 for the review! |
lib/model/legacy_app_data.dart
Outdated
| zulipVersion: account.zulipVersion!, | ||
| // no zulipMergeBase; legacy app didn't record it | ||
| zulipFeatureLevel: account.zulipFeatureLevel!, | ||
| ackedPushToken: drift.Value(account.ackedPushToken), |
There was a problem hiding this comment.
This will be the first time we store a non-null ackedPushToken in the new app, since we haven't done #322 yet. We don't update it when the server registers a new push token, so it might become stale…currently I think such a stale token could only flow into an unregister-token request, which we do on logout.
There was a problem hiding this comment.
Hmm interesting, good catch.
I think I'll drop this value, then — it doesn't really do any good (since until #322 it doesn't cause this app to stop repeatedly sending the token to the server), and at that one spot where it would get used I think it would be more likely to make the unregister-token request not work (by making it use a stale token) than to make it work. (Both of which are unlikely — I believe these tokens rarely change in practice.)
When we eventually do #322, that will cause the app to promptly store the current value, and there won't be any benefit to remembering the value the legacy app had stored in the past.
|
(Pushed a revision with that change.) |
|
OK, at this point I'm not going to get to writing any significant automated tests for this before the launch. So I'll file a followup issue for those, and go ahead and merge after tweaking the commit messages. (I also just pushed a revision expanding one comment, with details on when the legacy app's Account records started having a Effectively we'll just rely on
Speaking of that alpha testing, I have a couple more edits to harden the logic that I want to include for launch. I'll send those as a separate follow-up PR, though — that way the merged version of this in main can be the same logic that went into today's v30.0.257 beta, and the difference between the logic in v30.0.257 and the next build (for launch) can be expressed as a discrete commit of its own. That's helpful for looking at the history in Git to understand the history of the releases. |
|
And merged. Thanks again @PIG208 and @chrisbobbe for the reviews! |
Fixes #1070.
This is a draft because I'd like to write at least some tests before merging. (Unlike some of the other PRs we've merged in the past week with tests left as a follow-up, the job this is doing is especially sensitive to edge cases.)
But otherwise I believe this is ready. I've tested it manually on both Android and iOS, and the data gets carried along as intended. So in parallel with me writing those tests, I'd like to have additional eyes reading the code, to try to spot things I might have missed.