mirror of
				https://github.com/zulip/zulip.git
				synced 2025-11-04 05:53:43 +00:00 
			
		
		
		
	refactor: Remove stream_data.update_subscribers_count.
This removes a bit of complexity. If a piece of settings code needs to render a stream with subscribers, it just asks for it. We no longer have the brittle, action-at-a-distance mechanism of mutating the subscriber count on to the stream_data version of a sub. Stream subs are pretty small, so making copies of them is cheap, and the blueslip timings from the previous commit can help confirm that. There is some discussion of putting `subscriber_count` on the Stream model, which may eventually get us away from tracking it in `peer_data.js`, but we will cross that bridge when we get there. See https://github.com/zulip/zulip/issues/17101 for more details.
This commit is contained in:
		@@ -239,8 +239,7 @@ run_test("subscribers", () => {
 | 
				
			|||||||
    assert(ok);
 | 
					    assert(ok);
 | 
				
			||||||
    assert(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
					    assert(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
				
			||||||
    sub = stream_data.get_sub("Rome");
 | 
					    sub = stream_data.get_sub("Rome");
 | 
				
			||||||
    stream_data.update_subscribers_count(sub);
 | 
					    assert.equal(peer_data.get_subscriber_count(sub.stream_id), 1);
 | 
				
			||||||
    assert.equal(sub.subscriber_count, 1);
 | 
					 | 
				
			||||||
    const sub_email = "Rome:214125235@zulipdev.com:9991";
 | 
					    const sub_email = "Rome:214125235@zulipdev.com:9991";
 | 
				
			||||||
    stream_data.update_stream_email_address(sub, sub_email);
 | 
					    stream_data.update_stream_email_address(sub, sub_email);
 | 
				
			||||||
    assert.equal(sub.email_address, sub_email);
 | 
					    assert.equal(sub.email_address, sub_email);
 | 
				
			||||||
@@ -249,16 +248,14 @@ run_test("subscribers", () => {
 | 
				
			|||||||
    peer_data.add_subscriber(sub.stream_id, brutus.user_id);
 | 
					    peer_data.add_subscriber(sub.stream_id, brutus.user_id);
 | 
				
			||||||
    assert(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
					    assert(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
				
			||||||
    sub = stream_data.get_sub("Rome");
 | 
					    sub = stream_data.get_sub("Rome");
 | 
				
			||||||
    stream_data.update_subscribers_count(sub);
 | 
					    assert.equal(peer_data.get_subscriber_count(sub.stream_id), 1);
 | 
				
			||||||
    assert.equal(sub.subscriber_count, 1);
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
    // remove
 | 
					    // remove
 | 
				
			||||||
    ok = peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
 | 
					    ok = peer_data.remove_subscriber(sub.stream_id, brutus.user_id);
 | 
				
			||||||
    assert(ok);
 | 
					    assert(ok);
 | 
				
			||||||
    assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
					    assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
				
			||||||
    sub = stream_data.get_sub("Rome");
 | 
					    sub = stream_data.get_sub("Rome");
 | 
				
			||||||
    stream_data.update_subscribers_count(sub);
 | 
					    assert.equal(peer_data.get_subscriber_count(sub.stream_id), 0);
 | 
				
			||||||
    assert.equal(sub.subscriber_count, 0);
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
    // verify that checking subscription with undefined user id
 | 
					    // verify that checking subscription with undefined user id
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -280,8 +277,7 @@ run_test("subscribers", () => {
 | 
				
			|||||||
    assert(!ok);
 | 
					    assert(!ok);
 | 
				
			||||||
    assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
					    assert(!stream_data.is_user_subscribed(sub.stream_id, brutus.user_id));
 | 
				
			||||||
    sub = stream_data.get_sub("Rome");
 | 
					    sub = stream_data.get_sub("Rome");
 | 
				
			||||||
    stream_data.update_subscribers_count(sub);
 | 
					    assert.equal(peer_data.get_subscriber_count(sub.stream_id), 0);
 | 
				
			||||||
    assert.equal(sub.subscriber_count, 0);
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
    // Verify defensive code in set_subscribers, where the second parameter
 | 
					    // Verify defensive code in set_subscribers, where the second parameter
 | 
				
			||||||
    // can be undefined.
 | 
					    // can be undefined.
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -352,6 +352,25 @@ exports.get_unsorted_subs = function () {
 | 
				
			|||||||
    return Array.from(stream_info.values());
 | 
					    return Array.from(stream_info.values());
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					exports.get_sub_for_settings = (sub) => {
 | 
				
			||||||
 | 
					    // Since we make a copy of the sub here, it may eventually
 | 
				
			||||||
 | 
					    // make sense to get the other calculated fields here as
 | 
				
			||||||
 | 
					    // well, instead of using update_calculated_fields everywhere.
 | 
				
			||||||
 | 
					    const sub_count = peer_data.get_subscriber_count(sub.stream_id);
 | 
				
			||||||
 | 
					    return {
 | 
				
			||||||
 | 
					        ...sub,
 | 
				
			||||||
 | 
					        subscriber_count: sub_count,
 | 
				
			||||||
 | 
					    };
 | 
				
			||||||
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					function get_subs_for_settings(subs) {
 | 
				
			||||||
 | 
					    // We may eventually add subscribers to the subs here, rather than
 | 
				
			||||||
 | 
					    // delegating, so that we can more efficiently compute subscriber counts
 | 
				
			||||||
 | 
					    // (in bulk).  If that plan appears to have been aborted, feel free to
 | 
				
			||||||
 | 
					    // inline this.
 | 
				
			||||||
 | 
					    return subs.map((sub) => exports.get_sub_for_settings(sub));
 | 
				
			||||||
 | 
					}
 | 
				
			||||||
 | 
					
 | 
				
			||||||
exports.get_updated_unsorted_subs = function () {
 | 
					exports.get_updated_unsorted_subs = function () {
 | 
				
			||||||
    // This function is expensive in terms of calculating
 | 
					    // This function is expensive in terms of calculating
 | 
				
			||||||
    // some values (particularly stream counts) but avoids
 | 
					    // some values (particularly stream counts) but avoids
 | 
				
			||||||
@@ -368,7 +387,7 @@ exports.get_updated_unsorted_subs = function () {
 | 
				
			|||||||
        all_subs = all_subs.filter((sub) => sub.subscribed);
 | 
					        all_subs = all_subs.filter((sub) => sub.subscribed);
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    return all_subs;
 | 
					    return get_subs_for_settings(all_subs);
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
exports.num_subscribed_subs = function () {
 | 
					exports.num_subscribed_subs = function () {
 | 
				
			||||||
@@ -423,13 +442,6 @@ exports.get_colors = function () {
 | 
				
			|||||||
    return exports.subscribed_subs().map((sub) => sub.color);
 | 
					    return exports.subscribed_subs().map((sub) => sub.color);
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
exports.update_subscribers_count = function (sub) {
 | 
					 | 
				
			||||||
    // This is part of an unfortunate legacy hack, where we
 | 
					 | 
				
			||||||
    // put calculated fields onto the sub object instead of
 | 
					 | 
				
			||||||
    // letting callers build their own objects.
 | 
					 | 
				
			||||||
    sub.subscriber_count = peer_data.get_subscriber_count(sub.stream_id);
 | 
					 | 
				
			||||||
};
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
exports.update_stream_email_address = function (sub, email) {
 | 
					exports.update_stream_email_address = function (sub, email) {
 | 
				
			||||||
    sub.email_address = email;
 | 
					    sub.email_address = email;
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
@@ -462,6 +474,8 @@ exports.receives_notifications = function (stream_id, notification_name) {
 | 
				
			|||||||
};
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
exports.update_calculated_fields = function (sub) {
 | 
					exports.update_calculated_fields = function (sub) {
 | 
				
			||||||
 | 
					    // Note that we don't calculate subscriber counts here.
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    sub.is_realm_admin = page_params.is_admin;
 | 
					    sub.is_realm_admin = page_params.is_admin;
 | 
				
			||||||
    // Admin can change any stream's name & description either stream is public or
 | 
					    // Admin can change any stream's name & description either stream is public or
 | 
				
			||||||
    // private, subscribed or unsubscribed.
 | 
					    // private, subscribed or unsubscribed.
 | 
				
			||||||
@@ -485,7 +499,6 @@ exports.update_calculated_fields = function (sub) {
 | 
				
			|||||||
    if (sub.rendered_description !== undefined) {
 | 
					    if (sub.rendered_description !== undefined) {
 | 
				
			||||||
        sub.rendered_description = sub.rendered_description.replace("<p>", "").replace("</p>", "");
 | 
					        sub.rendered_description = sub.rendered_description.replace("<p>", "").replace("</p>", "");
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
    exports.update_subscribers_count(sub);
 | 
					 | 
				
			||||||
 | 
					
 | 
				
			||||||
    // Apply the defaults for our notification settings for rendering.
 | 
					    // Apply the defaults for our notification settings for rendering.
 | 
				
			||||||
    for (const setting of settings_config.stream_specific_notification_settings) {
 | 
					    for (const setting of settings_config.stream_specific_notification_settings) {
 | 
				
			||||||
@@ -786,7 +799,7 @@ exports.get_streams_for_settings_page = function () {
 | 
				
			|||||||
        exports.update_calculated_fields(sub);
 | 
					        exports.update_calculated_fields(sub);
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    return all_subs;
 | 
					    return get_subs_for_settings(all_subs);
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
 | 
					
 | 
				
			||||||
exports.sort_for_stream_settings = function (stream_ids, order) {
 | 
					exports.sort_for_stream_settings = function (stream_ids, order) {
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -213,7 +213,6 @@ exports.rerender_subscriptions_settings = function (sub) {
 | 
				
			|||||||
        blueslip.error("Undefined sub passed to function rerender_subscriptions_settings");
 | 
					        blueslip.error("Undefined sub passed to function rerender_subscriptions_settings");
 | 
				
			||||||
        return;
 | 
					        return;
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
    stream_data.update_subscribers_count(sub);
 | 
					 | 
				
			||||||
    stream_ui_updates.update_subscribers_count(sub);
 | 
					    stream_ui_updates.update_subscribers_count(sub);
 | 
				
			||||||
    stream_ui_updates.update_subscribers_list(sub);
 | 
					    stream_ui_updates.update_subscribers_list(sub);
 | 
				
			||||||
};
 | 
					};
 | 
				
			||||||
@@ -237,8 +236,10 @@ exports.add_sub_to_table = function (sub) {
 | 
				
			|||||||
        return;
 | 
					        return;
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    const html = render_subscription(sub);
 | 
					    const setting_sub = stream_data.get_sub_for_settings(sub);
 | 
				
			||||||
 | 
					    const html = render_subscription(setting_sub);
 | 
				
			||||||
    const settings_html = render_subscription_settings(sub);
 | 
					    const settings_html = render_subscription_settings(sub);
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    if (stream_create.get_name() === sub.name) {
 | 
					    if (stream_create.get_name() === sub.name) {
 | 
				
			||||||
        ui.get_content_element($(".streams-list")).prepend(html);
 | 
					        ui.get_content_element($(".streams-list")).prepend(html);
 | 
				
			||||||
        ui.reset_scrollbar($(".streams-list"));
 | 
					        ui.reset_scrollbar($(".streams-list"));
 | 
				
			||||||
@@ -287,7 +288,6 @@ exports.update_settings_for_subscribed = function (sub) {
 | 
				
			|||||||
    ).show();
 | 
					    ).show();
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    if (exports.is_sub_already_present(sub)) {
 | 
					    if (exports.is_sub_already_present(sub)) {
 | 
				
			||||||
        stream_data.update_subscribers_count(sub);
 | 
					 | 
				
			||||||
        stream_ui_updates.update_stream_row_in_settings_tab(sub);
 | 
					        stream_ui_updates.update_stream_row_in_settings_tab(sub);
 | 
				
			||||||
        stream_ui_updates.update_subscribers_count(sub, true);
 | 
					        stream_ui_updates.update_subscribers_count(sub, true);
 | 
				
			||||||
        stream_ui_updates.update_check_button_for_sub(sub);
 | 
					        stream_ui_updates.update_check_button_for_sub(sub);
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user