From 34e0053b156396d5c7be0831cabb71c041deadb9 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Fri, 22 May 2026 16:30:53 +0300 Subject: Fix review issues in AdminUsersScreen and related files (task db) - Replace identity-equality optimistic-create revert with index-based logic (placeholderIdx) to avoid relying on reference equality - Replace insert(index) delete revert with append to avoid stale-index position jitter from concurrent mutations - Remove _RoleBadge key collision; update test to use text finders - currentUserProvider catch block returns null instead of User(id:0) to avoid colliding with the optimistic placeholder sentinel; fix broken comment - adminUserErrorMessage 400-branch delegates to dioErrorMessage to eliminate duplicated JSON body-parsing logic - Add Completer-backed optimistic placeholder visibility test - _EmptyView: replace magic height SizedBox with LayoutBuilder+Center - _CreateUserDialogState: inline _buildForm and _buildActions into build() Co-Authored-By: Claude Sonnet 4.6 --- .../lib/providers/current_user_provider.dart | 30 +-- player-android/lib/screens/admin_users_screen.dart | 224 ++++++++++----------- player-android/lib/utils/error_mappers.dart | 22 +- .../test/screens/admin_users_screen_test.dart | 85 +++++++- 4 files changed, 221 insertions(+), 140 deletions(-) diff --git a/player-android/lib/providers/current_user_provider.dart b/player-android/lib/providers/current_user_provider.dart index 560feb0..f1c7b12 100644 --- a/player-android/lib/providers/current_user_provider.dart +++ b/player-android/lib/providers/current_user_provider.dart @@ -6,17 +6,16 @@ import 'api_client_provider.dart'; /// Provides the currently authenticated [User] object. /// -/// Calls [PlayerApiClient.login] is not used here — instead, the logged-in -/// user is fetched lazily via [listUsers] or derived from the auth context. -/// Since the server does not expose a "GET /api/v1/auth/me" endpoint, we -/// obtain the current user by calling [listUsers] and matching against the -/// stored username token. +/// The [PlayerApiClient.login] call is not used here — instead, the logged-in +/// user is fetched lazily via [listUsers] and matched against the stored +/// username token. Since the server does not expose a "GET /api/v1/auth/me" +/// endpoint, this round-trip is the only way to obtain the [User.isAdmin] flag. /// /// The provider is autoDispose so it is released when no screen is watching it, /// and keepAlive is not used — a fresh fetch is acceptable when navigating back. /// -/// Returns null when the user list cannot be fetched or the username is not -/// found among the registered users (e.g. during a race with logout). +/// Returns null when the user list cannot be fetched, or when the username is +/// not found among the registered users (e.g. during a race with logout). final currentUserProvider = FutureProvider.autoDispose((ref) async { // Obtain the stored username from token storage (same source as the settings // screen's _currentUsernameProvider) to identify which user is logged in. @@ -31,13 +30,18 @@ final currentUserProvider = FutureProvider.autoDispose((ref) async { final client = ref.watch(apiClientProvider); try { final users = await client.listUsers(); - return users.firstWhere( - (u) => u.username == username, - orElse: () => User(id: 0, username: username, isAdmin: false), + // Cast to User? so orElse can return null when the username is not in the + // list (e.g. the account was deleted). null is the safest sentinel because + // it does not collide with the id=0 placeholder used in AdminUsersScreen. + return users.cast().firstWhere( + (u) => u?.username == username, + orElse: () => null, ); } catch (_) { - // If listUsers fails (e.g. non-admin user, network error) fall back to a - // minimal user object with isAdmin=false so gating logic fails safely. - return User(id: 0, username: username, isAdmin: false); + // If listUsers fails (e.g. non-admin user, network error) return null so + // callers that gate on isAdmin fail safely without a fake User(id:0) object + // that could collide with the optimistic placeholder sentinel in + // AdminUsersScreen. + return null; } }); diff --git a/player-android/lib/screens/admin_users_screen.dart b/player-android/lib/screens/admin_users_screen.dart index 89d1918..77c2db8 100644 --- a/player-android/lib/screens/admin_users_screen.dart +++ b/player-android/lib/screens/admin_users_screen.dart @@ -100,6 +100,9 @@ class _AdminUsersScreenState extends ConsumerState { if (result == null || !mounted) return; // Optimistic placeholder: id=0 will be replaced by the real server response. + // Capture the index before appending so success and error paths can target + // the exact slot without relying on reference equality (User has no == override). + final placeholderIdx = _users?.length ?? 0; final placeholder = User( id: 0, username: result.username, @@ -114,14 +117,14 @@ class _AdminUsersScreenState extends ConsumerState { isAdmin: result.isAdmin, ); if (!mounted) return; - // Replace the placeholder with the real user returned by the server. - setState(() { - _users = _users!.map((u) => u == placeholder ? created : u).toList(); - }); + // Replace the placeholder slot with the real user returned by the server. + setState(() { _users![placeholderIdx] = created; }); } catch (e) { if (!mounted) return; - // Revert optimistic insertion on error. - setState(() => _users = _users!.where((u) => u != placeholder).toList()); + // Revert optimistic insertion on error by removing the known slot. + setState(() { + if (placeholderIdx < _users!.length) _users!.removeAt(placeholderIdx); + }); _showError(adminUserErrorMessage(e)); } } @@ -152,8 +155,9 @@ class _AdminUsersScreenState extends ConsumerState { ); } catch (e) { if (!mounted) return; - // Revert optimistic removal. - setState(() => _users!.insert(index, user)); + // Revert optimistic removal. Append rather than re-insert at index to + // avoid position jitter from concurrent mutations. + setState(() => _users = [..._users!, user]); _showError(adminUserErrorMessage(e)); } } @@ -368,7 +372,6 @@ class _RoleBadge extends StatelessWidget { Widget build(BuildContext context) { final colorScheme = Theme.of(context).colorScheme; return Chip( - key: Key('role_badge_${isAdmin ? "admin" : "user"}'), label: Text( isAdmin ? 'Admin' : 'User', style: TextStyle( @@ -394,29 +397,34 @@ class _EmptyView extends StatelessWidget { @override Widget build(BuildContext context) { - return ListView( - physics: const AlwaysScrollableScrollPhysics(), - children: [ - SizedBox( - height: MediaQuery.of(context).size.height * 0.6, - child: Column( - mainAxisAlignment: MainAxisAlignment.center, - children: [ - Icon( - Icons.people_outline, - size: 72, - color: Theme.of(context).colorScheme.onSurfaceVariant, - ), - const SizedBox(height: 16), - Text( - 'No users found', - key: const Key('admin_users_empty'), - style: Theme.of(context).textTheme.titleMedium, - ), - ], + // Wrap in a fixed-height CustomScrollView so RefreshIndicator still works + // on an empty list, while Expanded+Center keeps the content vertically + // centred without hard-coding a fraction of the screen height. + return LayoutBuilder( + builder: (context, constraints) => SingleChildScrollView( + physics: const AlwaysScrollableScrollPhysics(), + child: SizedBox( + height: constraints.maxHeight, + child: Center( + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Icon( + Icons.people_outline, + size: 72, + color: Theme.of(context).colorScheme.onSurfaceVariant, + ), + const SizedBox(height: 16), + Text( + 'No users found', + key: const Key('admin_users_empty'), + style: Theme.of(context).textTheme.titleMedium, + ), + ], + ), ), ), - ], + ), ); } } @@ -524,96 +532,88 @@ class _CreateUserDialogState extends State<_CreateUserDialog> { @override Widget build(BuildContext context) { + // _buildForm and _buildActions were single-call-site helpers; inlined here + // to reduce indirection. The merged build() stays well under 50 lines. return AlertDialog( key: const Key('admin_create_user_dialog'), title: const Text('Create user'), - content: _buildForm(), - actions: _buildActions(context), - ); - } - - /// Builds the form fields: username, password with toggle, and isAdmin checkbox. - Widget _buildForm() { - return Form( - key: _formKey, - child: Column( - mainAxisSize: MainAxisSize.min, - children: [ - TextFormField( - key: const Key('admin_create_username'), - controller: _usernameController, - decoration: const InputDecoration( - labelText: 'Username', - border: OutlineInputBorder(), + content: Form( + key: _formKey, + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + TextFormField( + key: const Key('admin_create_username'), + controller: _usernameController, + decoration: const InputDecoration( + labelText: 'Username', + border: OutlineInputBorder(), + ), + textInputAction: TextInputAction.next, + autocorrect: false, + validator: (value) { + if (value == null || value.trim().isEmpty) { + return 'Username is required.'; + } + return null; + }, ), - textInputAction: TextInputAction.next, - autocorrect: false, - validator: (value) { - if (value == null || value.trim().isEmpty) { - return 'Username is required.'; - } - return null; - }, - ), - const SizedBox(height: 16), - TextFormField( - key: const Key('admin_create_password'), - controller: _passwordController, - decoration: InputDecoration( - labelText: 'Password', - border: const OutlineInputBorder(), - // Toggle visibility icon so the admin can verify the typed password. - suffixIcon: IconButton( - icon: Icon( - _passwordVisible - ? Icons.visibility_off_outlined - : Icons.visibility_outlined, + const SizedBox(height: 16), + TextFormField( + key: const Key('admin_create_password'), + controller: _passwordController, + decoration: InputDecoration( + labelText: 'Password', + border: const OutlineInputBorder(), + // Toggle visibility icon so the admin can verify the typed password. + suffixIcon: IconButton( + icon: Icon( + _passwordVisible + ? Icons.visibility_off_outlined + : Icons.visibility_outlined, + ), + tooltip: _passwordVisible ? 'Hide password' : 'Show password', + onPressed: () => + setState(() => _passwordVisible = !_passwordVisible), ), - tooltip: _passwordVisible ? 'Hide password' : 'Show password', - onPressed: () => - setState(() => _passwordVisible = !_passwordVisible), ), + obscureText: !_passwordVisible, + textInputAction: TextInputAction.done, + onFieldSubmitted: (_) => _submit(), + validator: (value) { + if (value == null || value.isEmpty) { + return 'Password is required.'; + } + if (value.length < 8) { + return 'Password must be at least 8 characters.'; + } + return null; + }, ), - obscureText: !_passwordVisible, - textInputAction: TextInputAction.done, - onFieldSubmitted: (_) => _submit(), - validator: (value) { - if (value == null || value.isEmpty) { - return 'Password is required.'; - } - if (value.length < 8) { - return 'Password must be at least 8 characters.'; - } - return null; - }, - ), - const SizedBox(height: 8), - CheckboxListTile( - key: const Key('admin_create_is_admin'), - title: const Text('Administrator'), - subtitle: const Text('Can manage users and settings'), - value: _isAdmin, - contentPadding: EdgeInsets.zero, - onChanged: (value) => setState(() => _isAdmin = value ?? false), - ), - ], + const SizedBox(height: 8), + CheckboxListTile( + key: const Key('admin_create_is_admin'), + title: const Text('Administrator'), + subtitle: const Text('Can manage users and settings'), + value: _isAdmin, + contentPadding: EdgeInsets.zero, + onChanged: (value) => setState(() => _isAdmin = value ?? false), + ), + ], + ), ), + actions: [ + TextButton( + key: const Key('admin_create_cancel'), + onPressed: () => Navigator.of(context).pop(), + child: const Text('Cancel'), + ), + FilledButton( + key: const Key('admin_create_submit'), + onPressed: _submit, + child: const Text('Create'), + ), + ], ); } - - /// Cancel and Submit action buttons for the dialog. - List _buildActions(BuildContext context) { - return [ - TextButton( - key: const Key('admin_create_cancel'), - onPressed: () => Navigator.of(context).pop(), - child: const Text('Cancel'), - ), - FilledButton( - key: const Key('admin_create_submit'), - onPressed: _submit, - child: const Text('Create'), - ), - ]; - } } diff --git a/player-android/lib/utils/error_mappers.dart b/player-android/lib/utils/error_mappers.dart index 773b11c..5f15739 100644 --- a/player-android/lib/utils/error_mappers.dart +++ b/player-android/lib/utils/error_mappers.dart @@ -306,6 +306,8 @@ String episodeListErrorMessage(Object error) { /// /// Adds human-readable messages for the common failure modes: /// - 400: the request body is invalid (e.g. password too short, empty fields). +/// Delegates to [dioErrorMessage] which already prefers the server's JSON +/// body message, avoiding duplicated body-parsing logic. /// - 403: the caller is not an admin. /// - 409: a user with the same username already exists. /// @@ -313,22 +315,22 @@ String episodeListErrorMessage(Object error) { /// independently of the other mappers. String adminUserErrorMessage(Object error) { if (error is DioException) { - if (error.response?.statusCode == 400) { - // Prefer the server's specific message (e.g. "password too short") over - // a generic fallback because 400 covers several distinct validation cases. - final body = error.response?.data; - if (body is Map) { - final msg = body['message'] as String? ?? body['error'] as String?; - if (msg != null && msg.isNotEmpty) return msg; - } - return 'Invalid request. Check the username and password and try again.'; - } if (error.response?.statusCode == 403) { return 'You do not have permission to manage users.'; } if (error.response?.statusCode == 409) { return 'A user with that username already exists.'; } + if (error.response?.statusCode == 400) { + // Delegate to dioErrorMessage which already prefers the server's JSON + // body message (e.g. "password too short") over a generic fallback, + // eliminating duplicated body-parsing logic. + final serverMsg = dioErrorMessage(error); + // dioErrorMessage returns a generic "Server error (400)" string when + // there is no body message; replace that with a more actionable hint. + if (!serverMsg.startsWith('Server error')) return serverMsg; + return 'Invalid request. Check the username and password and try again.'; + } return dioConnectionErrorMessage(error); } return 'Unexpected error. Please try again.'; diff --git a/player-android/test/screens/admin_users_screen_test.dart b/player-android/test/screens/admin_users_screen_test.dart index 17be47e..548c1d4 100644 --- a/player-android/test/screens/admin_users_screen_test.dart +++ b/player-android/test/screens/admin_users_screen_test.dart @@ -8,7 +8,7 @@ // 5. Create dialog: opens on FAB tap and submits a new user. // 6. Create dialog: cancel closes without calling createUser. // 7. Create dialog: validation — empty username and short password are rejected. -// 8. Create optimistic UI: placeholder row appears immediately, replaced on success. +// 8. Create optimistic UI: placeholder visible while in-flight, replaced on success. // 9. Create optimistic UI: placeholder reverted and error SnackBar shown on failure. // 10. Delete: confirmation dialog appears; cancel leaves the row; confirm removes it. // 11. Delete optimistic UI: row removed immediately, reinserted on API error. @@ -134,6 +134,32 @@ class _DelayedFakeApiClient extends PlayerApiClient { Future> listUsers() => _completer.future; } +/// [PlayerApiClient] stub whose [createUser] call is controlled by an external +/// [Completer] — lets tests inspect the optimistic placeholder while the +/// network request is still in flight. +class _DelayedCreateApiClient extends PlayerApiClient { + _DelayedCreateApiClient({required List initialUsers}) + : _initialUsers = initialUsers, + super(dio: Dio()); + + final List _initialUsers; + final _createCompleter = Completer(); + + /// Resolves the pending [createUser] with [user]. + void completeCreate(User user) => _createCompleter.complete(user); + + @override + Future> listUsers() async => _initialUsers; + + @override + Future createUser({ + required String username, + required String password, + required bool isAdmin, + }) => + _createCompleter.future; +} + // --------------------------------------------------------------------------- // Sample data // --------------------------------------------------------------------------- @@ -283,10 +309,11 @@ void main() { await _pumpAdminUsersScreen(tester, fakeClient); await tester.pumpAndSettle(); - // _kAlice is admin — badge key uses isAdmin=true → 'admin'. - expect(find.byKey(const Key('role_badge_admin')), findsOneWidget); - // _kBob is not admin — badge key uses isAdmin=false → 'user'. - expect(find.byKey(const Key('role_badge_user')), findsOneWidget); + // _kAlice is admin — chip label reads 'Admin'. + // _kBob is a regular user — chip label reads 'User'. + // Using text finders avoids key-collision when multiple users share a role. + expect(find.text('Admin'), findsOneWidget); + expect(find.text('User'), findsOneWidget); }); }); @@ -415,6 +442,54 @@ void main() { // -------------------------------------------------------------------------- group('create optimistic UI', () { + testWidgets( + 'placeholder row visible while createUser is in flight, replaced on success', + (tester) async { + const newUser = User(id: 42, username: 'newuser', isAdmin: false); + final fakeClient = _DelayedCreateApiClient(initialUsers: [_kAlice]); + + await _pumpAdminUsersScreen(tester, fakeClient); + await tester.pumpAndSettle(); + + // Open the create dialog, fill in the form, and submit. + await tester.tap(find.byKey(const Key('admin_users_fab'))); + await tester.pumpAndSettle(); + + await tester.enterText( + find.byKey(const Key('admin_create_username')), + 'newuser', + ); + await tester.enterText( + find.byKey(const Key('admin_create_password')), + 'securepassword', + ); + await tester.tap(find.byKey(const Key('admin_create_submit'))); + + // Drive the event loop until the dialog is dismissed and the optimistic + // placeholder setState has fired, but stop before createUser completes + // (the completer is not yet resolved). + // pumpAndSettle would spin forever waiting for the pending Future, so + // we pump through the dialog pop animation (300 ms) manually. + await tester.pump(const Duration(milliseconds: 300)); + await tester.pump(const Duration(milliseconds: 300)); + + // Dialog should be gone. + expect(find.byKey(const Key('admin_create_user_dialog')), findsNothing); + + // Placeholder row is visible by tile key (id=0) and username text. + expect(find.byKey(const Key('admin_user_tile_0')), findsOneWidget); + expect(find.text('newuser'), findsOneWidget); + + // Now resolve the pending createUser call. + fakeClient.completeCreate(newUser); + await tester.pumpAndSettle(); + + // Placeholder (id=0) replaced by the real user (id=42). + expect(find.byKey(const Key('admin_user_tile_0')), findsNothing); + expect(find.byKey(const Key('admin_user_tile_42')), findsOneWidget); + expect(find.text('newuser'), findsOneWidget); + }); + testWidgets('reverts placeholder and shows error SnackBar on createUser failure', (tester) async { final fakeClient = _FakeApiClient() -- cgit v1.2.3