Skip to content

compose box: On failed message send, show error dialog #819

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 18, 2024
Merged

compose box: On failed message send, show error dialog #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
495a57d
compose box test [nfc]: Pull prepareComposeBox out to be reused
chrisbobbe Jul 17, 2024
81e2c9d
compose box test: Test that message-send request is made on send-butt…
chrisbobbe Jul 17, 2024
3256d1a
compose box: On failed message send, show error dialog
chrisbobbe Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions lib/widgets/compose_box.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:flutter/services.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import 'package:image_picker/image_picker.dart';

import '../api/exception.dart';
import '../api/model/model.dart';
import '../api/route/messages.dart';
import '../model/compose.dart';
Expand Down Expand Up @@ -716,7 +717,7 @@ class _SendButtonState extends State<_SendButton> {
|| widget.contentController.hasValidationErrors.value;
}

void _send() {
void _send() async {
if (_hasValidationErrors) {
final zulipLocalizations = ZulipLocalizations.of(context);
List<String> validationErrorMessages = [
Expand All @@ -735,9 +736,26 @@ class _SendButtonState extends State<_SendButton> {

final store = PerAccountStoreWidget.of(context);
final content = widget.contentController.textNormalized;
store.sendMessage(destination: widget.getDestination(), content: content);

widget.contentController.clear();

try {
// TODO(#720) clear content input only on success response;
// while waiting, put input(s) and send button into a disabled
// "working on it" state (letting input text be selected for copying).
await store.sendMessage(destination: widget.getDestination(), content: content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes more than the error behavior — it also means we wait to clear the content input until the request completes.

Our promptly clearing the input isn't entirely a good thing; that's what #720 is about. But if we take that out without doing the other work called for in #720, I think the effect is that there's a lack of feedback so that it feels like the send button didn't do anything, or is sluggish.

Trying this, I notice that even on my fast home network connection. Then to demonstrate how it behaves when the request is especially slow, try this patch:

--- lib/api/core.dart
+++ lib/api/core.dart
@@ -188,6 +188,7 @@ class ApiConnection {
     if (params != null) {
       request.bodyFields = encodeParameters(params)!;
     }
+    await Future<void>.delayed(const Duration(seconds: 2));
     return send(routeName, fromJson, request, overrideUserAgent: overrideUserAgent);
   }

The way to fix that looks something like:

final sendFuture = store.sendMessage(…);
widget.contentController.clear();

try {
  await sendFuture;
} on …

Or I guess more straightforwardly, we can just call clear before sendMessage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

} on ApiRequestException catch (e) {
if (!mounted) return;
final zulipLocalizations = ZulipLocalizations.of(context);
final message = switch (e) {
ZulipApiException() => zulipLocalizations.errorServerMessage(e.message),
_ => e.message,
};
showErrorDialog(context: context,
title: zulipLocalizations.errorMessageNotSent,
message: message);
return;
}
}

@override
Expand Down
120 changes: 97 additions & 23 deletions test/widgets/compose_box_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,49 @@
import 'package:checks/checks.dart';
import 'package:http/http.dart' as http;
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import 'package:flutter_test/flutter_test.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/compose_box.dart';
import 'package:zulip/widgets/store.dart';

import '../api/fake_api.dart';
import '../example_data.dart' as eg;
import '../flutter_checks.dart';
import '../model/binding.dart';
import '../stdlib_checks.dart';
import 'dialog_checks.dart';

void main() {
TestZulipBinding.ensureInitialized();

late PerAccountStore store;
late FakeApiConnection connection;

Future<GlobalKey<ComposeBoxController>> prepareComposeBox(WidgetTester tester, Narrow narrow) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());

store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
connection = store.connection as FakeApiConnection;

final controllerKey = GlobalKey<ComposeBoxController>();
await tester.pumpWidget(
MaterialApp(
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
supportedLocales: ZulipLocalizations.supportedLocales,
home: GlobalStoreWidget(
child: PerAccountStoreWidget(
accountId: eg.selfAccount.id,
child: ComposeBox(controllerKey: controllerKey, narrow: narrow)))));
await tester.pumpAndSettle();

return controllerKey;
}

group('ComposeContentController', () {
group('insertPadded', () {
// Like `parseMarkedText` in test/model/autocomplete_test.dart,
Expand Down Expand Up @@ -116,25 +147,10 @@ void main() {
});

group('ComposeBox textCapitalization', () {
late GlobalKey<ComposeBoxController> controllerKey;

Future<void> prepareComposeBox(WidgetTester tester, Narrow narrow) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());

controllerKey = GlobalKey();
await tester.pumpWidget(
MaterialApp(
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
supportedLocales: ZulipLocalizations.supportedLocales,
home: GlobalStoreWidget(
child: PerAccountStoreWidget(
accountId: eg.selfAccount.id,
child: ComposeBox(controllerKey: controllerKey, narrow: narrow)))));
await tester.pumpAndSettle();
}

void checkComposeBoxTextFields(WidgetTester tester, {required bool expectTopicTextField}) {
void checkComposeBoxTextFields(WidgetTester tester, {
required GlobalKey<ComposeBoxController> controllerKey,
required bool expectTopicTextField,
}) {
final composeBoxController = controllerKey.currentState!;

final topicTextField = tester.widgetList<TextField>(find.byWidgetPredicate(
Expand All @@ -155,13 +171,71 @@ void main() {
}

testWidgets('_StreamComposeBox', (tester) async {
await prepareComposeBox(tester, StreamNarrow(eg.stream().streamId));
checkComposeBoxTextFields(tester, expectTopicTextField: true);
final key = await prepareComposeBox(tester,
StreamNarrow(eg.stream().streamId));
checkComposeBoxTextFields(tester, controllerKey: key,
expectTopicTextField: true);
});

testWidgets('_FixedDestinationComposeBox', (tester) async {
await prepareComposeBox(tester, TopicNarrow.ofMessage(eg.streamMessage()));
checkComposeBoxTextFields(tester, expectTopicTextField: false);
final key = await prepareComposeBox(tester,
TopicNarrow.ofMessage(eg.streamMessage()));
checkComposeBoxTextFields(tester, controllerKey: key,
expectTopicTextField: false);
});
});

group('message-send request response', () {
Future<void> setupAndTapSend(WidgetTester tester, {
required void Function(int messageId) prepareResponse,
}) async {
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
await prepareComposeBox(tester, const TopicNarrow(123, 'some topic'));

final contentInputFinder = find.byWidgetPredicate(
(widget) => widget is TextField && widget.controller is ComposeContentController);
await tester.enterText(contentInputFinder, 'hello world');

prepareResponse(456);
await tester.tap(find.byTooltip(zulipLocalizations.composeBoxSendTooltip));
await tester.pump(Duration.zero);

check(connection.lastRequest).isA<http.Request>()
..method.equals('POST')
..url.path.equals('/api/v1/messages')
..bodyFields.deepEquals({
'type': 'stream',
'to': '123',
'topic': 'some topic',
'content': 'hello world',
'read_by_sender': 'true',
});
}

testWidgets('success', (tester) async {
await setupAndTapSend(tester, prepareResponse: (int messageId) {
connection.prepare(json: SendMessageResult(id: messageId).toJson());
});
final errorDialogs = tester.widgetList(find.byType(AlertDialog));
check(errorDialogs).isEmpty();
});

testWidgets('ZulipApiException', (tester) async {
await setupAndTapSend(tester, prepareResponse: (message) {
connection.prepare(
httpStatus: 400,
json: {
'result': 'error',
'code': 'BAD_REQUEST',
'msg': 'You do not have permission to initiate direct message conversations.',
});
});
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
await tester.tap(find.byWidget(checkErrorDialog(tester,
expectedTitle: zulipLocalizations.errorMessageNotSent,
expectedMessage: zulipLocalizations.errorServerMessage(
'You do not have permission to initiate direct message conversations.'),
)));
});
});
}