diff --git a/lib/model/message.dart b/lib/model/message.dart index 936bc3bb1f..26a8bb13a2 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -519,7 +519,8 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage content: newContent, prevContentSha256: sha256.convert(utf8.encode(originalRawContent)).toString()); // On success, we'll clear the status from _editMessageRequests - // when we get the event. + // when we get the event (or below, if in an unsubscribed channel). + if (_disposed) return; } catch (e) { // TODO(log) if e is something unexpected @@ -536,6 +537,15 @@ class MessageStoreImpl extends HasChannelStore with MessageStore, _OutboxMessage _notifyMessageListViewsForOneMessage(messageId); rethrow; } + + final message = messages[messageId]; + if (message is StreamMessage && subscriptions[message.streamId] == null) { + // The message is in an unsubscribed channel. We don't expect an event + // (see "third buggy behavior" in #1798) but we know the edit request + // succeeded, so, clear the pending-edit state. + _editMessageRequests.remove(messageId); + _notifyMessageListViewsForOneMessage(messageId); + } } @override @@ -889,13 +899,18 @@ const kSendMessageOfferRestoreWaitPeriod = Duration(seconds: 10); // TODO(#1441 /// timed out. not finished when /// wait period timed out. /// -/// Event received. -/// (any state) ─────────────────► (delete) +/// Event received, or [sendMessage] +/// request succeeds and we're sending to +/// an unsubscribed channel. +/// (any state) ───────────────────────────────────────► (delete) /// ``` /// /// During its lifecycle, it is guaranteed that the outbox message is deleted /// as soon a message event with a matching [MessageEvent.localMessageId] /// arrives. +/// If we're sending to an unsubscribed channel, we don't expect an event +/// (see "third buggy behavior" in #1798) so in that case +/// the outbox message is deleted when the [sendMessage] request succeeds. enum OutboxMessageState { /// The [sendMessage] HTTP request has started but the resulting /// [MessageEvent] hasn't arrived, and nor has the request failed. In this @@ -1148,6 +1163,19 @@ mixin _OutboxMessageStore on HasChannelStore { // The message event already arrived; nothing to do. return; } + + if (destination is StreamDestination && subscriptions[destination.streamId] == null) { + // We don't expect an event (we're sending to an unsubscribed channel); + // clear the loading spinner. + _outboxMessages.remove(localMessageId); + _outboxMessageDebounceTimers.remove(localMessageId)?.cancel(); + _outboxMessageWaitPeriodTimers.remove(localMessageId)?.cancel(); + for (final view in _messageListViews) { + view.notifyListenersIfOutboxMessagePresent(localMessageId); + } + return; + } + // The send request succeeded, so the message was definitely sent. // Cancel the timer that would have had us start presuming that the // send might have failed. diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 6e83356042..1e3465125e 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -1324,13 +1324,14 @@ class _SendButtonState extends State<_SendButton> { return; } - final store = PerAccountStoreWidget.of(context); + PerAccountStore store = PerAccountStoreWidget.of(context); final content = controller.content.textNormalized; controller.content.clear(); try { await store.sendMessage(destination: widget.getDestination(), content: content); + if (!mounted) return; } on ApiRequestException catch (e) { if (!mounted) return; final zulipLocalizations = ZulipLocalizations.of(context); @@ -1343,6 +1344,20 @@ class _SendButtonState extends State<_SendButton> { message: message); return; } + + store = PerAccountStoreWidget.of(context); + final destination = widget.getDestination(); + if ( + destination is StreamDestination + && store.subscriptions[destination.streamId] == null + ) { + // We don't get new-message events for unsubscribed channels, + // but we can refresh the view when a send-message request succeeds, + // so the user will at least see their own messages without having to + // exit and re-enter. See the "first buggy behavior" in + // https://github.com/zulip/zulip-flutter/issues/1798 . + MessageListPage.ancestorOf(context).refresh(AnchorCode.newest); + } } @override @@ -1854,7 +1869,7 @@ class _EditMessageBannerTrailing extends StatelessWidget { // disappears, which may be long after the banner disappears.) final pageContext = PageRoot.contextOf(context); - final store = PerAccountStoreWidget.of(pageContext); + PerAccountStore store = PerAccountStoreWidget.of(pageContext); final controller = composeBoxState.controller; if (controller is! EditMessageComposeBoxController) return; // TODO(log) final zulipLocalizations = ZulipLocalizations.of(pageContext); @@ -1885,6 +1900,7 @@ class _EditMessageBannerTrailing extends StatelessWidget { messageId: messageId, originalRawContent: originalRawContent, newContent: newContent); + if (!pageContext.mounted) return; } on ApiRequestException catch (e) { if (!pageContext.mounted) return; final zulipLocalizations = ZulipLocalizations.of(pageContext); @@ -1897,6 +1913,18 @@ class _EditMessageBannerTrailing extends StatelessWidget { message: message); return; } + + store = PerAccountStoreWidget.of(pageContext); + final messageListPageState = MessageListPage.ancestorOf(pageContext); + final narrow = messageListPageState.narrow; + if (narrow is ChannelNarrow && store.subscriptions[narrow.streamId] == null) { + // We don't get edit-message events for unsubscribed channels, + // but we can refresh the view when an edit-message request succeeds, + // so the user will at least see their updated message without having to + // exit and re-enter. See the "first buggy behavior" in + // https://github.com/zulip/zulip-flutter/issues/1798 . + messageListPageState.refresh(NumericAnchor(messageId)); + } } @override diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 31c24c0c0e..d5bbd50a86 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -53,6 +53,7 @@ void main() { /// Initialize [store] and the rest of the test state. Future prepare({ + Narrow narrow = const CombinedFeedNarrow(), ZulipStream? stream, bool isChannelSubscribed = true, int? zulipFeatureLevel, @@ -71,7 +72,7 @@ void main() { connection = store.connection as FakeApiConnection; notifiedCount = 0; messageList = MessageListView.init(store: store, - narrow: const CombinedFeedNarrow(), + narrow: narrow, anchor: AnchorCode.newest) ..addListener(() { notifiedCount++; @@ -172,11 +173,17 @@ void main() { check(store.outboxMessages).values.single.state; Future prepareOutboxMessage({ + Narrow narrow = const CombinedFeedNarrow(), MessageDestination? destination, + bool isChannelSubscribed = true, int? zulipFeatureLevel, }) async { message = eg.streamMessage(stream: stream); - await prepare(stream: stream, zulipFeatureLevel: zulipFeatureLevel); + await prepare( + narrow: narrow, + stream: stream, + isChannelSubscribed: isChannelSubscribed, + zulipFeatureLevel: zulipFeatureLevel); await prepareMessages([eg.streamMessage(stream: stream)]); connection.prepare(json: SendMessageResult(id: 1).toJson()); await store.sendMessage( @@ -368,6 +375,16 @@ void main() { checkNotNotified(); })); + test('hidden -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async { + await prepareOutboxMessage( + // Not CombinedFeedNarrow, + // which always hides outbox messages in unsubscribed channels. + narrow: ChannelNarrow(stream.streamId), + isChannelSubscribed: false); + check(store.outboxMessages).isEmpty(); + checkNotNotified(); + })); + test('waiting -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessage(); async.elapse(kLocalEchoDebounceDuration); @@ -400,6 +417,28 @@ void main() { checkNotNotified(); })); + test('waiting -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async { + await prepare( + // Not CombinedFeedNarrow, + // which always hides outbox messages in unsubscribed channels. + narrow: ChannelNarrow(stream.streamId), + stream: stream, + isChannelSubscribed: false); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(json: SendMessageResult(id: 1).toJson(), + delay: kLocalEchoDebounceDuration + Duration(seconds: 1)); + final future = store.sendMessage( + destination: streamDestination, content: 'content'); + async.elapse(kLocalEchoDebounceDuration); + checkState().equals(OutboxMessageState.waiting); + checkNotifiedOnce(); + + async.elapse(Duration(seconds: 1)); + await check(future).completes(); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + test('waitPeriodExpired -> (delete) when event arrives before send request fails', () => awaitFakeAsync((async) async { // Set up an error to fail `sendMessage` with a delay, leaving time for // the message event to arrive. @@ -435,6 +474,27 @@ void main() { checkNotifiedOnce(); })); + test('waitPeriodExpired -> (delete) because send succeeded in unsubscribed channel', () => awaitFakeAsync((async) async { + await prepare( + // Not CombinedFeedNarrow, + // which always hides outbox messages in unsubscribed channels. + narrow: ChannelNarrow(stream.streamId), + isChannelSubscribed: false); + await prepareMessages([eg.streamMessage(stream: stream)]); + connection.prepare(json: SendMessageResult(id: 1).toJson(), + delay: kSendMessageOfferRestoreWaitPeriod + Duration(seconds: 1)); + final future = store.sendMessage( + destination: streamDestination, content: 'content'); + async.elapse(kSendMessageOfferRestoreWaitPeriod); + checkState().equals(OutboxMessageState.waitPeriodExpired); + checkNotified(count: 2); + + async.elapse(Duration(seconds: 1)); + await check(future).completes(); + check(store.outboxMessages).isEmpty(); + checkNotifiedOnce(); + })); + test('failed -> (delete) because event received', () => awaitFakeAsync((async) async { await prepareOutboxMessageToFailAfterDelay(Duration.zero); await check(outboxMessageFailFuture).throws(); @@ -993,6 +1053,30 @@ void main() { check(store.getEditMessageErrorStatus(message.id)).isNull(); checkNotNotified(); })); + + test('request succeeds, in unsubscribed channel', () => awaitFakeAsync((async) async { + final channel = eg.stream(); + message = eg.streamMessage(stream: channel, sender: eg.selfUser); + await prepare( + narrow: ChannelNarrow(channel.streamId), + stream: channel, + isChannelSubscribed: false, + ); + await prepareMessages([message]); + check(connection.takeRequests()).length.equals(1); // message-list fetchInitial + + connection.prepare( + json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + unawaited(store.editMessage(messageId: message.id, + originalRawContent: 'old content', newContent: 'new content')); + checkRequest(message.id, prevContent: 'old content', content: 'new content'); + checkNotifiedOnce(); + + async.elapse(Duration(seconds: 1)); + // Outbox status cleared already (we don't expect an edit-message event). + check(store.getEditMessageErrorStatus(message.id)).isNull(); + checkNotifiedOnce(); + })); }); group('selfCanDeleteMessage', () { diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index d27e374e3b..4e9729fb60 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -14,6 +14,7 @@ import 'package:flutter/material.dart'; import 'package:image_picker/image_picker.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/model/narrow.dart'; import 'package:zulip/api/route/channels.dart'; import 'package:zulip/api/route/messages.dart'; import 'package:zulip/model/localizations.dart'; @@ -34,6 +35,7 @@ import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../flutter_checks.dart'; import '../model/binding.dart'; +import '../model/content_test.dart'; import '../model/message_list_test.dart'; import '../model/store_checks.dart'; import '../model/test_store.dart'; @@ -972,6 +974,59 @@ void main() { 'You do not have permission to initiate direct message conversations.'), ))); }); + + testWidgets('if channel is unsubscribed, refresh on message-send success', (tester) async { + // Regression test for the "first buggy behavior" + // in https://github.com/zulip/zulip-flutter/issues/1798 . + TypingNotifier.debugEnable = false; + addTearDown(TypingNotifier.debugReset); + assert(MessageStoreImpl.debugOutboxEnable); + + final channel = eg.stream(); + final narrow = eg.topicNarrow(channel.streamId, 'some topic'); + final messages = [eg.streamMessage(stream: channel, topic: 'some topic')]; + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + await prepareComposeBox(tester, + narrow: narrow, + streams: [channel], + messages: messages); + assert(store.subscriptions[channel.streamId] == null); + + await enterContent(tester, 'hello world'); + + connection.prepare( + json: SendMessageResult(id: 456).toJson(), delay: Duration(seconds: 1)); + await tester.tap(find.byTooltip(zulipLocalizations.composeBoxSendTooltip)); + await tester.pump(Duration(milliseconds: 500)); + check(connection.takeRequests()).single.isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages'); + + final newMessage = eg.streamMessage( + stream: channel, topic: 'some topic', + content: '

hello world

'); + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: [...messages, newMessage]).toJson()); + await tester.pump(Duration(milliseconds: 500)); + check(connection.lastRequest).isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(resolveApiNarrowForServer( + narrow.apiEncode(), connection.zulipFeatureLevel!)), + 'anchor': 'newest', + 'num_before': '100', + 'num_after': '100', + 'allow_empty_topic_name': 'true', + }); + check(find.descendant( + of: find.byType(MessageWithPossibleSender), + matching: find.text('hello world')) + ).findsOne(); + // Regression coverage for the "third buggy behavior" + // in https://github.com/zulip/zulip-flutter/issues/1798 . + check(find.byType(OutboxMessageWithPossibleSender)).findsNothing(); + }); }); group('sending to empty topic', () { @@ -2052,6 +2107,21 @@ void main() { checkContentInputValue(tester, expectedContentText); } + /// Check whether the message in the message list says "SAVING EDIT…". + /// + /// This state is tested more thoroughly + /// in test/widgets/message_list_test.dart, naturally. + void checkEditInProgressInMsglist(WidgetTester tester, + {required int messageId, required bool expected}) { + final messageFinder = find.byWidgetPredicate((widget) => + widget is MessageWithPossibleSender && widget.item.message.id == messageId); + + check(find.descendant( + of: messageFinder, + matching: find.text('SAVING EDIT…'), + ).evaluate().length).equals(expected ? 1 : 0); + } + void testSmoke({required Narrow narrow, required _EditInteractionStart start}) { testWidgets('smoke: $narrow, ${start.message()}', (tester) async { await prepareEditMessage(tester, narrow: narrow); @@ -2102,6 +2172,10 @@ void main() { prevContent: 'foo', content: 'some new content[file.jpg](/path/file.jpg)'); await tester.pump(Duration.zero); checkNotInEditingMode(tester, narrow: narrow); + + // We'll say "SAVING EDIT…" in the message list until the event arrives. + // (No need to make the event arrive here; message-list tests do that.) + checkEditInProgressInMsglist(tester, messageId: messageId, expected: true); }); } testSmoke(narrow: channelNarrow, start: _EditInteractionStart.actionSheet); @@ -2359,6 +2433,57 @@ void main() { testCancel(narrow: channelNarrow, start: _EditInteractionStart.restoreFailedEdit); // testCancel(narrow: topicNarrow, start: _EditInteractionStart.restoreFailedEdit); // testCancel(narrow: dmNarrow, start: _EditInteractionStart.restoreFailedEdit); + + testWidgets('if channel is unsubscribed, refresh on message-edit success', (tester) async { + // Regression test for the "first buggy behavior" + // in https://github.com/zulip/zulip-flutter/issues/1798 . + + final channel = eg.stream(); + final narrow = ChannelNarrow(channel.streamId); + final message = eg.streamMessage(stream: channel, sender: eg.selfUser); + + await prepareComposeBox(tester, narrow: narrow, streams: [channel]); + await store.addMessages([message]); + check(store.subscriptions[channel.streamId]).isNull(); + await tester.pump(); // message list updates + + await startEditInteractionFromActionSheet(tester, + messageId: message.id, originalRawContent: 'foo'); + await tester.pump(Duration(seconds: 1)); // fetch-raw-content request + checkContentInputValue(tester, 'foo'); + + final newMarkdownContent = ContentExample.emojiUnicode.markdown!; + await enterContent(tester, newMarkdownContent); + + connection.prepare(json: UpdateMessageResult().toJson(), delay: Duration(seconds: 1)); + await tester.tap(find.widgetWithText(ZulipWebUiKitButton, 'Save')); + await tester.pump(Duration(milliseconds: 500)); + checkRequest(message.id, prevContent: 'foo', content: newMarkdownContent); + + final updatedMessage = + Message.fromJson(message.toJson()..['content'] = ContentExample.emojiUnicode.html); + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: [updatedMessage]).toJson()); + await tester.pump(Duration(milliseconds: 500)); + check(connection.lastRequest).isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/messages') + ..url.queryParameters.deepEquals({ + 'narrow': jsonEncode(resolveApiNarrowForServer( + narrow.apiEncode(), connection.zulipFeatureLevel!)), + 'anchor': '${message.id}', + 'num_before': '100', + 'num_after': '100', + 'allow_empty_topic_name': 'true', + }); + check(find.descendant( + of: find.byType(MessageWithPossibleSender), + matching: find.text(ContentExample.emojiUnicode.expectedText!)) + ).findsOne(); + // Regression coverage for the "third buggy behavior" + // in https://github.com/zulip/zulip-flutter/issues/1798 . + checkEditInProgressInMsglist(tester, messageId: message.id, expected: false); + }); }); }