refactor: Extract is_subscriber_subset().

Extracting the function makes it a bit easier to
test and use in a generic way.

Also, I wanted this to live in stream_data, so that
it's easier to find if we change how we model
subscriber data.

Finally, I use _.every to do the subset check
instead of `_.difference`, since _.difference
is actually N-squared:

  _.difference = restArguments(function(array, rest) {
    rest = flatten(rest, true, true);
    return _.filter(array, function(value){
      return !_.contains(rest, value);
    });
  });

And we don't actually want to build a list only
to check that it's zero vs. nonzero length.

We now do this, which short circuits as soon
as it finds any key that is only in sub1:

    return _.every(sub1.subscribers.keys(), (key) => {
        return sub2_set.has(key);
    });
This commit is contained in:
Steve Howell
2020-01-14 18:35:33 +00:00
committed by Tim Abbott
parent 34b21bc0ee
commit c2af2c1fd1
3 changed files with 57 additions and 8 deletions

View File

@@ -850,6 +850,45 @@ run_test('filter inactives', () => {
assert(stream_data.is_filtering_inactives()); assert(stream_data.is_filtering_inactives());
}); });
run_test('is_subscriber_subset', () => {
function make_sub(user_ids) {
const sub = {};
stream_data.set_subscribers(sub, user_ids);
return sub;
}
const sub_a = make_sub([1, 2]);
const sub_b = make_sub([2, 3]);
const sub_c = make_sub([1, 2, 3]);
// The bogus case should not come up in normal
// use.
// We simply punt on any calculation if
// a stream has no subscriber info (like
// maybe Zephyr?).
const bogus = {}; // no subscribers
const matrix = [
[sub_a, sub_a, true],
[sub_a, sub_b, false],
[sub_a, sub_c, true],
[sub_b, sub_a, false],
[sub_b, sub_b, true],
[sub_b, sub_c, true],
[sub_c, sub_a, false],
[sub_c, sub_b, false],
[sub_c, sub_c, true],
[bogus, bogus, false],
];
_.each(matrix, (row) => {
assert.equal(
stream_data.is_subscriber_subset(row[0], row[1]),
row[2]
);
});
});
run_test('invite_streams', () => { run_test('invite_streams', () => {
// add default stream // add default stream
const orie = { const orie = {

View File

@@ -817,14 +817,12 @@ exports.warn_if_private_stream_is_linked = function (linked_stream) {
return; return;
} }
if (compose_stream.subscribers && linked_stream.subscribers) { if (stream_data.is_subscriber_subset(compose_stream, linked_stream)) {
const compose_stream_sub = compose_stream.subscribers.keys(); // Don't warn if subscribers list of current
const mentioned_stream_sub = linked_stream.subscribers.keys(); // compose_stream is a subset of linked_stream's
// Don't warn if subscribers list of current compose_stream is a subset of // subscribers list, because everyone will be
// mentioned_stream subscribers list. // subscribed to the linked stream.
if (_.difference(compose_stream_sub, mentioned_stream_sub).length === 0) { return;
return;
}
} }
const stream_name = linked_stream.name; const stream_name = linked_stream.name;

View File

@@ -145,6 +145,18 @@ exports.subscribe_myself = function (sub) {
stream_info.set_true(sub.name, sub); stream_info.set_true(sub.name, sub);
}; };
exports.is_subscriber_subset = function (sub1, sub2) {
if (sub1.subscribers && sub2.subscribers) {
const sub2_set = sub2.subscribers;
return _.every(sub1.subscribers.keys(), (key) => {
return sub2_set.has(key);
});
}
return false;
};
exports.unsubscribe_myself = function (sub) { exports.unsubscribe_myself = function (sub) {
// Remove user from subscriber's list // Remove user from subscriber's list
const user_id = people.my_current_user_id(); const user_id = people.my_current_user_id();