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

topic_mentions: Fix the incorrect large @-mention notification warning.

Browse Source 
Earlier, a 'large @-mention notification' warning that pops up
for stream wildcard mentions was shown for topic wildcard mentions
too, which is incorrect.

This commit fixes the incorrect behavior. We no longer show the
banner for @-topic mentions.

We don't need a banner for @-topic mentions, as those are much less
likely to be used without thinking about it and would rarely be spammy
for a lot of people.

Fixes #27767.
This commit is contained in:
Prakhar Pratyush
2023-11-21 11:34:19 +05:30
committed by Tim Abbott
parent 33b164f63a
commit 768be7d46d
11 changed files with 69 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
web/e2e-tests/mention.test.ts
View File

@@ -26,8 +26,8 @@ async function test_mention(page: Page): Promise<void> {
zulip_test.get_subscriber_count(zulip_test.get_sub("Verona").stream_id),
);
const threshold = await page.evaluate(() => {
zulip_test.set_wildcard_mention_large_stream_threshold(5);
return zulip_test.wildcard_mention_large_stream_threshold;
zulip_test.set_stream_wildcard_mention_large_stream_threshold(5);
return zulip_test.stream_wildcard_mention_large_stream_threshold;
});
assert.ok(stream_size > threshold);
await page.click("#compose-send-button");

4
web/src/compose_actions.js
View File

@@ -75,8 +75,8 @@ function clear_box() {
// TODO: Better encapsulate at-mention warnings.
compose_validate.clear_topic_resolved_warning();
compose_validate.clear_wildcard_warnings($("#compose_banners"));
compose_validate.set_user_acknowledged_wildcard_flag(false);
compose_validate.clear_stream_wildcard_warnings($("#compose_banners"));
compose_validate.set_user_acknowledged_stream_wildcard_flag(false);
compose_state.set_recipient_edited_manually(false);
clear_textarea();

6
web/src/compose_setup.js
View File

@@ -108,8 +108,8 @@ export function initialize() {
event.preventDefault();
const {$banner_container, is_edit_input} = get_input_info(event);
const $row = $(event.target).closest(".message_row");
compose_validate.clear_wildcard_warnings($banner_container);
compose_validate.set_user_acknowledged_wildcard_flag(true);
compose_validate.clear_stream_wildcard_warnings($banner_container);
compose_validate.set_user_acknowledged_stream_wildcard_flag(true);
if (is_edit_input) {
message_edit.save_message_row_edit($row);
} else if (event.target.dataset.validationTrigger === "schedule") {
@@ -118,7 +118,7 @@ export function initialize() {
// We need to set this flag to true here because `open_send_later_menu` validates the message and sets
// the user acknowledged wildcard flag back to 'false' and we don't want that to happen because then it
// would again show the wildcard warning banner when we actually send the message from 'send-later' modal.
compose_validate.set_user_acknowledged_wildcard_flag(true);
compose_validate.set_user_acknowledged_stream_wildcard_flag(true);
} else {
compose.finish();
}

56
web/src/compose_validate.js
View File

@@ -4,7 +4,7 @@ import * as resolved_topic from "../shared/src/resolved_topic";
import render_compose_banner from "../templates/compose_banner/compose_banner.hbs";
import render_not_subscribed_warning from "../templates/compose_banner/not_subscribed_warning.hbs";
import render_private_stream_warning from "../templates/compose_banner/private_stream_warning.hbs";
import render_wildcard_warning from "../templates/compose_banner/wildcard_warning.hbs";
import render_stream_wildcard_warning from "../templates/compose_banner/stream_wildcard_warning.hbs";
import render_compose_limit_indicator from "../templates/compose_limit_indicator.hbs";
import * as channel from "./channel";
@@ -22,12 +22,12 @@ import * as stream_data from "./stream_data";
import * as sub_store from "./sub_store";
import * as util from "./util";
let user_acknowledged_wildcard = false;
let user_acknowledged_stream_wildcard = false;
let upload_in_progress = false;
let message_too_long = false;
let recipient_disallowed = false;
export let wildcard_mention_large_stream_threshold = 15;
export let stream_wildcard_mention_large_stream_threshold = 15;
export function set_upload_in_progress(status) {
upload_in_progress = status;
@@ -280,7 +280,7 @@ export function warn_if_topic_resolved(topic_changed) {
}
}
function show_wildcard_warnings(opts) {
function show_stream_wildcard_warnings(opts) {
const subscriber_count = peer_data.get_subscriber_count(opts.stream_id) || 0;
const stream_name = sub_store.maybe_get_stream_name(opts.stream_id);
const is_edit_container = opts.$banner_container.closest(".edit_form_banners").length > 0;
@@ -294,11 +294,11 @@ function show_wildcard_warnings(opts) {
button_text = $t({defaultMessage: "Yes, save"});
}
const wildcard_template = render_wildcard_warning({
const stream_wildcard_template = render_stream_wildcard_warning({
banner_type: compose_banner.WARNING,
subscriber_count,
stream_name,
wildcard_mention: opts.wildcard_mention,
stream_wildcard_mention: opts.stream_wildcard_mention,
button_text,
hide_close_button: true,
classname,
@@ -308,28 +308,28 @@ function show_wildcard_warnings(opts) {
// only show one error for any number of @all or @everyone mentions
if (opts.$banner_container.find(`.${CSS.escape(classname)}`).length === 0) {
compose_banner.append_compose_banner_to_banner_list(
wildcard_template,
stream_wildcard_template,
opts.$banner_container,
);
} else {
// if there is already a banner, replace it with the new one
compose_banner.update_or_append_banner(
wildcard_template,
stream_wildcard_template,
classname,
opts.$banner_container,
);
}
user_acknowledged_wildcard = false;
user_acknowledged_stream_wildcard = false;
}
export function clear_wildcard_warnings($banner_container) {
export function clear_stream_wildcard_warnings($banner_container) {
const classname = compose_banner.CLASSNAMES.wildcard_warning;
$banner_container.find(`.${CSS.escape(classname)}`).remove();
}
export function set_user_acknowledged_wildcard_flag(value) {
user_acknowledged_wildcard = value;
export function set_user_acknowledged_stream_wildcard_flag(value) {
user_acknowledged_stream_wildcard = value;
}
export function get_invalid_recipient_emails() {
@@ -379,11 +379,11 @@ function is_recipient_large_stream() {
return (
compose_state.stream_id() &&
peer_data.get_subscriber_count(compose_state.stream_id()) >
wildcard_mention_large_stream_threshold
stream_wildcard_mention_large_stream_threshold
);
}
function wildcard_mention_allowed_in_large_stream() {
function stream_wildcard_mention_allowed_in_large_stream() {
if (
page_params.realm_wildcard_mention_policy ===
settings_config.wildcard_mention_policy_values.by_everyone.code
@@ -428,11 +428,11 @@ function wildcard_mention_allowed_in_large_stream() {
}
export function wildcard_mention_allowed() {
return !is_recipient_large_stream() || wildcard_mention_allowed_in_large_stream();
return !is_recipient_large_stream() || stream_wildcard_mention_allowed_in_large_stream();
}
export function set_wildcard_mention_large_stream_threshold(value) {
wildcard_mention_large_stream_threshold = value;
export function set_stream_wildcard_mention_large_stream_threshold(value) {
stream_wildcard_mention_large_stream_threshold = value;
}
export function validate_stream_message_mentions(opts) {
@@ -442,14 +442,14 @@ export function validate_stream_message_mentions(opts) {
// stream, check if they permission to do so. If yes, warn them
// if they haven't acknowledged the wildcard warning yet.
if (
opts.wildcard_mention !== null &&
subscriber_count > wildcard_mention_large_stream_threshold
opts.stream_wildcard_mention !== null &&
subscriber_count > stream_wildcard_mention_large_stream_threshold
) {
if (!wildcard_mention_allowed_in_large_stream()) {
if (!stream_wildcard_mention_allowed_in_large_stream()) {
compose_banner.show_error_message(
$t({
defaultMessage:
"You do not have permission to use wildcard mentions in this stream.",
"You do not have permission to use stream wildcard mentions in this stream.",
}),
compose_banner.CLASSNAMES.wildcards_not_allowed,
opts.$banner_container,
@@ -457,8 +457,8 @@ export function validate_stream_message_mentions(opts) {
return false;
}
if (!user_acknowledged_wildcard) {
show_wildcard_warnings(opts);
if (!user_acknowledged_stream_wildcard) {
show_stream_wildcard_warnings(opts);
$("#compose-send-button").prop("disabled", false);
compose_ui.hide_compose_spinner();
@@ -466,10 +466,10 @@ export function validate_stream_message_mentions(opts) {
}
} else {
// the message no longer contains @all or @everyone
clear_wildcard_warnings(opts.$banner_container);
clear_stream_wildcard_warnings(opts.$banner_container);
}
// at this point, the user has either acknowledged the warning or removed @all / @everyone
user_acknowledged_wildcard = false;
user_acknowledged_stream_wildcard = false;
return true;
}
@@ -570,14 +570,16 @@ function validate_stream_message(scheduling_message) {
return false;
}
const wildcard_mention = util.find_wildcard_mentions(compose_state.message_content());
const stream_wildcard_mention = util.find_stream_wildcard_mentions(
compose_state.message_content(),
);
if (
!validate_stream_message_address_info(sub.name) ||
!validate_stream_message_mentions({
stream_id: sub.stream_id,
$banner_container,
wildcard_mention,
stream_wildcard_mention,
scheduling_message,
})
) {

9
web/src/message_edit.js
View File

@@ -958,14 +958,13 @@ export function save_message_row_edit($row) {
changed = old_content !== new_content;
}
const already_has_wildcard_mention =
message.stream_wildcard_mentioned || message.topic_wildcard_mentioned;
if (!already_has_wildcard_mention) {
const wildcard_mention = util.find_wildcard_mentions(new_content);
const already_has_stream_wildcard_mention = message.stream_wildcard_mentioned;
if (!already_has_stream_wildcard_mention) {
const stream_wildcard_mention = util.find_stream_wildcard_mentions(new_content);
const is_stream_message_mentions_valid = compose_validate.validate_stream_message_mentions({
stream_id,
$banner_container,
wildcard_mention,
stream_wildcard_mention,
});
if (!is_stream_message_mentions_valid) {

4
web/src/util.ts
View File

@@ -192,8 +192,8 @@ export class CachedValue<T> {
}
}
export function find_wildcard_mentions(message_content: string): string | null {
const mention = message_content.match(/(^|\s)(@\*{2}(all|everyone|stream|topic)\*{2})($|\s)/);
export function find_stream_wildcard_mentions(message_content: string): string | null {
const mention = message_content.match(/(^|\s)(@\*{2}(all|everyone|stream)\*{2})($|\s)/);
if (mention === null) {
return null;
}

4
web/src/zulip_test.js
View File

@@ -3,8 +3,8 @@
// Puppeteer tests. It should not be used in the code itself.
export {
set_wildcard_mention_large_stream_threshold,
wildcard_mention_large_stream_threshold,
set_stream_wildcard_mention_large_stream_threshold,
stream_wildcard_mention_large_stream_threshold,
} from "./compose_validate";
export {private_message_recipient} from "./compose_state";
export {current as current_msg_list} from "./message_lists";

2
web/templates/compose_banner/wildcard_warning.hbs → web/templates/compose_banner/stream_wildcard_warning.hbs
View File

@@ -1,7 +1,7 @@
{{#> compose_banner }}
<p class="banner_message">
{{#tr}}
Are you sure you want to send @-mention notifications to the <strong>{subscriber_count}</strong> users subscribed to #{stream_name}? If not, please edit your message to remove the <strong>@{wildcard_mention}</strong> mention.
Are you sure you want to send @-mention notifications to the <strong>{subscriber_count}</strong> users subscribed to #{stream_name}? If not, please edit your message to remove the <strong>@{stream_wildcard_mention}</strong> mention.
{{/tr}}
</p>
{{/compose_banner}}

22
web/tests/compose_validate.test.js
View File

@@ -413,18 +413,22 @@ test_ui("validate_stream_message", ({override_rewire, mock_template}) => {
assert.equal(stream_id, 101);
return 16;
});
let wildcard_warning_rendered = false;
let stream_wildcard_warning_rendered = false;
$("#compose_banner_area .wildcard_warning").length = 0;
mock_template("compose_banner/wildcard_warning.hbs", false, (data) => {
wildcard_warning_rendered = true;
mock_template("compose_banner/stream_wildcard_warning.hbs", false, (data) => {
stream_wildcard_warning_rendered = true;
assert.equal(data.subscriber_count, 16);
});
override_rewire(compose_validate, "wildcard_mention_allowed_in_large_stream", () => true);
override_rewire(
compose_validate,
"stream_wildcard_mention_allowed_in_large_stream",
() => true,
);
compose_state.message_content("Hey @**all**");
assert.ok(!compose_validate.validate());
assert.equal($("#compose-send-button").prop("disabled"), false);
assert.ok(wildcard_warning_rendered);
assert.ok(stream_wildcard_warning_rendered);
let wildcards_not_allowed_rendered = false;
mock_template("compose_banner/compose_banner.hbs", false, (data) => {
@@ -433,12 +437,16 @@ test_ui("validate_stream_message", ({override_rewire, mock_template}) => {
data.banner_text,
$t({
defaultMessage:
"You do not have permission to use wildcard mentions in this stream.",
"You do not have permission to use stream wildcard mentions in this stream.",
}),
);
wildcards_not_allowed_rendered = true;
});
override_rewire(compose_validate, "wildcard_mention_allowed_in_large_stream", () => false);
override_rewire(
compose_validate,
"stream_wildcard_mention_allowed_in_large_stream",
() => false,
);
assert.ok(!compose_validate.validate());
assert.ok(wildcards_not_allowed_rendered);
});

4
web/tests/example1.test.js
View File

@@ -25,8 +25,8 @@ const util = zrequire("util");
// The most basic unit tests load up code, call functions,
// and assert truths:
assert.ok(!util.find_wildcard_mentions("boring text"));
assert.ok(util.find_wildcard_mentions("mention @**everyone**"));
assert.ok(!util.find_stream_wildcard_mentions("boring text"));
assert.ok(util.find_stream_wildcard_mentions("mention @**everyone**"));
// Let's test with people.js next. We'll show this technique:
// * get a false value

27
web/tests/util.test.js
View File

@@ -204,46 +204,33 @@ run_test("wildcard_mentions_regexp", () => {
"some_email@**stream**.com",
];
const messages_without_topic_mentions = [
"some text before @topic some text after",
"@topic",
"`@topic`",
"some_email@topic.com",
"`@**topic**`",
"some_email@**topic**.com",
];
let i;
for (i = 0; i < messages_with_all_mentions.length; i += 1) {
assert.ok(util.find_wildcard_mentions(messages_with_all_mentions[i]));
assert.ok(util.find_stream_wildcard_mentions(messages_with_all_mentions[i]));
}
for (i = 0; i < messages_with_everyone_mentions.length; i += 1) {
assert.ok(util.find_wildcard_mentions(messages_with_everyone_mentions[i]));
assert.ok(util.find_stream_wildcard_mentions(messages_with_everyone_mentions[i]));
}
for (i = 0; i < messages_with_stream_mentions.length; i += 1) {
assert.ok(util.find_wildcard_mentions(messages_with_stream_mentions[i]));
assert.ok(util.find_stream_wildcard_mentions(messages_with_stream_mentions[i]));
}
for (i = 0; i < messages_with_topic_mentions.length; i += 1) {
assert.ok(util.find_wildcard_mentions(messages_with_topic_mentions[i]));
assert.ok(!util.find_stream_wildcard_mentions(messages_with_topic_mentions[i]));
}
for (i = 0; i < messages_without_all_mentions.length; i += 1) {
assert.ok(!util.find_wildcard_mentions(messages_without_everyone_mentions[i]));
assert.ok(!util.find_stream_wildcard_mentions(messages_without_everyone_mentions[i]));
}
for (i = 0; i < messages_without_everyone_mentions.length; i += 1) {
assert.ok(!util.find_wildcard_mentions(messages_without_everyone_mentions[i]));
assert.ok(!util.find_stream_wildcard_mentions(messages_without_everyone_mentions[i]));
}
for (i = 0; i < messages_without_stream_mentions.length; i += 1) {
assert.ok(!util.find_wildcard_mentions(messages_without_stream_mentions[i]));
}
for (i = 0; i < messages_without_topic_mentions.length; i += 1) {
assert.ok(!util.find_wildcard_mentions(messages_without_topic_mentions[i]));
assert.ok(!util.find_stream_wildcard_mentions(messages_without_stream_mentions[i]));
}
});