compose: Use topicsPolicy and realmTopicsPolicy for per-channel topics policy#2231
compose: Use topicsPolicy and realmTopicsPolicy for per-channel topics policy#2231Ruhdee wants to merge 5 commits into
topicsPolicy and realmTopicsPolicy for per-channel topics policy#2231Conversation
398ae70 to
202c644
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Comments below.
Would you add a commit making InitialSnapshot.realmMandatoryTopics optional? Then the server will be free to drop it once most mobile users have upgraded their mobile-app installation to take that change. The UI should respond in the same way it does when new servers send a value for InitialSnapshot.realmTopicsPolicy that we don't recognize.
| final channelTopicsPolicy = store.streams[streamId]?.topicsPolicy; | ||
| if ( | ||
| channelTopicsPolicy != null | ||
| && channelTopicsPolicy != TopicsPolicy.inherit | ||
| ) { | ||
| return channelTopicsPolicy; | ||
| } | ||
|
|
||
| // Default to realm-level policy when topicsPolicy is not found, | ||
| // or when topicsPolicy is set to inherit. | ||
| return switch (store.realmTopicsPolicy) { | ||
| RealmTopicsPolicy.allowEmptyTopic => TopicsPolicy.allowEmptyTopic, | ||
| RealmTopicsPolicy.disableEmptyTopic => TopicsPolicy.disableEmptyTopic, | ||
| RealmTopicsPolicy.unknown || null => TopicsPolicy.unknown, | ||
| }; |
There was a problem hiding this comment.
- Can tighten up the code in
_effectiveTopicsPolicy; also remove a comment which repeats what's obvious in the code. - For whether to use the new feature or the old feature, just check whether the new feature is available, i.e. check if
store.realmMandatoryTopicsis non-null, rather than an explicit feature-level check. - Can pull out the "channel policy for realm policy" conversion into a method on
TopicsPolicy.
E.g. like this:
// TODO(#668): listen to [PerAccountStore] once we subscribe to topic policies.
TopicsPolicy get _effectiveTopicsPolicy {
final channelTopicsPolicy = store.streams[streamId]?.topicsPolicy;
if (channelTopicsPolicy case .inherit || null) {
final realmTopicsPolicy = store.realmTopicsPolicy
?? (store.realmMandatoryTopics
? .disableEmptyTopic
: .allowEmptyTopic);
return TopicsPolicy.forRealmPolicy(realmTopicsPolicy);
}
return channelTopicsPolicy;
}with this diff to add TopicsPolicy.forRealmPolicy:
diff --git lib/api/model/model.dart lib/api/model/model.dart
index e5c3f87c4..ddaeb24a3 100644
--- lib/api/model/model.dart
+++ lib/api/model/model.dart
@@ -811,6 +811,14 @@ enum TopicsPolicy {
emptyTopicOnly,
unknown;
+ /// The [TopicsPolicy] corresponding to the given [RealmTopicsPolicy].
+ static TopicsPolicy forRealmPolicy(RealmTopicsPolicy realmPolicy) =>
+ switch (realmPolicy) {
+ .allowEmptyTopic => .allowEmptyTopic,
+ .disableEmptyTopic => .disableEmptyTopic,
+ .unknown => .unknown,
+ };
+
static TopicsPolicy fromApiValue(String value) => _byApiValue[value] ?? unknown;
static final _byApiValue = _$TopicsPolicyEnumMap
There was a problem hiding this comment.
Oh, also: let's instead make _effectiveTopicsPolicy a public method on ChannelStore, maybe just above selfHasContentAccess and selfCanSendMessage. That way we can offer it to other consumers, if desired, in the future.
There was a problem hiding this comment.
?? (store.realmMandatoryTopics
? .disableEmptyTopic
: .allowEmptyTopic);
Is this formatting (no. of spaces) intentional in the suggested code?
| final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | ||
| final finder = find.byWidgetPredicate((widget) => widget is TextField | ||
| && widget.decoration?.hintText == zulipLocalizations.composeBoxTopicHintText); | ||
| && widget.controller is ComposeTopicController); |
There was a problem hiding this comment.
- The following test: "TopicAutocomplete options appear, disappear, and change correctly" fails without the change because the test calls finder twice, once before any interaction with the topic input, and once after interaction.
- Earlier, when
realmMandatoryTopicswas used, it defaulted to true. This meant that no matter the interaction status,hintTextwould be equal tocomposeBoxTopicHintText("Topic"). - Now that
realmTopicsPolicyis used and it defaults tounknown, orallowEmptyTopicaccording to your suggestion,hintTextchanges according to interaction status. So now line 619 returns no element because thehintTextchanged
Line 619 of test/widgets/autocomplete_test.dart:
check(tester.widget<TextField>(topicInputFinder).controller!.text)
.equals(topic3.name.displayName!);
Thus, I made that change and used controller to identify the widget as I felt it is less brittle than matching hintText string. If there is a better way to make that change, please suggest it.
|
Thank you for the review @chrisbobbe. I have addressed all the feedback, PTAL. Doubt: Should the TODO comment about subscribing to store be changed now that |
|
@chrisbobbe It's been more than a week, would appreciate a review! |
Co-authored-by: Ruhaan <ruhaansande@gmail.com>
Co-authored-by: Ruhaan <ruhaansande@gmail.com>
This commit starts treating realm_mandatory_topics as optional so newer servers can stop sending it as it was deprecated in feature level 392. See: https://zulip.com/api/changelog
e5bca43 to
c8f78f1
Compare
|
c825aeb to
1ca1269
Compare
…opics policy Fixes zulip#1604. New 'topicsPolicy' and 'realmTopicsPolicy' were introduced in feature level 392. For older servers, 'effectiveTopicsPolicy' falls back to 'realmMandatoryTopics'. See: https://zulip.com/api/changelog
Fixes #1604.
Completes #1956.
First 3 commits are from #1956. This PR specifically addresses the second paragraph of this comment: #1956 (comment)
Changes Made:
realmMandatoryTopicsand replace withrealmTopicsPolicy.Doubts:
realmMandatoryTopicsconsidered unnecessary?mandatoryfrom effective topics policy for code clarity, and because using switch statement ontopicsPolicyis awkward due totopicsPolicy.unknownandtopicsPolicy.inherit. Is current approach fine?