From 4a1040ae296b2a10ff4bd062acbbab8737c3ff5b Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 21 Aug 2024 16:40:15 +0100 Subject: [PATCH 01/11] Error page widget --- app/lib/common/toolkit/errors/error_page.dart | 102 ++++++++++++++++++ .../space/pages/space_details_page.dart | 19 +++- app/lib/l10n/app_en.arb | 4 + .../shell_routers/home_shell_router.dart | 4 +- app/pubspec.lock | 8 ++ app/pubspec.yaml | 1 + 6 files changed, 132 insertions(+), 6 deletions(-) create mode 100644 app/lib/common/toolkit/errors/error_page.dart diff --git a/app/lib/common/toolkit/errors/error_page.dart b/app/lib/common/toolkit/errors/error_page.dart new file mode 100644 index 000000000000..2132296428f8 --- /dev/null +++ b/app/lib/common/toolkit/errors/error_page.dart @@ -0,0 +1,102 @@ +import 'package:flutter/material.dart'; +import 'package:quickalert/models/quickalert_options.dart'; +import 'package:quickalert/quickalert.dart'; +import 'package:quickalert/widgets/quickalert_container.dart'; +import 'package:flutter_gen/gen_l10n/l10n.dart'; + +enum ErrorCode { + notFound, + other, +} + +ErrorCode _guessError(Object error) { + final errorStr = error.toString(); + // yay, string-based error guessing! + if (errorStr.contains('not found')) { + return ErrorCode.notFound; + } + return ErrorCode.other; +} + +/// ErrorPage shows a full-screen error to the user (covering other internal errors) +/// +class ErrorPage extends StatelessWidget { + /// Put this widget in the Background of the screen to give context + final Widget background; + final Object error; + final StackTrace stack; + final VoidCallback? onRetryTap; + + final String? title; + final String? text; + final String Function(Object error)? textBuilder; + + /// Dialog Border Radius + final double borderRadius; + + const ErrorPage({ + super.key, + required this.background, + required this.error, + required this.stack, + this.title, + this.text, + this.textBuilder, + this.onRetryTap, + this.borderRadius = 15.0, + }); + + @override + Widget build(BuildContext context) { + return Stack( + children: [ + background, + AlertDialog( + contentPadding: EdgeInsets.zero, + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(borderRadius), + ), + content: errorDialog(context), + ), + ], + ); + } + + Widget errorDialog(BuildContext context) { + final theme = Theme.of(context); + final lang = L10n.of(context); + final err = _guessError(error); + QuickAlertOptions options = QuickAlertOptions( + title: title ?? + switch (err) { + ErrorCode.notFound => lang.notFound, + _ => lang.fatalError, + }, + text: text ?? (textBuilder != null ? textBuilder!(error) : null), + type: switch (err) { + ErrorCode.notFound => QuickAlertType.warning, + _ => QuickAlertType.error, + }, + showCancelBtn: true, + showConfirmBtn: false, + cancelBtnText: lang.back, + borderRadius: borderRadius, + ); + if (onRetryTap != null) { + options.showConfirmBtn = true; + options.confirmBtnColor = theme.primaryColor; + options.confirmBtnText = lang.retry; + options.onConfirmBtnTap = () { + onRetryTap!(); + }; + } + + return _ActerErrorAlert( + options: options, + ); + } +} + +class _ActerErrorAlert extends QuickAlertContainer { + const _ActerErrorAlert({required super.options}); +} diff --git a/app/lib/features/space/pages/space_details_page.dart b/app/lib/features/space/pages/space_details_page.dart index e48c68c647ac..bc6e3b7faa3a 100644 --- a/app/lib/features/space/pages/space_details_page.dart +++ b/app/lib/features/space/pages/space_details_page.dart @@ -1,5 +1,6 @@ import 'package:acter/common/providers/room_providers.dart'; import 'package:acter/common/providers/space_providers.dart'; +import 'package:acter/common/toolkit/errors/error_page.dart'; import 'package:acter/common/widgets/scrollable_list_tab_scroller.dart'; import 'package:acter/features/space/dialogs/suggested_rooms.dart'; import 'package:acter/features/space/providers/space_navbar_provider.dart'; @@ -149,14 +150,24 @@ class _SpaceDetailsPageState extends ConsumerState { }, ); }, - error: (e, s) { - _log.severe('Failed to load tabs in space', e, s); - return Text(L10n.of(context).loadingFailed(e)); - }, + error: (e, s) => loadingError(e, s), loading: () => const SpaceDetailsSkeletons(), ); } + Widget loadingError(Object error, StackTrace stack) { + _log.severe('Failed to load tabs in space', error, stack); + return ErrorPage( + background: const SpaceDetailsSkeletons(), + error: error, + stack: stack, + textBuilder: L10n.of(context).loadingFailed, + onRetryTap: () { + ref.invalidate(spaceProvider(widget.spaceId)); + }, + ); + } + Widget spaceHeaderUI(Widget menuBarWidget) { final displayName = ref.watch(roomDisplayNameProvider(widget.spaceId)).valueOrNull; diff --git a/app/lib/l10n/app_en.arb b/app/lib/l10n/app_en.arb index 4aebcd935bf2..3a00e1a78db3 100644 --- a/app/lib/l10n/app_en.arb +++ b/app/lib/l10n/app_en.arb @@ -91,6 +91,8 @@ "@awaitingConfirmation": {}, "awaitingConfirmationDescription": "These email addresses have not yet been confirmed. Please go to your inbox and check for the confirmation link.", "@awaitingConfirmationDescription": {}, + "back": "Back", + "@back": {}, "block": "Block", "@block": {}, "blockedUsers": "Blocked Users", @@ -761,6 +763,8 @@ "@noUsersFoundWithSpecifiedSearchTerm": {}, "notEnoughPowerLevelForInvites": "Not enough permission level for invites, ask administrator to change it", "@notEnoughPowerLevelForInvites": {}, + "notFound" : "404 - Not Found", + "@notFound": {}, "notes": "Notes", "@notes": {}, "notGoing": "Not Going", diff --git a/app/lib/router/shell_routers/home_shell_router.dart b/app/lib/router/shell_routers/home_shell_router.dart index 352dce09ef5e..d85b48c9453c 100644 --- a/app/lib/router/shell_routers/home_shell_router.dart +++ b/app/lib/router/shell_routers/home_shell_router.dart @@ -309,8 +309,8 @@ final homeShellRoutes = [ pageBuilder: (context, state) { return NoTransitionPage( key: state.pageKey, - child: SpaceDetailsPage( - spaceId: state.pathParameters['spaceId']!, + child: const SpaceDetailsPage( + spaceId: '!asdf', //state.pathParameters['spaceId']!, ), ); }, diff --git a/app/pubspec.lock b/app/pubspec.lock index 62c90e82a5fa..d35c907bd0f7 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -1837,6 +1837,14 @@ packages: url: "https://pub.dev" source: hosted version: "0.0.7+10" + quickalert: + dependency: "direct main" + description: + name: quickalert + sha256: b5d62b1e20b08cc0ff5f40b6da519bdc7a5de6082f13d90572cf4e72eea56c5e + url: "https://pub.dev" + source: hosted + version: "1.1.0" quiver: dependency: transitive description: diff --git a/app/pubspec.yaml b/app/pubspec.yaml index e8d78f63f483..929967471e26 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -143,6 +143,7 @@ dependencies: open_filex: ^4.5.0 phosphor_flutter: ^2.1.0 device_calendar: ^4.3.2 + quickalert: ^1.1.0 dev_dependencies: flutter_test: From efa1982bc482708e684ac12744893ae455ad0145 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 21 Aug 2024 17:25:40 +0100 Subject: [PATCH 02/11] Move openBugReport into feature --- app/lib/config/app_shell.dart | 29 +------------- .../bug_report/actions/open_bug_report.dart | 38 +++++++++++++++++++ .../features/home/widgets/sidebar_widget.dart | 2 +- .../search/widgets/quick_actions_builder.dart | 2 +- 4 files changed, 42 insertions(+), 29 deletions(-) create mode 100644 app/lib/features/bug_report/actions/open_bug_report.dart diff --git a/app/lib/config/app_shell.dart b/app/lib/config/app_shell.dart index 43c27a9283b3..470458307675 100644 --- a/app/lib/config/app_shell.dart +++ b/app/lib/config/app_shell.dart @@ -6,6 +6,7 @@ import 'package:acter/common/utils/device.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/utils/utils.dart'; import 'package:acter/features/auth/pages/logged_out_screen.dart'; +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; import 'package:acter/features/calendar_sync/calendar_sync.dart'; import 'package:acter/features/cross_signing/widgets/cross_signing.dart'; import 'package:acter/features/home/providers/client_providers.dart'; @@ -13,7 +14,6 @@ import 'package:acter/features/home/providers/navigation.dart'; import 'package:acter/features/home/widgets/sidebar_widget.dart'; import 'package:acter/features/settings/providers/settings_providers.dart'; import 'package:acter_flutter_sdk/acter_flutter_sdk.dart'; -import 'package:dart_date/dart_date.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_adaptive_scaffold/flutter_adaptive_scaffold.dart'; @@ -27,32 +27,7 @@ import 'package:shake_detector/shake_detector.dart'; final _log = Logger('a3::home::home_shell'); -ScreenshotController screenshotController = ScreenshotController(); -bool bugReportOpen = false; - -Future openBugReport(BuildContext context) async { - if (bugReportOpen) { - return; - } - final cacheDir = await appCacheDir(); - // rage shake disallows dot in filename - int timestamp = DateTime.now().timestamp; - final imagePath = await screenshotController.captureAndSave( - cacheDir, - fileName: 'screenshot_$timestamp.png', - ); - if (context.mounted) { - bugReportOpen = true; - await context.pushNamed( - Routes.bugReport.name, - queryParameters: imagePath != null ? {'screenshot': imagePath} : {}, - ); - bugReportOpen = false; - } else { - // ignore: avoid_print - print('not mounted :('); - } -} +final ScreenshotController screenshotController = ScreenshotController(); class AppShell extends ConsumerStatefulWidget { final StatefulNavigationShell navigationShell; diff --git a/app/lib/features/bug_report/actions/open_bug_report.dart b/app/lib/features/bug_report/actions/open_bug_report.dart new file mode 100644 index 000000000000..5d574cf30768 --- /dev/null +++ b/app/lib/features/bug_report/actions/open_bug_report.dart @@ -0,0 +1,38 @@ +import 'package:acter/config/app_shell.dart'; +import 'package:acter/common/utils/routes.dart'; +import 'package:acter_flutter_sdk/acter_flutter_sdk.dart'; +import 'package:dart_date/dart_date.dart'; +import 'package:flutter/material.dart'; +import 'package:go_router/go_router.dart'; +import 'package:logging/logging.dart'; + +final _log = Logger('a3::bug_report::open_bug_report'); + +bool _bugReportOpen = false; + +Future openBugReport(BuildContext context) async { + if (_bugReportOpen) { + return; + } + final cacheDir = await appCacheDir(); + // rage shake disallows dot in filename + final timestamp = DateTime.now().timestamp; + final Map queryParams = {}; + final imagePath = await screenshotController.captureAndSave( + cacheDir, + fileName: 'screenshot_$timestamp.png', + ); + if (imagePath != null) { + queryParams['screenshot'] = imagePath; + } + if (!context.mounted) { + _log.warning('Trying to open the bugreport without being mounted'); + return; + } + _bugReportOpen = true; + await context.pushNamed( + Routes.bugReport.name, + queryParameters: queryParams, + ); + _bugReportOpen = false; +} diff --git a/app/lib/features/home/widgets/sidebar_widget.dart b/app/lib/features/home/widgets/sidebar_widget.dart index 515c4d04f948..8d01babcc208 100644 --- a/app/lib/features/home/widgets/sidebar_widget.dart +++ b/app/lib/features/home/widgets/sidebar_widget.dart @@ -4,8 +4,8 @@ import 'package:acter/common/tutorial_dialogs/bottom_navigation_tutorials/bottom import 'package:acter/common/utils/constants.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/widgets/user_avatar.dart'; +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; import 'package:acter/features/home/data/keys.dart'; -import 'package:acter/config/app_shell.dart'; import 'package:acter/features/home/widgets/activities_icon.dart'; import 'package:acter/features/home/widgets/chats_icon.dart'; import 'package:acter/router/providers/router_providers.dart'; diff --git a/app/lib/features/search/widgets/quick_actions_builder.dart b/app/lib/features/search/widgets/quick_actions_builder.dart index 3247e7f334ca..a75b27f16951 100644 --- a/app/lib/features/search/widgets/quick_actions_builder.dart +++ b/app/lib/features/search/widgets/quick_actions_builder.dart @@ -2,7 +2,7 @@ import 'package:acter/common/providers/space_providers.dart'; import 'package:acter/common/themes/colors/color_scheme.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/utils/utils.dart'; -import 'package:acter/config/app_shell.dart'; +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; import 'package:acter/features/search/model/keys.dart'; import 'package:acter/features/settings/providers/settings_providers.dart'; import 'package:acter/features/spaces/model/keys.dart'; From 420077e2c44d6b0178e9af76f4ff0cc6a2bf4131 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Wed, 21 Aug 2024 17:39:22 +0100 Subject: [PATCH 03/11] Link bug report button to error-page --- app/lib/common/toolkit/errors/error_page.dart | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/app/lib/common/toolkit/errors/error_page.dart b/app/lib/common/toolkit/errors/error_page.dart index 2132296428f8..65b898d68830 100644 --- a/app/lib/common/toolkit/errors/error_page.dart +++ b/app/lib/common/toolkit/errors/error_page.dart @@ -1,6 +1,8 @@ +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; import 'package:flutter/material.dart'; import 'package:quickalert/models/quickalert_options.dart'; import 'package:quickalert/quickalert.dart'; +import 'package:quickalert/widgets/quickalert_buttons.dart'; import 'package:quickalert/widgets/quickalert_container.dart'; import 'package:flutter_gen/gen_l10n/l10n.dart'; @@ -30,6 +32,7 @@ class ErrorPage extends StatelessWidget { final String? title; final String? text; final String Function(Object error)? textBuilder; + final bool includeBugReportButton; /// Dialog Border Radius final double borderRadius; @@ -44,6 +47,7 @@ class ErrorPage extends StatelessWidget { this.textBuilder, this.onRetryTap, this.borderRadius = 15.0, + this.includeBugReportButton = true, }); @override @@ -93,10 +97,45 @@ class ErrorPage extends StatelessWidget { return _ActerErrorAlert( options: options, + includeBugReportButton: includeBugReportButton, ); } } class _ActerErrorAlert extends QuickAlertContainer { - const _ActerErrorAlert({required super.options}); + final bool includeBugReportButton; + const _ActerErrorAlert({ + required super.options, + this.includeBugReportButton = true, + }); + + @override + Widget buildButtons() { + return _ActerErrorActionButtons(options: options); + } + + @override + Widget buildHeader(context) { + final orginalHeader = super.buildHeader(context); + if (!includeBugReportButton) { + return orginalHeader; + } + return Stack( + children: [ + orginalHeader, + Positioned( + right: 10, + top: 10, + child: TextButton( + child: Text(L10n.of(context).reportBug), + onPressed: () => openBugReport(context), + ), + ), + ], + ); + } +} + +class _ActerErrorActionButtons extends QuickAlertButtons { + const _ActerErrorActionButtons({required super.options}); } From 882bd5257a8b8783671d12aac32d0f8ab892d976 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 22 Aug 2024 11:09:46 +0100 Subject: [PATCH 04/11] Further bundle bug reporter, make it disable-able --- app/.env | 2 +- .../common/providers/common_providers.dart | 3 - app/lib/config/app_shell.dart | 3 +- .../bug_report/actions/submit_bug_report.dart | 90 ++++++++++++++++++ .../bug_report/pages/bug_report_page.dart | 95 +------------------ .../providers/bug_report_providers.dart | 7 ++ .../features/home/widgets/sidebar_widget.dart | 7 +- .../search/widgets/quick_actions_builder.dart | 40 ++++---- 8 files changed, 130 insertions(+), 117 deletions(-) create mode 100644 app/lib/features/bug_report/actions/submit_bug_report.dart create mode 100644 app/lib/features/bug_report/providers/bug_report_providers.dart diff --git a/app/.env b/app/.env index 6245a81088c1..0356806b48b4 100644 --- a/app/.env +++ b/app/.env @@ -115,7 +115,7 @@ RAGESHAKE_APP_VERSION=DEV #enven:const #enven:type=String -RAGESHAKE_URL=http://localhost:8003/api/submit +RAGESHAKE_URL= diff --git a/app/lib/common/providers/common_providers.dart b/app/lib/common/providers/common_providers.dart index 5494456db6ac..a702bd890815 100644 --- a/app/lib/common/providers/common_providers.dart +++ b/app/lib/common/providers/common_providers.dart @@ -12,9 +12,6 @@ import 'package:riverpod/riverpod.dart'; final _log = Logger('a3::common::common_providers'); -// Loading Providers -final loadingProvider = StateProvider((ref) => false); - final genericUpdatesStream = StreamProvider.family((ref, key) async* { final client = ref.watch(alwaysClientProvider); diff --git a/app/lib/config/app_shell.dart b/app/lib/config/app_shell.dart index 470458307675..478f004a6b52 100644 --- a/app/lib/config/app_shell.dart +++ b/app/lib/config/app_shell.dart @@ -7,6 +7,7 @@ import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/utils/utils.dart'; import 'package:acter/features/auth/pages/logged_out_screen.dart'; import 'package:acter/features/bug_report/actions/open_bug_report.dart'; +import 'package:acter/features/bug_report/providers/bug_report_providers.dart'; import 'package:acter/features/calendar_sync/calendar_sync.dart'; import 'package:acter/features/cross_signing/widgets/cross_signing.dart'; import 'package:acter/features/home/providers/client_providers.dart'; @@ -65,7 +66,7 @@ class AppShellState extends ConsumerState { Future initShake() async { // shake is possible in only actual mobile devices - if (await isRealPhone()) { + if (isBugReportingEnabled && await isRealPhone()) { detector = ShakeDetector.autoStart( shakeThresholdGravity: 30.0, onShake: () { diff --git a/app/lib/features/bug_report/actions/submit_bug_report.dart b/app/lib/features/bug_report/actions/submit_bug_report.dart new file mode 100644 index 000000000000..03619b8fdecb --- /dev/null +++ b/app/lib/features/bug_report/actions/submit_bug_report.dart @@ -0,0 +1,90 @@ +import 'dart:convert'; +import 'dart:io'; +import 'dart:math'; + +import 'package:acter/config/env.g.dart'; +import 'package:acter/config/setup.dart'; +import 'package:acter_flutter_sdk/acter_flutter_sdk.dart'; +import 'package:http/http.dart' as http; +import 'package:http_parser/http_parser.dart'; +import 'package:logging/logging.dart'; +import 'package:path/path.dart'; + +final _log = Logger('a3::bug_report'); + +Future submitBugReport({ + bool withLog = false, + bool withPrevLogFile = false, + bool withUserId = false, + required String description, + String? screenshotPath, +}) async { + final sdk = await ActerSdk.instance; + + final request = http.MultipartRequest('POST', Uri.parse(Env.rageshakeUrl)); + request.fields.addAll({ + 'text': description, + 'user_agent': userAgent, + 'app': Env + .rageshakeAppName, // should be same as one among github_project_mappings + 'version': Env.rageshakeAppVersion, + }); + if (withUserId) { + final client = sdk.currentClient; + if (client != null) { + request.fields.addAll({'UserId': client.userId().toString()}); + } + } + if (withLog) { + String logFile = sdk.api.rotateLogFile(); + if (logFile.isNotEmpty) { + request.files.add( + http.MultipartFile.fromBytes( + 'log', + File(logFile).readAsBytesSync(), + filename: basename(logFile), + contentType: MediaType('text', 'plain'), + ), + ); + } + } + if (withPrevLogFile) { + String? prevLogFile = sdk.previousLogPath; + if (prevLogFile != null) { + request.files.add( + http.MultipartFile.fromBytes( + 'log', + File(prevLogFile).readAsBytesSync(), + filename: + '${basenameWithoutExtension(prevLogFile)}-${Random().nextInt(10000)}.log', // randomize to ensure the server doesn't overwrite any previous one... + contentType: MediaType('text', 'plain'), + ), + ); + } + } + if (screenshotPath != null) { + _log.info('sending with screenshot'); + request.files.add( + http.MultipartFile.fromBytes( + 'file', + File(screenshotPath).readAsBytesSync(), + filename: basename(screenshotPath), + contentType: MediaType('image', 'png'), + ), + ); + } + _log.info('sending ${Env.rageshakeUrl}'); + final resp = await request.send(); + if (resp.statusCode == HttpStatus.ok) { + Map json = jsonDecode(await resp.stream.bytesToString()); + if (screenshotPath != null) { + await File(screenshotPath).delete(); + } + // example - https://github.com/bitfriend/acter-bugs/issues/9 + return json['report_url'] ?? ''; + } else { + String body = await resp.stream.bytesToString(); + _log.severe('Sending bug report failed with ${resp.statusCode}: $body'); + throw '${resp.statusCode}: $body'; + } +} diff --git a/app/lib/features/bug_report/pages/bug_report_page.dart b/app/lib/features/bug_report/pages/bug_report_page.dart index 5c349b110934..65ed4f2ca610 100644 --- a/app/lib/features/bug_report/pages/bug_report_page.dart +++ b/app/lib/features/bug_report/pages/bug_report_page.dart @@ -1,102 +1,17 @@ -import 'dart:convert'; import 'dart:io'; -import 'dart:math'; - -import 'package:acter/common/providers/common_providers.dart'; import 'package:acter/common/toolkit/buttons/primary_action_button.dart'; import 'package:acter/common/utils/utils.dart'; -import 'package:acter/config/env.g.dart'; -import 'package:acter/config/setup.dart'; -import 'package:acter_flutter_sdk/acter_flutter_sdk.dart'; +import 'package:acter/features/bug_report/actions/submit_bug_report.dart'; +import 'package:acter/features/bug_report/providers/bug_report_providers.dart'; import 'package:flutter/material.dart'; import 'package:flutter_easyloading/flutter_easyloading.dart'; import 'package:flutter_gen/gen_l10n/l10n.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; -import 'package:http/http.dart' as http; -import 'package:http_parser/http_parser.dart'; import 'package:logging/logging.dart'; -import 'package:path/path.dart'; final _log = Logger('a3::bug_report'); -Future report({ - bool withLog = false, - bool withPrevLogFile = false, - bool withUserId = false, - required String description, - String? screenshotPath, -}) async { - final sdk = await ActerSdk.instance; - - final request = http.MultipartRequest('POST', Uri.parse(Env.rageshakeUrl)); - request.fields.addAll({ - 'text': description, - 'user_agent': userAgent, - 'app': Env - .rageshakeAppName, // should be same as one among github_project_mappings - 'version': Env.rageshakeAppVersion, - }); - if (withUserId) { - final client = sdk.currentClient; - if (client != null) { - request.fields.addAll({'UserId': client.userId().toString()}); - } - } - if (withLog) { - String logFile = sdk.api.rotateLogFile(); - if (logFile.isNotEmpty) { - request.files.add( - http.MultipartFile.fromBytes( - 'log', - File(logFile).readAsBytesSync(), - filename: basename(logFile), - contentType: MediaType('text', 'plain'), - ), - ); - } - } - if (withPrevLogFile) { - String? prevLogFile = sdk.previousLogPath; - if (prevLogFile != null) { - request.files.add( - http.MultipartFile.fromBytes( - 'log', - File(prevLogFile).readAsBytesSync(), - filename: - '${basenameWithoutExtension(prevLogFile)}-${Random().nextInt(10000)}.log', // randomize to ensure the server doesn't overwrite any previous one... - contentType: MediaType('text', 'plain'), - ), - ); - } - } - if (screenshotPath != null) { - _log.info('sending with screenshot'); - request.files.add( - http.MultipartFile.fromBytes( - 'file', - File(screenshotPath).readAsBytesSync(), - filename: basename(screenshotPath), - contentType: MediaType('image', 'png'), - ), - ); - } - _log.info('sending ${Env.rageshakeUrl}'); - final resp = await request.send(); - if (resp.statusCode == HttpStatus.ok) { - Map json = jsonDecode(await resp.stream.bytesToString()); - if (screenshotPath != null) { - await File(screenshotPath).delete(); - } - // example - https://github.com/bitfriend/acter-bugs/issues/9 - return json['report_url'] ?? ''; - } else { - String body = await resp.stream.bytesToString(); - _log.severe('Sending bug report failed with ${resp.statusCode}: $body'); - throw '${resp.statusCode}: $body'; - } -} - class BugReportPage extends ConsumerStatefulWidget { static const titleField = Key('bug-report-title'); static const includeScreenshot = Key('bug-report-include-screenshot'); @@ -123,10 +38,10 @@ class _BugReportState extends ConsumerState { bool withUserId = false; Future reportBug(BuildContext context) async { - final loadingNotifier = ref.read(loadingProvider.notifier); + final loadingNotifier = ref.read(bugReporterLoadingProvider.notifier); try { loadingNotifier.update((state) => true); - String reportUrl = await report( + String reportUrl = await submitBugReport( withLog: withLogFile, withPrevLogFile: withPrevLogFile, withUserId: withUserId, @@ -156,7 +71,7 @@ class _BugReportState extends ConsumerState { @override Widget build(BuildContext context) { - final isLoading = ref.watch(loadingProvider); + final isLoading = ref.watch(bugReporterLoadingProvider); return ConstrainedBox( constraints: const BoxConstraints(minWidth: 350), child: Form( diff --git a/app/lib/features/bug_report/providers/bug_report_providers.dart b/app/lib/features/bug_report/providers/bug_report_providers.dart new file mode 100644 index 000000000000..1ea64a0bdbc0 --- /dev/null +++ b/app/lib/features/bug_report/providers/bug_report_providers.dart @@ -0,0 +1,7 @@ +import 'package:acter/config/env.g.dart'; +import 'package:riverpod/riverpod.dart'; + +// Loading Providers +final bugReporterLoadingProvider = StateProvider((ref) => false); + +const isBugReportingEnabled = Env.rageshakeUrl != ''; diff --git a/app/lib/features/home/widgets/sidebar_widget.dart b/app/lib/features/home/widgets/sidebar_widget.dart index 8d01babcc208..d05b4fc477a7 100644 --- a/app/lib/features/home/widgets/sidebar_widget.dart +++ b/app/lib/features/home/widgets/sidebar_widget.dart @@ -5,6 +5,7 @@ import 'package:acter/common/utils/constants.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/widgets/user_avatar.dart'; import 'package:acter/features/bug_report/actions/open_bug_report.dart'; +import 'package:acter/features/bug_report/providers/bug_report_providers.dart'; import 'package:acter/features/home/data/keys.dart'; import 'package:acter/features/home/widgets/activities_icon.dart'; import 'package:acter/features/home/widgets/chats_icon.dart'; @@ -164,7 +165,7 @@ class SidebarWidget extends ConsumerWidget { crossAxisAlignment: CrossAxisAlignment.center, children: [ ...menu, - ..._trailing(context), + if (isBugReportingEnabled) ..._bugReporter(context), ], ), ), @@ -188,13 +189,13 @@ class SidebarWidget extends ConsumerWidget { ), ), ), - ..._trailing(context), + if (isBugReportingEnabled) ..._bugReporter(context), ], ), ); } - List _trailing(BuildContext context) { + List _bugReporter(BuildContext context) { return [ const Divider(indent: 18, endIndent: 18), InkWell( diff --git a/app/lib/features/search/widgets/quick_actions_builder.dart b/app/lib/features/search/widgets/quick_actions_builder.dart index a75b27f16951..5b8c97f7f24e 100644 --- a/app/lib/features/search/widgets/quick_actions_builder.dart +++ b/app/lib/features/search/widgets/quick_actions_builder.dart @@ -3,6 +3,7 @@ import 'package:acter/common/themes/colors/color_scheme.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/utils/utils.dart'; import 'package:acter/features/bug_report/actions/open_bug_report.dart'; +import 'package:acter/features/bug_report/providers/bug_report_providers.dart'; import 'package:acter/features/search/model/keys.dart'; import 'package:acter/features/settings/providers/settings_providers.dart'; import 'package:acter/features/spaces/model/keys.dart'; @@ -129,27 +130,28 @@ class QuickActionsBuilder extends ConsumerWidget { onPressed: () => routeTo(context, Routes.createSpace), label: Text(L10n.of(context).createSpace), ), - OutlinedButton.icon( - key: QuickJumpKeys.bugReport, - style: OutlinedButton.styleFrom( - foregroundColor: Theme.of(context).colorScheme.textHighlight, - side: BorderSide( - width: 1, - color: Theme.of(context).colorScheme.textHighlight, + if (isBugReportingEnabled) + OutlinedButton.icon( + key: QuickJumpKeys.bugReport, + style: OutlinedButton.styleFrom( + foregroundColor: Theme.of(context).colorScheme.textHighlight, + side: BorderSide( + width: 1, + color: Theme.of(context).colorScheme.textHighlight, + ), ), + icon: const Icon(Atlas.bug_clipboard_thin, size: 18), + label: Text( + L10n.of(context).reportBug, + style: Theme.of(context).textTheme.labelMedium, + ), + onPressed: () async { + if (popBeforeRoute) { + Navigator.pop(context); + } + await openBugReport(context); + }, ), - icon: const Icon(Atlas.bug_clipboard_thin, size: 18), - label: Text( - L10n.of(context).reportBug, - style: Theme.of(context).textTheme.labelMedium, - ), - onPressed: () async { - if (popBeforeRoute) { - Navigator.pop(context); - } - await openBugReport(context); - }, - ), ], ), ); From 41a29083f1425ecb4d45c2a092d71bdf8a903fad Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 22 Aug 2024 12:16:42 +0100 Subject: [PATCH 05/11] Error Page for pins (with test) --- .../common/toolkit/errors/error_dialog.dart | 204 ++++++++++++++++++ app/lib/common/toolkit/errors/error_page.dart | 112 ++-------- app/lib/common/toolkit/errors/util.dart | 13 ++ .../features/pins/pages/pins_list_page.dart | 23 +- app/test/features/pins/error_pages_test.dart | 24 +++ app/test/helpers/error_helpers.dart | 34 +++ app/test/helpers/mock_pins_providers.dart | 23 ++ app/test/helpers/test_util.dart | 25 +++ 8 files changed, 355 insertions(+), 103 deletions(-) create mode 100644 app/lib/common/toolkit/errors/error_dialog.dart create mode 100644 app/lib/common/toolkit/errors/util.dart create mode 100644 app/test/features/pins/error_pages_test.dart create mode 100644 app/test/helpers/error_helpers.dart create mode 100644 app/test/helpers/mock_pins_providers.dart diff --git a/app/lib/common/toolkit/errors/error_dialog.dart b/app/lib/common/toolkit/errors/error_dialog.dart new file mode 100644 index 000000000000..1b3ccbd782ff --- /dev/null +++ b/app/lib/common/toolkit/errors/error_dialog.dart @@ -0,0 +1,204 @@ +import 'package:acter/common/toolkit/errors/util.dart'; +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; +import 'package:acter/features/bug_report/providers/bug_report_providers.dart'; +import 'package:flutter/material.dart'; +import 'package:quickalert/widgets/quickalert_buttons.dart'; +import 'package:quickalert/widgets/quickalert_container.dart'; +import 'package:quickalert/models/quickalert_options.dart'; +import 'package:quickalert/quickalert.dart'; +import 'package:flutter_gen/gen_l10n/l10n.dart'; + +class ActerErrorDialog extends StatelessWidget { + static const retryBtn = Key('error-dialog-retry-btn'); + + final Object error; + final StackTrace stack; + final VoidCallback? onRetryTap; + + final String? title; + final String? text; + final String Function(Object error)? textBuilder; + final bool includeBugReportButton; + + /// Dialog Border Radius + final double borderRadius; + + const ActerErrorDialog({ + super.key, + required this.error, + required this.stack, + this.onRetryTap, + this.title, + this.text, + this.textBuilder, + this.includeBugReportButton = true, + this.borderRadius = 15.0, + }); + + static Future show({ + /// BuildContext + required BuildContext context, + required Object error, + required StackTrace stack, + + /// Title of the dialog + String? title, + + /// Text of the dialog + String? text, + VoidCallback? onRetryTap, + String Function(Object error)? textBuilder, + bool includeBugReportButton = true, + + /// Dialog Border Radius + double borderRadius = 15.0, + }) { + return showGeneralDialog( + context: context, + pageBuilder: (context, anim1, __) => ActerErrorDialog( + error: error, + stack: stack, + title: title, + text: text, + textBuilder: textBuilder, + onRetryTap: onRetryTap, + borderRadius: borderRadius, + includeBugReportButton: includeBugReportButton, + ), + ); + } + + @override + Widget build(BuildContext context) { + return AlertDialog( + contentPadding: EdgeInsets.zero, + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(borderRadius), + ), + content: errorDialog(context), + ); + } + + Widget errorDialog(BuildContext context) { + final theme = Theme.of(context); + final lang = L10n.of(context); + final err = ErrorCode.guessFromError(error); + QuickAlertOptions options = QuickAlertOptions( + title: title ?? + switch (err) { + ErrorCode.notFound => lang.notFound, + _ => lang.fatalError, + }, + text: text ?? (textBuilder != null ? textBuilder!(error) : null), + type: switch (err) { + ErrorCode.notFound => QuickAlertType.warning, + _ => QuickAlertType.error, + }, + showCancelBtn: true, + showConfirmBtn: false, + cancelBtnText: lang.back, + borderRadius: borderRadius, + ); + if (onRetryTap != null) { + options.showConfirmBtn = true; + options.confirmBtnColor = theme.primaryColor; + options.confirmBtnText = lang.retry; + options.onConfirmBtnTap = () { + onRetryTap!(); + }; + } + + return _ActerErrorAlert( + options: options, + includeBugReportButton: includeBugReportButton, + ); + } +} + +class _ActerErrorAlert extends QuickAlertContainer { + final bool includeBugReportButton; + const _ActerErrorAlert({ + required super.options, + this.includeBugReportButton = true, + }); + + @override + Widget buildButtons() { + return _ActerErrorActionButtons(options: options); + } + + @override + Widget buildHeader(context) { + final orginalHeader = super.buildHeader(context); + if (!includeBugReportButton || !isBugReportingEnabled) { + return orginalHeader; + } + return Stack( + children: [ + orginalHeader, + Positioned( + right: 10, + top: 10, + child: TextButton( + child: Text(L10n.of(context).reportBug), + onPressed: () => openBugReport(context), + ), + ), + ], + ); + } +} + +class _ActerErrorActionButtons extends QuickAlertButtons { + const _ActerErrorActionButtons({required super.options}); + + @override + Widget buildButton({ + BuildContext? context, + required bool isOkayBtn, + required String text, + VoidCallback? onTap, + }) { + final btnText = Text( + text, + style: defaultTextStyle(isOkayBtn), + ); + if (isOkayBtn) { + return buildOkayBtn(context: context, btnText: btnText, onTap: onTap); + } + return buildCancelBtn(btnText: btnText, onTap: onTap); + } + + Widget buildOkayBtn({ + BuildContext? context, + required Widget btnText, + VoidCallback? onTap, + }) { + return MaterialButton( + key: ActerErrorDialog.retryBtn, + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(15.0), + ), + color: options.confirmBtnColor ?? Theme.of(context!).primaryColor, + onPressed: onTap, + child: Center( + child: Padding( + padding: const EdgeInsets.all(7.5), + child: btnText, + ), + ), + ); + } + + Widget buildCancelBtn({ + required Widget btnText, + VoidCallback? onTap, + }) { + return GestureDetector( + onTap: onTap, + child: Center( + child: btnText, + ), + ); + } +} diff --git a/app/lib/common/toolkit/errors/error_page.dart b/app/lib/common/toolkit/errors/error_page.dart index 65b898d68830..7124f2b6108f 100644 --- a/app/lib/common/toolkit/errors/error_page.dart +++ b/app/lib/common/toolkit/errors/error_page.dart @@ -1,28 +1,11 @@ -import 'package:acter/features/bug_report/actions/open_bug_report.dart'; +import 'package:acter/common/toolkit/errors/error_dialog.dart'; import 'package:flutter/material.dart'; -import 'package:quickalert/models/quickalert_options.dart'; -import 'package:quickalert/quickalert.dart'; -import 'package:quickalert/widgets/quickalert_buttons.dart'; -import 'package:quickalert/widgets/quickalert_container.dart'; -import 'package:flutter_gen/gen_l10n/l10n.dart'; - -enum ErrorCode { - notFound, - other, -} - -ErrorCode _guessError(Object error) { - final errorStr = error.toString(); - // yay, string-based error guessing! - if (errorStr.contains('not found')) { - return ErrorCode.notFound; - } - return ErrorCode.other; -} /// ErrorPage shows a full-screen error to the user (covering other internal errors) /// class ErrorPage extends StatelessWidget { + static const dialogKey = Key('error-page-dialog'); + /// Put this widget in the Background of the screen to give context final Widget background; final Object error; @@ -55,87 +38,18 @@ class ErrorPage extends StatelessWidget { return Stack( children: [ background, - AlertDialog( - contentPadding: EdgeInsets.zero, - shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(borderRadius), - ), - content: errorDialog(context), + ActerErrorDialog( + key: dialogKey, + error: error, + stack: stack, + includeBugReportButton: includeBugReportButton, + text: text, + textBuilder: textBuilder, + title: title, + onRetryTap: onRetryTap, + borderRadius: borderRadius, ), ], ); } - - Widget errorDialog(BuildContext context) { - final theme = Theme.of(context); - final lang = L10n.of(context); - final err = _guessError(error); - QuickAlertOptions options = QuickAlertOptions( - title: title ?? - switch (err) { - ErrorCode.notFound => lang.notFound, - _ => lang.fatalError, - }, - text: text ?? (textBuilder != null ? textBuilder!(error) : null), - type: switch (err) { - ErrorCode.notFound => QuickAlertType.warning, - _ => QuickAlertType.error, - }, - showCancelBtn: true, - showConfirmBtn: false, - cancelBtnText: lang.back, - borderRadius: borderRadius, - ); - if (onRetryTap != null) { - options.showConfirmBtn = true; - options.confirmBtnColor = theme.primaryColor; - options.confirmBtnText = lang.retry; - options.onConfirmBtnTap = () { - onRetryTap!(); - }; - } - - return _ActerErrorAlert( - options: options, - includeBugReportButton: includeBugReportButton, - ); - } -} - -class _ActerErrorAlert extends QuickAlertContainer { - final bool includeBugReportButton; - const _ActerErrorAlert({ - required super.options, - this.includeBugReportButton = true, - }); - - @override - Widget buildButtons() { - return _ActerErrorActionButtons(options: options); - } - - @override - Widget buildHeader(context) { - final orginalHeader = super.buildHeader(context); - if (!includeBugReportButton) { - return orginalHeader; - } - return Stack( - children: [ - orginalHeader, - Positioned( - right: 10, - top: 10, - child: TextButton( - child: Text(L10n.of(context).reportBug), - onPressed: () => openBugReport(context), - ), - ), - ], - ); - } -} - -class _ActerErrorActionButtons extends QuickAlertButtons { - const _ActerErrorActionButtons({required super.options}); } diff --git a/app/lib/common/toolkit/errors/util.dart b/app/lib/common/toolkit/errors/util.dart new file mode 100644 index 000000000000..4b09dfad6bae --- /dev/null +++ b/app/lib/common/toolkit/errors/util.dart @@ -0,0 +1,13 @@ +enum ErrorCode { + notFound, + other; + + static ErrorCode guessFromError(Object error) { + final errorStr = error.toString(); + // yay, string-based error guessing! + if (errorStr.contains('not found')) { + return ErrorCode.notFound; + } + return ErrorCode.other; + } +} diff --git a/app/lib/features/pins/pages/pins_list_page.dart b/app/lib/features/pins/pages/pins_list_page.dart index c6dfee883a2b..d96a415ba1c8 100644 --- a/app/lib/features/pins/pages/pins_list_page.dart +++ b/app/lib/features/pins/pages/pins_list_page.dart @@ -2,6 +2,7 @@ import 'dart:math'; import 'package:acter/common/providers/space_providers.dart'; import 'package:acter/common/toolkit/buttons/primary_action_button.dart'; +import 'package:acter/common/toolkit/errors/error_page.dart'; import 'package:acter/common/utils/routes.dart'; import 'package:acter/common/widgets/acter_search_widget.dart'; import 'package:acter/common/widgets/add_button_with_can_permission.dart'; @@ -88,10 +89,24 @@ class _AllPinsPageConsumerState extends ConsumerState { Expanded( child: pinsLoader.when( data: (pins) => _buildPinsList(pins), - error: (e, s) { - _log.severe('Failed to load pins', e, s); - return Center( - child: Text(L10n.of(context).loadingFailed(e)), + error: (error, stack) { + _log.severe('Failed to load pins', error, stack); + return ErrorPage( + background: const PinListSkeleton(), + error: error, + stack: stack, + textBuilder: L10n.of(context).loadingFailed, + onRetryTap: () { + if (searchValue.isNotEmpty) { + ref.invalidate( + pinListSearchProvider( + (spaceId: widget.spaceId, searchText: searchValue), + ), + ); + } else { + ref.invalidate(pinListProvider(widget.spaceId)); + } + }, ); }, loading: () => const PinListSkeleton(), diff --git a/app/test/features/pins/error_pages_test.dart b/app/test/features/pins/error_pages_test.dart new file mode 100644 index 000000000000..5d5b67c524a8 --- /dev/null +++ b/app/test/features/pins/error_pages_test.dart @@ -0,0 +1,24 @@ +import 'package:acter/common/providers/space_providers.dart'; +import 'package:acter/features/pins/pages/pins_list_page.dart'; +import 'package:acter/features/pins/providers/pins_provider.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../helpers/error_helpers.dart'; +import '../../helpers/mock_pins_providers.dart'; +import '../../helpers/test_util.dart'; + +void main() { + group('Pins Error Pages', () { + testWidgets('full list', (tester) async { + final mockedPinListNotifier = MockAsyncPinListNotifier(); + await tester.pumpProviderWidget( + overrides: [ + pinListProvider.overrideWith(() => mockedPinListNotifier), + hasSpaceWithPermissionProvider.overrideWith((_, ref) => false), + ], + child: const PinsListPage(), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); + }); +} diff --git a/app/test/helpers/error_helpers.dart b/app/test/helpers/error_helpers.dart new file mode 100644 index 000000000000..dfde86512978 --- /dev/null +++ b/app/test/helpers/error_helpers.dart @@ -0,0 +1,34 @@ +import 'package:acter/common/toolkit/errors/error_dialog.dart'; +import 'package:acter/common/toolkit/errors/error_page.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'test_util.dart'; + +extension ErrorPageExtensions on WidgetTester { + Future ensureErrorPageWorks() async { + await pumpProviderScopeOnce(); + expect( + find.byKey(ErrorPage.dialogKey), + findsOneWidget, + reason: 'Error Dialog not present', + ); + } + + Future ensureErrorPageWithRetryWorks() async { + await ensureErrorPageWorks(); + + expect( + find.byKey(ActerErrorDialog.retryBtn), + findsOneWidget, + reason: 'Confirm Button not present', + ); + + await tap(find.byKey(ActerErrorDialog.retryBtn)); + await pumpProviderScope(times: 2); + // dialog is gone on retry working + expect( + find.byKey(ErrorPage.dialogKey), + findsNothing, + reason: 'Error Dialog did not disappear', + ); + } +} diff --git a/app/test/helpers/mock_pins_providers.dart b/app/test/helpers/mock_pins_providers.dart new file mode 100644 index 000000000000..de679442d113 --- /dev/null +++ b/app/test/helpers/mock_pins_providers.dart @@ -0,0 +1,23 @@ +import 'dart:async'; +import 'package:acter/features/pins/providers/notifiers/pins_notifiers.dart'; +import 'package:acter_flutter_sdk/acter_flutter_sdk_ffi.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:mockito/mockito.dart'; +import 'package:riverpod/riverpod.dart'; + +class MockAsyncPinListNotifier + extends FamilyAsyncNotifier, String?> + with Mock + implements AsyncPinListNotifier { + bool shouldFail = true; + + @override + Future> build(String? arg) async { + if (shouldFail) { + shouldFail = !shouldFail; + throw 'Expected fail'; + } + + return []; + } +} diff --git a/app/test/helpers/test_util.dart b/app/test/helpers/test_util.dart index af0ab17f20c3..1d451e050f77 100644 --- a/app/test/helpers/test_util.dart +++ b/app/test/helpers/test_util.dart @@ -2,6 +2,8 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'test_wrapper_widget.dart'; + extension PumpUntilFound on WidgetTester { Future pumpProviderScope({ int times = 10, @@ -13,6 +15,13 @@ extension PumpUntilFound on WidgetTester { } } + Future pumpProviderScopeOnce({ + Duration duration = const Duration(milliseconds: 100), + }) async { + final ProviderScope scope = widget(find.byType(ProviderScope)); + await pumpWidget(scope, duration: duration); + } + Future pumpUntilMatches( Finder finder, Matcher matcher, { @@ -34,3 +43,19 @@ extension PumpUntilFound on WidgetTester { } } } + +extension ActerProviderTesting on WidgetTester { + Future pumpProviderWidget({ + List? overrides, + required Widget child, + }) async { + await pumpWidget( + ProviderScope( + overrides: overrides ?? [], + child: InActerContextTestWrapper( + child: child, + ), + ), + ); + } +} From 2eda3a3e24d501121b48d174715a838a57fb95f8 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 22 Aug 2024 13:03:35 +0100 Subject: [PATCH 06/11] More Pin List Page Tests --- app/test/features/pins/error_pages_test.dart | 64 +++++++++++++++++++- app/test/helpers/mock_pins_providers.dart | 3 +- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/app/test/features/pins/error_pages_test.dart b/app/test/features/pins/error_pages_test.dart index 5d5b67c524a8..2d5506b8fccc 100644 --- a/app/test/features/pins/error_pages_test.dart +++ b/app/test/features/pins/error_pages_test.dart @@ -1,4 +1,6 @@ +import 'package:acter/common/providers/room_providers.dart'; import 'package:acter/common/providers/space_providers.dart'; +import 'package:acter/common/widgets/acter_search_widget.dart'; import 'package:acter/features/pins/pages/pins_list_page.dart'; import 'package:acter/features/pins/providers/pins_provider.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -8,9 +10,9 @@ import '../../helpers/mock_pins_providers.dart'; import '../../helpers/test_util.dart'; void main() { - group('Pins Error Pages', () { + group('Pin List Error Pages', () { testWidgets('full list', (tester) async { - final mockedPinListNotifier = MockAsyncPinListNotifier(); + final mockedPinListNotifier = RetryMockAsyncPinListNotifier(); await tester.pumpProviderWidget( overrides: [ pinListProvider.overrideWith(() => mockedPinListNotifier), @@ -20,5 +22,63 @@ void main() { ); await tester.ensureErrorPageWithRetryWorks(); }); + testWidgets('full list with search', (tester) async { + bool shouldFail = true; + + await tester.pumpProviderWidget( + overrides: [ + searchValueProvider + .overrideWith((_) => 'some string'), // set a search string + pinListSearchProvider.overrideWith((_, params) async { + if (shouldFail) { + shouldFail = false; + throw 'Some Error'; + } else { + return []; + } + }), + hasSpaceWithPermissionProvider.overrideWith((_, ref) => false), + ], + child: const PinsListPage(), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); + + testWidgets('space list', (tester) async { + final mockedPinListNotifier = RetryMockAsyncPinListNotifier(); + await tester.pumpProviderWidget( + overrides: [ + roomDisplayNameProvider.overrideWith((a, b) => 'test'), + pinListProvider.overrideWith(() => mockedPinListNotifier), + hasSpaceWithPermissionProvider.overrideWith((_, ref) => false), + ], + child: const PinsListPage( + spaceId: '!test', + ), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); + testWidgets('space list with search', (tester) async { + bool shouldFail = true; + + await tester.pumpProviderWidget( + overrides: [ + roomDisplayNameProvider.overrideWith((a, b) => 'test'), + searchValueProvider + .overrideWith((_) => 'some other string'), // set a search string + pinListSearchProvider.overrideWith((_, params) async { + if (shouldFail) { + shouldFail = false; + throw 'Some Error'; + } else { + return []; + } + }), + hasSpaceWithPermissionProvider.overrideWith((_, ref) => false), + ], + child: const PinsListPage(spaceId: '!something'), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); }); } diff --git a/app/test/helpers/mock_pins_providers.dart b/app/test/helpers/mock_pins_providers.dart index de679442d113..5f53ad25b80d 100644 --- a/app/test/helpers/mock_pins_providers.dart +++ b/app/test/helpers/mock_pins_providers.dart @@ -5,7 +5,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:mockito/mockito.dart'; import 'package:riverpod/riverpod.dart'; -class MockAsyncPinListNotifier +class RetryMockAsyncPinListNotifier extends FamilyAsyncNotifier, String?> with Mock implements AsyncPinListNotifier { @@ -14,6 +14,7 @@ class MockAsyncPinListNotifier @override Future> build(String? arg) async { if (shouldFail) { + // toggle failure so the retry works shouldFail = !shouldFail; throw 'Expected fail'; } From 5df23abe1e1978734329565a57908d483969280b Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Thu, 22 Aug 2024 14:00:38 +0100 Subject: [PATCH 07/11] Testing error on Pin Details page --- .../spaces/select_space_form_field.dart | 2 +- .../widgets/attachment_section.dart | 4 +- .../comments/widgets/comments_section.dart | 10 +- app/lib/features/home/widgets/space_chip.dart | 122 ++++++++---------- .../features/pins/pages/pin_details_page.dart | 112 ++++++++++------ .../space/sheets/link_room_sheet.dart | 2 +- app/test/features/pins/error_pages_test.dart | 18 +++ app/test/helpers/error_helpers.dart | 43 ++++-- app/test/helpers/mock_pins_providers.dart | 38 ++++++ 9 files changed, 223 insertions(+), 128 deletions(-) diff --git a/app/lib/common/widgets/spaces/select_space_form_field.dart b/app/lib/common/widgets/spaces/select_space_form_field.dart index 29677d4d2c30..15e1b10a06ed 100644 --- a/app/lib/common/widgets/spaces/select_space_form_field.dart +++ b/app/lib/common/widgets/spaces/select_space_form_field.dart @@ -100,7 +100,7 @@ class SelectSpaceFormField extends ConsumerWidget { data: (space) { if (space == null) return Text(currentId!); return SpaceChip( - space: space, + spaceId: space.roomId, onTapOpenSpaceDetail: false, useCompatView: useCompatView, onTapSelectSpace: () { diff --git a/app/lib/features/attachments/widgets/attachment_section.dart b/app/lib/features/attachments/widgets/attachment_section.dart index 960c9b9a0265..e7f42a56e822 100644 --- a/app/lib/features/attachments/widgets/attachment_section.dart +++ b/app/lib/features/attachments/widgets/attachment_section.dart @@ -39,7 +39,7 @@ class AttachmentSectionWidget extends ConsumerWidget { _log.severe('Failed to load attachment manager', e, s); return onError(context, e); }, - loading: () => loading(context), + loading: loading, ); } @@ -52,7 +52,7 @@ class AttachmentSectionWidget extends ConsumerWidget { ); } - Widget loading(BuildContext context) { + static Widget loading() { return const Skeletonizer( child: SizedBox( height: 100, diff --git a/app/lib/features/comments/widgets/comments_section.dart b/app/lib/features/comments/widgets/comments_section.dart index 8e8a70193a84..66eaccf26e64 100644 --- a/app/lib/features/comments/widgets/comments_section.dart +++ b/app/lib/features/comments/widgets/comments_section.dart @@ -36,7 +36,7 @@ class CommentsSection extends ConsumerWidget { ); } - Widget inBox(BuildContext context, Widget child) { + static Widget _inBox(BuildContext context, Widget child) { return Padding( padding: const EdgeInsets.all(12), child: Column( @@ -64,14 +64,14 @@ class CommentsSection extends ConsumerWidget { } Widget found(BuildContext context, CommentsManager manager) { - return inBox(context, CommentsList(manager: manager)); + return _inBox(context, CommentsList(manager: manager)); } Widget onError(BuildContext context, Object error) { - return inBox(context, Text(L10n.of(context).loadingFailed(error))); + return _inBox(context, Text(L10n.of(context).loadingFailed(error))); } - Widget loading(BuildContext context) { - return inBox(context, Text(L10n.of(context).loading)); + static Widget loading(BuildContext context) { + return _inBox(context, Text(L10n.of(context).loading)); } } diff --git a/app/lib/features/home/widgets/space_chip.dart b/app/lib/features/home/widgets/space_chip.dart index dda8703f511e..91d61845bb17 100644 --- a/app/lib/features/home/widgets/space_chip.dart +++ b/app/lib/features/home/widgets/space_chip.dart @@ -1,4 +1,4 @@ -import 'package:acter/common/providers/space_providers.dart'; +import 'package:acter/common/providers/room_providers.dart'; import 'package:acter/router/utils.dart'; import 'package:acter_avatar/acter_avatar.dart'; import 'package:flutter/material.dart'; @@ -10,16 +10,14 @@ import 'package:skeletonizer/skeletonizer.dart'; final _log = Logger('a3::home::space_chip'); class SpaceChip extends ConsumerWidget { - final SpaceItem? space; - final String? spaceId; + final String spaceId; final bool onTapOpenSpaceDetail; final bool useCompatView; final VoidCallback? onTapSelectSpace; const SpaceChip({ super.key, - this.space, - this.spaceId, + required this.spaceId, this.onTapOpenSpaceDetail = true, this.useCompatView = false, this.onTapSelectSpace, @@ -27,82 +25,70 @@ class SpaceChip extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { - if (space == null) { - if (spaceId == null) { - throw L10n.of(context).spaceOrSpaceIdMustBeProvided; - } - final spaceLoader = ref.watch(briefSpaceItemProvider(spaceId!)); - return spaceLoader.when( - data: (space) => renderSpace(context, space), - error: (e, s) { - _log.severe('Failed to load brief of space', e, s); - return Chip( - label: Text(L10n.of(context).loadingFailed(e)), - ); - }, - loading: () => renderLoading(spaceId!), - ); + if (useCompatView) { + return renderCompactView(context, ref); } - return renderSpace(context, space!); + return renderFullChip(context, ref); } - Widget renderLoading(String spaceId) { + static Widget loading() { return Skeletonizer( child: Chip( avatar: ActerAvatar( - options: AvatarOptions( - AvatarInfo(uniqueId: spaceId), + options: const AvatarOptions( + AvatarInfo(uniqueId: 'unique Id'), size: 24, ), ), - label: Text(spaceId), + label: const Text('unique name'), ), ); } - Widget renderSpace(BuildContext context, SpaceItem space) { - String spaceName = space.avatarInfo.displayName ?? space.roomId; - return useCompatView - ? Row( - children: [ - Text(L10n.of(context).inSpaceLabelInline), - Text(L10n.of(context).colonCharacter), - InkWell( - onTap: () { - if (!onTapOpenSpaceDetail) { - if (onTapSelectSpace != null) { - onTapSelectSpace!(); - return; - } - } - goToSpace(context, space.roomId); - }, - child: Text( - spaceName, - style: Theme.of(context).textTheme.labelLarge!.copyWith( - decoration: TextDecoration.underline, - ), - ), - ), - ], - ) - : InkWell( - onTap: () { - if (onTapOpenSpaceDetail) goToSpace(context, space.roomId); - }, - child: Chip( - avatar: ActerAvatar( - options: AvatarOptions( - AvatarInfo( - uniqueId: space.roomId, - displayName: space.avatarInfo.displayName, - avatar: space.avatarInfo.avatar, - ), - size: 24, + Widget renderCompactView(BuildContext context, WidgetRef ref) { + final displayName = + ref.watch(roomDisplayNameProvider(spaceId)).valueOrNull ?? spaceId; + return Row( + children: [ + Text(L10n.of(context).inSpaceLabelInline), + Text(L10n.of(context).colonCharacter), + InkWell( + onTap: () { + if (!onTapOpenSpaceDetail) { + if (onTapSelectSpace != null) { + onTapSelectSpace!(); + return; + } + } + goToSpace(context, spaceId); + }, + child: Text( + displayName, + style: Theme.of(context).textTheme.labelLarge!.copyWith( + decoration: TextDecoration.underline, ), - ), - label: Text(spaceName), - ), - ); + ), + ), + ], + ); + } + + Widget renderFullChip(BuildContext context, WidgetRef ref) { + final avatarInfo = ref.watch(roomAvatarInfoProvider(spaceId)); + final spaceName = avatarInfo.displayName ?? spaceId; + return InkWell( + onTap: () { + if (onTapOpenSpaceDetail) goToSpace(context, spaceId); + }, + child: Chip( + avatar: ActerAvatar( + options: AvatarOptions( + avatarInfo, + size: 24, + ), + ), + label: Text(spaceName), + ), + ); } } diff --git a/app/lib/features/pins/pages/pin_details_page.dart b/app/lib/features/pins/pages/pin_details_page.dart index d9f1662d1346..21cf66a66050 100644 --- a/app/lib/features/pins/pages/pin_details_page.dart +++ b/app/lib/features/pins/pages/pin_details_page.dart @@ -1,5 +1,6 @@ import 'package:acter/common/providers/common_providers.dart'; import 'package:acter/common/providers/room_providers.dart'; +import 'package:acter/common/toolkit/errors/error_page.dart'; import 'package:acter/common/widgets/edit_html_description_sheet.dart'; import 'package:acter/common/widgets/edit_title_sheet.dart'; import 'package:acter/common/widgets/render_html.dart'; @@ -75,49 +76,83 @@ class _PinDetailsPageState extends ConsumerState { ), ); }, - loading: () => Skeletonizer( - child: Text(L10n.of(context).loadingPin), - ), - error: (e, s) { - _log.severe('Error loading pin', e, s); - return Text( - L10n.of(context).errorLoadingPin(e), + loading: _contentLoader, + error: (error, stack) { + _log.severe('Error loading pin', error, stack); + return ErrorPage( + background: _contentLoader(), + error: error, + stack: stack, + onRetryTap: () { + ref.invalidate(pinProvider(widget.pinId)); + }, ); }, ); } - // pin actions menu builder - Widget _buildActionMenu() { - final pinData = ref.watch(pinProvider(widget.pinId)); - return pinData.when( - data: (pin) { - //Get my membership details - final membership = - ref.watch(roomMembershipProvider(pin.roomIdStr())).valueOrNull; - final canRedactData = ref.watch(canRedactProvider(pin)); - bool canPost = membership?.canString('CanPostPin') == true; - bool canRedact = canRedactData.valueOrNull == true; - return PopupMenuButton( - key: PinDetailsPage.actionMenuKey, - icon: const Icon(Atlas.dots_vertical_thin), - itemBuilder: (context) => - _buildAppBarActionMenuItems(pin, canPost, canRedact), - ); - }, - loading: () => Skeletonizer(child: Text(L10n.of(context).loadingPin)), - error: (e, s) { - _log.severe('Error loading pin', e, s); - return const SizedBox.shrink(); - }, + Widget _contentLoader() { + // not the nicest loading animation, but + return SingleChildScrollView( + child: Column( + children: [ + const SizedBox(height: 20), + _loadingPinHeaderUI(), + const SizedBox(height: 20), + AttachmentSectionWidget.loading(), + const SizedBox(height: 20), + CommentsSection.loading(context), + ], + ), + ); + } + + Widget _loadingPinHeaderUI() { + return Padding( + padding: const EdgeInsets.symmetric(horizontal: 16), + child: Column( + mainAxisAlignment: MainAxisAlignment.start, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Row( + children: [ + const Skeletonizer(child: Bone.circle(size: 100)), + const SizedBox(width: 12), + Expanded( + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Skeletonizer( + child: Text( + 'no text at all', + style: Theme.of(context).textTheme.titleSmall, + ), + ), + SpaceChip.loading(), + ], + ), + ), + ], + ), + ], + ), ); } - List> _buildAppBarActionMenuItems( - ActerPin pin, - bool canPost, - bool canRedact, - ) { + // pin actions menu builder + Widget _buildActionMenu() { + final pin = ref.watch(pinProvider(widget.pinId)).valueOrNull; + if (pin == null) { + return const SizedBox.shrink(); + } + final roomId = pin.roomIdStr(); + //Get my membership details + final membership = + ref.watch(roomMembershipProvider(pin.roomIdStr())).valueOrNull; + final canRedactData = ref.watch(canRedactProvider(pin)); + bool canPost = membership?.canString('CanPostPin') == true; + bool canRedact = canRedactData.valueOrNull == true; List> actions = []; //Check for can post pin permission @@ -160,7 +195,6 @@ class _PinDetailsPageState extends ConsumerState { //DELETE PIN MENU ITEM if (canRedact) { - final roomId = pin.roomIdStr(); actions.add( PopupMenuItem( onTap: () => showRedactDialog( @@ -176,7 +210,11 @@ class _PinDetailsPageState extends ConsumerState { ); } - return actions; + return PopupMenuButton( + key: PinDetailsPage.actionMenuKey, + icon: const Icon(Atlas.dots_vertical_thin), + itemBuilder: (context) => actions, + ); } Widget _buildPinHeaderUI(ActerPin pin) { diff --git a/app/lib/features/space/sheets/link_room_sheet.dart b/app/lib/features/space/sheets/link_room_sheet.dart index 0cec9e0581b9..3d46a1c2e5f5 100644 --- a/app/lib/features/space/sheets/link_room_sheet.dart +++ b/app/lib/features/space/sheets/link_room_sheet.dart @@ -134,7 +134,7 @@ class _LinkRoomPageConsumerState extends ConsumerState { children: [ Text(L10n.of(context).parentSpace), SpaceChip( - space: space, + spaceId: space.roomId, onTapOpenSpaceDetail: false, ), ], diff --git a/app/test/features/pins/error_pages_test.dart b/app/test/features/pins/error_pages_test.dart index 2d5506b8fccc..a97610dd67fa 100644 --- a/app/test/features/pins/error_pages_test.dart +++ b/app/test/features/pins/error_pages_test.dart @@ -1,6 +1,8 @@ +import 'package:acter/common/providers/common_providers.dart'; import 'package:acter/common/providers/room_providers.dart'; import 'package:acter/common/providers/space_providers.dart'; import 'package:acter/common/widgets/acter_search_widget.dart'; +import 'package:acter/features/pins/pages/pin_details_page.dart'; import 'package:acter/features/pins/pages/pins_list_page.dart'; import 'package:acter/features/pins/providers/pins_provider.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -81,4 +83,20 @@ void main() { await tester.ensureErrorPageWithRetryWorks(); }); }); + + group('Pin Details Error Page', () { + testWidgets('shows error and retries', (tester) async { + final mockedPinNotifier = RetryMockAsyncPinNotifier(); + await tester.pumpProviderWidget( + overrides: [ + roomDisplayNameProvider.overrideWith((a, b) => 'no name'), + roomMembershipProvider.overrideWith((a, b) => null), + canRedactProvider.overrideWith((a, b) => false), + pinProvider.overrideWith(() => mockedPinNotifier), + ], + child: const PinDetailsPage(pinId: 'pinId'), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); + }); } diff --git a/app/test/helpers/error_helpers.dart b/app/test/helpers/error_helpers.dart index dfde86512978..b3472094d8a0 100644 --- a/app/test/helpers/error_helpers.dart +++ b/app/test/helpers/error_helpers.dart @@ -1,20 +1,28 @@ import 'package:acter/common/toolkit/errors/error_dialog.dart'; import 'package:acter/common/toolkit/errors/error_page.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'test_util.dart'; extension ErrorPageExtensions on WidgetTester { - Future ensureErrorPageWorks() async { + Future ensureErrorPageWorks({dumpOnError = true}) async { await pumpProviderScopeOnce(); - expect( - find.byKey(ErrorPage.dialogKey), - findsOneWidget, - reason: 'Error Dialog not present', - ); + try { + expect( + find.byKey(ErrorPage.dialogKey), + findsOneWidget, + reason: 'Error Dialog not present', + ); + } catch (e) { + if (dumpOnError) { + debugDumpApp(); + } + rethrow; + } } - Future ensureErrorPageWithRetryWorks() async { - await ensureErrorPageWorks(); + Future ensureErrorPageWithRetryWorks({dumpOnError = true}) async { + await ensureErrorPageWorks(dumpOnError: dumpOnError); expect( find.byKey(ActerErrorDialog.retryBtn), @@ -24,11 +32,18 @@ extension ErrorPageExtensions on WidgetTester { await tap(find.byKey(ActerErrorDialog.retryBtn)); await pumpProviderScope(times: 2); - // dialog is gone on retry working - expect( - find.byKey(ErrorPage.dialogKey), - findsNothing, - reason: 'Error Dialog did not disappear', - ); + try { + // dialog is gone on retry working + expect( + find.byKey(ErrorPage.dialogKey), + findsNothing, + reason: 'Error Dialog did not disappear', + ); + } catch (e) { + if (dumpOnError) { + debugDumpApp(); + } + rethrow; + } } } diff --git a/app/test/helpers/mock_pins_providers.dart b/app/test/helpers/mock_pins_providers.dart index 5f53ad25b80d..52c7e941cbaa 100644 --- a/app/test/helpers/mock_pins_providers.dart +++ b/app/test/helpers/mock_pins_providers.dart @@ -22,3 +22,41 @@ class RetryMockAsyncPinListNotifier return []; } } + +class RetryMockAsyncPinNotifier + extends AutoDisposeFamilyAsyncNotifier + with Mock + implements AsyncPinNotifier { + bool shouldFail = true; + + @override + Future build(String arg) async { + if (shouldFail) { + // toggle failure so the retry works + shouldFail = !shouldFail; + throw 'Expected fail'; + } + return MockActerPin(); + } +} + +class MockActerPin extends Fake implements ActerPin { + @override + String roomIdStr() => '!roomId'; + + @override + String eventIdStr() => '\$evtId'; + + @override + String title() => 'Pin Title'; + + @override + Future attachments() => + Completer().future; + + @override + Future comments() => Completer().future; + + @override + MsgContent? content() => null; +} From d4c06cf6c3fc6c5134ce9548c573ce8142006718 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 23 Aug 2024 12:08:14 +0100 Subject: [PATCH 08/11] Remove SpaceDetailsPage error causer --- app/lib/router/shell_routers/home_shell_router.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/lib/router/shell_routers/home_shell_router.dart b/app/lib/router/shell_routers/home_shell_router.dart index d85b48c9453c..352dce09ef5e 100644 --- a/app/lib/router/shell_routers/home_shell_router.dart +++ b/app/lib/router/shell_routers/home_shell_router.dart @@ -309,8 +309,8 @@ final homeShellRoutes = [ pageBuilder: (context, state) { return NoTransitionPage( key: state.pageKey, - child: const SpaceDetailsPage( - spaceId: '!asdf', //state.pathParameters['spaceId']!, + child: SpaceDetailsPage( + spaceId: state.pathParameters['spaceId']!, ), ); }, From 4bc367f03ce9e52e37ad51e8e8d5eeb420d648ba Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 23 Aug 2024 13:04:51 +0100 Subject: [PATCH 09/11] Add tests for space details error page with retry --- app/lib/common/providers/space_providers.dart | 6 ++ app/lib/common/widgets/spaces/space_info.dart | 39 +++++-------- .../features/space/widgets/space_toolbar.dart | 7 +-- .../features/spaces/error_pages_test.dart | 55 +++++++++++++++++++ app/test/helpers/error_helpers.dart | 2 +- app/test/helpers/mock_space_providers.dart | 31 +++++++++++ 6 files changed, 108 insertions(+), 32 deletions(-) create mode 100644 app/test/features/spaces/error_pages_test.dart create mode 100644 app/test/helpers/mock_space_providers.dart diff --git a/app/lib/common/providers/space_providers.dart b/app/lib/common/providers/space_providers.dart index 66c20581f8ed..ccd81f285c4e 100644 --- a/app/lib/common/providers/space_providers.dart +++ b/app/lib/common/providers/space_providers.dart @@ -49,6 +49,12 @@ final spaceProvider = throw 'Space not found'; }); +final spaceIsBookmarkedProvider = + FutureProvider.family((ref, spaceId) async { + final space = await ref.watch(spaceProvider(spaceId).future); + return space.isBookmarked(); +}); + /// Attempts to map a spaceId to the space, but could come back empty (null) rather than throw. /// keeps up to date with underlying client even if the space wasn't found initially, final maybeSpaceProvider = diff --git a/app/lib/common/widgets/spaces/space_info.dart b/app/lib/common/widgets/spaces/space_info.dart index ee75cdd2dd87..4373d08753cf 100644 --- a/app/lib/common/widgets/spaces/space_info.dart +++ b/app/lib/common/widgets/spaces/space_info.dart @@ -29,13 +29,11 @@ class SpaceInfo extends ConsumerWidget { Widget build(BuildContext context, WidgetRef ref) { final spaceLoader = ref.watch(spaceProvider(spaceId)); return spaceLoader.when( - data: (space) => Row( - mainAxisAlignment: MainAxisAlignment.start, + data: (space) => Wrap( children: [ VisibilityChip(roomId: spaceId), const SizedBox(width: 5), acterSpaceInfoUI(context, ref, space), - const Spacer(), ], ), error: (e, s) { @@ -50,26 +48,17 @@ class SpaceInfo extends ConsumerWidget { return Skeletonizer( child: Row( children: [ - Tooltip( - message: '', - child: Icon( - Atlas.glasses_vision_thin, - size: size, - ), + Icon( + Atlas.glasses_vision_thin, + size: size, ), - Tooltip( - message: '', - child: Icon( - Atlas.lock_clipboard_thin, - size: size, - ), + Icon( + Atlas.lock_clipboard_thin, + size: size, ), - Tooltip( - message: '', - child: Icon( - Atlas.shield_exclamation_thin, - size: size, - ), + Icon( + Atlas.shield_exclamation_thin, + size: size, ), ], ), @@ -77,9 +66,7 @@ class SpaceInfo extends ConsumerWidget { } Widget acterSpaceInfoUI(BuildContext context, WidgetRef ref, Space space) { - final isActerLoader = ref.watch(isActerSpaceForSpace(space)); - final widgetLoader = isActerLoader.whenData((isActer) { - if (isActer) return const SizedBox.shrink(); + if (ref.watch(isActerSpaceForSpace(space)).valueOrNull != true) { return Padding( padding: const EdgeInsets.only(right: 3), child: Tooltip( @@ -91,7 +78,7 @@ class SpaceInfo extends ConsumerWidget { ), ), ); - }); - return widgetLoader.valueOrNull ?? const SizedBox.shrink(); + } + return const SizedBox.shrink(); } } diff --git a/app/lib/features/space/widgets/space_toolbar.dart b/app/lib/features/space/widgets/space_toolbar.dart index 45a9302f697f..cb194b88b320 100644 --- a/app/lib/features/space/widgets/space_toolbar.dart +++ b/app/lib/features/space/widgets/space_toolbar.dart @@ -26,11 +26,8 @@ class SpaceToolbar extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { final membership = ref.watch(roomMembershipProvider(spaceId)).valueOrNull; - final isBookmarked = ref.watch( - spaceProvider(spaceId).select( - (asyncValue) => (asyncValue.valueOrNull?.isBookmarked()) == true, - ), - ); + final isBookmarked = + ref.watch(spaceIsBookmarkedProvider(spaceId)).valueOrNull ?? false; final invited = ref.watch(spaceInvitedMembersProvider(spaceId)).valueOrNull ?? []; final showInviteBtn = membership?.canString('CanInvite') == true; diff --git a/app/test/features/spaces/error_pages_test.dart b/app/test/features/spaces/error_pages_test.dart new file mode 100644 index 000000000000..af2a0d1cdb0b --- /dev/null +++ b/app/test/features/spaces/error_pages_test.dart @@ -0,0 +1,55 @@ +import 'package:acter/common/providers/room_providers.dart'; +import 'package:acter/common/providers/space_providers.dart'; +import 'package:acter/common/widgets/spaces/space_info.dart'; +import 'package:acter/features/bookmarks/providers/bookmarks_provider.dart'; +import 'package:acter/features/space/pages/space_details_page.dart'; +import 'package:acter/features/space/providers/space_navbar_provider.dart'; +import 'package:acter/features/space/providers/suggested_provider.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../helpers/error_helpers.dart'; +import '../../helpers/mock_space_providers.dart'; +import '../../helpers/test_util.dart'; + +void main() { + group('Space Details Error Page', () { + testWidgets('shows error and retries', (tester) async { + bool shouldFail = true; + await tester.pumpProviderWidget( + overrides: [ + // mocking so we can display the page in general + roomVisibilityProvider.overrideWith((a, b) => null), + roomDisplayNameProvider.overrideWith((a, b) => null), + parentAvatarInfosProvider.overrideWith((a, b) => []), + roomAvatarProvider.overrideWith((a, b) => null), + membersIdsProvider.overrideWith((a, b) => []), + roomAvatarInfoProvider + .overrideWith(() => MockRoomAvatarInfoNotifier()), + roomMembershipProvider.overrideWith((a, b) => null), + isBookmarkedProvider.overrideWith((a, b) => false), + spaceInvitedMembersProvider.overrideWith((a, b) => []), + shouldShowSuggestedProvider.overrideWith((a, b) => false), + isActerSpaceForSpace.overrideWith((a, b) => false), + + // the actual failing ones + spaceProvider.overrideWith((a, b) { + if (shouldFail) { + // toggle failure so the retry works + shouldFail = !shouldFail; + throw 'Expected fail: Space not loaded'; + } + return MockSpace(); + }), + tabsProvider.overrideWith((ref, id) async { + await ref.watch( + spaceProvider(id).future, + ); // let this fail internally + return []; + }), + ], + child: const SpaceDetailsPage(spaceId: '!spaceId'), + ); + await tester.ensureErrorPageWithRetryWorks(); + }); + }); +} diff --git a/app/test/helpers/error_helpers.dart b/app/test/helpers/error_helpers.dart index b3472094d8a0..80b27dccd854 100644 --- a/app/test/helpers/error_helpers.dart +++ b/app/test/helpers/error_helpers.dart @@ -31,7 +31,7 @@ extension ErrorPageExtensions on WidgetTester { ); await tap(find.byKey(ActerErrorDialog.retryBtn)); - await pumpProviderScope(times: 2); + await pumpProviderScope(times: 5); try { // dialog is gone on retry working expect( diff --git a/app/test/helpers/mock_space_providers.dart b/app/test/helpers/mock_space_providers.dart new file mode 100644 index 000000000000..648e20fb083e --- /dev/null +++ b/app/test/helpers/mock_space_providers.dart @@ -0,0 +1,31 @@ +import 'package:acter/common/providers/notifiers/room_notifiers.dart'; +import 'package:acter/common/providers/notifiers/space_notifiers.dart'; +import 'package:acter_avatar/acter_avatar.dart'; +import 'package:acter_flutter_sdk/acter_flutter_sdk_ffi.dart'; +import 'package:riverpod/riverpod.dart'; +import 'package:mockito/mockito.dart'; + +class MockRoomAvatarInfoNotifier extends FamilyNotifier + with Mock + implements RoomAvatarInfoNotifier { + @override + AvatarInfo build(arg) => AvatarInfo(uniqueId: arg); +} + +class RetryMockAsyncSpaceNotifier extends FamilyAsyncNotifier + with Mock + implements AsyncMaybeSpaceNotifier { + bool shouldFail = true; + + @override + Future build(String arg) async { + if (shouldFail) { + // toggle failure so the retry works + shouldFail = !shouldFail; + throw 'Expected fail: Space not loaded'; + } + return MockSpace(); + } +} + +class MockSpace extends Fake implements Space {} From 2a2d38849f25cc093d4e35a81c7003ce4403f88a Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 23 Aug 2024 13:11:28 +0100 Subject: [PATCH 10/11] Fixing broken imports --- app/integration_test/tests/bug_reporter.dart | 1 + app/lib/features/home/widgets/space_chip.dart | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/integration_test/tests/bug_reporter.dart b/app/integration_test/tests/bug_reporter.dart index b4f1ea8064e2..c63961f472d8 100644 --- a/app/integration_test/tests/bug_reporter.dart +++ b/app/integration_test/tests/bug_reporter.dart @@ -1,3 +1,4 @@ +import 'package:acter/features/bug_report/actions/open_bug_report.dart'; import 'package:acter/features/bug_report/pages/bug_report_page.dart'; import 'package:acter/features/home/data/keys.dart'; import 'package:acter/config/app_shell.dart'; diff --git a/app/lib/features/home/widgets/space_chip.dart b/app/lib/features/home/widgets/space_chip.dart index 91d61845bb17..b2a6fa2f35e3 100644 --- a/app/lib/features/home/widgets/space_chip.dart +++ b/app/lib/features/home/widgets/space_chip.dart @@ -4,11 +4,8 @@ import 'package:acter_avatar/acter_avatar.dart'; import 'package:flutter/material.dart'; import 'package:flutter_gen/gen_l10n/l10n.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; -import 'package:logging/logging.dart'; import 'package:skeletonizer/skeletonizer.dart'; -final _log = Logger('a3::home::space_chip'); - class SpaceChip extends ConsumerWidget { final String spaceId; final bool onTapOpenSpaceDetail; From 024c15e2502fe5bdce99bb2c3fd29cce6d087db0 Mon Sep 17 00:00:00 2001 From: Benjamin Kampmann Date: Fri, 23 Aug 2024 16:14:23 +0100 Subject: [PATCH 11/11] Provide Error and stack over bug reporter --- .../common/toolkit/errors/error_dialog.dart | 15 +- .../bug_report/actions/open_bug_report.dart | 6 +- .../bug_report/actions/submit_bug_report.dart | 9 +- .../bug_report/pages/bug_report_page.dart | 161 ++++++++++++------ app/lib/l10n/app_en.arb | 2 + app/lib/router/general_router.dart | 2 + app/pubspec.lock | 28 +-- 7 files changed, 154 insertions(+), 69 deletions(-) diff --git a/app/lib/common/toolkit/errors/error_dialog.dart b/app/lib/common/toolkit/errors/error_dialog.dart index 1b3ccbd782ff..8975cf74d7f4 100644 --- a/app/lib/common/toolkit/errors/error_dialog.dart +++ b/app/lib/common/toolkit/errors/error_dialog.dart @@ -109,6 +109,8 @@ class ActerErrorDialog extends StatelessWidget { } return _ActerErrorAlert( + error: error, + stack: stack, options: options, includeBugReportButton: includeBugReportButton, ); @@ -117,8 +119,13 @@ class ActerErrorDialog extends StatelessWidget { class _ActerErrorAlert extends QuickAlertContainer { final bool includeBugReportButton; + final Object error; + final StackTrace stack; + const _ActerErrorAlert({ required super.options, + required this.error, + required this.stack, this.includeBugReportButton = true, }); @@ -141,7 +148,13 @@ class _ActerErrorAlert extends QuickAlertContainer { top: 10, child: TextButton( child: Text(L10n.of(context).reportBug), - onPressed: () => openBugReport(context), + onPressed: () => openBugReport( + context, + queryParams: { + 'error': error.toString(), + 'stack': stack.toString(), + }, + ), ), ), ], diff --git a/app/lib/features/bug_report/actions/open_bug_report.dart b/app/lib/features/bug_report/actions/open_bug_report.dart index 5d574cf30768..891fc0dac09f 100644 --- a/app/lib/features/bug_report/actions/open_bug_report.dart +++ b/app/lib/features/bug_report/actions/open_bug_report.dart @@ -10,14 +10,16 @@ final _log = Logger('a3::bug_report::open_bug_report'); bool _bugReportOpen = false; -Future openBugReport(BuildContext context) async { +Future openBugReport( + BuildContext context, { + Map queryParams = const {}, +}) async { if (_bugReportOpen) { return; } final cacheDir = await appCacheDir(); // rage shake disallows dot in filename final timestamp = DateTime.now().timestamp; - final Map queryParams = {}; final imagePath = await screenshotController.captureAndSave( cacheDir, fileName: 'screenshot_$timestamp.png', diff --git a/app/lib/features/bug_report/actions/submit_bug_report.dart b/app/lib/features/bug_report/actions/submit_bug_report.dart index 03619b8fdecb..c9700300b7dd 100644 --- a/app/lib/features/bug_report/actions/submit_bug_report.dart +++ b/app/lib/features/bug_report/actions/submit_bug_report.dart @@ -16,25 +16,28 @@ Future submitBugReport({ bool withLog = false, bool withPrevLogFile = false, bool withUserId = false, - required String description, + required String title, String? screenshotPath, + Map extraFields = const {}, }) async { final sdk = await ActerSdk.instance; final request = http.MultipartRequest('POST', Uri.parse(Env.rageshakeUrl)); request.fields.addAll({ - 'text': description, + 'text': title, 'user_agent': userAgent, 'app': Env .rageshakeAppName, // should be same as one among github_project_mappings 'version': Env.rageshakeAppVersion, }); + request.fields.addAll(extraFields); if (withUserId) { final client = sdk.currentClient; if (client != null) { - request.fields.addAll({'UserId': client.userId().toString()}); + request.fields['UserId'] = client.userId().toString(); } } + request.fields.addAll(extraFields); if (withLog) { String logFile = sdk.api.rotateLogFile(); if (logFile.isNotEmpty) { diff --git a/app/lib/features/bug_report/pages/bug_report_page.dart b/app/lib/features/bug_report/pages/bug_report_page.dart index 65ed4f2ca610..558c8ca867f7 100644 --- a/app/lib/features/bug_report/pages/bug_report_page.dart +++ b/app/lib/features/bug_report/pages/bug_report_page.dart @@ -22,8 +22,15 @@ class BugReportPage extends ConsumerStatefulWidget { static const submitBtn = Key('bug-report-submit'); static const pageKey = Key('bug-report'); final String? imagePath; + final String? error; + final String? stack; - const BugReportPage({super.key = pageKey, this.imagePath}); + const BugReportPage({ + super.key = pageKey, + this.imagePath, + this.error, + this.stack, + }); @override ConsumerState createState() => _BugReportState(); @@ -32,21 +39,36 @@ class BugReportPage extends ConsumerStatefulWidget { class _BugReportState extends ConsumerState { final formKey = GlobalKey(debugLabel: 'Bug report form key'); final titleController = TextEditingController(); + final descController = TextEditingController(); bool withScreenshot = false; bool withLogFile = false; bool withPrevLogFile = false; bool withUserId = false; + bool submitErrorAndStackTrace = true; Future reportBug(BuildContext context) async { final loadingNotifier = ref.read(bugReporterLoadingProvider.notifier); try { loadingNotifier.update((state) => true); + final Map extraFields = {}; + if (submitErrorAndStackTrace) { + if (widget.error != null) { + extraFields['error'] = widget.error.toString(); + } + if (widget.stack != null) { + extraFields['stack'] = widget.stack.toString(); + } + } + if (descController.text.isNotEmpty) { + extraFields['description'] = descController.text; + } String reportUrl = await submitBugReport( withLog: withLogFile, withPrevLogFile: withPrevLogFile, withUserId: withUserId, - description: titleController.text, + title: titleController.text, screenshotPath: withScreenshot ? widget.imagePath : null, + extraFields: extraFields, ); String? issueId = getIssueId(reportUrl); loadingNotifier.update((state) => false); @@ -58,7 +80,7 @@ class _BugReportState extends ConsumerState { } return true; } catch (e, s) { - _log.severe('Failed to repor bug', e, s); + _log.severe('Failed to report bug', e, s); loadingNotifier.update((state) => false); if (!context.mounted) return false; EasyLoading.showError( @@ -84,17 +106,30 @@ class _BugReportState extends ConsumerState { mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.start, children: [ - const SizedBox(height: 10), - Text(L10n.of(context).bugReportDescription), const SizedBox(height: 10), TextFormField( key: BugReportPage.titleField, controller: titleController, + decoration: InputDecoration( + hintText: L10n.of(context).bugReportDescription, + ), validator: (newValue) => newValue == null || newValue.isEmpty ? L10n.of(context).emptyDescription : null, ), const SizedBox(height: 10), + TextFormField( + keyboardType: TextInputType.multiline, + textInputAction: TextInputAction.newline, + controller: descController, + minLines: 4, + autofocus: true, + maxLines: 4, + decoration: InputDecoration( + hintText: L10n.of(context).description, + ), + ), + const SizedBox(height: 10), CheckboxListTile( key: BugReportPage.includeUserId, title: Text(L10n.of(context).includeUserId), @@ -104,50 +139,10 @@ class _BugReportState extends ConsumerState { }), controlAffinity: ListTileControlAffinity.leading, ), - CheckboxListTile( - key: BugReportPage.includeLog, - title: Text(L10n.of(context).includeLog), - value: withLogFile, - onChanged: (bool? value) => setState(() { - withLogFile = value ?? true; - }), - controlAffinity: ListTileControlAffinity.leading, - ), - CheckboxListTile( - key: BugReportPage.includePrevLog, - title: Text(L10n.of(context).includePrevLog), - value: withPrevLogFile, - onChanged: (bool? value) => setState(() { - withPrevLogFile = value ?? true; - }), - controlAffinity: ListTileControlAffinity.leading, - ), - const SizedBox(height: 10), - CheckboxListTile( - key: BugReportPage.includeScreenshot, - title: Text(L10n.of(context).includeScreenshot), - value: withScreenshot, - onChanged: (bool? value) => setState(() { - withScreenshot = value ?? true; - }), - controlAffinity: ListTileControlAffinity.leading, - enabled: widget.imagePath != null, - ), - const SizedBox(height: 10), - if (withScreenshot) - Image.file( - File(widget.imagePath!), - key: BugReportPage.screenshot, - width: MediaQuery.of(context).size.width * 0.8, - errorBuilder: ( - BuildContext context, - Object error, - StackTrace? stackTrace, - ) { - return Text(L10n.of(context).couldNotLoadImage(error)); - }, - ), - if (withScreenshot) const SizedBox(height: 10), + const Divider(endIndent: 10, indent: 10), + ...renderErrorOptions(), + ...renderLogOptions(), + ...renderForScreenShot(), const Divider(endIndent: 10, indent: 10), const SizedBox(height: 10), isLoading @@ -171,4 +166,72 @@ class _BugReportState extends ConsumerState { ), ); } + + List renderErrorOptions() { + if (widget.error == null) return []; + return [ + CheckboxListTile( + title: Text(L10n.of(context).includeErrorAndStackTrace), + value: submitErrorAndStackTrace, + onChanged: (bool? value) => setState(() { + submitErrorAndStackTrace = value ?? true; + }), + controlAffinity: ListTileControlAffinity.leading, + ), + ]; + } + + List renderLogOptions() { + return [ + CheckboxListTile( + key: BugReportPage.includeLog, + title: Text(L10n.of(context).includeLog), + value: withLogFile, + onChanged: (bool? value) => setState(() { + withLogFile = value ?? true; + }), + controlAffinity: ListTileControlAffinity.leading, + ), + CheckboxListTile( + key: BugReportPage.includePrevLog, + title: Text(L10n.of(context).includePrevLog), + value: withPrevLogFile, + onChanged: (bool? value) => setState(() { + withPrevLogFile = value ?? true; + }), + controlAffinity: ListTileControlAffinity.leading, + ), + ]; + } + + List renderForScreenShot() { + if (widget.imagePath == null) return []; + return [ + const SizedBox(height: 10), + CheckboxListTile( + key: BugReportPage.includeScreenshot, + title: Text(L10n.of(context).includeScreenshot), + value: withScreenshot, + onChanged: (bool? value) => setState(() { + withScreenshot = value ?? true; + }), + controlAffinity: ListTileControlAffinity.leading, + ), + const SizedBox(height: 10), + if (withScreenshot) + Image.file( + File(widget.imagePath!), + key: BugReportPage.screenshot, + width: MediaQuery.of(context).size.width * 0.8, + errorBuilder: ( + BuildContext context, + Object error, + StackTrace? stackTrace, + ) { + return Text(L10n.of(context).couldNotLoadImage(error)); + }, + ), + if (withScreenshot) const SizedBox(height: 10), + ]; + } } diff --git a/app/lib/l10n/app_en.arb b/app/lib/l10n/app_en.arb index 4ffbcc327c27..ce136d1a1c94 100644 --- a/app/lib/l10n/app_en.arb +++ b/app/lib/l10n/app_en.arb @@ -1780,6 +1780,8 @@ "@includePrevLog": {}, "includeScreenshot": "Include screenshot", "@includeScreenshot": {}, + "includeErrorAndStackTrace": "Include Error & Stacktrace", + "@includeErrorAndStackTrace": {}, "jumpTo": "jump to", "@jumpTo": {}, "noMatchingPinsFound": "no matching pins found", diff --git a/app/lib/router/general_router.dart b/app/lib/router/general_router.dart index 0fec48a02a74..6b12b7fdfde5 100644 --- a/app/lib/router/general_router.dart +++ b/app/lib/router/general_router.dart @@ -96,6 +96,8 @@ final generalRoutes = [ pageBuilder: (context, state) => DialogPage( builder: (BuildContext context) => BugReportPage( imagePath: state.uri.queryParameters['screenshot'], + error: state.uri.queryParameters['error'], + stack: state.uri.queryParameters['stack'], ), ), ), diff --git a/app/pubspec.lock b/app/pubspec.lock index bfc3f41c34d3..1c34b59316d6 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -1289,18 +1289,18 @@ packages: dependency: transitive description: name: leak_tracker - sha256: "3f87a60e8c63aecc975dda1ceedbc8f24de75f09e4856ea27daf8958f2f0ce05" + sha256: "7f0df31977cb2c0b88585095d168e689669a2cc9b97c309665e3386f3e9d341a" url: "https://pub.dev" source: hosted - version: "10.0.5" + version: "10.0.4" leak_tracker_flutter_testing: dependency: transitive description: name: leak_tracker_flutter_testing - sha256: "932549fb305594d82d7183ecd9fa93463e9914e1b67cacc34bc40906594a1806" + sha256: "06e98f569d004c1315b991ded39924b21af84cf14cc94791b8aea337d25b57f8" url: "https://pub.dev" source: hosted - version: "3.0.5" + version: "3.0.3" leak_tracker_testing: dependency: transitive description: @@ -1361,10 +1361,10 @@ packages: dependency: transitive description: name: material_color_utilities - sha256: f7142bb1154231d7ea5f96bc7bde4bda2a0945d2806bb11670e30b850d56bdec + sha256: "0e0a020085b65b6083975e499759762399b4475f766c21668c4ecca34ea74e5a" url: "https://pub.dev" source: hosted - version: "0.11.1" + version: "0.8.0" matrix_link_text: dependency: transitive description: @@ -1449,10 +1449,10 @@ packages: dependency: transitive description: name: meta - sha256: bdb68674043280c3428e9ec998512fb681678676b3c54e773629ffe74419f8c7 + sha256: "7687075e408b093f36e6bbf6c91878cc0d4cd10f409506f7bc996f68220b9136" url: "https://pub.dev" source: hosted - version: "1.15.0" + version: "1.12.0" mime: dependency: "direct main" description: @@ -1682,10 +1682,10 @@ packages: dependency: transitive description: name: platform - sha256: "9b71283fc13df574056616011fb138fd3b793ea47cc509c189a6c3fa5f8a1a65" + sha256: "12220bb4b65720483f8fa9450b4332347737cf8213dd2840d8b2c823e47243ec" url: "https://pub.dev" source: hosted - version: "3.1.5" + version: "3.1.4" plugin_platform_interface: dependency: transitive description: @@ -2295,10 +2295,10 @@ packages: dependency: transitive description: name: test_api - sha256: "5b8a98dafc4d5c4c9c72d8b31ab2b23fc13422348d2997120294d3bac86b4ddb" + sha256: "9955ae474176f7ac8ee4e989dadfb411a58c30415bcfb648fa04b2b8a03afa7f" url: "https://pub.dev" source: hosted - version: "0.7.2" + version: "0.7.0" timeago: dependency: transitive description: @@ -2559,10 +2559,10 @@ packages: dependency: transitive description: name: vm_service - sha256: f652077d0bdf60abe4c1f6377448e8655008eef28f128bc023f7b5e8dfeb48fc + sha256: "3923c89304b715fb1eb6423f017651664a03bf5f4b29983627c4da791f74a4ec" url: "https://pub.dev" source: hosted - version: "14.2.4" + version: "14.2.1" volume_controller: dependency: transitive description: