paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-09 16:37:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: Remove subscribers from stream_data subs.

Browse Source 
This sets us up to use better system-wide data structures
for tracking subscribers.

Basically, instead of storing subscriber data on the
"sub" objects in stream_data.js, we instead have a
parallel data structure called stream_subscribers.

We also have stream_create, stream_edit, and friends
use helper functions rather than accessing
sub.subscribers directly.
This commit is contained in:
Steve Howell
2021-01-12 15:05:24 +00:00
committed by Tim Abbott
parent a175ce65cf
commit 58dcc70a35
13 changed files with 243 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
frontend_tests/node_tests/compose.js
View File

@@ -22,8 +22,6 @@ set_global("compose_actions", {
update_placeholder_text: noop, update_placeholder_text: noop,
}); });
const {LazySet} = zrequire("lazy_set");
const _navigator = { const _navigator = {
platform: "", platform: "",
}; };
@@ -961,16 +959,19 @@ run_test("finish", () => {
}); });
run_test("warn_if_private_stream_is_linked", () => { run_test("warn_if_private_stream_is_linked", () => {
stream_data.add_sub({ const test_sub = {
name: compose_state.stream_name(), name: compose_state.stream_name(),
subscribers: new LazySet([1, 2]),
stream_id: 99, stream_id: 99,
}); };
stream_data.add_sub(test_sub);
stream_data.set_subscribers(test_sub, [1, 2]);
let denmark = { let denmark = {
stream_id: 100,
name: "Denmark", name: "Denmark",
subscribers: new LazySet([1, 2, 3]),
}; };
stream_data.set_subscribers(denmark, [1, 2, 3]);
function test_noop_case(invite_only) { function test_noop_case(invite_only) {
compose_state.set_message_type("stream"); compose_state.set_message_type("stream");
@@ -1016,7 +1017,6 @@ run_test("warn_if_private_stream_is_linked", () => {
denmark = { denmark = {
invite_only: true, invite_only: true,
name: "Denmark", name: "Denmark",
subscribers: new LazySet([1]),
}; };
compose.warn_if_private_stream_is_linked(denmark); compose.warn_if_private_stream_is_linked(denmark);

50
frontend_tests/node_tests/stream_data.js
View File

@@ -4,7 +4,7 @@ const {strict: assert} = require("assert");
const _ = require("lodash"); const _ = require("lodash");
const {set_global, stub_out_jquery, zrequire} = require("../zjsunit/namespace"); const {set_global, stub_out_jquery, with_field, zrequire} = require("../zjsunit/namespace");
const {run_test} = require("../zjsunit/test"); const {run_test} = require("../zjsunit/test");
set_global("page_params", { set_global("page_params", {
@@ -179,7 +179,7 @@ run_test("unsubscribe", () => {
run_test("subscribers", () => { run_test("subscribers", () => {
stream_data.clear_subscriptions(); stream_data.clear_subscriptions();
let sub = {name: "Rome", subscribed: true, stream_id: 1}; let sub = {name: "Rome", subscribed: true, stream_id: 1001};
stream_data.add_sub(sub); stream_data.add_sub(sub);
@@ -203,7 +203,7 @@ run_test("subscribers", () => {
people.add_active_user(george); people.add_active_user(george);
function potential_subscriber_ids() { function potential_subscriber_ids() {
const users = stream_data.potential_subscribers(sub); const users = stream_data.potential_subscribers(sub.stream_id);
return users.map((u) => u.user_id).sort(); return users.map((u) => u.user_id).sort();
} }
@@ -268,7 +268,7 @@ run_test("subscribers", () => {
const bad_stream_id = 999999; const bad_stream_id = 999999;
blueslip.expect( blueslip.expect(
"warn", "warn",
"We got a remove_subscriber call for a non-existent stream " + bad_stream_id, "We got a remove_subscriber call for an untracked stream " + bad_stream_id,
); );
ok = stream_data.remove_subscriber(bad_stream_id, brutus.user_id); ok = stream_data.remove_subscriber(bad_stream_id, brutus.user_id);
assert(!ok); assert(!ok);
@@ -313,7 +313,7 @@ run_test("subscribers", () => {
assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), undefined); assert.equal(stream_data.is_user_subscribed(sub.stream_id, brutus.user_id), undefined);
// Verify that we don't crash and return false for a bad stream. // Verify that we don't crash and return false for a bad stream.
blueslip.expect("warn", "We got an add_subscriber call for a non-existent stream."); blueslip.expect("warn", "We got an add_subscriber call for an untracked stream: 9999999");
ok = stream_data.add_subscriber(9999999, brutus.user_id); ok = stream_data.add_subscriber(9999999, brutus.user_id);
assert(!ok); assert(!ok);
@@ -607,7 +607,7 @@ run_test("get_subscriber_count", () => {
}; };
stream_data.clear_subscriptions(); stream_data.clear_subscriptions();
blueslip.expect("warn", "We got a get_subscriber_count count call for a non-existent stream."); blueslip.expect("warn", "We got a get_subscriber_count call for an untracked stream: 102");
assert.equal(stream_data.get_subscriber_count(india.stream_id), undefined); assert.equal(stream_data.get_subscriber_count(india.stream_id), undefined);
stream_data.add_sub(india); stream_data.add_sub(india);
@@ -630,9 +630,8 @@ run_test("get_subscriber_count", () => {
stream_data.add_subscriber(india.stream_id, 103); stream_data.add_subscriber(india.stream_id, 103);
assert.equal(stream_data.get_subscriber_count(india.stream_id), 2); assert.equal(stream_data.get_subscriber_count(india.stream_id), 2);
const sub = stream_data.get_sub_by_name("India"); stream_data.remove_subscriber(india.stream_id, 103);
delete sub.subscribers; assert.deepStrictEqual(stream_data.get_subscriber_count(india.stream_id), 1);
assert.deepStrictEqual(stream_data.get_subscriber_count(india.stream_id), 0);
}); });
run_test("notifications", () => { run_test("notifications", () => {
@@ -976,15 +975,15 @@ run_test("filter inactives", () => {
}); });
run_test("is_subscriber_subset", () => { run_test("is_subscriber_subset", () => {
function make_sub(user_ids) { function make_sub(stream_id, user_ids) {
const sub = {}; const sub = {stream_id};
stream_data.set_subscribers(sub, user_ids); stream_data.set_subscribers(sub, user_ids);
return sub; return sub;
} }
const sub_a = make_sub([1, 2]); const sub_a = make_sub(301, [1, 2]);
const sub_b = make_sub([2, 3]); const sub_b = make_sub(302, [2, 3]);
const sub_c = make_sub([1, 2, 3]); const sub_c = make_sub(303, [1, 2, 3]);
// The bogus case should not come up in normal // The bogus case should not come up in normal
// use. // use.
@@ -1088,3 +1087,26 @@ run_test("all_topics_in_cache", () => {
sub.first_message_id = 2; sub.first_message_id = 2;
assert.equal(stream_data.all_topics_in_cache(sub), true); assert.equal(stream_data.all_topics_in_cache(sub), true);
}); });
run_test("warn if subscribers are missing", () => {
// This should only happen in this contrived test situation.
stream_data.clear_subscriptions();
const sub = {
name: "test",
stream_id: 3,
can_access_subscribers: true,
};
with_field(
stream_data,
"get_sub_by_id",
() => sub,
() => {
blueslip.expect("warn", "We called is_user_subscribed for an untracked stream: 3");
stream_data.is_user_subscribed(sub.stream_id, me.user_id);
blueslip.expect("warn", "We called get_subscribers for an untracked stream: 3");
assert.deepEqual(stream_data.get_subscribers(sub.stream_id), []);
},
);
});

21
frontend_tests/node_tests/stream_edit.js
View File

@@ -7,8 +7,6 @@ const {set_global, zrequire} = require("../zjsunit/namespace");
const {run_test} = require("../zjsunit/test"); const {run_test} = require("../zjsunit/test");
const {make_zjquery} = require("../zjsunit/zjquery"); const {make_zjquery} = require("../zjsunit/zjquery");
const {LazySet} = zrequire("lazy_set");
const noop = () => {}; const noop = () => {};
stub_templates(() => noop); stub_templates(() => noop);
@@ -79,16 +77,17 @@ const denmark = {
stream_id: 1, stream_id: 1,
name: "Denmark", name: "Denmark",
subscribed: true, subscribed: true,
subscribers: new LazySet([me.user_id, mark.user_id]),
render_subscribers: true, render_subscribers: true,
should_display_subscription_button: true, should_display_subscription_button: true,
}; };
stream_data.set_subscribers(denmark, [me.user_id, mark.user_id]);
const sweden = { const sweden = {
stream_id: 2, stream_id: 2,
name: "Sweden", name: "Sweden",
subscribed: false, subscribed: false,
subscribers: new LazySet([mark.user_id, jill.user_id]),
}; };
stream_data.set_subscribers(sweden, [mark.user_id, jill.user_id]);
const subs = [denmark, sweden]; const subs = [denmark, sweden];
for (const sub of subs) { for (const sub of subs) {
@@ -134,7 +133,7 @@ run_test("subscriber_pills", () => {
let add_subscribers_request = false; let add_subscribers_request = false;
stream_edit.invite_user_to_stream = (user_ids, sub) => { stream_edit.invite_user_to_stream = (user_ids, sub) => {
assert.equal(sub.stream_id, denmark.stream_id); assert.equal(sub.stream_id, denmark.stream_id);
assert.deepEqual(user_ids.sort(), expected_user_ids); assert.deepEqual(user_ids.sort(), expected_user_ids.sort());
add_subscribers_request = true; add_subscribers_request = true;
}; };
@@ -197,8 +196,8 @@ run_test("subscriber_pills", () => {
(function test_source() { (function test_source() {
const result = config.source.call(fake_this); const result = config.source.call(fake_this);
const taken_ids = stream_pill.get_stream_ids(stream_edit.pill_widget); const taken_ids = stream_pill.get_stream_ids(stream_edit.pill_widget);
const stream_ids = result.map((stream) => stream.stream_id).sort(); const stream_ids = Array.from(result, (stream) => stream.stream_id).sort();
let expected_ids = subs.map((stream) => stream.stream_id).sort(); let expected_ids = Array.from(subs, (stream) => stream.stream_id).sort();
expected_ids = expected_ids.filter((id) => !taken_ids.includes(id)); expected_ids = expected_ids.filter((id) => !taken_ids.includes(id));
assert.deepEqual(stream_ids, expected_ids); assert.deepEqual(stream_ids, expected_ids);
})(); })();
@@ -232,9 +231,9 @@ run_test("subscriber_pills", () => {
// We cannot subscribe ourselves (`me`) as // We cannot subscribe ourselves (`me`) as
// we are already subscribed to denmark stream. // we are already subscribed to denmark stream.
const potential_denmark_stream_subscribers = denmark.subscribers const potential_denmark_stream_subscribers = Array.from(
.map() stream_data.get_subscribers(denmark.stream_id),
.filter((id) => id !== me.user_id); ).filter((id) => id !== me.user_id);
// denmark.stream_id is stubbed. Thus request is // denmark.stream_id is stubbed. Thus request is
// sent to add all subscribers of stream Denmark. // sent to add all subscribers of stream Denmark.
@@ -269,7 +268,7 @@ run_test("subscriber_pills", () => {
// But only one request for mark is sent even though a mark user // But only one request for mark is sent even though a mark user
// pill is created and mark is also a subscriber of Denmark stream. // pill is created and mark is also a subscriber of Denmark stream.
user_pill.get_user_ids = () => [mark.user_id, fred.user_id]; user_pill.get_user_ids = () => [mark.user_id, fred.user_id];
stream_pill.get_user_ids = () => denmark.subscribers.map(); stream_pill.get_user_ids = () => stream_data.get_subscribers(denmark.stream_id);
expected_user_ids = potential_denmark_stream_subscribers.concat(fred.user_id); expected_user_ids = potential_denmark_stream_subscribers.concat(fred.user_id);
add_subscribers_handler(event); add_subscribers_handler(event);
}); });

5
frontend_tests/node_tests/stream_events.js
View File

@@ -421,9 +421,12 @@ run_test("remove_deactivated_user_from_all_streams", () => {
dev_help.can_access_subscribers = true; dev_help.can_access_subscribers = true;
// assert starting state
assert(!stream_data.is_user_subscribed(dev_help.stream_id, george.user_id));
// verify that deactivating user should unsubscribe user from all streams // verify that deactivating user should unsubscribe user from all streams
assert(stream_data.add_subscriber(dev_help.stream_id, george.user_id)); assert(stream_data.add_subscriber(dev_help.stream_id, george.user_id));
assert(dev_help.subscribers.has(george.user_id)); assert(stream_data.is_user_subscribed(dev_help.stream_id, george.user_id));
stream_events.remove_deactivated_user_from_all_streams(george.user_id); stream_events.remove_deactivated_user_from_all_streams(george.user_id);

17
frontend_tests/node_tests/subs.js
View File

@@ -47,8 +47,9 @@ run_test("filter_table", () => {
name: "Denmark", name: "Denmark",
stream_id: 1, stream_id: 1,
description: "Copenhagen", description: "Copenhagen",
subscribers: {size: 1}, subscribers: [1],
stream_weekly_traffic: null, stream_weekly_traffic: null,
color: "red",
}, },
{ {
elem: "poland", elem: "poland",
@@ -56,8 +57,9 @@ run_test("filter_table", () => {
name: "Poland", name: "Poland",
stream_id: 2, stream_id: 2,
description: "monday", description: "monday",
subscribers: {size: 3}, subscribers: [1, 2, 3],
stream_weekly_traffic: 13, stream_weekly_traffic: 13,
color: "red",
}, },
{ {
elem: "pomona", elem: "pomona",
@@ -65,8 +67,9 @@ run_test("filter_table", () => {
name: "Pomona", name: "Pomona",
stream_id: 3, stream_id: 3,
description: "college", description: "college",
subscribers: {size: 0}, subscribers: [],
stream_weekly_traffic: 0, stream_weekly_traffic: 0,
color: "red",
}, },
{ {
elem: "cpp", elem: "cpp",
@@ -74,8 +77,9 @@ run_test("filter_table", () => {
name: "C++", name: "C++",
stream_id: 4, stream_id: 4,
description: "programming lang", description: "programming lang",
subscribers: {size: 2}, subscribers: [1, 2],
stream_weekly_traffic: 6, stream_weekly_traffic: 6,
color: "red",
}, },
{ {
elem: "zzyzx", elem: "zzyzx",
@@ -83,13 +87,14 @@ run_test("filter_table", () => {
name: "Zzyzx", name: "Zzyzx",
stream_id: 5, stream_id: 5,
description: "california town", description: "california town",
subscribers: {size: 2}, subscribers: [1, 2],
stream_weekly_traffic: 6, stream_weekly_traffic: 6,
color: "red",
}, },
]; ];
for (const sub of sub_row_data) { for (const sub of sub_row_data) {
stream_data.add_sub(sub); stream_data.create_sub_from_server_data(sub);
} }
let populated_subs; let populated_subs;

118
frontend_tests/node_tests/typeahead_helper.js
View File

@@ -25,7 +25,6 @@ const pygments_data = zrequire("pygments_data", "generated/pygments_data.json");
const actual_pygments_data = {...pygments_data}; const actual_pygments_data = {...pygments_data};
const ct = zrequire("composebox_typeahead"); const ct = zrequire("composebox_typeahead");
const th = zrequire("typeahead_helper"); const th = zrequire("typeahead_helper");
const {LazySet} = zrequire("lazy_set");
let next_id = 0; let next_id = 0;
@@ -42,18 +41,27 @@ stream_data.create_streams([
]); ]);
run_test("sort_streams", () => { run_test("sort_streams", () => {
const popular = new LazySet([1, 2, 3, 4, 5, 6]); const popular = [1, 2, 3, 4, 5, 6];
const unpopular = new LazySet([1]); const unpopular = [1];
let test_streams = [ let test_streams = [
{name: "Dev", pin_to_top: false, subscribers: unpopular, subscribed: true}, {stream_id: 101, name: "Dev", pin_to_top: false, subscribers: unpopular, subscribed: true},
{name: "Docs", pin_to_top: false, subscribers: popular, subscribed: true}, {stream_id: 102, name: "Docs", pin_to_top: false, subscribers: popular, subscribed: true},
{name: "Derp", pin_to_top: false, subscribers: unpopular, subscribed: true}, {stream_id: 103, name: "Derp", pin_to_top: false, subscribers: unpopular, subscribed: true},
{name: "Denmark", pin_to_top: true, subscribers: popular, subscribed: true}, {stream_id: 104, name: "Denmark", pin_to_top: true, subscribers: popular, subscribed: true},
{name: "dead", pin_to_top: false, subscribers: unpopular, subscribed: true}, {stream_id: 105, name: "dead", pin_to_top: false, subscribers: unpopular, subscribed: true},
]; ];
test_streams.forEach(stream_data.update_calculated_fields);
function process_test_streams() {
for (const test_stream of test_streams) {
stream_data.set_subscribers(test_stream, test_stream.subscribers);
delete test_stream.subscribers;
stream_data.update_calculated_fields(test_stream);
}
}
process_test_streams();
stream_data.is_active = function (sub) { stream_data.is_active = function (sub) {
return sub.name !== "dead"; return sub.name !== "dead";
@@ -68,12 +76,44 @@ run_test("sort_streams", () => {
// Test sort streams with description // Test sort streams with description
test_streams = [ test_streams = [
{name: "Dev", description: "development help", subscribers: unpopular, subscribed: true}, {
{name: "Docs", description: "writing docs", subscribers: popular, subscribed: true}, stream_id: 201,
{name: "Derp", description: "derping around", subscribers: unpopular, subscribed: true}, name: "Dev",
{name: "Denmark", description: "visiting Denmark", subscribers: popular, subscribed: true}, description: "development help",
{name: "dead", description: "dead stream", subscribers: unpopular, subscribed: true}, subscribers: unpopular,
subscribed: true,
},
{
stream_id: 202,
name: "Docs",
description: "writing docs",
subscribers: popular,
subscribed: true,
},
{
stream_id: 203,
name: "Derp",
description: "derping around",
subscribers: unpopular,
subscribed: true,
},
{
stream_id: 204,
name: "Denmark",
description: "visiting Denmark",
subscribers: popular,
subscribed: true,
},
{
stream_id: 205,
name: "dead",
description: "dead stream",
subscribers: unpopular,
subscribed: true,
},
]; ];
process_test_streams();
test_streams.forEach(stream_data.update_calculated_fields); test_streams.forEach(stream_data.update_calculated_fields);
test_streams = th.sort_streams(test_streams, "wr"); test_streams = th.sort_streams(test_streams, "wr");
assert.deepEqual(test_streams[0].name, "Docs"); // Description match assert.deepEqual(test_streams[0].name, "Docs"); // Description match
@@ -84,14 +124,50 @@ run_test("sort_streams", () => {
// Test sort both subscribed and unsubscribed streams. // Test sort both subscribed and unsubscribed streams.
test_streams = [ test_streams = [
{name: "Dev", description: "Some devs", subscribed: true, subscribers: popular}, {
{name: "East", description: "Developing east", subscribed: true, subscribers: popular}, stream_id: 301,
{name: "New", description: "No match", subscribed: true, subscribers: popular}, name: "Dev",
{name: "Derp", description: "Always Derping", subscribed: false, subscribers: popular}, description: "Some devs",
{name: "Ether", description: "Destroying ether", subscribed: false, subscribers: popular}, subscribed: true,
{name: "Mew", description: "Cat mews", subscribed: false, subscribers: popular}, subscribers: popular,
},
{
stream_id: 302,
name: "East",
description: "Developing east",
subscribed: true,
subscribers: popular,
},
{
stream_id: 303,
name: "New",
description: "No match",
subscribed: true,
subscribers: popular,
},
{
stream_id: 304,
name: "Derp",
description: "Always Derping",
subscribed: false,
subscribers: popular,
},
{
stream_id: 305,
name: "Ether",
description: "Destroying ether",
subscribed: false,
subscribers: popular,
},
{
stream_id: 306,
name: "Mew",
description: "Cat mews",
subscribed: false,
subscribers: popular,
},
]; ];
test_streams.forEach(stream_data.update_calculated_fields); process_test_streams();
test_streams = th.sort_streams(test_streams, "d"); test_streams = th.sort_streams(test_streams, "d");
assert.deepEqual(test_streams[0].name, "Dev"); // Subscribed and stream name starts with query assert.deepEqual(test_streams[0].name, "Dev"); // Subscribed and stream name starts with query

4
frontend_tests/puppeteer_tests/09-mention.js
View File

@@ -17,7 +17,9 @@ async function test_mention(page) {
await common.ensure_enter_does_not_send(page); await common.ensure_enter_does_not_send(page);
console.log("Checking for all everyone warning"); console.log("Checking for all everyone warning");
const stream_size = await page.evaluate(() => stream_data.get_sub("Verona").subscribers.size); const stream_size = await page.evaluate(() =>
stream_data.get_subscriber_count(stream_data.get_sub("Verona").stream_id),
);
const threshold = await page.evaluate(() => { const threshold = await page.evaluate(() => {
compose.wildcard_mention_large_stream_threshold = 5; compose.wildcard_mention_large_stream_threshold = 5;
return compose.wildcard_mention_large_stream_threshold; return compose.wildcard_mention_large_stream_threshold;

2
static/js/stream_create.js
View File

@@ -289,7 +289,7 @@ exports.show_new_stream_modal = function () {
const elem = $(this); const elem = $(this);
const stream_id = Number.parseInt(elem.attr("data-stream-id"), 10); const stream_id = Number.parseInt(elem.attr("data-stream-id"), 10);
const checked = elem.find("input").prop("checked"); const checked = elem.find("input").prop("checked");
const subscriber_ids = stream_data.get_sub_by_id(stream_id).subscribers; const subscriber_ids = new Set(stream_data.get_subscribers(stream_id));
$("#user-checkboxes label.checkbox").each(function () { $("#user-checkboxes label.checkbox").each(function () {
const user_elem = $(this); const user_elem = $(this);

92
static/js/stream_data.js
View File

@@ -93,6 +93,12 @@ let filter_out_inactives = false;
const stream_ids_by_name = new FoldDict(); const stream_ids_by_name = new FoldDict();
const default_stream_ids = new Set(); const default_stream_ids = new Set();
// This maps a stream_id to a LazySet of user_ids who are subscribed.
// We maintain the invariant that this has keys for all all stream_ids
// that we track in the other data structures. We intialize it during
// clear_subscriptions.
let stream_subscribers;
exports.stream_privacy_policy_values = { exports.stream_privacy_policy_values = {
public: { public: {
code: "public", code: "public",
@@ -133,8 +139,11 @@ exports.stream_post_policy_values = {
}; };
exports.clear_subscriptions = function () { exports.clear_subscriptions = function () {
// This function is only used once at page load, and then
// it should only be used in tests.
stream_info = new BinaryDict((sub) => sub.subscribed); stream_info = new BinaryDict((sub) => sub.subscribed);
subs_by_stream_id = new Map(); subs_by_stream_id = new Map();
stream_subscribers = new Map();
}; };
exports.clear_subscriptions(); exports.clear_subscriptions();
@@ -193,10 +202,14 @@ exports.subscribe_myself = function (sub) {
}; };
exports.is_subscriber_subset = function (sub1, sub2) { exports.is_subscriber_subset = function (sub1, sub2) {
if (sub1.subscribers && sub2.subscribers) { const stream_id1 = sub1.stream_id;
const sub2_set = sub2.subscribers; const stream_id2 = sub2.stream_id;
return Array.from(sub1.subscribers.keys()).every((key) => sub2_set.has(key)); const sub1_set = stream_subscribers.get(stream_id1);
const sub2_set = stream_subscribers.get(stream_id2);
if (sub1_set && sub2_set) {
return Array.from(sub1_set.keys()).every((key) => sub2_set.has(key));
} }
return false; return false;
@@ -216,8 +229,8 @@ exports.add_sub = function (sub) {
// We use create_sub_from_server_data at page load. // We use create_sub_from_server_data at page load.
// We use create_streams for new streams in live-update events. // We use create_streams for new streams in live-update events.
if (!Object.prototype.hasOwnProperty.call(sub, "subscribers")) { if (!stream_subscribers.has(sub.stream_id)) {
sub.subscribers = new LazySet([]); exports.set_subscribers(sub, []);
} }
stream_info.set(sub.name, sub); stream_info.set(sub.name, sub);
@@ -431,11 +444,13 @@ exports.get_colors = function () {
}; };
exports.update_subscribers_count = function (sub) { exports.update_subscribers_count = function (sub) {
const count = sub.subscribers.size; // This is part of an unfortunate legacy hack, where we
sub.subscriber_count = count; // put calculated fields onto the sub object instead of
// letting callers build their own objects.
sub.subscriber_count = exports.get_subscriber_count(sub.stream_id);
}; };
exports.potential_subscribers = function (sub) { exports.potential_subscribers = function (stream_id) {
/* /*
This is a list of unsubscribed users This is a list of unsubscribed users
for the current stream, who the current for the current stream, who the current
@@ -453,11 +468,13 @@ exports.potential_subscribers = function (sub) {
may be moot now for other reasons.) may be moot now for other reasons.)
*/ */
const subscribers = stream_subscribers.get(stream_id);
function is_potential_subscriber(person) { function is_potential_subscriber(person) {
// Use verbose style to force better test // Use verbose style to force better test
// coverage, plus we may add more conditions over // coverage, plus we may add more conditions over
// time. // time.
if (sub.subscribers.has(person.user_id)) { if (subscribers.has(person.user_id)) {
return false; return false;
} }
@@ -472,15 +489,14 @@ exports.update_stream_email_address = function (sub, email) {
}; };
exports.get_subscriber_count = function (stream_id) { exports.get_subscriber_count = function (stream_id) {
const sub = exports.get_sub_by_id(stream_id); const subscribers = stream_subscribers.get(stream_id);
if (sub === undefined) {
blueslip.warn("We got a get_subscriber_count count call for a non-existent stream."); if (!subscribers) {
blueslip.warn("We got a get_subscriber_count call for an untracked stream: " + stream_id);
return undefined; return undefined;
} }
if (!sub.subscribers) {
return 0; return subscribers.size;
}
return sub.subscribers.size;
}; };
exports.update_stream_post_policy = function (sub, stream_post_policy) { exports.update_stream_post_policy = function (sub, stream_post_policy) {
@@ -693,14 +709,26 @@ exports.maybe_get_stream_name = function (stream_id) {
return stream.name; return stream.name;
}; };
exports.get_subscribers = (stream_id) => {
const subscribers = stream_subscribers.get(stream_id);
if (typeof subscribers === "undefined") {
blueslip.warn("We called get_subscribers for an untracked stream: " + stream_id);
return [];
}
return Array.from(subscribers.keys());
};
exports.set_subscribers = function (sub, user_ids) { exports.set_subscribers = function (sub, user_ids) {
sub.subscribers = new LazySet(user_ids || []); const subscribers = new LazySet(user_ids || []);
stream_subscribers.set(sub.stream_id, subscribers);
}; };
exports.add_subscriber = function (stream_id, user_id) { exports.add_subscriber = function (stream_id, user_id) {
const sub = exports.get_sub_by_id(stream_id); const subscribers = stream_subscribers.get(stream_id);
if (typeof sub === "undefined") { if (typeof subscribers === "undefined") {
blueslip.warn("We got an add_subscriber call for a non-existent stream."); blueslip.warn("We got an add_subscriber call for an untracked stream: " + stream_id);
return false; return false;
} }
const person = people.get_by_user_id(user_id); const person = people.get_by_user_id(user_id);
@@ -708,23 +736,23 @@ exports.add_subscriber = function (stream_id, user_id) {
blueslip.error("We tried to add invalid subscriber: " + user_id); blueslip.error("We tried to add invalid subscriber: " + user_id);
return false; return false;
} }
sub.subscribers.add(user_id); subscribers.add(user_id);
return true; return true;
}; };
exports.remove_subscriber = function (stream_id, user_id) { exports.remove_subscriber = function (stream_id, user_id) {
const sub = exports.get_sub_by_id(stream_id); const subscribers = stream_subscribers.get(stream_id);
if (typeof sub === "undefined") { if (typeof subscribers === "undefined") {
blueslip.warn("We got a remove_subscriber call for a non-existent stream " + stream_id); blueslip.warn("We got a remove_subscriber call for an untracked stream " + stream_id);
return false; return false;
} }
if (!sub.subscribers.has(user_id)) { if (!subscribers.has(user_id)) {
blueslip.warn("We tried to remove invalid subscriber: " + user_id); blueslip.warn("We tried to remove invalid subscriber: " + user_id);
return false; return false;
} }
sub.subscribers.delete(user_id); subscribers.delete(user_id);
return true; return true;
}; };
@@ -744,7 +772,13 @@ exports.is_user_subscribed = function (stream_id, user_id) {
return undefined; return undefined;
} }
return sub.subscribers.has(user_id); const subscribers = stream_subscribers.get(stream_id);
if (typeof subscribers === "undefined") {
blueslip.warn("We called is_user_subscribed for an untracked stream: " + stream_id);
return false;
}
return subscribers.has(user_id);
}; };
exports.create_streams = function (streams) { exports.create_streams = function (streams) {
@@ -902,9 +936,7 @@ exports.sort_for_stream_settings = function (stream_ids, order) {
} }
function by_subscriber_count(id_a, id_b) { function by_subscriber_count(id_a, id_b) {
const out = const out = exports.get_subscriber_count(id_b) - exports.get_subscriber_count(id_a);
exports.get_sub_by_id(id_b).subscribers.size -
exports.get_sub_by_id(id_a).subscribers.size;
if (out === 0) { if (out === 0) {
return by_stream_name(id_a, id_b); return by_stream_name(id_a, id_b);
} }

5
static/js/stream_edit.js
View File

@@ -327,11 +327,12 @@ function show_subscription_settings(sub) {
const list = get_subscriber_list(sub_settings); const list = get_subscriber_list(sub_settings);
list.empty(); list.empty();
const users = exports.get_users_from_subscribers(sub.subscribers); const user_ids = stream_data.get_subscribers(sub.stream_id);
const users = exports.get_users_from_subscribers(user_ids);
exports.sort_but_pin_current_user_on_top(users); exports.sort_but_pin_current_user_on_top(users);
function get_users_for_subscriber_typeahead() { function get_users_for_subscriber_typeahead() {
const potential_subscribers = stream_data.potential_subscribers(sub); const potential_subscribers = stream_data.potential_subscribers(stream_id);
return user_pill.filter_taken_users(potential_subscribers, exports.pill_widget); return user_pill.filter_taken_users(potential_subscribers, exports.pill_widget);
} }

10
static/js/stream_pill.js
View File

@@ -36,13 +36,11 @@ exports.get_stream_name_from_item = function (item) {
function get_user_ids_from_subs(items) { function get_user_ids_from_subs(items) {
let user_ids = []; let user_ids = [];
const stream_ids = items.map((item) => item.stream_id); for (const item of items) {
for (const stream_id of stream_ids) { // only some of our items have streams (for copy-from-stream)
const sub = stream_data.get_sub_by_id(stream_id); if (item.stream_id !== undefined) {
if (!sub) { user_ids = user_ids.concat(stream_data.get_subscribers(item.stream_id));
continue;
} }
user_ids = user_ids.concat(sub.subscribers.map());
} }
return user_ids; return user_ids;
} }

3
static/js/stream_ui_updates.js
View File

@@ -194,7 +194,8 @@ exports.update_subscribers_list = function (sub) {
if (!sub.can_access_subscribers) { if (!sub.can_access_subscribers) {
$(".subscriber_list_settings_container").hide(); $(".subscriber_list_settings_container").hide();
} else { } else {
const users = stream_edit.get_users_from_subscribers(sub.subscribers); const subscribers = stream_data.get_subscribers(sub.stream_id);
const users = stream_edit.get_users_from_subscribers(subscribers);
/* /*
We try to find a subscribers list that is already in the We try to find a subscribers list that is already in the

4
static/js/typeahead_helper.js
View File

@@ -389,7 +389,9 @@ exports.compare_by_activity = function (stream_a, stream_b) {
if (diff !== 0) { if (diff !== 0) {
return diff; return diff;
} }
diff = stream_b.subscribers.size - stream_a.subscribers.size; diff =
stream_data.get_subscriber_count(stream_b.stream_id) -
stream_data.get_subscriber_count(stream_a.stream_id);
if (diff !== 0) { if (diff !== 0) {
return diff; return diff;
} }