widgets: Prevent edits to widgets.

As of now, editing a widget doesn't update the rendered content.
It's important to ensure that existing votes or options added later on
don't get deleted when rendered.
This seems more complex than it's worth.

For now, we just prevent edits to widgets.
This commit makes the UI clearer that editing widgets isn't allowed.

See also:
https://github.com/zulip/zulip/issues/14229
https://github.com/zulip/zulip/issues/14799

Fixes #17156
This commit is contained in:
Ganesh Pawar
2021-01-31 22:53:48 +05:30
committed by Tim Abbott
parent 04da50a7bb
commit ddf2127035
6 changed files with 51 additions and 4 deletions

View File

@@ -68,6 +68,10 @@ run_test("get_editability", () => {
// is true, we can edit the topic if there is one. // is true, we can edit the topic if there is one.
message.type = "stream"; message.type = "stream";
assert.equal(get_editability(message, 45), editability_types.TOPIC_ONLY); assert.equal(get_editability(message, 45), editability_types.TOPIC_ONLY);
// Right now, we prevent users from editing widgets.
message.submessages = ["/poll"];
assert.equal(get_editability(message, 45), editability_types.TOPIC_ONLY);
delete message.submessages;
message.type = "private"; message.type = "private";
assert.equal(get_editability(message, 45), editability_types.NO_LONGER); assert.equal(get_editability(message, 45), editability_types.NO_LONGER);
// If we don't pass a second argument, treat it as 0 // If we don't pass a second argument, treat it as 0

View File

@@ -78,6 +78,13 @@ export function is_topic_editable(message, edit_limit_seconds_buffer = 0) {
); );
} }
function is_widget_message(message) {
if (message.submessages && message.submessages.length !== 0) {
return true;
}
return false;
}
export function get_editability(message, edit_limit_seconds_buffer = 0) { export function get_editability(message, edit_limit_seconds_buffer = 0) {
if (!message) { if (!message) {
return editability_types.NO; return editability_types.NO;
@@ -85,6 +92,7 @@ export function get_editability(message, edit_limit_seconds_buffer = 0) {
if (!is_topic_editable(message, edit_limit_seconds_buffer)) { if (!is_topic_editable(message, edit_limit_seconds_buffer)) {
return editability_types.NO; return editability_types.NO;
} }
if (message.failed_request) { if (message.failed_request) {
// TODO: For completely failed requests, we should be able // TODO: For completely failed requests, we should be able
// to "edit" the message, but it won't really be like // to "edit" the message, but it won't really be like
@@ -104,7 +112,11 @@ export function get_editability(message, edit_limit_seconds_buffer = 0) {
return editability_types.NO; return editability_types.NO;
} }
if (page_params.realm_message_content_edit_limit_seconds === 0 && message.sent_by_me) { if (
page_params.realm_message_content_edit_limit_seconds === 0 &&
message.sent_by_me &&
!is_widget_message(message)
) {
return editability_types.FULL; return editability_types.FULL;
} }
@@ -117,7 +129,8 @@ export function get_editability(message, edit_limit_seconds_buffer = 0) {
edit_limit_seconds_buffer + edit_limit_seconds_buffer +
(message.timestamp - Date.now() / 1000) > (message.timestamp - Date.now() / 1000) >
0 && 0 &&
message.sent_by_me message.sent_by_me &&
!is_widget_message(message)
) { ) {
return editability_types.FULL; return editability_types.FULL;
} }
@@ -345,6 +358,7 @@ function edit_message(row, raw_content) {
message_id: message.id, message_id: message.id,
is_editable, is_editable,
is_content_editable: editability === editability_types.FULL, is_content_editable: editability === editability_types.FULL,
is_widget_message: is_widget_message(message),
has_been_editable: editability !== editability_types.NO, has_been_editable: editability !== editability_types.NO,
topic: message.topic, topic: message.topic,
content: raw_content, content: raw_content,

View File

@@ -67,7 +67,8 @@
{{#if has_been_editable}} {{#if has_been_editable}}
<div class="message-edit-timer-control-group"> <div class="message-edit-timer-control-group">
<span class="message_edit_countdown_timer"></span> <span class="message_edit_countdown_timer"></span>
<span><i id="message_edit_tooltip" class="tippy-zulip-tooltip message_edit_tooltip fa fa-question-circle" aria-hidden="true" data-tippy-content="{{#tr}}This organization is configured to restrict editing of message content to {minutes_to_edit} minutes after it is sent.{{/tr}}"></i> <span><i id="message_edit_tooltip" class="tippy-zulip-tooltip message_edit_tooltip fa fa-question-circle" aria-hidden="true"
{{#if is_widget_message}} data-tippy-content="{{#tr}}Widgets cannot be edited.{{/tr}}" {{else}} data-tippy-content="{{#tr}}This organization is configured to restrict editing of message content to {minutes_to_edit} minutes after it is sent.{{/tr}}" {{/if}}></i>
</span> </span>
</div> </div>
{{/if}} {{/if}}

View File

@@ -3,7 +3,7 @@ import re
from typing import Any, Optional, Tuple from typing import Any, Optional, Tuple
from zerver.lib.message import SendMessageRequest from zerver.lib.message import SendMessageRequest
from zerver.models import SubMessage from zerver.models import Message, SubMessage
def get_widget_data(content: str) -> Tuple[Optional[str], Optional[str]]: def get_widget_data(content: str) -> Tuple[Optional[str], Optional[str]]:
@@ -77,3 +77,8 @@ def do_widget_post_save_actions(send_request: SendMessageRequest) -> None:
) )
submessage.save() submessage.save()
send_request.submessages = SubMessage.get_raw_db_rows([message_id]) send_request.submessages = SubMessage.get_raw_db_rows([message_id])
def is_widget_message(message: Message) -> bool:
# Right now all messages that are widgetized use submessage, and vice versa.
return message.submessage_set.exists()

View File

@@ -187,6 +187,24 @@ class EditMessageTest(ZulipTestCase):
result = self.client_get("/json/messages/" + str(msg_id)) result = self.client_get("/json/messages/" + str(msg_id))
self.assert_json_error(result, "Invalid message(s)") self.assert_json_error(result, "Invalid message(s)")
# Right now, we prevent users from editing widgets.
def test_edit_submessage(self) -> None:
self.login("hamlet")
msg_id = self.send_stream_message(
self.example_user("hamlet"),
"Scotland",
topic_name="editing",
content="/poll Games?\nYES\nNO",
)
result = self.client_patch(
"/json/messages/" + str(msg_id),
{
"message_id": msg_id,
"content": "/poll Games?\nYES\nNO\nMaybe",
},
)
self.assert_json_error(result, "Widgets cannot be edited.")
def test_edit_message_no_permission(self) -> None: def test_edit_message_no_permission(self) -> None:
self.login("hamlet") self.login("hamlet")
msg_id = self.send_stream_message( msg_id = self.send_stream_message(

View File

@@ -24,6 +24,7 @@ from zerver.lib.streams import access_stream_by_id
from zerver.lib.timestamp import datetime_to_timestamp from zerver.lib.timestamp import datetime_to_timestamp
from zerver.lib.topic import LEGACY_PREV_TOPIC, REQ_topic from zerver.lib.topic import LEGACY_PREV_TOPIC, REQ_topic
from zerver.lib.validator import check_bool, check_string_in, to_non_negative_int from zerver.lib.validator import check_bool, check_string_in, to_non_negative_int
from zerver.lib.widget import is_widget_message
from zerver.models import Message, Realm, UserProfile from zerver.models import Message, Realm, UserProfile
@@ -139,6 +140,10 @@ def update_message_backend(
else: else:
raise JsonableError(_("You don't have permission to edit this message")) raise JsonableError(_("You don't have permission to edit this message"))
# Right now, we prevent users from editing widgets.
if content is not None and is_widget_message(message):
return json_error(_("Widgets cannot be edited."))
# If there is a change to the content, check that it hasn't been too long # If there is a change to the content, check that it hasn't been too long
# Allow an extra 20 seconds since we potentially allow editing 15 seconds # Allow an extra 20 seconds since we potentially allow editing 15 seconds
# past the limit, and in case there are network issues, etc. The 15 comes # past the limit, and in case there are network issues, etc. The 15 comes