From f2e182a3880557de2562d67a29c539d2bcb0c3a1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jul 2024 13:55:23 -0700 Subject: [PATCH 1/3] actions [nfc]: Move markNarrowAsRead to new file This function isn't quite part of the UI -- it's mostly a driver around some API requests. But it doesn't belong in the API code either, because it interacts with UI widgets in order to give progress and failure feedback to the user. Because there's nontrivial logic in here and it isn't really specific to this particular piece of UI, we'd like to reuse much of this code, in particular for #131 marking as unread (as discussed at #779). This new file provides an appropriate home for that reusable code. Before we can actually reuse this for marking as unread, it'll need some refactoring. Before we undertake refactoring it, it should have tests that operate at the layer of this function itself, so that they can easily be adapted to cover the mark-unread case too. There are existing tests that cover this pretty thoroughly, but they act on the UI in the message list that drives it. So we'll port those tests over in the next couple of commits. --- lib/widgets/actions.dart | 138 ++++++++++++++++++++++++++++++++++ lib/widgets/message_list.dart | 124 +----------------------------- 2 files changed, 139 insertions(+), 123 deletions(-) create mode 100644 lib/widgets/actions.dart diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart new file mode 100644 index 0000000000..56b018dc90 --- /dev/null +++ b/lib/widgets/actions.dart @@ -0,0 +1,138 @@ +/// Methods that act through the Zulip API and show feedback in the UI. +/// +/// The methods in this file can be thought of as higher-level wrappers for +/// some of the Zulip API endpoint binding methods in `lib/api/route/`. +/// But they don't belong in `lib/api/`, because they also interact with widgets +/// in order to present success or error feedback to the user through the UI. +library; + +import 'package:flutter/material.dart'; +import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; + +import '../api/model/model.dart'; +import '../api/model/narrow.dart'; +import '../api/route/messages.dart'; +import '../model/narrow.dart'; +import 'dialog.dart'; +import 'store.dart'; + +Future markNarrowAsRead( + BuildContext context, + Narrow narrow, + bool useLegacy, // TODO(server-6) +) async { + final store = PerAccountStoreWidget.of(context); + final connection = store.connection; + if (useLegacy) { + return await _legacyMarkNarrowAsRead(context, narrow); + } + + // Compare web's `mark_all_as_read` in web/src/unread_ops.js + // and zulip-mobile's `markAsUnreadFromMessage` in src/action-sheets/index.js . + final zulipLocalizations = ZulipLocalizations.of(context); + final scaffoldMessenger = ScaffoldMessenger.of(context); + // Use [AnchorCode.oldest], because [AnchorCode.firstUnread] + // will be the oldest non-muted unread message, which would + // result in muted unreads older than the first unread not + // being processed. + Anchor anchor = AnchorCode.oldest; + int responseCount = 0; + int updatedCount = 0; + + // Include `is:unread` in the narrow. That has a database index, so + // this can be an important optimization in narrows with a lot of history. + // The server applies the same optimization within the (deprecated) + // specialized endpoints for marking messages as read; see + // `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`. + final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); + + while (true) { + final result = await updateMessageFlagsForNarrow(connection, + anchor: anchor, + // [AnchorCode.oldest] is an anchor ID lower than any valid + // message ID; and follow-up requests will have already + // processed the anchor ID, so we just want this to be + // unconditionally false. + includeAnchor: false, + // There is an upper limit of 5000 messages per batch + // (numBefore + numAfter <= 5000) enforced on the server. + // See `update_message_flags_in_narrow` in zerver/views/message_flags.py . + // zulip-mobile uses `numAfter` of 5000, but web uses 1000 + // for more responsive feedback. See zulip@f0d87fcf6. + numBefore: 0, + numAfter: 1000, + narrow: apiNarrow, + op: UpdateMessageFlagsOp.add, + flag: MessageFlag.read); + if (!context.mounted) { + scaffoldMessenger.clearSnackBars(); + return; + } + responseCount++; + updatedCount += result.updatedCount; + + if (result.foundNewest) { + if (responseCount > 1) { + // We previously showed an in-progress [SnackBar], so say we're done. + // There may be a backlog of [SnackBar]s accumulated in the queue + // so be sure to clear them out here. + scaffoldMessenger + ..clearSnackBars() + ..showSnackBar(SnackBar(behavior: SnackBarBehavior.floating, + content: Text(zulipLocalizations.markAsReadComplete(updatedCount)))); + } + return; + } + + if (result.lastProcessedId == null) { + // No messages were in the range of the request. + // This should be impossible given that `foundNewest` was false + // (and that our `numAfter` was positive.) + await showErrorDialog(context: context, + title: zulipLocalizations.errorMarkAsReadFailedTitle, + message: zulipLocalizations.errorInvalidResponse); + return; + } + anchor = NumericAnchor(result.lastProcessedId!); + + // The task is taking a while, so tell the user we're working on it. + // No need to say how many messages, as the [MarkAsUnread] widget + // should follow along. + // TODO: Ideally we'd have a progress widget here that showed up based + // on actual time elapsed -- so it could appear before the first + // batch returns, if that takes a while -- and that then stuck + // around continuously until the task ends. For now we use a + // series of [SnackBar]s, which may feel a bit janky. + // There is complexity in tracking the status of each [SnackBar], + // due to having no way to determine which is currently active, + // or if there is an active one at all. Resetting the [SnackBar] here + // results in the same message popping in and out and the user experience + // is better for now if we allow them to run their timer through + // and clear the backlog later. + scaffoldMessenger.showSnackBar(SnackBar(behavior: SnackBarBehavior.floating, + content: Text(zulipLocalizations.markAsReadInProgress))); + } +} + +Future _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async { + final store = PerAccountStoreWidget.of(context); + final connection = store.connection; + switch (narrow) { + case CombinedFeedNarrow(): + await markAllAsRead(connection); + case StreamNarrow(:final streamId): + await markStreamAsRead(connection, streamId: streamId); + case TopicNarrow(:final streamId, :final topic): + await markTopicAsRead(connection, streamId: streamId, topicName: topic); + case DmNarrow(): + final unreadDms = store.unreads.dms[narrow]; + // Silently ignore this race-condition as the outcome + // (no unreads in this narrow) was the desired end-state + // of pushing the button. + if (unreadDms == null) return; + await updateMessageFlags(connection, + messages: unreadDms, + op: UpdateMessageFlagsOp.add, + flag: MessageFlag.read); + } +} diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 5b620c7592..5cebbbe019 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -6,12 +6,11 @@ import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; import 'package:intl/intl.dart'; import '../api/model/model.dart'; -import '../api/model/narrow.dart'; -import '../api/route/messages.dart'; import '../model/message_list.dart'; import '../model/narrow.dart'; import '../model/store.dart'; import 'action_sheet.dart'; +import 'actions.dart'; import 'compose_box.dart'; import 'content.dart'; import 'dialog.dart'; @@ -1007,124 +1006,3 @@ final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); // TODO(#95) need dark-theme color (this one comes from the Figma) final _kMessageTimestampColor = const HSLColor.fromAHSL(1, 0, 0, 0.5).toColor(); - -Future markNarrowAsRead( - BuildContext context, - Narrow narrow, - bool useLegacy, // TODO(server-6) -) async { - final store = PerAccountStoreWidget.of(context); - final connection = store.connection; - if (useLegacy) { - return await _legacyMarkNarrowAsRead(context, narrow); - } - - // Compare web's `mark_all_as_read` in web/src/unread_ops.js - // and zulip-mobile's `markAsUnreadFromMessage` in src/action-sheets/index.js . - final zulipLocalizations = ZulipLocalizations.of(context); - final scaffoldMessenger = ScaffoldMessenger.of(context); - // Use [AnchorCode.oldest], because [AnchorCode.firstUnread] - // will be the oldest non-muted unread message, which would - // result in muted unreads older than the first unread not - // being processed. - Anchor anchor = AnchorCode.oldest; - int responseCount = 0; - int updatedCount = 0; - - // Include `is:unread` in the narrow. That has a database index, so - // this can be an important optimization in narrows with a lot of history. - // The server applies the same optimization within the (deprecated) - // specialized endpoints for marking messages as read; see - // `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`. - final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); - - while (true) { - final result = await updateMessageFlagsForNarrow(connection, - anchor: anchor, - // [AnchorCode.oldest] is an anchor ID lower than any valid - // message ID; and follow-up requests will have already - // processed the anchor ID, so we just want this to be - // unconditionally false. - includeAnchor: false, - // There is an upper limit of 5000 messages per batch - // (numBefore + numAfter <= 5000) enforced on the server. - // See `update_message_flags_in_narrow` in zerver/views/message_flags.py . - // zulip-mobile uses `numAfter` of 5000, but web uses 1000 - // for more responsive feedback. See zulip@f0d87fcf6. - numBefore: 0, - numAfter: 1000, - narrow: apiNarrow, - op: UpdateMessageFlagsOp.add, - flag: MessageFlag.read); - if (!context.mounted) { - scaffoldMessenger.clearSnackBars(); - return; - } - responseCount++; - updatedCount += result.updatedCount; - - if (result.foundNewest) { - if (responseCount > 1) { - // We previously showed an in-progress [SnackBar], so say we're done. - // There may be a backlog of [SnackBar]s accumulated in the queue - // so be sure to clear them out here. - scaffoldMessenger - ..clearSnackBars() - ..showSnackBar(SnackBar(behavior: SnackBarBehavior.floating, - content: Text(zulipLocalizations.markAsReadComplete(updatedCount)))); - } - return; - } - - if (result.lastProcessedId == null) { - // No messages were in the range of the request. - // This should be impossible given that `foundNewest` was false - // (and that our `numAfter` was positive.) - await showErrorDialog(context: context, - title: zulipLocalizations.errorMarkAsReadFailedTitle, - message: zulipLocalizations.errorInvalidResponse); - return; - } - anchor = NumericAnchor(result.lastProcessedId!); - - // The task is taking a while, so tell the user we're working on it. - // No need to say how many messages, as the [MarkAsUnread] widget - // should follow along. - // TODO: Ideally we'd have a progress widget here that showed up based - // on actual time elapsed -- so it could appear before the first - // batch returns, if that takes a while -- and that then stuck - // around continuously until the task ends. For now we use a - // series of [SnackBar]s, which may feel a bit janky. - // There is complexity in tracking the status of each [SnackBar], - // due to having no way to determine which is currently active, - // or if there is an active one at all. Resetting the [SnackBar] here - // results in the same message popping in and out and the user experience - // is better for now if we allow them to run their timer through - // and clear the backlog later. - scaffoldMessenger.showSnackBar(SnackBar(behavior: SnackBarBehavior.floating, - content: Text(zulipLocalizations.markAsReadInProgress))); - } -} - -Future _legacyMarkNarrowAsRead(BuildContext context, Narrow narrow) async { - final store = PerAccountStoreWidget.of(context); - final connection = store.connection; - switch (narrow) { - case CombinedFeedNarrow(): - await markAllAsRead(connection); - case StreamNarrow(:final streamId): - await markStreamAsRead(connection, streamId: streamId); - case TopicNarrow(:final streamId, :final topic): - await markTopicAsRead(connection, streamId: streamId, topicName: topic); - case DmNarrow(): - final unreadDms = store.unreads.dms[narrow]; - // Silently ignore this race-condition as the outcome - // (no unreads in this narrow) was the desired end-state - // of pushing the button. - if (unreadDms == null) return; - await updateMessageFlags(connection, - messages: unreadDms, - op: UpdateMessageFlagsOp.add, - flag: MessageFlag.read); - } -} From f53dfd9b464623b9c6a8a7e94fb48b8f96a60e8b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jul 2024 13:55:33 -0700 Subject: [PATCH 2/3] msglist test [nfc]: Fix a test name I think the old name here was used for this function in an early draft of the PR that added it. In any case it doesn't refer to anything now. --- test/widgets/message_list_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index bf14b874c0..0e944d97a6 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -916,7 +916,7 @@ void main() { await tester.pumpAndSettle(); // process pending timers }); - testWidgets('markAllMessagesAsRead uses is:unread optimization', (WidgetTester tester) async { + testWidgets('markNarrowAsRead uses is:unread optimization', (WidgetTester tester) async { const narrow = CombinedFeedNarrow(); await setupMessageListPage(tester, narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); From d439e6525a806c3c36e89f2a4d4fe7cf7bc95d0a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jul 2024 15:17:28 -0700 Subject: [PATCH 3/3] actions test: Port most MarkAsReadWidget tests to markNarrowAsRead This sets us up to be able to refactor this function for other use cases that aren't exercised by MarkAsReadWidget, and have good tests for those cases too. There are a couple of pieces of functionality of the widget's _handlePress method that probably should be within the function markNarrowAsRead instead, so that they can be reused together. For the tests for those, we port them too but mark them as skipped, with TODO comments. After refactoring to move that functionality inside, we can enable the tests. The new markNarrowAsRead unit tests do most of the work of tests for MarkAsReadWidget. We leave behind a couple of smoke tests that mostly just show it is indeed using markNarrowAsRead, plus the tests whose ported versions are skipped. --- test/widgets/actions_test.dart | 284 ++++++++++++++++++++++++++++ test/widgets/message_list_test.dart | 177 +---------------- 2 files changed, 291 insertions(+), 170 deletions(-) create mode 100644 test/widgets/actions_test.dart diff --git a/test/widgets/actions_test.dart b/test/widgets/actions_test.dart new file mode 100644 index 0000000000..759cfa4218 --- /dev/null +++ b/test/widgets/actions_test.dart @@ -0,0 +1,284 @@ +import 'dart:convert'; + +import 'package:checks/checks.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_gen/gen_l10n/zulip_localizations.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:zulip/api/model/initial_snapshot.dart'; +import 'package:zulip/api/model/narrow.dart'; +import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/localizations.dart'; +import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/actions.dart'; +import 'package:zulip/widgets/store.dart'; +import 'package:zulip/widgets/theme.dart'; + +import '../api/fake_api.dart'; +import '../example_data.dart' as eg; +import '../model/binding.dart'; +import '../model/unreads_checks.dart'; +import '../stdlib_checks.dart'; +import 'dialog_checks.dart'; + +void main() { + TestZulipBinding.ensureInitialized(); + + group('markNarrowAsRead', () { + late PerAccountStore store; + late FakeApiConnection connection; + late BuildContext context; + + Future prepare(WidgetTester tester, { + UnreadMessagesSnapshot? unreadMsgs, + }) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( + unreadMsgs: unreadMsgs)); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + connection = store.connection as FakeApiConnection; + + await tester.pumpWidget(Builder(builder: (context) => + MaterialApp( + theme: zulipThemeData(context), + localizationsDelegates: ZulipLocalizations.localizationsDelegates, + supportedLocales: ZulipLocalizations.supportedLocales, + home: GlobalStoreWidget( + child: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: const Scaffold( + body: Placeholder())))))); + await tester.pumpAndSettle(); + context = tester.element(find.byType(Placeholder)); + } + + testWidgets('smoke test on modern server', (tester) async { + final narrow = TopicNarrow.ofMessage(eg.streamMessage()); + await prepare(tester); + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 11, updatedCount: 3, + firstProcessedId: null, lastProcessedId: null, + foundOldest: true, foundNewest: true).toJson()); + markNarrowAsRead(context, narrow, false); + await tester.pump(Duration.zero); + final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': 'oldest', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': jsonEncode(apiNarrow), + 'op': 'add', + 'flag': 'read', + }); + }); + + + testWidgets('use is:unread optimization', (WidgetTester tester) async { + const narrow = CombinedFeedNarrow(); + await prepare(tester); + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 11, updatedCount: 3, + firstProcessedId: null, lastProcessedId: null, + foundOldest: true, foundNewest: true).toJson()); + markNarrowAsRead(context, narrow, false); + await tester.pump(Duration.zero); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': 'oldest', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': json.encode([{'operator': 'is', 'operand': 'unread'}]), + 'op': 'add', + 'flag': 'read', + }); + }); + + testWidgets('pagination', (WidgetTester tester) async { + // Check that `lastProcessedId` returned from an initial + // response is used as `anchorId` for the subsequent request. + final narrow = TopicNarrow.ofMessage(eg.streamMessage()); + await prepare(tester); + + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 1000, updatedCount: 890, + firstProcessedId: 1, lastProcessedId: 1989, + foundOldest: true, foundNewest: false).toJson()); + markNarrowAsRead(context, narrow, false); + final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': 'oldest', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': jsonEncode(apiNarrow), + 'op': 'add', + 'flag': 'read', + }); + + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 20, updatedCount: 10, + firstProcessedId: 2000, lastProcessedId: 2023, + foundOldest: false, foundNewest: true).toJson()); + await tester.pumpAndSettle(); + check(find.bySubtype().evaluate()).length.equals(1); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': '1989', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': jsonEncode(apiNarrow), + 'op': 'add', + 'flag': 'read', + }); + }); + + testWidgets('on mark-all-as-read when Unreads.oldUnreadsMissing: true', (tester) async { + const narrow = CombinedFeedNarrow(); + await prepare(tester); + store.unreads.oldUnreadsMissing = true; + + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 11, updatedCount: 3, + firstProcessedId: null, lastProcessedId: null, + foundOldest: true, foundNewest: true).toJson()); + markNarrowAsRead(context, narrow, false); + await tester.pump(Duration.zero); + await tester.pumpAndSettle(); + check(store.unreads.oldUnreadsMissing).isFalse(); + }, skip: true, // TODO move this functionality inside markNarrowAsRead + ); + + testWidgets('on invalid response', (WidgetTester tester) async { + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + final narrow = TopicNarrow.ofMessage(eg.streamMessage()); + await prepare(tester); + connection.prepare(json: UpdateMessageFlagsForNarrowResult( + processedCount: 1000, updatedCount: 0, + firstProcessedId: null, lastProcessedId: null, + foundOldest: true, foundNewest: false).toJson()); + markNarrowAsRead(context, narrow, false); + await tester.pump(Duration.zero); + final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags/narrow') + ..bodyFields.deepEquals({ + 'anchor': 'oldest', + 'include_anchor': 'false', + 'num_before': '0', + 'num_after': '1000', + 'narrow': jsonEncode(apiNarrow), + 'op': 'add', + 'flag': 'read', + }); + + await tester.pumpAndSettle(); + checkErrorDialog(tester, + expectedTitle: zulipLocalizations.errorMarkAsReadFailedTitle, + expectedMessage: zulipLocalizations.errorInvalidResponse); + }); + + testWidgets('CombinedFeedNarrow on legacy server', (WidgetTester tester) async { + const narrow = CombinedFeedNarrow(); + await prepare(tester); + // Might as well test with oldUnreadsMissing: true. + store.unreads.oldUnreadsMissing = true; + + connection.zulipFeatureLevel = 154; + connection.prepare(json: {}); + markNarrowAsRead(context, narrow, true); // TODO move legacy-server check inside markNarrowAsRead + await tester.pump(Duration.zero); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/mark_all_as_read') + ..bodyFields.deepEquals({}); + + // Check that [Unreads.handleAllMessagesReadSuccess] wasn't called; + // in the legacy protocol, that'd be redundant with the mark-read event. + check(store.unreads).oldUnreadsMissing.isTrue(); + }); + + testWidgets('StreamNarrow on legacy server', (WidgetTester tester) async { + final stream = eg.stream(); + final narrow = StreamNarrow(stream.streamId); + await prepare(tester); + connection.zulipFeatureLevel = 154; + connection.prepare(json: {}); + markNarrowAsRead(context, narrow, true); // TODO move legacy-server check inside markNarrowAsRead + await tester.pump(Duration.zero); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/mark_stream_as_read') + ..bodyFields.deepEquals({ + 'stream_id': stream.streamId.toString(), + }); + }); + + testWidgets('TopicNarrow on legacy server', (WidgetTester tester) async { + final narrow = TopicNarrow.ofMessage(eg.streamMessage()); + await prepare(tester); + connection.zulipFeatureLevel = 154; + connection.prepare(json: {}); + markNarrowAsRead(context, narrow, true); // TODO move legacy-server check inside markNarrowAsRead + await tester.pump(Duration.zero); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/mark_topic_as_read') + ..bodyFields.deepEquals({ + 'stream_id': narrow.streamId.toString(), + 'topic_name': narrow.topic, + }); + }); + + testWidgets('DmNarrow on legacy server', (WidgetTester tester) async { + final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + final unreadMsgs = eg.unreadMsgs(dms: [ + UnreadDmSnapshot(otherUserId: eg.otherUser.userId, + unreadMessageIds: [message.id]), + ]); + await prepare(tester, unreadMsgs: unreadMsgs); + connection.zulipFeatureLevel = 154; + connection.prepare(json: + UpdateMessageFlagsResult(messages: [message.id]).toJson()); + markNarrowAsRead(context, narrow, true); // TODO move legacy-server check inside markNarrowAsRead + await tester.pump(Duration.zero); + check(connection.lastRequest).isA() + ..method.equals('POST') + ..url.path.equals('/api/v1/messages/flags') + ..bodyFields.deepEquals({ + 'messages': jsonEncode([message.id]), + 'op': 'add', + 'flag': 'read', + }); + }); + + testWidgets('catch-all api errors', (WidgetTester tester) async { + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + const narrow = CombinedFeedNarrow(); + await prepare(tester); + connection.prepare(exception: http.ClientException('Oops')); + markNarrowAsRead(context, narrow, false); + await tester.pump(Duration.zero); + await tester.pumpAndSettle(); + checkErrorDialog(tester, + expectedTitle: zulipLocalizations.errorMarkAsReadFailedTitle, + expectedMessage: 'NetworkException: Oops (ClientException: Oops)'); + }, skip: true, // TODO move this functionality inside markNarrowAsRead + ); + }); +} diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 0e944d97a6..c804b02af1 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -30,7 +30,6 @@ import '../model/content_test.dart'; import '../model/message_list_test.dart'; import '../model/test_store.dart'; import '../flutter_checks.dart'; -import '../model/unreads_checks.dart'; import '../stdlib_checks.dart'; import '../test_images.dart'; import 'content_checks.dart'; @@ -882,6 +881,10 @@ void main() { }); group('onPressed behavior', () { + // The markNarrowAsRead function has detailed unit tests of its own. + // These tests cover functionality that's outside that function, + // and a couple of smoke tests showing this button is wired up to it. + final message = eg.streamMessage(flags: []); final unreadMsgs = eg.unreadMsgs(streams: [ UnreadStreamSnapshot(streamId: message.streamId, topic: message.topic, @@ -916,34 +919,7 @@ void main() { await tester.pumpAndSettle(); // process pending timers }); - testWidgets('markNarrowAsRead uses is:unread optimization', (WidgetTester tester) async { - const narrow = CombinedFeedNarrow(); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - connection.prepare(json: UpdateMessageFlagsForNarrowResult( - processedCount: 11, updatedCount: 3, - firstProcessedId: null, lastProcessedId: null, - foundOldest: true, foundNewest: true).toJson()); - await tester.tap(find.byType(MarkAsReadWidget)); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/messages/flags/narrow') - ..bodyFields.deepEquals({ - 'anchor': 'oldest', - 'include_anchor': 'false', - 'num_before': '0', - 'num_after': '1000', - 'narrow': json.encode([{'operator': 'is', 'operand': 'unread'}]), - 'op': 'add', - 'flag': 'read', - }); - - await tester.pumpAndSettle(); // process pending timers - }); - - testWidgets('markNarrowAsRead pagination', (WidgetTester tester) async { + testWidgets('pagination', (WidgetTester tester) async { // Check that `lastProcessedId` returned from an initial // response is used as `anchorId` for the subsequent request. final narrow = TopicNarrow.ofMessage(message); @@ -956,19 +932,10 @@ void main() { firstProcessedId: 1, lastProcessedId: 1989, foundOldest: true, foundNewest: false).toJson()); await tester.tap(find.byType(MarkAsReadWidget)); - final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); check(connection.lastRequest).isA() ..method.equals('POST') ..url.path.equals('/api/v1/messages/flags/narrow') - ..bodyFields.deepEquals({ - 'anchor': 'oldest', - 'include_anchor': 'false', - 'num_before': '0', - 'num_after': '1000', - 'narrow': jsonEncode(apiNarrow), - 'op': 'add', - 'flag': 'read', - }); + ..bodyFields['anchor'].equals('oldest'); connection.prepare(json: UpdateMessageFlagsForNarrowResult( processedCount: 20, updatedCount: 10, @@ -979,15 +946,7 @@ void main() { check(connection.lastRequest).isA() ..method.equals('POST') ..url.path.equals('/api/v1/messages/flags/narrow') - ..bodyFields.deepEquals({ - 'anchor': '1989', - 'include_anchor': 'false', - 'num_before': '0', - 'num_after': '1000', - 'narrow': jsonEncode(apiNarrow), - 'op': 'add', - 'flag': 'read', - }); + ..bodyFields['anchor'].equals('1989'); }); testWidgets('markNarrowAsRead on mark-all-as-read when Unreads.oldUnreadsMissing: true', (tester) async { @@ -1006,128 +965,6 @@ void main() { check(store.unreads.oldUnreadsMissing).isFalse(); }); - testWidgets('markNarrowAsRead on invalid response', (WidgetTester tester) async { - final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - final narrow = TopicNarrow.ofMessage(message); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - connection.prepare(json: UpdateMessageFlagsForNarrowResult( - processedCount: 1000, updatedCount: 0, - firstProcessedId: null, lastProcessedId: null, - foundOldest: true, foundNewest: false).toJson()); - await tester.tap(find.byType(MarkAsReadWidget)); - final apiNarrow = narrow.apiEncode()..add(ApiNarrowIsUnread()); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/messages/flags/narrow') - ..bodyFields.deepEquals({ - 'anchor': 'oldest', - 'include_anchor': 'false', - 'num_before': '0', - 'num_after': '1000', - 'narrow': jsonEncode(apiNarrow), - 'op': 'add', - 'flag': 'read', - }); - - await tester.pumpAndSettle(); - checkErrorDialog(tester, - expectedTitle: zulipLocalizations.errorMarkAsReadFailedTitle, - expectedMessage: zulipLocalizations.errorInvalidResponse); - }); - - testWidgets('CombinedFeedNarrow on legacy server', (WidgetTester tester) async { - const narrow = CombinedFeedNarrow(); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - // Might as well test with oldUnreadsMissing: true. - store.unreads.oldUnreadsMissing = true; - - connection.zulipFeatureLevel = 154; - connection.prepare(json: {}); - await tester.tap(find.byType(MarkAsReadWidget)); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/mark_all_as_read') - ..bodyFields.deepEquals({}); - - await tester.pumpAndSettle(); // process pending timers - - // Check that [Unreads.handleAllMessagesReadSuccess] wasn't called; - // in the legacy protocol, that'd be redundant with the mark-read event. - check(store.unreads).oldUnreadsMissing.isTrue(); - }); - - testWidgets('StreamNarrow on legacy server', (WidgetTester tester) async { - final narrow = StreamNarrow(message.streamId); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - connection.zulipFeatureLevel = 154; - connection.prepare(json: {}); - await tester.tap(find.byType(MarkAsReadWidget)); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/mark_stream_as_read') - ..bodyFields.deepEquals({ - 'stream_id': message.streamId.toString(), - }); - - await tester.pumpAndSettle(); // process pending timers - }); - - testWidgets('TopicNarrow on legacy server', (WidgetTester tester) async { - final narrow = TopicNarrow.ofMessage(message); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - connection.zulipFeatureLevel = 154; - connection.prepare(json: {}); - await tester.tap(find.byType(MarkAsReadWidget)); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/mark_topic_as_read') - ..bodyFields.deepEquals({ - 'stream_id': narrow.streamId.toString(), - 'topic_name': narrow.topic, - }); - - await tester.pumpAndSettle(); // process pending timers - }); - - testWidgets('DmNarrow on legacy server', (WidgetTester tester) async { - final message = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); - final narrow = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); - final unreadMsgs = eg.unreadMsgs(dms: [ - UnreadDmSnapshot(otherUserId: eg.otherUser.userId, - unreadMessageIds: [message.id]), - ]); - await setupMessageListPage(tester, - narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); - check(isMarkAsReadButtonVisible(tester)).isTrue(); - - connection.zulipFeatureLevel = 154; - connection.prepare(json: - UpdateMessageFlagsResult(messages: [message.id]).toJson()); - await tester.tap(find.byType(MarkAsReadWidget)); - check(connection.lastRequest).isA() - ..method.equals('POST') - ..url.path.equals('/api/v1/messages/flags') - ..bodyFields.deepEquals({ - 'messages': jsonEncode([message.id]), - 'op': 'add', - 'flag': 'read', - }); - - await tester.pumpAndSettle(); // process pending timers - }); - testWidgets('catch-all api errors', (WidgetTester tester) async { final zulipLocalizations = GlobalLocalizations.zulipLocalizations; const narrow = CombinedFeedNarrow();