From 76f8a16bf9a94cd41c377dcf69aec603f513d8fa Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 4 Dec 2024 16:09:27 -0800 Subject: [PATCH] action_sheet: Implement resolve/unresolve in topic action sheet Fixes: #744 --- assets/l10n/app_en.arb | 16 +++ lib/api/model/model.dart | 6 + lib/generated/l10n/zulip_localizations.dart | 24 ++++ .../l10n/zulip_localizations_ar.dart | 12 ++ .../l10n/zulip_localizations_en.dart | 12 ++ .../l10n/zulip_localizations_ja.dart | 12 ++ .../l10n/zulip_localizations_nb.dart | 12 ++ .../l10n/zulip_localizations_pl.dart | 12 ++ .../l10n/zulip_localizations_ru.dart | 12 ++ .../l10n/zulip_localizations_sk.dart | 12 ++ lib/widgets/action_sheet.dart | 84 ++++++++++++ lib/widgets/inbox.dart | 7 +- lib/widgets/message_list.dart | 20 ++- test/widgets/action_sheet_test.dart | 128 +++++++++++++++++- 14 files changed, 359 insertions(+), 10 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index ee7e96c35f..ba02a819b1 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -92,6 +92,22 @@ "@actionSheetOptionUnfollowTopic": { "description": "Label for unfollowing a topic on action sheet." }, + "actionSheetOptionResolveTopic": "Mark as resolved", + "@actionSheetOptionResolveTopic": { + "description": "Label for the 'Mark as resolved' button on the topic action sheet." + }, + "actionSheetOptionUnresolveTopic": "Mark as unresolved", + "@actionSheetOptionUnresolveTopic": { + "description": "Label for the 'Mark as unresolved' button on the topic action sheet." + }, + "errorResolveTopicFailedTitle": "Failed to mark topic as resolved", + "@errorResolveTopicFailedTitle": { + "description": "Error title when marking a topic as resolved failed." + }, + "errorUnresolveTopicFailedTitle": "Failed to mark topic as unresolved", + "@errorUnresolveTopicFailedTitle": { + "description": "Error title when marking a topic as unresolved failed." + }, "actionSheetOptionCopyMessageText": "Copy message text", "@actionSheetOptionCopyMessageText": { "description": "Label for copy message text button on action sheet." diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 03af104baf..fad8ddc5bc 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -695,6 +695,12 @@ extension type const TopicName(String _value) { /// The key to use for "same topic as" comparisons. String canonicalize() => apiName.toLowerCase(); + /// Whether the topic starts with [resolvedTopicPrefix]. + bool get isResolved => _value.startsWith(resolvedTopicPrefix); + + /// This [TopicName] plus the [resolvedTopicPrefix] prefix. + TopicName resolve() => TopicName(resolvedTopicPrefix + _value); + /// A [TopicName] with [resolvedTopicPrefixRegexp] stripped if present. TopicName unresolve() => TopicName(_value.replaceFirst(resolvedTopicPrefixRegexp, '')); diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index b6fbb70769..9be039ef72 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -243,6 +243,30 @@ abstract class ZulipLocalizations { /// **'Unfollow topic'** String get actionSheetOptionUnfollowTopic; + /// Label for the 'Mark as resolved' button on the topic action sheet. + /// + /// In en, this message translates to: + /// **'Mark as resolved'** + String get actionSheetOptionResolveTopic; + + /// Label for the 'Mark as unresolved' button on the topic action sheet. + /// + /// In en, this message translates to: + /// **'Mark as unresolved'** + String get actionSheetOptionUnresolveTopic; + + /// Error title when marking a topic as resolved failed. + /// + /// In en, this message translates to: + /// **'Failed to mark topic as resolved'** + String get errorResolveTopicFailedTitle; + + /// Error title when marking a topic as unresolved failed. + /// + /// In en, this message translates to: + /// **'Failed to mark topic as unresolved'** + String get errorUnresolveTopicFailedTitle; + /// Label for copy message text button on action sheet. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 025b4b1444..2e59dae8af 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Unfollow topic'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Copy message text'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index 9467d33428..a0e86b7d35 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Unfollow topic'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Copy message text'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index f363ee0043..6e7f1559ac 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Unfollow topic'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Copy message text'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index 35b3e86fe5..cabd4897e9 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Unfollow topic'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Copy message text'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index 0594722d31..0a2a2bf1d7 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Nie śledź wątku'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Skopiuj tekst wiadomości'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index 879559fed4..99bd72c62f 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Не отслеживать тему'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Скопировать текст сообщения'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index af87dfd949..13646eafd5 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -79,6 +79,18 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get actionSheetOptionUnfollowTopic => 'Prestať sledovať tému'; + @override + String get actionSheetOptionResolveTopic => 'Mark as resolved'; + + @override + String get actionSheetOptionUnresolveTopic => 'Mark as unresolved'; + + @override + String get errorResolveTopicFailedTitle => 'Failed to mark topic as resolved'; + + @override + String get errorUnresolveTopicFailedTitle => 'Failed to mark topic as unresolved'; + @override String get actionSheetOptionCopyMessageText => 'Skopírovať text správy'; diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index a689d86550..7c3ea6e622 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -166,9 +166,13 @@ class ActionSheetCancelButton extends StatelessWidget { /// Show a sheet of actions you can take on a topic. /// /// Needs a [PageRoot] ancestor. +/// +/// The API request for resolving/unresolving a topic needs a message ID. +/// If [someMessageIdInTopic] is null, the button for that will be absent. void showTopicActionSheet(BuildContext context, { required int channelId, required TopicName topic, + required int? someMessageIdInTopic, }) { final pageContext = PageRoot.contextOf(context); @@ -245,6 +249,12 @@ void showTopicActionSheet(BuildContext context, { pageContext: pageContext); })); + if (someMessageIdInTopic != null) { + optionButtons.add(ResolveUnresolveButton(pageContext: pageContext, + topic: topic, + someMessageIdInTopic: someMessageIdInTopic)); + } + if (optionButtons.isEmpty) { // TODO(a11y): This case makes a no-op gesture handler; as a consequence, // we're presenting some UI (to people who use screen-reader software) as @@ -377,6 +387,80 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton { } } +class ResolveUnresolveButton extends ActionSheetMenuItemButton { + ResolveUnresolveButton({ + super.key, + required this.topic, + required this.someMessageIdInTopic, + required super.pageContext, + }) : _actionIsResolve = !topic.isResolved; + + /// The topic that the action sheet was opened for. + /// + /// There might not currently be any messages with this topic; + /// see dartdoc of [ActionSheetMenuItemButton]. + final TopicName topic; + + /// The message ID that was passed when opening the action sheet. + /// + /// The message with this ID might currently not exist, + /// or might exist with a different topic; + /// see dartdoc of [ActionSheetMenuItemButton]. + final int someMessageIdInTopic; + + final bool _actionIsResolve; + + @override + IconData get icon => _actionIsResolve ? ZulipIcons.check : ZulipIcons.check_remove; + + @override + String label(ZulipLocalizations zulipLocalizations) { + return _actionIsResolve + ? zulipLocalizations.actionSheetOptionResolveTopic + : zulipLocalizations.actionSheetOptionUnresolveTopic; + } + + @override void onPressed() async { + final zulipLocalizations = ZulipLocalizations.of(pageContext); + final store = PerAccountStoreWidget.of(pageContext); + + // We *could* check here if the topic has changed since the action sheet was + // opened (see dartdoc of [ActionSheetMenuItemButton]) and abort if so. + // We simplify by not doing so. + // There's already an inherent race that that check wouldn't help with: + // when you tap the button, an intervening topic change may already have + // happened, just not reached us in an event yet. + // Discussion, including about what web does: + // https://github.com/zulip/zulip-flutter/pull/1301#discussion_r1936181560 + + try { + await updateMessage(store.connection, + messageId: someMessageIdInTopic, + topic: _actionIsResolve ? topic.resolve() : topic.unresolve(), + propagateMode: PropagateMode.changeAll, + sendNotificationToOldThread: false, + sendNotificationToNewThread: true, + ); + } catch (e) { + if (!pageContext.mounted) return; + + String? errorMessage; + switch (e) { + case ZulipApiException(): + errorMessage = e.message; + // TODO(#741) specific messages for common errors, like network errors + // (support with reusable code) + default: + } + + final title = _actionIsResolve + ? zulipLocalizations.errorResolveTopicFailedTitle + : zulipLocalizations.errorUnresolveTopicFailedTitle; + showErrorDialog(context: pageContext, title: title, message: errorMessage); + } + } +} + /// Show a sheet of actions you can take on a message in the message list. /// /// Must have a [MessageListPage] ancestor. diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 6dbe31ce04..799f763f1c 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -507,7 +507,8 @@ class _TopicItem extends StatelessWidget { @override Widget build(BuildContext context) { - final _StreamSectionTopicData(:topic, :count, :hasMention) = data; + final _StreamSectionTopicData( + :topic, :count, :hasMention, :lastUnreadId) = data; final store = PerAccountStoreWidget.of(context); final subscription = store.subscriptions[streamId]!; @@ -525,7 +526,9 @@ class _TopicItem extends StatelessWidget { MessageListPage.buildRoute(context: context, narrow: narrow)); }, onLongPress: () => showTopicActionSheet(context, - channelId: streamId, topic: topic), + channelId: streamId, + topic: topic, + someMessageIdInTopic: lastUnreadId), child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 34), child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ const SizedBox(width: 63), diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c28116ee15..9f21a19fb0 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -404,8 +404,20 @@ class MessageListAppBarTitle extends StatelessWidget { width: double.infinity, child: GestureDetector( behavior: HitTestBehavior.translucent, - onLongPress: () => showTopicActionSheet(context, - channelId: streamId, topic: topic), + onLongPress: () { + final someMessage = MessageListPage.ancestorOf(context) + .model?.messages.firstOrNull; + // If someMessage is null, the topic action sheet won't have a + // resolve/unresolve button. That seems OK; in that case we're + // either still fetching messages (and the user can reopen the + // sheet after that finishes) or there aren't any messages to + // act on anyway. + assert(someMessage == null || narrow.containsMessage(someMessage)); + showTopicActionSheet(context, + channelId: streamId, + topic: topic, + someMessageIdInTopic: someMessage?.id); + }, child: Column( crossAxisAlignment: willCenterTitle ? CrossAxisAlignment.center : CrossAxisAlignment.start, @@ -1127,7 +1139,9 @@ class StreamMessageRecipientHeader extends StatelessWidget { MessageListPage.buildRoute(context: context, narrow: TopicNarrow.ofMessage(message))), onLongPress: () => showTopicActionSheet(context, - channelId: message.streamId, topic: topic), + channelId: message.streamId, + topic: topic, + someMessageIdInTopic: message.id), child: ColoredBox( color: backgroundColor, child: Row( diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 2bb07b4946..7da94cfd36 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -227,9 +227,10 @@ void main() { } checkButton('Follow topic'); + checkButton('Mark as resolved'); } - testWidgets('show from inbox', (tester) async { + testWidgets('show from inbox; message in Unreads but not in MessageStore', (tester) async { await prepare(unreadMsgs: eg.unreadMsgs(count: 1, channels: [eg.unreadChannelMsgs( streamId: someChannel.streamId, @@ -237,6 +238,17 @@ void main() { unreadMessageIds: [someMessage.id], )])); await showFromInbox(tester); + check(store.unreads.isUnread(someMessage.id)).isNotNull().isTrue(); + check(store.messages).not((it) => it.containsKey(someMessage.id)); + checkButtons(); + }); + + testWidgets('show from inbox; message in Unreads and in MessageStore', (tester) async { + await prepare(); + await store.addMessage(someMessage); + await showFromInbox(tester); + check(store.unreads.isUnread(someMessage.id)).isNotNull().isTrue(); + check(store.messages)[someMessage.id].isNotNull(); checkButtons(); }); @@ -246,6 +258,13 @@ void main() { checkButtons(); }); + testWidgets('show from app bar: resolve/unresolve not offered when msglist empty', (tester) async { + await prepare(); + await showFromAppBar(tester, messages: []); + check(findButtonForLabel('Mark as resolved')).findsNothing(); + check(findButtonForLabel('Mark as unresolved')).findsNothing(); + }); + testWidgets('show from recipient header', (tester) async { await prepare(); await showFromRecipientHeader(tester); @@ -289,10 +308,6 @@ void main() { } void checkButtons(List expectedButtonFinders) { - if (expectedButtonFinders.isEmpty) { - check(actionSheetFinder).findsNothing(); - return; - } check(actionSheetFinder).findsOne(); for (final buttonFinder in expectedButtonFinders) { @@ -450,6 +465,109 @@ void main() { } }); }); + + group('ResolveUnresolveButton', () { + void checkRequest(int messageId, String topic) { + check(connection.takeRequests()).single.isA() + ..method.equals('PATCH') + ..url.path.equals('/api/v1/messages/$messageId') + ..bodyFields.deepEquals({ + 'topic': topic, + 'propagate_mode': 'change_all', + 'send_notification_to_old_thread': 'false', + 'send_notification_to_new_thread': 'true', + }); + } + + testWidgets('resolve: happy path from inbox; message in Unreads but not MessageStore', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: 'zulip'); + await prepare( + topic: 'zulip', + unreadMsgs: eg.unreadMsgs(count: 1, + channels: [eg.unreadChannelMsgs( + streamId: someChannel.streamId, + topic: 'zulip', + unreadMessageIds: [message.id], + )])); + await showFromInbox(tester, topic: 'zulip'); + check(store.messages).not((it) => it.containsKey(message.id)); + connection.prepare(json: UpdateMessageResult().toJson()); + await tester.tap(findButtonForLabel('Mark as resolved')); + await tester.pumpAndSettle(); + + checkNoErrorDialog(tester); + checkRequest(message.id, '✔ zulip'); + }); + + testWidgets('resolve: happy path from inbox; message in Unreads and MessageStore', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: 'zulip'); + await prepare(topic: 'zulip'); + await store.addMessage(message); + await showFromInbox(tester, topic: 'zulip'); + check(store.unreads.isUnread(message.id)).isNotNull().isTrue(); + check(store.messages)[message.id].isNotNull(); + connection.prepare(json: UpdateMessageResult().toJson()); + await tester.tap(findButtonForLabel('Mark as resolved')); + await tester.pumpAndSettle(); + + checkNoErrorDialog(tester); + checkRequest(message.id, '✔ zulip'); + }); + + testWidgets('unresolve: happy path', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: '✔ zulip'); + await prepare(topic: '✔ zulip'); + await showFromAppBar(tester, topic: '✔ zulip', messages: [message]); + connection.takeRequests(); + connection.prepare(json: UpdateMessageResult().toJson()); + await tester.tap(findButtonForLabel('Mark as unresolved')); + await tester.pumpAndSettle(); + + checkNoErrorDialog(tester); + checkRequest(message.id, 'zulip'); + }); + + testWidgets('unresolve: weird prefix', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: '✔ ✔ zulip'); + await prepare(topic: '✔ ✔ zulip'); + await showFromAppBar(tester, topic: '✔ ✔ zulip', messages: [message]); + connection.takeRequests(); + connection.prepare(json: UpdateMessageResult().toJson()); + await tester.tap(findButtonForLabel('Mark as unresolved')); + await tester.pumpAndSettle(); + + checkNoErrorDialog(tester); + checkRequest(message.id, 'zulip'); + }); + + testWidgets('resolve: request fails', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: 'zulip'); + await prepare(topic: 'zulip'); + await showFromRecipientHeader(tester, message: message); + connection.takeRequests(); + connection.prepare(exception: http.ClientException('Oops')); + await tester.tap(findButtonForLabel('Mark as resolved')); + await tester.pumpAndSettle(); + checkRequest(message.id, '✔ zulip'); + + checkErrorDialog(tester, + expectedTitle: 'Failed to mark topic as resolved'); + }); + + testWidgets('unresolve: request fails', (tester) async { + final message = eg.streamMessage(stream: someChannel, topic: '✔ zulip'); + await prepare(topic: '✔ zulip'); + await showFromRecipientHeader(tester, message: message); + connection.takeRequests(); + connection.prepare(exception: http.ClientException('Oops')); + await tester.tap(findButtonForLabel('Mark as unresolved')); + await tester.pumpAndSettle(); + checkRequest(message.id, 'zulip'); + + checkErrorDialog(tester, + expectedTitle: 'Failed to mark topic as unresolved'); + }); + }); }); group('message action sheet', () {