-
Notifications
You must be signed in to change notification settings - Fork 350
Add user groups to store and API binding #1688
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
Conversation
chrisbobbe
left a comment
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! Just one comment below, otherwise LGTM please merge at will.
I wonder if we want to sometimes do a smoke test with the earliest supported server version (7.0 now) to make sure we're not accidentally breaking support for some servers.
| // final int? creatorId; // not using; ignore | ||
|
|
||
| final bool isSystemGroup; | ||
| bool deactivated; |
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.
The doc says "New in Zulip 10.0 (feature level 290)"; I think we want to make this optional?
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.
Oh good catch, thanks. And I guess we can default it to false — I think before that feature level, there just was no concept of a deactivated group, so all groups were effectively active.
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.
before that feature level, there just was no concept of a deactivated group
Hmm, realm_user_groups in register-queue says:
Changes: Prior to Zulip 10.0 (feature level 294), deactivated groups were included for all the clients.
I'm not sure whether or not that's consistent with that.
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.
Yeah, that caused me to have to reread a couple of times. But I think the story is:
- Starting in 290, there was such a thing as a deactivated group.
- Then starting in 294, deactivated groups were kept out of the list by default.
See also the 290 and 294 entries in the changelog https://zulip.com/api/changelog . In particular 290 includes:
- POST /user_groups/{user_group_id}/deactivate: Added new API endpoint to deactivate a user group.
- POST /register, GET /user_groups: Added deactivated field in the user group objects to identify deactivated user groups.
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.
Cool. This makes sense then:
And I guess we can default it to false
Hmm, with a live server, then? Yeah, in principle that would probably be a good thing to do. I think that's been a rare class of bug for us to have, though, and it's an expensive type of test to set up and maintain (and to run), so I don't expect it to be a priority any time soon. Effectively we've been relying on Zulip's high-quality API docs, including its meticulous change notices; and on both author and reviewer reading those API docs, like you just demonstrated 🙂 |
|
Huh, infra failure in CI: Rerunning. |
|
Retried a couple of times; same error on all three tries so far. |
|
… Well huh. That commit is Flutter's That shouldn't happen. And that probably explains why we're failing to fetch an engine artifact from there. |
We currently don't look at the list of groups at all. When we start doing so, we'll be sure to properly handle the possibility that some of the groups are deactivated.
This has gotten rather long. That also makes diffs touching this line harder to read.
Fixes zulip#662. The "proxy" store mixin foreshadows a pattern I plan to use for the other substores too, to get the proxy boilerplate out of the central store.dart and into the individual substore files. That'll happen in an independent PR, though.
|
OK, worked around that infra issue in #1690. Now CI is failing only here: I'll fix that too; but it's unrelated to this PR, so I'll go ahead and merge. |
Fixes #662, toward #233 user-group @-mentions.