Skip to content

Edit-message (8/n): Add/use prevContentSha256 param #1496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 3, 2025
Merged

Edit-message (8/n): Add/use prevContentSha256 param #1496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19aa212
api: Add prevContentSha256 param to updateMessage route
chrisbobbe May 1, 2025
6ff20f6
model: Use prevContentSha256 in edit-message method
chrisbobbe May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ Future<UpdateMessageResult> updateMessage(
bool? sendNotificationToOldThread,
bool? sendNotificationToNewThread,
String? content,
String? prevContentSha256,
int? streamId,
}) {
return connection.patch('updateMessage', UpdateMessageResult.fromJson, 'messages/$messageId', {
Expand All @@ -283,6 +284,7 @@ Future<UpdateMessageResult> updateMessage(
if (sendNotificationToOldThread != null) 'send_notification_to_old_thread': sendNotificationToOldThread,
if (sendNotificationToNewThread != null) 'send_notification_to_new_thread': sendNotificationToNewThread,
if (content != null) 'content': RawParameter(content),
if (prevContentSha256 != null) 'prev_content_sha256': RawParameter(prevContentSha256),
if (streamId != null) 'stream_id': streamId,
});
}
Expand Down
14 changes: 12 additions & 2 deletions lib/model/message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'dart:convert';

import 'package:crypto/crypto.dart';

import '../api/model/events.dart';
import '../api/model/model.dart';
import '../api/route/messages.dart';
Expand Down Expand Up @@ -51,7 +53,11 @@ mixin MessageStore {
/// See also:
/// * [getEditMessageErrorStatus]
/// * [takeFailedMessageEdit]
void editMessage({required int messageId, required String newContent});
void editMessage({
required int messageId,
required String originalRawContent,
required String newContent,
});

/// Forgets the failed edit request and returns the attempted new content.
///
Expand Down Expand Up @@ -171,6 +177,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
@override
void editMessage({
required int messageId,
required String originalRawContent,
required String newContent,
}) async {
if (_editMessageRequests.containsKey(messageId)) {
Expand All @@ -181,7 +188,10 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
hasError: false, newContent: newContent);
_notifyMessageListViewsForOneMessage(messageId);
try {
await updateMessage(connection, messageId: messageId, content: newContent);
await updateMessage(connection,
messageId: messageId,
content: newContent,
prevContentSha256: sha256.convert(utf8.encode(originalRawContent)).toString());
// On success, we'll clear the status from _editMessageRequests
// when we get the event.
} catch (e) {
Expand Down
9 changes: 7 additions & 2 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,9 +753,14 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor
return _messages.getEditMessageErrorStatus(messageId);
}
@override
void editMessage({required int messageId, required String newContent}) {
void editMessage({
required int messageId,
required String originalRawContent,
required String newContent,
}) {
assert(!_disposed);
return _messages.editMessage(messageId: messageId, newContent: newContent);
return _messages.editMessage(messageId: messageId,
originalRawContent: originalRawContent, newContent: newContent);
}
@override
String takeFailedMessageEdit(int messageId) {
Expand Down
16 changes: 16 additions & 0 deletions test/api/route/messages_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ void main() {
bool? sendNotificationToOldThread,
bool? sendNotificationToNewThread,
String? content,
String? prevContentSha256,
int? streamId,
required Map<String, String> expected,
}) async {
Expand All @@ -464,6 +465,7 @@ void main() {
sendNotificationToOldThread: sendNotificationToOldThread,
sendNotificationToNewThread: sendNotificationToNewThread,
content: content,
prevContentSha256: prevContentSha256,
streamId: streamId,
);
check(connection.lastRequest).isA<http.Request>()
Expand All @@ -473,6 +475,20 @@ void main() {
return result;
}

test('pure content change', () {
return FakeApiConnection.with_((connection) async {
connection.prepare(json: UpdateMessageResult().toJson());
await checkUpdateMessage(connection,
messageId: eg.streamMessage().id,
content: 'asdf',
prevContentSha256: '34a780ad578b997db55b260beb60b501f3e04d30ba1a51fcf43cd8dd1241780d',
expected: {
'content': 'asdf',
'prev_content_sha256': '34a780ad578b997db55b260beb60b501f3e04d30ba1a51fcf43cd8dd1241780d',
});
});
});

test('topic/content change', () {
// A separate test exercises `streamId`;
// the API doesn't allow changing channel and content at the same time.
Expand Down
59 changes: 42 additions & 17 deletions test/model/message_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'dart:convert';
import 'dart:io';

import 'package:checks/checks.dart';
import 'package:crypto/crypto.dart';
import 'package:http/http.dart' as http;
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/events.dart';
Expand Down Expand Up @@ -136,11 +137,16 @@ void main() {
check(connection.takeRequests()).length.equals(1); // message-list fetchInitial
}

void checkRequest(int messageId, String content) {
void checkRequest(int messageId, {
required String prevContent,
required String content,
}) {
final prevContentSha256 = sha256.convert(utf8.encode(prevContent)).toString();
check(connection.takeRequests()).single.isA<http.Request>()
..method.equals('PATCH')
..url.path.equals('/api/v1/messages/$messageId')
..bodyFields.deepEquals({
'prev_content_sha256': prevContentSha256,
'content': content,
});
}
Expand All @@ -151,8 +157,11 @@ void main() {

connection.prepare(
json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
checkRequest(message.id, 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkRequest(message.id,
prevContent: 'old content',
content: 'new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand All @@ -179,8 +188,11 @@ void main() {

connection.prepare(
json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
checkRequest(message.id, 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkRequest(message.id,
prevContent: 'old content',
content: 'new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand All @@ -189,8 +201,11 @@ void main() {
check(store.getEditMessageErrorStatus(otherMessage.id)).isNull();
connection.prepare(
json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
store.editMessage(messageId: otherMessage.id, newContent: 'other message new content');
checkRequest(otherMessage.id, 'other message new content');
store.editMessage(messageId: otherMessage.id,
originalRawContent: 'other message old content', newContent: 'other message new content');
checkRequest(otherMessage.id,
prevContent: 'other message old content',
content: 'other message new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand Down Expand Up @@ -221,7 +236,8 @@ void main() {
check(store.getEditMessageErrorStatus(message.id)).isNull();

connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();
async.elapse(Duration(seconds: 1));
check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
Expand All @@ -233,7 +249,8 @@ void main() {
check(store.getEditMessageErrorStatus(message.id)).isNull();

connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();
async.elapse(Duration(seconds: 1));
check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
Expand All @@ -255,12 +272,14 @@ void main() {

connection.prepare(
json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
async.elapse(Duration(milliseconds: 500));
check(connection.takeRequests()).length.equals(1);
checkNotifiedOnce();

await check(store.editMessage(messageId: message.id, newContent: 'newer content'))
await check(store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'newer content'))
.isA<Future<void>>().throws<StateError>();
check(connection.takeRequests()).isEmpty();
}));
Expand All @@ -273,7 +292,8 @@ void main() {

connection.prepare(
httpException: const SocketException('failed'), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand All @@ -294,7 +314,8 @@ void main() {

connection.prepare(
httpException: const SocketException('failed'), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();

async.elapse(Duration(seconds: 1));
Expand All @@ -314,7 +335,8 @@ void main() {

connection.prepare(
httpException: const SocketException('failed'), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();

async.elapse(Duration(seconds: 1));
Expand All @@ -333,7 +355,8 @@ void main() {
check(store.getEditMessageErrorStatus(message.id)).isNull();

connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();
async.elapse(Duration(seconds: 1));
check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue();
Expand All @@ -349,7 +372,8 @@ void main() {
check(store.getEditMessageErrorStatus(message.id)).isNull();

connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand All @@ -373,7 +397,8 @@ void main() {

connection.prepare(
json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1));
store.editMessage(messageId: message.id, newContent: 'new content');
store.editMessage(messageId: message.id,
originalRawContent: 'old content', newContent: 'new content');
checkNotifiedOnce();

async.elapse(Duration(milliseconds: 500));
Expand Down