mirror of
				https://github.com/zulip/zulip.git
				synced 2025-11-03 21:43:21 +00:00 
			
		
		
		
	left sidebar: Add support to unstar all messages in topic.
This adds support for unstarring all (starred) messages from a particular topic, from the topic popover. The earlier implementation of this in #16898 was reverted inbc55aa6a01(#17429) because it had two problems- 1. The crash reported inbc55aa6a01was due to message_store returning undefined. This happens when the message itself hasn't been fetched from the server yet, but we know that the message is starred from the ids in `page_params` in `starred_messages.js`. This commit handles this case explicitly. Note that, we simply ignore those messages which we haven't fetched, and because of this, it may happen that we don't unstar some messages from that topic. The correct implementation for this would be to ask the backend for starred IDs in a topic. 2. The earlier implementation actually unstarred **all** messages. This was because it grabbed the topic and stream_id from the topic popover `data` attributes, after the topic popover had been closed. This passed `undefined`, which the function then interpreted as an action to unstar all messages. With this commit, we use the confirm_dialog widget, which eliminates the need to store this data in the DOM.
This commit is contained in:
		
				
					committed by
					
						
						Tim Abbott
					
				
			
			
				
	
			
			
			
						parent
						
							f17a52b2f3
						
					
				
				
					commit
					42aea49784
				
			@@ -9,7 +9,9 @@ const {run_test} = require("../zjsunit/test");
 | 
				
			|||||||
const page_params = set_global("page_params", {});
 | 
					const page_params = set_global("page_params", {});
 | 
				
			||||||
 | 
					
 | 
				
			||||||
zrequire("timerender");
 | 
					zrequire("timerender");
 | 
				
			||||||
 | 
					const message_store = zrequire("message_store");
 | 
				
			||||||
const starred_messages = zrequire("starred_messages");
 | 
					const starred_messages = zrequire("starred_messages");
 | 
				
			||||||
 | 
					const stream_popover = zrequire("stream_popover");
 | 
				
			||||||
const top_left_corner = zrequire("top_left_corner");
 | 
					const top_left_corner = zrequire("top_left_corner");
 | 
				
			||||||
 | 
					
 | 
				
			||||||
run_test("add starred", (override) => {
 | 
					run_test("add starred", (override) => {
 | 
				
			||||||
@@ -38,6 +40,48 @@ run_test("remove starred", (override) => {
 | 
				
			|||||||
    assert.equal(starred_messages.get_count(), 1);
 | 
					    assert.equal(starred_messages.get_count(), 1);
 | 
				
			||||||
});
 | 
					});
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					run_test("get starred ids in topic", () => {
 | 
				
			||||||
 | 
					    for (const id of [1, 2, 3, 4, 5]) {
 | 
				
			||||||
 | 
					        starred_messages.starred_ids.add(id);
 | 
				
			||||||
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    assert.deepEqual(
 | 
				
			||||||
 | 
					        starred_messages.get_starred_message_ids_in_topic(undefined, "topic name"),
 | 
				
			||||||
 | 
					        [],
 | 
				
			||||||
 | 
					    );
 | 
				
			||||||
 | 
					    assert.deepEqual(starred_messages.get_starred_message_ids_in_topic(3, undefined), []);
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    // id: 1 isn't inserted, to test handling the case
 | 
				
			||||||
 | 
					    // when message_store.get() returns undefined
 | 
				
			||||||
 | 
					    message_store.create_mock_message({
 | 
				
			||||||
 | 
					        id: 2,
 | 
				
			||||||
 | 
					        type: "private",
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					    message_store.create_mock_message({
 | 
				
			||||||
 | 
					        // Different stream
 | 
				
			||||||
 | 
					        id: 3,
 | 
				
			||||||
 | 
					        type: "stream",
 | 
				
			||||||
 | 
					        stream_id: 19,
 | 
				
			||||||
 | 
					        topic: "topic",
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					    message_store.create_mock_message({
 | 
				
			||||||
 | 
					        // Different topic
 | 
				
			||||||
 | 
					        id: 4,
 | 
				
			||||||
 | 
					        type: "stream",
 | 
				
			||||||
 | 
					        stream_id: 20,
 | 
				
			||||||
 | 
					        topic: "some other topic",
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					    message_store.create_mock_message({
 | 
				
			||||||
 | 
					        // Correct match
 | 
				
			||||||
 | 
					        id: 5,
 | 
				
			||||||
 | 
					        type: "stream",
 | 
				
			||||||
 | 
					        stream_id: 20,
 | 
				
			||||||
 | 
					        topic: "topic",
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    assert.deepEqual(starred_messages.get_starred_message_ids_in_topic(20, "topic"), [5]);
 | 
				
			||||||
 | 
					});
 | 
				
			||||||
 | 
					
 | 
				
			||||||
run_test("initialize", (override) => {
 | 
					run_test("initialize", (override) => {
 | 
				
			||||||
    starred_messages.starred_ids.clear();
 | 
					    starred_messages.starred_ids.clear();
 | 
				
			||||||
    for (const id of [1, 2, 3]) {
 | 
					    for (const id of [1, 2, 3]) {
 | 
				
			||||||
@@ -59,6 +103,7 @@ run_test("rerender_ui", () => {
 | 
				
			|||||||
    page_params.starred_message_counts = true;
 | 
					    page_params.starred_message_counts = true;
 | 
				
			||||||
    with_overrides((override) => {
 | 
					    with_overrides((override) => {
 | 
				
			||||||
        const stub = make_stub();
 | 
					        const stub = make_stub();
 | 
				
			||||||
 | 
					        override(stream_popover, "hide_topic_popover", () => {});
 | 
				
			||||||
        override(top_left_corner, "update_starred_count", stub.f);
 | 
					        override(top_left_corner, "update_starred_count", stub.f);
 | 
				
			||||||
        starred_messages.rerender_ui();
 | 
					        starred_messages.rerender_ui();
 | 
				
			||||||
        assert.equal(stub.num_calls, 1);
 | 
					        assert.equal(stub.num_calls, 1);
 | 
				
			||||||
@@ -69,6 +114,7 @@ run_test("rerender_ui", () => {
 | 
				
			|||||||
    page_params.starred_message_counts = false;
 | 
					    page_params.starred_message_counts = false;
 | 
				
			||||||
    with_overrides((override) => {
 | 
					    with_overrides((override) => {
 | 
				
			||||||
        const stub = make_stub();
 | 
					        const stub = make_stub();
 | 
				
			||||||
 | 
					        override(stream_popover, "hide_topic_popover", () => {});
 | 
				
			||||||
        override(top_left_corner, "update_starred_count", stub.f);
 | 
					        override(top_left_corner, "update_starred_count", stub.f);
 | 
				
			||||||
        starred_messages.rerender_ui();
 | 
					        starred_messages.rerender_ui();
 | 
				
			||||||
        assert.equal(stub.num_calls, 1);
 | 
					        assert.equal(stub.num_calls, 1);
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -120,3 +120,8 @@ export function unstar_all_messages() {
 | 
				
			|||||||
    const starred_msg_ids = starred_messages.get_starred_msg_ids();
 | 
					    const starred_msg_ids = starred_messages.get_starred_msg_ids();
 | 
				
			||||||
    send_flag_update_for_messages(starred_msg_ids, "starred", "remove");
 | 
					    send_flag_update_for_messages(starred_msg_ids, "starred", "remove");
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					export function unstar_all_messages_in_topic(stream_id, topic) {
 | 
				
			||||||
 | 
					    const starred_message_ids = starred_messages.get_starred_message_ids_in_topic(stream_id, topic);
 | 
				
			||||||
 | 
					    send_flag_update_for_messages(starred_message_ids, "starred", "remove");
 | 
				
			||||||
 | 
					}
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -1,9 +1,11 @@
 | 
				
			|||||||
import $ from "jquery";
 | 
					import $ from "jquery";
 | 
				
			||||||
 | 
					
 | 
				
			||||||
import render_confirm_unstar_all_messages from "../templates/confirm_unstar_all_messages.hbs";
 | 
					import render_confirm_unstar_all_messages from "../templates/confirm_unstar_all_messages.hbs";
 | 
				
			||||||
 | 
					import render_confirm_unstar_all_messages_in_topic from "../templates/confirm_unstar_all_messages_in_topic.hbs";
 | 
				
			||||||
 | 
					
 | 
				
			||||||
import * as confirm_dialog from "./confirm_dialog";
 | 
					import * as confirm_dialog from "./confirm_dialog";
 | 
				
			||||||
import * as message_flags from "./message_flags";
 | 
					import * as message_flags from "./message_flags";
 | 
				
			||||||
 | 
					import * as message_store from "./message_store";
 | 
				
			||||||
import * as stream_popover from "./stream_popover";
 | 
					import * as stream_popover from "./stream_popover";
 | 
				
			||||||
import * as top_left_corner from "./top_left_corner";
 | 
					import * as top_left_corner from "./top_left_corner";
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -43,6 +45,32 @@ export function get_starred_msg_ids() {
 | 
				
			|||||||
    return Array.from(starred_ids);
 | 
					    return Array.from(starred_ids);
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					export function get_starred_message_ids_in_topic(stream_id, topic) {
 | 
				
			||||||
 | 
					    if (stream_id === undefined || topic === undefined) {
 | 
				
			||||||
 | 
					        return [];
 | 
				
			||||||
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    return Array.from(starred_ids).filter((id) => {
 | 
				
			||||||
 | 
					        const message = message_store.get(id);
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					        if (message === undefined) {
 | 
				
			||||||
 | 
					            // We know the `id` from the initial data fetch from page_params,
 | 
				
			||||||
 | 
					            // but the message itself hasn't been fetched from the server yet.
 | 
				
			||||||
 | 
					            // We ignore this message, since we can't check if it belongs to
 | 
				
			||||||
 | 
					            // the topic, but it could, meaning this implementation isn't
 | 
				
			||||||
 | 
					            // completely correct. The right way to do this would be to ask the
 | 
				
			||||||
 | 
					            // backend for starred IDs from the topic.
 | 
				
			||||||
 | 
					            return false;
 | 
				
			||||||
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					        return (
 | 
				
			||||||
 | 
					            message.type === "stream" &&
 | 
				
			||||||
 | 
					            message.stream_id === stream_id &&
 | 
				
			||||||
 | 
					            message.topic.toLowerCase() === topic.toLowerCase()
 | 
				
			||||||
 | 
					        );
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
export function rerender_ui() {
 | 
					export function rerender_ui() {
 | 
				
			||||||
    let count = get_count();
 | 
					    let count = get_count();
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -51,6 +79,7 @@ export function rerender_ui() {
 | 
				
			|||||||
        count = 0;
 | 
					        count = 0;
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    stream_popover.hide_topic_popover();
 | 
				
			||||||
    top_left_corner.update_starred_count(count);
 | 
					    top_left_corner.update_starred_count(count);
 | 
				
			||||||
    stream_popover.hide_starred_messages_popover();
 | 
					    stream_popover.hide_starred_messages_popover();
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
@@ -67,3 +96,22 @@ export function confirm_unstar_all_messages() {
 | 
				
			|||||||
        on_click: message_flags.unstar_all_messages,
 | 
					        on_click: message_flags.unstar_all_messages,
 | 
				
			||||||
    });
 | 
					    });
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					export function confirm_unstar_all_messages_in_topic(stream_id, topic) {
 | 
				
			||||||
 | 
					    function on_click() {
 | 
				
			||||||
 | 
					        message_flags.unstar_all_messages_in_topic(stream_id, topic);
 | 
				
			||||||
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    const modal_parent = $(".left-sidebar-modal-holder");
 | 
				
			||||||
 | 
					    const html_body = render_confirm_unstar_all_messages_in_topic({
 | 
				
			||||||
 | 
					        topic,
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    confirm_dialog.launch({
 | 
				
			||||||
 | 
					        parent: modal_parent,
 | 
				
			||||||
 | 
					        html_heading: i18n.t("Unstar messages in topic"),
 | 
				
			||||||
 | 
					        html_body,
 | 
				
			||||||
 | 
					        html_yes_button: i18n.t("Unstar messages"),
 | 
				
			||||||
 | 
					        on_click,
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					}
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -234,6 +234,8 @@ function build_topic_popover(opts) {
 | 
				
			|||||||
    const is_muted = muting.is_topic_muted(sub.stream_id, topic_name);
 | 
					    const is_muted = muting.is_topic_muted(sub.stream_id, topic_name);
 | 
				
			||||||
    const can_mute_topic = !is_muted;
 | 
					    const can_mute_topic = !is_muted;
 | 
				
			||||||
    const can_unmute_topic = is_muted;
 | 
					    const can_unmute_topic = is_muted;
 | 
				
			||||||
 | 
					    const has_starred_messages =
 | 
				
			||||||
 | 
					        starred_messages.get_starred_message_ids_in_topic(sub.stream_id, topic_name).length > 0;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    const content = render_topic_sidebar_actions({
 | 
					    const content = render_topic_sidebar_actions({
 | 
				
			||||||
        stream_name: sub.name,
 | 
					        stream_name: sub.name,
 | 
				
			||||||
@@ -243,6 +245,7 @@ function build_topic_popover(opts) {
 | 
				
			|||||||
        can_unmute_topic,
 | 
					        can_unmute_topic,
 | 
				
			||||||
        is_realm_admin: sub.is_realm_admin,
 | 
					        is_realm_admin: sub.is_realm_admin,
 | 
				
			||||||
        color: sub.color,
 | 
					        color: sub.color,
 | 
				
			||||||
 | 
					        has_starred_messages,
 | 
				
			||||||
    });
 | 
					    });
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    $(elt).popover({
 | 
					    $(elt).popover({
 | 
				
			||||||
@@ -432,6 +435,19 @@ export function register_stream_handlers() {
 | 
				
			|||||||
        starred_messages.confirm_unstar_all_messages();
 | 
					        starred_messages.confirm_unstar_all_messages();
 | 
				
			||||||
    });
 | 
					    });
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    // Unstar all messages in topic
 | 
				
			||||||
 | 
					    $("body").on("click", ".sidebar-popover-unstar-all-in-topic", (e) => {
 | 
				
			||||||
 | 
					        e.preventDefault();
 | 
				
			||||||
 | 
					        e.stopPropagation();
 | 
				
			||||||
 | 
					        const topic_name = $(".sidebar-popover-unstar-all-in-topic").attr("data-topic-name");
 | 
				
			||||||
 | 
					        const stream_id = $(".sidebar-popover-unstar-all-in-topic").attr("data-stream-id");
 | 
				
			||||||
 | 
					        hide_topic_popover();
 | 
				
			||||||
 | 
					        starred_messages.confirm_unstar_all_messages_in_topic(
 | 
				
			||||||
 | 
					            Number.parseInt(stream_id, 10),
 | 
				
			||||||
 | 
					            topic_name,
 | 
				
			||||||
 | 
					        );
 | 
				
			||||||
 | 
					    });
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    // Toggle displaying starred message count
 | 
					    // Toggle displaying starred message count
 | 
				
			||||||
    $("body").on("click", "#toggle_display_starred_msg_count", (e) => {
 | 
					    $("body").on("click", "#toggle_display_starred_msg_count", (e) => {
 | 
				
			||||||
        hide_starred_messages_popover();
 | 
					        hide_starred_messages_popover();
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -0,0 +1,5 @@
 | 
				
			|||||||
 | 
					<p>
 | 
				
			||||||
 | 
					    {{#tr this}}
 | 
				
			||||||
 | 
					    Are you sure you want to unstar all messages in topic <b>__topic__</b>?  This action cannot be undone.
 | 
				
			||||||
 | 
					    {{/tr}}
 | 
				
			||||||
 | 
					</p>
 | 
				
			||||||
@@ -34,6 +34,16 @@
 | 
				
			|||||||
        </a>
 | 
					        </a>
 | 
				
			||||||
    </li>
 | 
					    </li>
 | 
				
			||||||
    {{/if}}
 | 
					    {{/if}}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					    {{#if has_starred_messages}}
 | 
				
			||||||
 | 
					    <li>
 | 
				
			||||||
 | 
					        <a href="#" class="sidebar-popover-unstar-all-in-topic" data-stream-id="{{ stream_id }}" data-topic-name="{{ topic_name }}">
 | 
				
			||||||
 | 
					            <i class="fa fa-star-o" aria-hidden="true"></i>
 | 
				
			||||||
 | 
					            {{#tr this}}Unstar all messages in topic{{/tr}}
 | 
				
			||||||
 | 
					        </a>
 | 
				
			||||||
 | 
					    </li>
 | 
				
			||||||
 | 
					    {{/if}}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    <li>
 | 
					    <li>
 | 
				
			||||||
        <a tabindex="0" class="sidebar-popover-mark-topic-read" data-stream-id="{{ stream_id }}" data-topic-name="{{ topic_name }}">
 | 
					        <a tabindex="0" class="sidebar-popover-mark-topic-read" data-stream-id="{{ stream_id }}" data-topic-name="{{ topic_name }}">
 | 
				
			||||||
            <i class="fa fa-book" aria-hidden="true"></i>
 | 
					            <i class="fa fa-book" aria-hidden="true"></i>
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user