From 285c1f467e3464ec850f4c2837cc3e062c5c82cb Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 1 May 2025 15:43:39 -0700 Subject: [PATCH 1/2] store [nfc]: Move store.sendMessage up near other MessageStore proxy impls Thanks Greg for noticing this: https://github.com/zulip/zulip-flutter/pull/1484#discussion_r2070836515 --- lib/model/store.dart | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 939120113e..be25737b38 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -737,6 +737,11 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor void unregisterMessageList(MessageListView view) => _messages.unregisterMessageList(view); @override + Future sendMessage({required MessageDestination destination, required String content}) { + assert(!_disposed); + return _messages.sendMessage(destination: destination, content: content); + } + @override void reconcileMessages(List messages) { _messages.reconcileMessages(messages); // TODO(#649) notify [unreads] of the just-fetched messages @@ -904,12 +909,6 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor } } - @override - Future sendMessage({required MessageDestination destination, required String content}) { - assert(!_disposed); - return _messages.sendMessage(destination: destination, content: content); - } - static List _sortCustomProfileFields(List initialCustomProfileFields) { // TODO(server): The realm-wide field objects have an `order` property, // but the actual API appears to be that the fields should be shown in From f1c0e0f24e06d5f3d75ab40e442a7b0c7a17d4e4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 23 Apr 2025 19:54:11 -0700 Subject: [PATCH 2/2] model: Implement edit-message methods on MessageStore Related: #126 --- lib/model/message.dart | 87 +++++++++++ lib/model/store.dart | 15 ++ test/model/message_test.dart | 269 +++++++++++++++++++++++++++++++++++ 3 files changed, 371 insertions(+) diff --git a/lib/model/message.dart b/lib/model/message.dart index fd5de1adbd..cd1e869a16 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -35,6 +35,36 @@ mixin MessageStore { /// All [Message] objects in the resulting list will be present in /// [this.messages]. void reconcileMessages(List messages); + + /// Whether the current edit request for the given message, if any, has failed. + /// + /// Will be null if there is no current edit request. + /// Will be false if the current request hasn't failed + /// and the update-message event hasn't arrived. + bool? getEditMessageErrorStatus(int messageId); + + /// Edit a message's content, via a request to the server. + /// + /// Should only be called when there is no current edit request for [messageId], + /// i.e., [getEditMessageErrorStatus] returns null for [messageId]. + /// + /// See also: + /// * [getEditMessageErrorStatus] + /// * [takeFailedMessageEdit] + void editMessage({required int messageId, required String newContent}); + + /// Forgets the failed edit request and returns the attempted new content. + /// + /// Should only be called when there is a failed request, + /// per [getEditMessageErrorStatus]. + String takeFailedMessageEdit(int messageId); +} + +class _EditMessageRequestStatus { + _EditMessageRequestStatus({required this.hasError, required this.newContent}); + + bool hasError; + final String newContent; } class MessageStoreImpl extends PerAccountStoreBase with MessageStore { @@ -132,6 +162,56 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } } + @override + bool? getEditMessageErrorStatus(int messageId) => + _editMessageRequests[messageId]?.hasError; + + final Map _editMessageRequests = {}; + + @override + void editMessage({ + required int messageId, + required String newContent, + }) async { + if (_editMessageRequests.containsKey(messageId)) { + throw StateError('an edit request is already in progress'); + } + + _editMessageRequests[messageId] = _EditMessageRequestStatus( + hasError: false, newContent: newContent); + _notifyMessageListViewsForOneMessage(messageId); + try { + await updateMessage(connection, messageId: messageId, content: newContent); + // On success, we'll clear the status from _editMessageRequests + // when we get the event. + } catch (e) { + // TODO(log) if e is something unexpected + + final status = _editMessageRequests[messageId]; + if (status == null) { + // The event actually arrived before this request failed + // (can happen with network issues). + // Or, the message was deleted. + return; + } + status.hasError = true; + _notifyMessageListViewsForOneMessage(messageId); + } + } + + @override + String takeFailedMessageEdit(int messageId) { + final status = _editMessageRequests.remove(messageId); + _notifyMessageListViewsForOneMessage(messageId); + if (status == null) { + throw StateError('called takeFailedMessageEdit, but no edit'); + } + if (!status.hasError) { + throw StateError("called takeFailedMessageEdit, but edit hasn't failed"); + } + return status.newContent; + } + void handleUserTopicEvent(UserTopicEvent event) { for (final view in _messageListViews) { view.handleUserTopicEvent(event); @@ -183,6 +263,12 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { // The message is guaranteed to be edited. // See also: https://zulip.com/api/get-events#update_message message.editState = MessageEditState.edited; + + // Clear the edit-message progress feedback. + // This makes a rare bug where we might clear the feedback too early, + // if the user raced with themself to edit the same message + // from multiple clients. + _editMessageRequests.remove(message.id); } if (event.renderedContent != null) { assert(message.contentType == 'text/html', @@ -245,6 +331,7 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { void handleDeleteMessageEvent(DeleteMessageEvent event) { for (final messageId in event.messageIds) { messages.remove(messageId); + _editMessageRequests.remove(messageId); } for (final view in _messageListViews) { view.handleDeleteMessageEvent(event); diff --git a/lib/model/store.dart b/lib/model/store.dart index be25737b38..1414589c34 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -747,6 +747,21 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor // TODO(#649) notify [unreads] of the just-fetched messages // TODO(#650) notify [recentDmConversationsView] of the just-fetched messages } + @override + bool? getEditMessageErrorStatus(int messageId) { + assert(!_disposed); + return _messages.getEditMessageErrorStatus(messageId); + } + @override + void editMessage({required int messageId, required String newContent}) { + assert(!_disposed); + return _messages.editMessage(messageId: messageId, newContent: newContent); + } + @override + String takeFailedMessageEdit(int messageId) { + assert(!_disposed); + return _messages.takeFailedMessageEdit(messageId); + } @override Set get debugMessageListViews => _messages.debugMessageListViews; diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 1f774e32b9..4658857e8a 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -1,10 +1,13 @@ import 'dart:convert'; +import 'dart:io'; import 'package:checks/checks.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/submessage.dart'; +import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/message_list.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; @@ -13,6 +16,7 @@ import '../api/fake_api.dart'; import '../api/model/model_checks.dart'; import '../api/model/submessage_checks.dart'; import '../example_data.dart' as eg; +import '../fake_async.dart'; import '../stdlib_checks.dart'; import 'message_list_test.dart'; import 'store_checks.dart'; @@ -123,6 +127,271 @@ void main() { }); }); + group('edit-message methods', () { + late StreamMessage message; + Future prepareEditMessage() async { + await prepare(); + message = eg.streamMessage(); + await prepareMessages([message]); + check(connection.takeRequests()).length.equals(1); // message-list fetchInitial + } + + void checkRequest(int messageId, String content) { + check(connection.takeRequests()).single.isA() + ..method.equals('PATCH') + ..url.path.equals('/api/v1/messages/$messageId') + ..bodyFields.deepEquals({ + 'content': content, + }); + } + + test('smoke', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkRequest(message.id, 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Mid-request + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + + async.elapse(Duration(milliseconds: 500)); + // Request has succeeded; event hasn't arrived + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + checkNotNotified(); + + await store.handleEvent(eg.updateMessageEditEvent(message)); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + })); + + test('concurrent edits on different messages', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + final otherMessage = eg.streamMessage(); + await store.addMessage(otherMessage); + checkNotifiedOnce(); + + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkRequest(message.id, 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Mid-first request + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + 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'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // First request has succeeded; event hasn't arrived + // Mid-second request + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + check(store.getEditMessageErrorStatus(otherMessage.id)).isNotNull().isFalse(); + checkNotNotified(); + + // First event arrives + await store.handleEvent(eg.updateMessageEditEvent(message)); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Second request has succeeded; event hasn't arrived + check(store.getEditMessageErrorStatus(otherMessage.id)).isNotNull().isFalse(); + checkNotNotified(); + + // Second event arrives + await store.handleEvent(eg.updateMessageEditEvent(otherMessage)); + check(store.getEditMessageErrorStatus(otherMessage.id)).isNull(); + checkNotifiedOnce(); + })); + + test('request fails', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + async.elapse(Duration(seconds: 1)); + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); + checkNotifiedOnce(); + })); + + test('request fails; take failed edit', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + async.elapse(Duration(seconds: 1)); + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); + checkNotifiedOnce(); + + check(store.takeFailedMessageEdit(message.id)).equals('new content'); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + })); + + test('takeFailedMessageEdit throws StateError when nothing to take', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + check(() => store.takeFailedMessageEdit(message.id)).throws(); + })); + + test('editMessage throws StateError if editMessage already in progress for same message', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + + connection.prepare( + json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, 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')) + .isA>().throws(); + check(connection.takeRequests()).isEmpty(); + })); + + test('event arrives, then request fails', () => awaitFakeAsync((async) async { + // This can happen with network issues. + + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + httpException: const SocketException('failed'), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + await store.handleEvent(eg.updateMessageEditEvent(message)); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + + async.flushTimers(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotNotified(); + })); + + test('request fails, then event arrives', () => awaitFakeAsync((async) async { + // This can happen with network issues. + + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + httpException: const SocketException('failed'), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(seconds: 1)); + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); + checkNotifiedOnce(); + + await store.handleEvent(eg.updateMessageEditEvent(message)); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + })); + + test('request fails, then event arrives; take failed edit in between', () => awaitFakeAsync((async) async { + // This can happen with network issues. + + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + httpException: const SocketException('failed'), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(seconds: 1)); + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); + checkNotifiedOnce(); + check(store.takeFailedMessageEdit(message.id)).equals('new content'); + checkNotifiedOnce(); + + await store.handleEvent(eg.updateMessageEditEvent(message)); // no error + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); // content updated + })); + + test('request fails, then message deleted', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + async.elapse(Duration(seconds: 1)); + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isTrue(); + checkNotifiedOnce(); + + await store.handleEvent(eg.deleteMessageEvent([message])); // no error + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + })); + + test('message deleted while request in progress; we get failure response', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare(apiException: eg.apiBadRequest(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Mid-request + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + checkNotNotified(); + + await store.handleEvent(eg.deleteMessageEvent([message])); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Request failure, but status has already been cleared + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotNotified(); + })); + + test('message deleted while request in progress but we get success response', () => awaitFakeAsync((async) async { + await prepareEditMessage(); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + + connection.prepare( + json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + store.editMessage(messageId: message.id, newContent: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Mid-request + check(store.getEditMessageErrorStatus(message.id)).isNotNull().isFalse(); + checkNotNotified(); + + await store.handleEvent(eg.deleteMessageEvent([message])); + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + + async.elapse(Duration(milliseconds: 500)); + // Request success + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotNotified(); + })); + }); + group('handleMessageEvent', () { test('from empty', () async { await prepare();