Respect realm setting for mandatory topics #1296

PIG208 · 2025-01-21T23:12:28Z

This tracks mandatory_topics from realm settings and set up the compose box to read it. This does not yet track for updates to the setting (that'd be #668, out-of-scope for this PR).

This prepares for #1250.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2025-01-24T21:45:29Z

test/widgets/compose_box_test.dart

@@ -44,7 +44,6 @@ void main() {
  Future<void> prepareComposeBox(WidgetTester tester, {
    required Narrow narrow,
    User? selfUser,
-    int? realmWaitingPeriodThreshold,


Commit-message nit:

compose test [nfc]: Remove unused prepareComposeBox param The parameter `realmWaitingPeriodThreshold` was added in commit 157418c26f003b783036dd5184364434cee7d197 but it was never used. Signed-off-by: Zixuan James Li <[email protected]>

For readability, I like use just the first 9 characters of a commit ID.

Bump on truncating commit IDs to 9 characters :)

Oops, missed that. Fixed!

chrisbobbe · 2025-01-24T22:01:52Z

test/widgets/compose_box_test.dart

@@ -75,6 +62,25 @@ void main() {
    controller = tester.state<ComposeBoxState>(find.byType(ComposeBox)).controller;
  }

+  Future<void> prepare(WidgetTester tester, {


compose test [nfc]: Extract prepareComposeBox

In a file that's entirely about compose-box tests (compose_box_test.dart), and in the same scope (just at the top of the main method), it feels confusing to have one function named prepare and another named prepareComposeBox. If I were writing a test, I don't think I would know which function to use, except by reading their implementations.

How about instead of this refactor, we make prepare more flexible (if needed) so it can be used in the new tests?

chrisbobbe · 2025-01-24T22:03:10Z

test/widgets/compose_box_test.dart

@@ -48,6 +48,8 @@ void main() {
    if (narrow case ChannelNarrow(:var streamId) || TopicNarrow(: var streamId)) {
      assert(store.streams.values.any((stream) => stream.streamId == streamId),
        'Add a channel with "streamId" the same as of $narrow.streamId to the store.');
+      assert(store.users.values.any((user) => user.userId == account.userId),
+        'Add an user with "userId" the same as of $account.userId to the store.');


nit: "a user"

The assertion will be removed as account is replaced with zulipFeatureLevel.

chrisbobbe · 2025-01-24T22:05:55Z

lib/widgets/compose_box.dart

    _update();
  }

+  PerAccountStore store;
+
  // TODO: subscribe to this value:
  //   https://zulip.com/help/require-topics


What's the condition for this existing TODO to be finished, and can we be more concrete about it? It looks like it's meant to be a TODO(#668), right? Does ComposeTopicController need to add a PerAccountStore listener?

Does ComposeTopicController need to add a PerAccountStore listener?

Right. We may include that as a part of #668 or its follow-up. This would be something for the future us to triage. I will update to mention the issue.

chrisbobbe · 2025-01-24T22:09:04Z

lib/widgets/compose_box.dart

    switch (widget.narrow) {
      case ChannelNarrow():
-        _controller = StreamComposeBoxController();
+        final store = PerAccountStoreWidget.of(context);
+        _controller ??= StreamComposeBoxController(store: store);


Does the old value of _controller (if non-null) need to be dispose()d?

I think _controller shouldn't be disposed until the ComposeBox is disposed; we only set it once when initializing, and update its reference to the store if there is a new store.

Ah right, I see; I read too fast and thought we were replacing the controller here, rather than updating its reference to the store.

PIG208 · 2025-01-27T20:46:41Z

Updated the PR. Thanks for the review!

chrisbobbe

Thanks! LGTM except some nits; I'll mark for Greg's review.

chrisbobbe · 2025-01-28T20:25:26Z

test/widgets/compose_box_test.dart

@@ -44,7 +44,6 @@ void main() {
  Future<void> prepareComposeBox(WidgetTester tester, {
    required Narrow narrow,
    User? selfUser,
-    int? realmWaitingPeriodThreshold,


Bump on truncating commit IDs to 9 characters :)

chrisbobbe · 2025-01-28T20:26:16Z

test/widgets/compose_box_test.dart

-    List<User> users = const [],
+    List<User> otherUsers = const [],


nit: the commit message says otherUser which doesn't match this name

gnprice · 2025-01-29T01:58:15Z

Looks like CI is failing, and there are also a number of added commits that weren't in yesterday's revision of the branch. Are those meant for a later PR?

PIG208 · 2025-01-29T02:56:04Z

Ahh yes. Those are for general chat (whose base is the tip of this PR).

Pushed to fix this.

gnprice

Thanks both! Generally this all looks good. A few comments, mainly in the tests.

gnprice · 2025-01-29T21:44:43Z

test/example_data.dart

@@ -890,6 +891,7 @@ InitialSnapshot initialSnapshot({
      emojiset: Emojiset.google,
    ),
    userTopics: userTopics,
+    realmMandatoryTopics: realmMandatoryTopics ?? false,


Probably true is the best default for test data — test data should be boring, and Zulip's behavior when a topic is empty/omitted is a lot more complicated (both before and after introducing "general chat") than the behavior of not allowing that.

gnprice · 2025-01-29T21:50:31Z

lib/widgets/compose_box.dart

+        _controller ??= StreamComposeBoxController(store: store);
+        (controller as StreamComposeBoxController).topic.store = store;


Only one of these two lines does anything on a given call of this function, right? So this would probably be clearer as an if/else on whether the controller is null.

gnprice · 2025-01-29T21:56:29Z

lib/widgets/compose_box.dart

@@ -1370,15 +1375,16 @@ class _ComposeBoxState extends State<ComposeBox> implements ComposeBoxState {
      return _ComposeBoxContainer(body: null, errorBanner: errorBanner);
    }

+    final controller = _controller!;


nit / handy pattern:

Suggested change

final controller = _controller!;

final controller = this.controller;

gnprice · 2025-01-29T21:59:04Z

test/widgets/compose_box_test.dart

+      checkMessageNotSent(tester);
+    });
+
+    testWidgets('if topics are mandatory, reject (no topic)', (tester) async {


nit:

Suggested change

testWidgets('if topics are mandatory, reject (no topic)', (tester) async {

testWidgets('if topics are mandatory, reject "(no topic)"', (tester) async {

Otherwise it reads like: "if topics are mandatory, reject. And by the way, no topic."

gnprice · 2025-01-29T22:03:19Z

test/widgets/compose_box_test.dart

+    testWidgets('empty topic -> (no topic)', (tester) async {
+      await setupAndTapSend(tester, topicInputText: '');


Regardless of the defaults in the example data, this test case should make explicit that it's setting realmMandatoryTopics to false — that's critical to understanding the intended behavior of the test.

To understand any given test case, the reader shouldn't have to look up any of the details in example_data.dart — any details that matter should be explicit in the test case.

gnprice · 2025-01-29T23:13:08Z

test/widgets/compose_box_test.dart

+        ..bodyFields.deepEquals({
+          'type': 'stream',
+          'to': channel.streamId.toString(),
+          'topic': '(no topic)',
+          'content': 'test content',
+          'read_by_sender': 'true',
+        });


Let's simplify this to focus on just the part that this test is about:

Suggested change

..bodyFields.deepEquals({

'type': 'stream',

'to': channel.streamId.toString(),

'topic': '(no topic)',

'content': 'test content',

'read_by_sender': 'true',

});

..bodyFields['topic'].equals('(no topic)');

(The .method and .url.path checks probably still belong, as they're part of confirming that the request was to the expected endpoint — effectively they provide the namespace that the parameter name 'topic' draws its meaning from.)

PIG208 · 2025-01-29T23:40:02Z

Thanks for the review! The PR has been updated.

This covers some of the fields that can be inconsistent (`zulipFeatureLevel` in particular) when a test is not set up properly Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

The parameter `realmWaitingPeriodThreshold` was added in commit 157418c but it was never used. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2025-01-30T23:39:57Z

Thanks! Looks good; merging.

PIG208 added the a-compose Compose box, autocomplete, attaching files/images label Jan 21, 2025

PIG208 force-pushed the pr-mandatory branch 3 times, most recently from ce0794e to dce220f Compare January 21, 2025 23:27

PIG208 requested a review from chrisbobbe January 21, 2025 23:27

PIG208 assigned chrisbobbe Jan 21, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 22, 2025

PIG208 force-pushed the pr-mandatory branch from dce220f to 03f6dc3 Compare January 22, 2025 21:33

PIG208 mentioned this pull request Jan 22, 2025

Support general chat #1297

Open

PIG208 force-pushed the pr-mandatory branch 2 times, most recently from f0dccf5 to 7c6e2ba Compare January 23, 2025 17:35

chrisbobbe reviewed Jan 24, 2025

View reviewed changes

PIG208 force-pushed the pr-mandatory branch from 7c6e2ba to 2a5f159 Compare January 27, 2025 20:46

chrisbobbe reviewed Jan 28, 2025

View reviewed changes

chrisbobbe requested a review from gnprice January 28, 2025 20:29

chrisbobbe assigned gnprice and unassigned gnprice Jan 28, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jan 28, 2025

PIG208 force-pushed the pr-mandatory branch 2 times, most recently from da29ecb to 2e0e38a Compare January 28, 2025 21:07

PIG208 force-pushed the pr-mandatory branch from 97fe5ec to 2e0e38a Compare January 29, 2025 02:55

PIG208 force-pushed the pr-mandatory branch from 2e0e38a to 4f5d147 Compare January 29, 2025 03:01

gnprice reviewed Jan 29, 2025

View reviewed changes

PIG208 force-pushed the pr-mandatory branch from 4f5d147 to 7b1a3ee Compare January 29, 2025 23:39

store test: Check for data consistency when adding account

8decc4e

This covers some of the fields that can be inconsistent (`zulipFeatureLevel` in particular) when a test is not set up properly Signed-off-by: Zixuan James Li <[email protected]>

PIG208 added 4 commits January 30, 2025 15:38

store: Add realmMandatoryTopics realm setting

3098349

Signed-off-by: Zixuan James Li <[email protected]>

compose test [nfc]: Remove unused prepareComposeBox param

9e5f803

The parameter `realmWaitingPeriodThreshold` was added in commit 157418c but it was never used. Signed-off-by: Zixuan James Li <[email protected]>

compose test [nfc]: Rename param from user to otherUsers

f76b003

Signed-off-by: Zixuan James Li <[email protected]>

compose: Respect realm setting for mandatory topics

f1646c4

Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the pr-mandatory branch from 7b1a3ee to f1646c4 Compare January 30, 2025 23:39

gnprice merged commit f1646c4 into zulip:main Jan 30, 2025

PIG208 deleted the pr-mandatory branch January 30, 2025 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect realm setting for mandatory topics #1296

Respect realm setting for mandatory topics #1296

PIG208 commented Jan 21, 2025

chrisbobbe left a comment

chrisbobbe Jan 24, 2025

chrisbobbe Jan 28, 2025

PIG208 Jan 28, 2025

chrisbobbe Jan 24, 2025

chrisbobbe Jan 24, 2025

PIG208 Jan 27, 2025

chrisbobbe Jan 24, 2025

PIG208 Jan 27, 2025

chrisbobbe Jan 24, 2025

PIG208 Jan 24, 2025

chrisbobbe Jan 24, 2025

PIG208 commented Jan 27, 2025

chrisbobbe left a comment

chrisbobbe Jan 28, 2025

chrisbobbe Jan 28, 2025

gnprice commented Jan 29, 2025

PIG208 commented Jan 29, 2025 •

edited

Loading

gnprice left a comment

gnprice Jan 29, 2025

gnprice Jan 29, 2025

gnprice Jan 29, 2025

gnprice Jan 29, 2025

gnprice Jan 29, 2025

gnprice Jan 29, 2025

PIG208 commented Jan 29, 2025

gnprice commented Jan 30, 2025

		List<User> users = const [],
		List<User> otherUsers = const [],

		_controller ??= StreamComposeBoxController(store: store);
		(controller as StreamComposeBoxController).topic.store = store;

	final controller = _controller!;
	final controller = this.controller;

	testWidgets('if topics are mandatory, reject (no topic)', (tester) async {
	testWidgets('if topics are mandatory, reject "(no topic)"', (tester) async {

		testWidgets('empty topic -> (no topic)', (tester) async {
		await setupAndTapSend(tester, topicInputText: '');

Respect realm setting for mandatory topics #1296

Respect realm setting for mandatory topics #1296

Conversation

PIG208 commented Jan 21, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 27, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Jan 29, 2025

PIG208 commented Jan 29, 2025 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 29, 2025

gnprice commented Jan 30, 2025

PIG208 commented Jan 29, 2025 •

edited

Loading