narrow: Mark messages as read when applying -is:dm filter.

It will mark messages as read when applying -is:dm filter alone or coupled with stream and topic filters. But it will not mark messages as read when there is also search term present. Fixes #25113.
zulip · syed-rafat · Jun 2, 2023 · Jun 2, 2023 · Jun 2, 2023 · 97fd8d229e352a7e73f8c334a838fd5547479805
commit 97fd8d229e352a7e73f8c334a838fd5547479805
diff --git a/web/src/filter.js b/web/src/filter.js
@@ -550,11 +550,36 @@ export class Filter {
             return true;
         }
 
+        const term_types = this.sorted_term_types();
+
         // TODO: Some users really hate it when Zulip marks messages as read
         // in interleaved views, so we will eventually have a setting
         // that early-exits before the subsequent checks.
         // (in which case, is_common_narrow would also need to be modified)
 
+        // -is:dm excludes direct messages from all messages
+        if (_.isEqual(term_types, ["not-is-dm"])) {
+            return true;
+        }
+
+        // Excluding direct messages from stream and topic does not
+        // accomplish anything, still adding code to be consistent with our design.
+
+        if (_.isEqual(term_types, ["stream", "not-is-dm"])) {
+            return true;
+        }
+
+        if (_.isEqual(term_types, ["topic", "not-is-dm"])) {
+            return true;
+        }
+
+        // Excluding direct messages from stream and topic does not
+        // accomplish anything, but we are still letting user mark
+        // mark messages as read to be consistent with our design.
+        if (_.isEqual(term_types, ["stream", "topic", "not-is-dm"])) {
+            return true;
+        }
+
         return false;
     }
 
@@ -571,10 +596,6 @@ export class Filter {
     // https://paper.dropbox.com/doc/Navbar-behavior-table--AvnMKN4ogj3k2YF5jTbOiVv_AQ-cNOGtu7kSdtnKBizKXJge
     // common narrows show a narrow description and allow the user to
     // close search bar UI and show the narrow description UI.
-    //
-    // TODO: We likely will want to rewrite this to not piggy-back on
-    // can_mark_messages_read, since that might gain some more complex behavior
-    // with near: narrows.
     is_common_narrow() {
         // can_mark_messages_read tests the following filters:
         // stream, stream + topic,

diff --git a/web/tests/filter.test.js b/web/tests/filter.test.js
@@ -194,6 +194,52 @@ test("basics", () => {
     assert.ok(!filter.is_personal_filter());
     assert.ok(!filter.is_conversation_view());
 
+    operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("stream"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_negated_operand("stream", "not-is-dm"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+
+    operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("stream"));
+    assert.ok(filter.has_operator("topic"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.has_negated_operand("stream", "foo"));
+    assert.ok(!filter.has_negated_operand("topic", "bar"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+    assert.ok(filter.can_bucket_by("stream"));
+    assert.ok(filter.can_bucket_by("stream", "topic"));
+
+    operators = [
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.has_operator("topic"));
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.has_negated_operand("topic", "bar"));
+    assert.ok(filter.can_apply_locally());
+    assert.ok(!filter.is_personal_filter());
+
     operators = [{operator: "is", operand: "dm"}];
     filter = new Filter(operators);
     assert.ok(filter.contains_only_private_messages());
@@ -204,6 +250,15 @@ test("basics", () => {
     assert.ok(!filter.is_personal_filter());
     assert.ok(!filter.is_conversation_view());
 
+    operators = [{operator: "is", operand: "dm", negated: true}];
+    filter = new Filter(operators);
+    assert.ok(!filter.contains_only_private_messages());
+    assert.ok(filter.can_mark_messages_read());
+    assert.ok(filter.supports_collapsing_recipients());
+    assert.ok(!filter.has_operator("search"));
+    assert.ok(!filter.is_personal_filter());
+    assert.ok(!filter.is_conversation_view());
+
     // "is:private" was renamed to "is:dm"
     operators = [{operator: "is", operand: "private"}];
     filter = new Filter(operators);
@@ -444,6 +499,37 @@ test("can_mark_messages_read", () => {
     filter = new Filter(stream_negated_topic_operators);
     assert.ok(!filter.can_mark_messages_read());
 
+    const stream_negated_dm_operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(stream_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_has_operands(stream_negated_dm_operators);
+    assert_not_mark_read_with_is_operands(stream_negated_dm_operators);
+    assert_not_mark_read_when_searching(stream_negated_dm_operators);
+
+    const stream_topic_negated_dm_operators = [
+        {operator: "stream", operand: "foo"},
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(stream_topic_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_has_operands(stream_topic_negated_dm_operators);
+    assert_not_mark_read_with_is_operands(stream_topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(stream_topic_negated_dm_operators);
+
+    const topic_negated_dm_operators = [
+        {operator: "topic", operand: "bar"},
+        {operator: "is", operand: "dm", negated: true},
+    ];
+    filter = new Filter(topic_negated_dm_operators);
+    assert.ok(filter.can_mark_messages_read());
+    assert_not_mark_read_with_is_operands(topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(topic_negated_dm_operators);
+    assert_not_mark_read_when_searching(topic_negated_dm_operators);
+
     const dm = [{operator: "dm", operand: "joe@example.com,"}];
 
     const dm_negated = [{operator: "dm", operand: "joe@example.com,", negated: true}];
@@ -469,6 +555,10 @@ test("can_mark_messages_read", () => {
     assert_not_mark_read_with_has_operands(is_dm);
     assert_not_mark_read_when_searching(is_dm);
 
+    const not_is_dm = [{operator: "is", operand: "dm", negated: true}];
+    filter = new Filter(not_is_dm);
+    assert.ok(filter.can_mark_messages_read());
+
     const in_all = [{operator: "in", operand: "all"}];
     filter = new Filter(in_all);
     assert.ok(filter.can_mark_messages_read());