-
Notifications
You must be signed in to change notification settings - Fork 309
store: Update streams
and subscriptions
on stream-create/delete events
#183
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
store: Update streams
and subscriptions
on stream-create/delete events
#183
Conversation
I'm not sure if I've followed the right ordering in the added code—in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! All LGTM with a suggestion on the ordering.
I'm not sure if I've followed the right ordering in the added code—in
lib/api/model
, I've tried to follow the API doc; inlib/model
, I've tried to followlib/api/model/model.dart
.
Yeah. In lib/api/model/events.dart
I originally wanted to follow the ordering in the API doc… until I went to implement the realm_user
events. Search the page for "realm_user op:" and you'll see why: realm_user/update
events are way up near the top, while realm_user/add
and realm_user/remove
events come quite a bit later. In between are message
, several flavors of subscription
, and others.
I definitely wanted the different op
values for a given type
to be together, so it wasn't going to work to stick to the docs' ordering.
That means that in order to compare the lists for completeness we'll need to do something else anyway. So we might as well put the events in an order that makes sense to us semantically.
Given that, I'd put these event types in events.dart
in the same order as you have them in handleEvent
. More generally, I'd go for the same broad order as in lib/api/model/model.dart
, which is also the one in zulip-mobile's src/api/modelTypes.js
:
- first info about the realm as a whole,
- then about users,
- then about streams and subscriptions,
- then about messages.
ac17901
to
245f296
Compare
Makes sense! Thanks for the review; revision pushed. |
(Handling `op: update` events is zulip#182, as mentioned in the TODOs.)
245f296
to
4e11ea7
Compare
Thanks! Merging. |
Technically maxFileUploadSizeMib is for the whole server, I think, but it seems fine to treat it as realm info for the purpose of ordering these. See Greg's bulleted list here: zulip#183 (review)
Technically maxFileUploadSizeMib is for the whole server, I think, but it seems fine to treat it as realm info for the purpose of ordering these. See Greg's bulleted list here: zulip#183 (review)
Also, add
addStream
/addStreams
helpers toPerAccountStoreTestExtension
. We'll use those soon, to write tests for message-link creation for quote-and-reply.Fixes: #181
Related: #182