subscription: Fix unexpected blueslip warnings on add subscriber.

In stream settings, if user add subscriber to unsubscribed public
stream from `Add` input widget it gives lots of blueslip warnings,
cause user isn't subscribed to public stream.

Fix this by changing condition to `sub.can_access_subscriber` from
`sub.subscribed` in blueslip warning, cause user can access
subscribers in such cases even if not subscribed to stream.

Tweaked by tabbott to make the node tests pass.
This commit is contained in:
YJDave 2018-03-19 10:22:37 +05:30 committed by Tim Abbott
parent 64dac1bb2e
commit 2cbfcbb740
4 changed files with 34 additions and 10 deletions

View File

@ -1,5 +1,7 @@
set_global('$', function () { set_global('$', function () {
}); });
set_global('blueslip', {});
global.blueslip.warn = function () {};
zrequire('util'); zrequire('util');
zrequire('stream_data'); zrequire('stream_data');
@ -36,6 +38,7 @@ people.add(bob);
stream_id: 101, stream_id: 101,
name: 'social', name: 'social',
subscribed: true, subscribed: true,
can_access_subscribers: true,
}; };
stream_data.add_sub('social', sub); stream_data.add_sub('social', sub);
stream_data.set_subscribers(sub, [me.user_id, alice.user_id]); stream_data.set_subscribers(sub, [me.user_id, alice.user_id]);

View File

@ -168,6 +168,7 @@ zrequire('marked', 'third/marked/lib/marked');
people.add(george); people.add(george);
stream_data.set_subscribers(sub, [fred.user_id, george.user_id]); stream_data.set_subscribers(sub, [fred.user_id, george.user_id]);
stream_data.update_calculated_fields(sub);
assert(stream_data.user_is_subscribed('Rome', 'FRED@zulip.com')); assert(stream_data.user_is_subscribed('Rome', 'FRED@zulip.com'));
assert(stream_data.user_is_subscribed('Rome', 'fred@zulip.com')); assert(stream_data.user_is_subscribed('Rome', 'fred@zulip.com'));
assert(stream_data.user_is_subscribed('Rome', 'george@zulip.com')); assert(stream_data.user_is_subscribed('Rome', 'george@zulip.com'));
@ -247,8 +248,17 @@ zrequire('marked', 'third/marked/lib/marked');
// Verify that we noop and don't crash when unsubscribed. // Verify that we noop and don't crash when unsubscribed.
sub.subscribed = false; sub.subscribed = false;
stream_data.update_calculated_fields(sub);
ok = stream_data.add_subscriber('Rome', brutus.user_id); ok = stream_data.add_subscriber('Rome', brutus.user_id);
assert(ok); assert(ok);
assert.equal(stream_data.user_is_subscribed('Rome', email), true);
stream_data.remove_subscriber('Rome', brutus.user_id);
assert.equal(stream_data.user_is_subscribed('Rome', email), false);
stream_data.add_subscriber('Rome', brutus.user_id);
assert.equal(stream_data.user_is_subscribed('Rome', email), true);
sub.invite_only = true;
stream_data.update_calculated_fields(sub);
assert.equal(stream_data.user_is_subscribed('Rome', email), undefined); assert.equal(stream_data.user_is_subscribed('Rome', email), undefined);
stream_data.remove_subscriber('Rome', brutus.user_id); stream_data.remove_subscriber('Rome', brutus.user_id);
assert.equal(stream_data.user_is_subscribed('Rome', email), undefined); assert.equal(stream_data.user_is_subscribed('Rome', email), undefined);

View File

@ -7,6 +7,10 @@ zrequire('pm_conversations');
zrequire('people'); zrequire('people');
zrequire('util'); zrequire('util');
zrequire('stream_data'); zrequire('stream_data');
zrequire('narrow');
zrequire('hash_util');
zrequire('marked', 'third/marked/lib/marked');
var _ = require('node_modules/underscore/underscore.js');
var th = zrequire('typeahead_helper'); var th = zrequire('typeahead_helper');
stream_data.create_streams([ stream_data.create_streams([
@ -30,6 +34,7 @@ stream_data.create_streams([
{name: 'Denmark', pin_to_top: true, subscribers: popular, subscribed: true}, {name: 'Denmark', pin_to_top: true, subscribers: popular, subscribed: true},
{name: 'dead', pin_to_top: false, subscribers: unpopular, subscribed: true}, {name: 'dead', pin_to_top: false, subscribers: unpopular, subscribed: true},
]; ];
_.each(test_streams, stream_data.update_calculated_fields);
global.stream_data.is_active = function (sub) { global.stream_data.is_active = function (sub) {
return sub.name !== 'dead'; return sub.name !== 'dead';
@ -50,6 +55,7 @@ stream_data.create_streams([
{name: 'Denmark', description: 'visiting Denmark', subscribers: popular, subscribed: true}, {name: 'Denmark', description: 'visiting Denmark', subscribers: popular, subscribed: true},
{name: 'dead', description: 'dead stream', subscribers: unpopular, subscribed: true}, {name: 'dead', description: 'dead stream', subscribers: unpopular, subscribed: true},
]; ];
_.each(test_streams, 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
assert.deepEqual(test_streams[1].name, "Denmark"); // Popular stream assert.deepEqual(test_streams[1].name, "Denmark"); // Popular stream
@ -59,13 +65,14 @@ stream_data.create_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}, {name: 'Dev', description: 'Some devs', subscribed: true, subscribers: popular},
{name: 'East', description: 'Developing east', subscribed: true}, {name: 'East', description: 'Developing east', subscribed: true, subscribers: popular},
{name: 'New', description: 'No match', subscribed: true}, {name: 'New', description: 'No match', subscribed: true, subscribers: popular},
{name: 'Derp', description: 'Always Derping', subscribed: false}, {name: 'Derp', description: 'Always Derping', subscribed: false, subscribers: popular},
{name: 'Ether', description: 'Destroying ether', subscribed: false}, {name: 'Ether', description: 'Destroying ether', subscribed: false, subscribers: popular},
{name: 'Mew', description: 'Cat mews', subscribed: false}, {name: 'Mew', description: 'Cat mews', subscribed: false, subscribers: popular},
]; ];
_.each(test_streams, stream_data.update_calculated_fields);
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
@ -187,6 +194,11 @@ _.each(matches, function (person) {
stream_data.add_subscriber("Dev", people.get_user_id(subscriber_email_2)); stream_data.add_subscriber("Dev", people.get_user_id(subscriber_email_2));
stream_data.add_subscriber("Dev", people.get_user_id(subscriber_email_3)); stream_data.add_subscriber("Dev", people.get_user_id(subscriber_email_3));
var dev_sub = stream_data.get_sub("Dev");
var linux_sub = stream_data.get_sub("Linux");
stream_data.update_calculated_fields(dev_sub);
stream_data.update_calculated_fields(linux_sub);
// For spliting based on whether a PM was sent // For spliting based on whether a PM was sent
global.pm_conversations.set_partner(5); global.pm_conversations.set_partner(5);
global.pm_conversations.set_partner(6); global.pm_conversations.set_partner(6);

View File

@ -363,11 +363,10 @@ exports.remove_subscriber = function (stream_name, user_id) {
exports.user_is_subscribed = function (stream_name, user_email) { exports.user_is_subscribed = function (stream_name, user_email) {
var sub = exports.get_sub(stream_name); var sub = exports.get_sub(stream_name);
if (typeof sub === 'undefined' || !sub.subscribed) { if (typeof sub === 'undefined' || !sub.can_access_subscribers) {
// If we don't know about the stream, or we ourselves are not // If we don't know about the stream, or we ourselves can not access subscriber list,
// subscribed, we can't keep track of the subscriber list in general,
// so we return undefined (treated as falsy if not explicitly handled). // so we return undefined (treated as falsy if not explicitly handled).
blueslip.warn("We got a user_is_subscribed call for a non-existent or unsubscribed stream."); blueslip.warn("We got a user_is_subscribed call for a non-existent or inaccessible stream.");
return; return;
} }
var user_id = people.get_user_id(user_email); var user_id = people.get_user_id(user_email);