Skip to content

Move more data to PerAccountStoreBase #1467

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

Merged
merged 14 commits into from
Apr 8, 2025
Merged

Move more data to PerAccountStoreBase #1467

merged 14 commits into from
Apr 8, 2025

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Apr 4, 2025

This follows up on #1464 by moving several more members to PerAccountStoreBase so as to share them across our substores: the global store, account ID, self-user ID, getters for the realm URL and account, and the method for resolving a URL reference based on the realm URL.

@PIG208 it seems we were thinking on similar lines today 🙂 — I saw #1466 when this was partly written, and incorporated the parts of it for migrating the tests in the commit that starts using PerAccountStoreBase for selfUserId. (Some substore tests had been constructing the substores directly, and need to start getting them from a PerAccountStore.)

After this, there are a few small items that I think we'll also want to move: queueId (as in #1466), zulipFeatureLevel, zulipVersion, and that might be it.

Also in this general direction, we'll want to organize the various realm settings into a RealmSettingsStore. That'll be post-launch, though.

Selected commit messages

8217a76 store [nfc]: Fix "will throw if disposed" comment on account to be more specific

As far as I can see, just dispose won't cause this to throw.
I think this related point is the one this comment was intended
to make.

84d0327 store [nfc]: Move global store up to PerAccountStoreBase too

0aeeb35 store [nfc]: Move accountId to PerAccountStoreBase

833069d store [nfc]: Move account getter up to PerAccountStoreBase

678f1cd store [nfc]: Move realmUrl up to PerAccountStoreBase

00bb86e store [nfc]: Move tryResolveUrl up to base, and use it more

db2eab6 user [nfc]: Move selfUserId to PerAccountStoreBase

This selfUserId getter has 16 other references across 11 files.
Thankfully our architecture, and in particular this way of combining
the substores:

class PerAccountStore extends PerAccountStoreBase with ChangeNotifier,
EmojiStore, UserStore, ChannelStore, MessageStore {

has meant that those all refer to it on the main PerAccountStore type,
rather than on an individual substore like UserStore. So none of
those need to be touched when we rearrange like this where the data
is stored.

706af43 store [nfc]: Get selfUserId on substores from PerAccountStoreBase

Co-authored-by: Zixuan James Li [email protected]

313f8a9 unreads test [nfc]: Accept that a whole PerAccountStore is used for tests

We've had a couple of approaches we've experimented with for how to write
the tests for a given substore within our data model:

  • Operate on the given substore's individual API.

  • Operate on the API of the overall PerAccountStore, though focusing on
    the particular portion of that API provided by the given substore.

In general I think the outcome of these experiments is that we want to
be writing our tests against the API of the whole PerAccountStore.

Fundamentally that's for the reasons described here:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing
because the overall PerAccountStore is the layer whose API corresponds
naturally to the Zulip server API, which is an interface that's
external to the app and so is the real interface we're committed to
supporting.

This particular test file is one which we originally wrote on the
other side of the experiment, operating on the individual Unreads
substore.

But it already had some tension with that approach, ever since we
realized that that substore needed access to another substore, the
ChannelStore, in order to properly handle muting (ffa2d32,
0d03c8e). To handle that, these tests ended up making a whole
PerAccountStore after all, and had a TODO comment for trying to
fully make the substore-only approach work.

With the advent of CorePerAccountStore, there's no longer a realistic
prospect that we'd want to end up carrying that through. That's
because the Unreads now gets its value of selfUserId from a
CorePerAccountStore, and the only reasonable way to construct one of
those is as part of a whole PerAccountStore.

So resolve the experiment, at least as to this file: remove the
TODO comment, and rename the variable to just store accordingly.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Apr 4, 2025
@chrisbobbe chrisbobbe self-requested a review April 8, 2025 01:17
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One small comment below, otherwise LGTM; please merge at will.

@@ -11,13 +11,12 @@ import 'store.dart';
/// The model for tracking the typing status organized by narrows.
///
/// Listeners are notified when a typist is added or removed from any narrow.
class TypingStatus extends ChangeNotifier {
class TypingStatus extends PerAccountStoreBase with ChangeNotifier {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store [nfc]: Get selfUserId on substores from PerAccountStoreBase

This might be splitting hairs, but before this commit, we haven't treated TypingStatus as a "substore" in the same sense as the others: MessageStore and its MessageStoreImpl, EmojiStore and its EmojiStoreImpl, etc.; is it OK to start treating TypingStatus as a substore in just the way this commit does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not structured in the same way as those two and UserStore and ChannelStore. Instead it more resembles Unreads, RecentSenders, or RecentDmConversationsView (two of which also appear in this commit).

The actual MessageStoreImpl or EmojiStoreImpl etc., though, is very similar to TypingStatus or Unreads etc.:

  • It's associated with a particular PerAccountStore, and is effectively part of the implementation of PerAccountStore.
  • It in turn needs access to some of the core pieces of PerAccountStore, like the ApiConnection or self-user ID etc.

I think that first point makes a good definition for the term "substore".

gnprice and others added 14 commits April 7, 2025 19:59
…re specific

As far as I can see, just `dispose` won't cause this to throw.
I think this related point is the one this comment was intended
to make.
I think this change might be NFC, but I'm not sure.  The situation
where it could change the behavior is if there's some URL string
that would be accepted by `Uri.parse`, but rejected by `Uri.resolve`
with the realm URL as base URL, or vice versa.

If there is any such situation, then this behavior is more accurate.
If there isn't, then this is just a small simplification.

Noticed this because it's one of the handful of references to
the store's realmUrl, and another one is a call to tryResolveUrl.
This `selfUserId` getter has 16 other references across 11 files.
Thankfully our architecture, and in particular this way of combining
the substores:

  class PerAccountStore extends PerAccountStoreBase with ChangeNotifier,
    EmojiStore, UserStore, ChannelStore, MessageStore {

has meant that those all refer to it on the main PerAccountStore type,
rather than on an individual substore like UserStore.  So none of
those need to be touched when we rearrange like this where the data
is stored.
…ests

We've had a couple of approaches we've experimented with for how to write
the tests for a given substore within our data model:

 * Operate on the given substore's individual API.

 * Operate on the API of the overall PerAccountStore, though focusing on
   the particular portion of that API provided by the given substore.

In general I think the outcome of these experiments is that we want to
be writing our tests against the API of the whole PerAccountStore.

Fundamentally that's for the reasons described here:
  https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing
because the overall PerAccountStore is the layer whose API corresponds
naturally to the Zulip server API, which is an interface that's
external to the app and so is the real interface we're committed to
supporting.

This particular test file is one which we originally wrote on the
other side of the experiment, operating on the individual `Unreads`
substore.

But it already had some tension with that approach, ever since we
realized that that substore needed access to another substore, the
ChannelStore, in order to properly handle muting (ffa2d32,
0d03c8e).  To handle that, these tests ended up making a whole
PerAccountStore after all, and had a TODO comment for trying to
fully make the substore-only approach work.

With the advent of CorePerAccountStore, there's no longer a realistic
prospect that we'd want to end up carrying that through.  That's
because the Unreads now gets its value of `selfUserId` from a
CorePerAccountStore, and the only reasonable way to construct one of
those is as part of a whole PerAccountStore.

So resolve the experiment, at least as to this file: remove the
TODO comment, and rename the variable to just `store` accordingly.
@gnprice
Copy link
Member Author

gnprice commented Apr 8, 2025

Thanks for the review! Merging.

@gnprice gnprice merged commit ec9aa35 into zulip:main Apr 8, 2025
1 check passed
@gnprice gnprice deleted the pr-store branch April 8, 2025 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants