-
Notifications
You must be signed in to change notification settings - Fork 354
api: Add ZulipStream.topicsPolicy #1956
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
base: main
Are you sure you want to change the base?
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! Comments below.
Also, some commit-message nits:
api: Add ZulipStream.TopicsPolicy
Ingest String `topics_policy` field. The field can take be one of 4
possible values, inherit, allow_empty_topic, disable_empty_topic,
and empty_topic_only. Enum TopicsPolicy is created for these four
values.
Fixes: https://github.com/zulip/zulip-flutter/issues/1604
ZulipStream.TopicsPolicydoesn't refer to anything that exists; instead, sayZulipStream.topicsPolicy(referring to the field) orTopicsPolicy(referring to the enum).- The paragraph ("Ingest String…") just repeats what's already clear and expected in the diff; let's remove it, so there's less we need to read through.
- This work is a prerequisite for #1604 (making the data available to the app), but I'd like to keep #1604 open for the work of actually using the data in the compose-box UI code. So let's remove the "Fixes" line.
lib/api/model/model.dart
Outdated
| int? firstMessageId; | ||
|
|
||
| int? folderId; | ||
| TopicsPolicy topicsPolicy; |
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.
I would order this between messageRetentionDays and channelPostPolicy, here and everywhere else in this commit.
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.
Also, this field is new in Zulip Server 11, per the API doc. So if we make it required with no default value, we'll throw an error when getting this data from a server older than 11.
On those older servers, do channels always behave as though they have TopicsPolicy.inherit? If so, let's use that as the default value:
| TopicsPolicy topicsPolicy; | |
| @JsonKey(defaultValue: TopicsPolicy.inherit) // TODO(server-11) remove default value | |
| TopicsPolicy topicsPolicy; |
7cffea4 to
17fca1c
Compare
17fca1c to
02b5b4d
Compare
|
I see you've pushed changes; please comment here when this is ready for another review. |
02b5b4d to
22cdda8
Compare
|
Thanks for reviewing before! It's ready for another round. |
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 nit below.
|
I've also removed the "Fixes:" line from the PR description, for the same reason I mentioned above in #1956 (review). |
22cdda8 to
6639351
Compare
6639351 to
afc7562
Compare
|
I see you've pushed some updates; please comment here when this is ready for another review. |
afc7562 to
c172c95
Compare
|
ahh I forgot to comment. Let me rebase on top of the latest since it's been awhile. I don't think I have any more changes to add to this PR. Set up a different PR for the ui changes that requires this PR. |
|
@chrisbobbe CI is green. Thanks for the reminder! It's ready for review.. |
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 for the revision! Comments below.
lib/api/model/initial_snapshot.dart
Outdated
|
|
||
| static RealmTopicsPolicy fromApiValue(String apiValue)=> _byApiValue[apiValue] ?? allowEmptyTopic; | ||
|
|
||
| static final _byApiValue = _$RealmTopicsPolicyEnumMap.map((key, value) => MapEntry(value, key)); |
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.
nit: too-long line
| static final _byApiValue = _$RealmTopicsPolicyEnumMap.map((key, value) => MapEntry(value, key)); | |
| static final _byApiValue = _$RealmTopicsPolicyEnumMap | |
| .map((key, value) => MapEntry(value, key)); |
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.
it's unused. I removed them
lib/api/model/initial_snapshot.dart
Outdated
| allowEmptyTopic, | ||
| disableEmptyTopic; | ||
|
|
||
| static RealmTopicsPolicy fromApiValue(String apiValue)=> _byApiValue[apiValue] ?? allowEmptyTopic; |
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.
Instead of handling unknown values here, let's make an explicit unknown enum value, and ask the inner parts of the app code to handle that.
lib/api/model/model.dart
Outdated
| disableEmptyTopic, | ||
| emptyTopicOnly; | ||
|
|
||
| static TopicsPolicy fromApiValue(String apiValue) => _byApiValue[apiValue] ?? inherit; |
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.
(Similarly here, let's use an explicit unknown enum value, for inward app code to handle.)
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.
I don't think server sends random value and it's not null?
because @JsonKey(defaultValue: TopicsPolicy.inherit) is added #1956 (comment)
I believe this was the needed change.
static RealmTopicsPolicy fromApiValue(String apiValue) => _byApiValue[apiValue]!;a423b1c to
6f5525b
Compare
6f5525b to
d256017
Compare
|
@chrisbobbe Thanks for reviewing! It's ready for another round. |
Ingest String
topics_policyfield. Will be used to hide the topic input in the general-chat-only channel.