filter: Fix `adjusted_terms_if_moved` handling for non stream messages.

Previously, when "adjusted_terms_if_moved" was called for non stream
messages, it used to return "null" for adjusted terms. This is true
for some extent since the non stream messages can not be moved.

However, in case of narrow containing "with" operator, this would
be buggy since there, we want the narrow terms to be adjusted
according to the "with" operand to point to the current narrow,
rather than returning null.

This commit updates the method to return null only if  the "raw_terms"
dont contain "with" operator. Else, adjust the "raw_terms" according
to the message.

web/src/filter.ts

@@ -807,13 +807,27 @@ export class Filter {
     }
 
     static adjusted_terms_if_moved(raw_terms: NarrowTerm[], message: Message): NarrowTerm[] | null {
+        // In case of narrow containing non-stream messages, we replace the
+        // channel/topic/dm operators with singular dm operator corresponding
+        // to the message if it contains `with` operator.
         if (message.type !== "stream") {
-            // BUG: We should be replacing the channel/topic/dm
-            // operators with the singular dm operator for the
-            // conversation in question. This isn't very important,
-            // because direct messages cannot be moved, but it should
-            // be fixed, since it is triggerable incorrect behavior.
-            return null;
+            const contains_with_operator = raw_terms.some((term) => term.operator === "with");
+
+            if (!contains_with_operator) {
+                return null;
+            }
+            const conversation_terms = new Set(["channel", "topic", "dm"]);
+            const filtered_terms = raw_terms.filter((term) => {
+                const operator = Filter.canonicalize_operator(term.operator);
+                return !conversation_terms.has(operator);
+            });
+
+            assert(typeof message.display_recipient !== "string");
+            const dm_participants = message.display_recipient.map((user) => user.email);
+            const dm_operand = dm_participants.join(",");
+
+            const dm_conversation_terms = [{operator: "dm", operand: dm_operand, negated: false}];
+            return [...dm_conversation_terms, ...filtered_terms];
         }
 
         assert(typeof message.display_recipient === "string");

web/src/narrow_state.ts

@@ -3,7 +3,6 @@ import assert from "minimalistic-assert";
 import * as blueslip from "./blueslip";
 import {Filter} from "./filter";
 import * as message_lists from "./message_lists";
-import * as message_store from "./message_store";
 import {page_params} from "./page_params";
 import * as people from "./people";
 import type {NarrowTerm} from "./state_data";
@@ -286,18 +285,6 @@ export function _possible_unread_message_ids(
         sub = stream_sub(current_filter);
         topic_name = topic(current_filter);
 
-        const with_operand = current_filter.operands("with")[0]!;
-        const target_id = Number.parseInt(with_operand, 10);
-        const target_message = message_store.get(target_id)!;
-
-        if (target_message?.type === "private") {
-            // BUG: In theory, the fact that we've asserted
-            // !current_filter.requires_adjustment_for_moved_with_target
-            // should mean this is not possible; but
-            // filter.adjusted_terms_if_moved incorrectly does not
-            // ensure this. Once that bug is fixed, we can delete this case.
-            return [];
-        }
         if (sub === undefined || topic_name === undefined) {
             /* istanbul ignore next */
             return [];

web/tests/filter.test.js

@@ -1762,7 +1762,7 @@ test("try_adjusting_for_moved_with_target", ({override}) => {
     const messages = {
         12: {type: "stream", display_recipient: "Scotland", topic: "Test 1", id: 12},
         17: {type: "stream", display_recipient: "Verona", topic: "Test 2", id: 17},
-        2: {type: "direct", id: 2},
+        2: {type: "direct", id: 2, display_recipient: [{id: 3, email: "user3@zulip.com"}]},
     };
 
     override(message_store, "get", (msg_id) => messages[msg_id]);
@@ -1807,9 +1807,9 @@ test("try_adjusting_for_moved_with_target", ({override}) => {
     assert.deepEqual(filter.requires_adjustment_for_moved_with_target, true);
     assert.deepEqual(filter.terms(), terms);
 
-    // Since only "stream" recipient type can be moved, if the message
-    // is of any type other than "stream", same narrow terms are
-    // returned without the `with` operator.
+    // When the narrow consists of `channel` or `topic` operators, while
+    // the `with` operator corresponds to that of a direct message, then
+    // the narrow is adjusted to point to the narrow containing the message.
     terms = [
         {operator: "channel", operand: "Scotland", negated: false},
         {operator: "topic", operand: "Test 1", negated: false},
@@ -1818,7 +1818,9 @@ test("try_adjusting_for_moved_with_target", ({override}) => {
     filter = new Filter(terms);
     filter.try_adjusting_for_moved_with_target();
     assert.deepEqual(filter.requires_adjustment_for_moved_with_target, false);
-    assert.deepEqual(filter.terms(), terms);
+    assert.deepEqual(filter.terms(), [
+        {operator: "dm", operand: "user3@zulip.com", negated: false},
+    ]);
 });
 
 function make_private_sub(name, stream_id) {
@@ -2375,12 +2377,26 @@ run_test("equals", () => {
 });
 
 run_test("adjusted_terms_if_moved", () => {
-    // should return null for non-stream messages
+    // should return null for non-stream messages containing no
+    // `with` operator
     let raw_terms = [{operator: "channel", operand: "Foo"}];
     let message = {type: "private"};
     let result = Filter.adjusted_terms_if_moved(raw_terms, message);
     assert.strictEqual(result, null);
 
+    // should adjust terms to contain `dm` for non-stream messages
+    // if it contains `with` operator
+    message = {type: "private", id: 2, display_recipient: [{id: 3, email: "user3@zulip.com"}]};
+    raw_terms = [
+        {operator: "channel", operand: "Foo"},
+        {operator: "with", operand: `${message.id}`},
+    ];
+    result = Filter.adjusted_terms_if_moved(raw_terms, message);
+    assert.deepEqual(result, [
+        {operator: "dm", operand: "user3@zulip.com", negated: false},
+        {operator: "with", operand: "2"},
+    ]);
+
     // should return null if no terms are changed
     raw_terms = [{operator: "channel", operand: "general"}];
     message = {type: "stream", display_recipient: "general", topic: "discussion"};

web/tests/narrow_unread.test.js

@@ -76,7 +76,7 @@ run_test("get_unread_ids", () => {
         id: 102,
         type: "private",
         unread: true,
-        display_recipient: [{id: alice.user_id}],
+        display_recipient: [{id: alice.user_id, email: alice.email}],
     };
 
     const other_topic_message = {
@@ -249,7 +249,7 @@ run_test("get_unread_ids", () => {
     ];
     set_filter(terms);
     unread_ids = candidate_ids();
-    assert.deepEqual(unread_ids, []);
+    assert.deepEqual(unread_ids, [private_msg.id]);
 
     message_lists.set_current(undefined);
     blueslip.expect("error", "unexpected call to get_first_unread_info");