paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-02 21:13:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

compose: Extract warn_if_mentioning_unsubscribed_user.

Browse Source 
First, there are no more convoluted signals.

We also simplify the parameter to just the "mentioned"
object corresponding to either a user or a broadcast
mention.

For the user group scenario, this has always been dead
code, which you only realized when you got to the comment
at the bottom.  Now we actually do nothing.
And I moved the relevant commment to the
the typeahead code (with new wording).

I also moved the is_silent check to the caller.  I don't
feel too strongly about that either way. It's kind of silly
to call a function only to give that function an additional
responsibility to worry about.  On the other hand, I see
the logic of that function enforcing everything.  I went
with the former for now.

Arguably we should have a warning for silent mentions,
since doing a silent mention of somebody not on a stream
is a good indication of a typo.  I do understand the use
case, but the user can always ignore the warning.  Anyway,
we have decent test coverage on this.
This commit is contained in:
Steve Howell
2020-01-14 16:21:45 +00:00
committed by Tim Abbott
parent b91a19df43
commit 593049d551
4 changed files with 67 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
frontend_tests/node_tests/compose.js
View File

@@ -1098,13 +1098,9 @@ run_test('needs_subscribe_warning', () => {
});
run_test('on_events', () => {
(function test_usermention_completed_zulip_triggered() {
const handler = $(document).get_on_handler('usermention_completed.zulip');
let data = {
mentioned: {
email: 'foo@bar.com',
},
(function test_warn_if_mentioning_unsubscribed_user() {
let mentioned = {
email: 'foo@bar.com',
};
$('#compose_invite_users .compose_invite_user').length = 0;
@@ -1112,8 +1108,8 @@ run_test('on_events', () => {
function test_noop_case(msg_type, is_zephyr_mirror, mentioned_full_name) {
compose_state.set_message_type(msg_type);
page_params.realm_is_zephyr_mirror_realm = is_zephyr_mirror;
data.mentioned.full_name = mentioned_full_name;
handler({}, data);
mentioned.full_name = mentioned_full_name;
compose.warn_if_mentioning_unsubscribed_user(mentioned);
assert.equal($('#compose_invite_users').visible(), false);
}
@@ -1161,14 +1157,12 @@ run_test('on_events', () => {
}()),
];
data = {
mentioned: {
email: 'foo@bar.com',
full_name: 'Foo Barson',
},
mentioned = {
email: 'foo@bar.com',
full_name: 'Foo Barson',
};
handler({}, data);
compose.warn_if_mentioning_unsubscribed_user(mentioned);
assert.equal($('#compose_invite_users').visible(), true);
_.each(checks, function (f) { f(); });
@@ -1192,7 +1186,7 @@ run_test('on_events', () => {
// Now try to mention the same person again. The template should
// not render.
global.stub_templates(noop);
handler({}, data);
compose.warn_if_mentioning_unsubscribed_user(mentioned);
assert.equal($('#compose_invite_users').visible(), true);
assert(looked_for_existing);
}());

29
frontend_tests/node_tests/composebox_typeahead.js
View File

@@ -322,17 +322,18 @@ run_test('content_typeahead_selected', () => {
// mention
fake_this.completing = 'mention';
compose.warn_if_mentioning_unsubscribed_user = () => {};
fake_this.query = '@**Mark Tw';
fake_this.token = 'Mark Tw';
actual_value = ct.content_typeahead_selected.call(fake_this, twin1);
expected_value = '@**Mark Twin|105** ';
assert.equal(actual_value, expected_value);
let document_stub_trigger1_called = false;
$('document-stub').trigger = function (event, params) {
assert.equal(event, 'usermention_completed.zulip');
assert.deepEqual(params, { mentioned: othello, is_silent: false });
document_stub_trigger1_called = true;
let warned_for_mention = false;
compose.warn_if_mentioning_unsubscribed_user = (mentioned) => {
assert.equal(mentioned, othello);
warned_for_mention = true;
};
fake_this.query = '@oth';
@@ -340,6 +341,7 @@ run_test('content_typeahead_selected', () => {
actual_value = ct.content_typeahead_selected.call(fake_this, othello);
expected_value = '@**Othello, the Moor of Venice** ';
assert.equal(actual_value, expected_value);
assert(warned_for_mention);
fake_this.query = 'Hello @oth';
fake_this.token = 'oth';
@@ -361,11 +363,8 @@ run_test('content_typeahead_selected', () => {
// silent mention
fake_this.completing = 'silent_mention';
let document_stub_trigger2_called = false;
$('document-stub').trigger = function (event, params) {
assert.equal(event, 'usermention_completed.zulip');
assert.deepEqual(params, { mentioned: hamlet, is_silent: true });
document_stub_trigger2_called = true;
compose.warn_if_mentioning_unsubscribed_user = () => {
throw Error('unexpected call for silent mentions');
};
fake_this.query = '@_kin';
@@ -393,11 +392,8 @@ run_test('content_typeahead_selected', () => {
assert.equal(actual_value, expected_value);
// user group mention
let document_stub_group_trigger_called = false;
$('document-stub').trigger = function (event, params) {
assert.equal(event, 'usermention_completed.zulip');
assert.deepEqual(params, { user_group: backend });
document_stub_group_trigger_called = true;
compose.warn_if_mentioning_unsubscribed_user = () => {
throw Error('unexpected call for user groups');
};
fake_this.query = '@back';
@@ -486,9 +482,6 @@ run_test('content_typeahead_selected', () => {
assert(caret_called2);
assert(autosize_called);
assert(set_timeout_called);
assert(document_stub_trigger1_called);
assert(document_stub_group_trigger_called);
assert(document_stub_trigger2_called);
assert(warned_for_stream_link);
});

89
static/js/compose.js
View File

@@ -837,6 +837,44 @@ exports.warn_if_private_stream_is_linked = function (linked_stream) {
warning_area.show();
};
exports.warn_if_mentioning_unsubscribed_user = function (mentioned) {
if (compose_state.get_message_type() !== 'stream') {
return;
}
// Disable for Zephyr mirroring realms, since we never have subscriber lists there
if (page_params.realm_is_zephyr_mirror_realm) {
return;
}
const email = mentioned.email;
if (mentioned.full_name === 'all' || mentioned.full_name === 'everyone' || mentioned.full_name === 'stream') {
return; // don't check if @all or @everyone is subscribed to a stream
}
if (exports.needs_subscribe_warning(email)) {
const error_area = $("#compose_invite_users");
const existing_invites_area = $('#compose_invite_users .compose_invite_user');
const existing_invites = _.map($(existing_invites_area), function (user_row) {
return $(user_row).data('useremail');
});
if (existing_invites.indexOf(email) === -1) {
const context = {
email: email,
name: mentioned.full_name,
can_subscribe_other_users: page_params.can_subscribe_other_users,
};
const new_row = render_compose_invite_users(context);
error_area.append(new_row);
}
error_area.show();
}
};
exports.initialize = function () {
$('#stream_message_recipient_stream,#stream_message_recipient_topic,#private_message_recipient').on('keyup', update_fade);
$('#stream_message_recipient_stream,#stream_message_recipient_topic,#private_message_recipient').on('change', update_fade);
@@ -856,57 +894,6 @@ exports.initialize = function () {
upload.feature_check($("#compose #attach_files"));
// Show a warning if a user @-mentions someone who will not receive this message
$(document).on('usermention_completed.zulip', function (event, data) {
if (compose_state.get_message_type() !== 'stream') {
return;
}
if (data.is_silent) {
// We don't need to warn in case of silent mentions.
return;
}
// Disable for Zephyr mirroring realms, since we never have subscriber lists there
if (page_params.realm_is_zephyr_mirror_realm) {
return;
}
if (data !== undefined && data.mentioned !== undefined) {
const email = data.mentioned.email;
// warn if @all, @everyone or @stream is mentioned
if (data.mentioned.full_name === 'all' || data.mentioned.full_name === 'everyone' || data.mentioned.full_name === 'stream') {
return; // don't check if @all or @everyone is subscribed to a stream
}
if (exports.needs_subscribe_warning(email)) {
const error_area = $("#compose_invite_users");
const existing_invites_area = $('#compose_invite_users .compose_invite_user');
const existing_invites = _.map($(existing_invites_area), function (user_row) {
return $(user_row).data('useremail');
});
if (existing_invites.indexOf(email) === -1) {
const context = {
email: email,
name: data.mentioned.full_name,
can_subscribe_other_users: page_params.can_subscribe_other_users,
};
const new_row = render_compose_invite_users(context);
error_area.append(new_row);
}
error_area.show();
}
}
// User group mentions will fall through here. In the future,
// we may want to add some sort of similar warning for cases
// where nobody in the group is subscribed, but that decision
// can wait on user feedback.
});
$("#compose-all-everyone").on('click', '.compose-all-everyone-confirm', function (event) {
event.preventDefault();

10
static/js/composebox_typeahead.js
View File

@@ -770,11 +770,17 @@ exports.content_typeahead_selected = function (item, event) {
}
if (user_groups.is_user_group(item)) {
beginning += '@*' + item.name + '* ';
$(document).trigger('usermention_completed.zulip', {user_group: item});
// We could theoretically warn folks if they are
// mentioning a user group that literally has zero
// members where we are posting to, but we don't have
// that functionality yet, and we haven't gotten much
// feedback on this being an actual pitfall.
} else {
const mention_text = people.get_mention_syntax(item.full_name, item.user_id, is_silent);
beginning += mention_text + ' ';
$(document).trigger('usermention_completed.zulip', {mentioned: item, is_silent: is_silent});
if (!is_silent) {
compose.warn_if_mentioning_unsubscribed_user(item);
}
}
} else if (this.completing === 'slash') {
beginning = beginning.substring(0, beginning.length - this.token.length - 1) + "/" + item.name + " ";