From 8911551e2c7322eeb80db0e30e5260899bf084a5 Mon Sep 17 00:00:00 2001 From: Ashwin Date: Fri, 7 Aug 2026 08:45:36 +0530 Subject: [PATCH] feat: Implement unsaved changes guard in forms - Added UnsavedChangesGuard widget to prevent accidental loss of unsaved changes when navigating away from forms. - Integrated unsaved changes detection in TicketFormScreen and ProfileScreen. - Updated TicketFormScreen to compare current form state against a baseline to track unsaved changes. - Enhanced user experience by prompting for confirmation when attempting to leave a form with unsaved changes. - Refactored ticket submission logic to ensure proper validation and error handling. - Introduced TokenStorage service for secure storage of JWTs, migrating from SharedPreferences to FlutterSecureStorage. - Updated AuthService to utilize TokenStorage for managing access and refresh tokens. - Added unit tests for TokenStorage to ensure proper functionality and migration of tokens. - Created tests for unsaved changes functionality to verify correct behavior in various scenarios. --- .../android/app/src/main/AndroidManifest.xml | 11 + .../lib/screens/auth/org_create_screen.dart | 183 +++++++------- .../lib/screens/deals/deal_form_screen.dart | 57 +---- .../lib/screens/leads/lead_form_screen.dart | 141 ++++------- .../lib/screens/leads/leads_list_screen.dart | 18 +- .../lib/screens/settings/profile_screen.dart | 78 +++--- .../lib/screens/tasks/task_form_screen.dart | 68 +---- .../screens/tickets/ticket_form_screen.dart | 66 +++-- .../screens/tickets/tickets_list_screen.dart | 24 +- mobile/lib/services/auth_service.dart | 46 ++-- mobile/lib/services/token_storage.dart | 121 +++++++++ mobile/lib/widgets/forms/unsaved_changes.dart | 100 ++++++++ mobile/pubspec.lock | 48 ++++ mobile/pubspec.yaml | 3 + .../screens/auth/org_create_screen_test.dart | 40 ++- .../leads/leads_count_header_test.dart | 71 ++++++ .../screens/settings/profile_screen_test.dart | 86 +++++++ mobile/test/unit/token_storage_test.dart | 234 ++++++++++++++++++ mobile/test/widgets/unsaved_changes_test.dart | 133 ++++++++++ 19 files changed, 1153 insertions(+), 375 deletions(-) create mode 100644 mobile/lib/services/token_storage.dart create mode 100644 mobile/lib/widgets/forms/unsaved_changes.dart create mode 100644 mobile/test/screens/leads/leads_count_header_test.dart create mode 100644 mobile/test/screens/settings/profile_screen_test.dart create mode 100644 mobile/test/unit/token_storage_test.dart create mode 100644 mobile/test/widgets/unsaved_changes_test.dart diff --git a/mobile/android/app/src/main/AndroidManifest.xml b/mobile/android/app/src/main/AndroidManifest.xml index 49ee3232..528c0a58 100644 --- a/mobile/android/app/src/main/AndroidManifest.xml +++ b/mobile/android/app/src/main/AndroidManifest.xml @@ -1,8 +1,19 @@ + { context.go(AppRoutes.dashboard); } + /// The timezone is a guess the screen made, not something the user typed, + /// so only a name counts as work worth keeping. + bool get _hasUnsavedChanges => _nameController.text.trim().isNotEmpty; + @override Widget build(BuildContext context) { - return Scaffold( - backgroundColor: Colors.grey[50], - appBar: AppBar( - title: const Text('New Organization'), - backgroundColor: Colors.white, - foregroundColor: Colors.black, - elevation: 0, - ), - body: SafeArea( - child: SingleChildScrollView( - padding: const EdgeInsets.all(16), - child: Form( - key: _formKey, - child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - TextFormField( - controller: _nameController, - autofocus: true, - textCapitalization: TextCapitalization.words, - enabled: !_submitting, - decoration: const InputDecoration( - labelText: 'Organization name', - hintText: 'e.g. Acme Inc.', - border: OutlineInputBorder(), - ), - validator: (value) { - final name = value?.trim() ?? ''; - if (name.isEmpty) return 'Give the organization a name'; - // Mirrors the server's `validate_name`, which is the rule - // that actually decides. This only saves a round trip. - if (RegExp(r'''[~!@#$%^&*()+{}":;'/\[\]]''').hasMatch(name)) { - return 'Letters, numbers, spaces, hyphens and dots only'; - } - return null; - }, - ), - const SizedBox(height: 20), - if (_loadingZones) - const Padding( - padding: EdgeInsets.symmetric(vertical: 8), - child: LinearProgressIndicator(), - ) - else - DropdownButtonFormField( - initialValue: _timezone, - isExpanded: true, + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _submitting, + child: Scaffold( + backgroundColor: Colors.grey[50], + appBar: AppBar( + title: const Text('New Organization'), + backgroundColor: Colors.white, + foregroundColor: Colors.black, + elevation: 0, + ), + body: SafeArea( + child: SingleChildScrollView( + padding: const EdgeInsets.all(16), + child: Form( + key: _formKey, + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + TextFormField( + controller: _nameController, + autofocus: true, + textCapitalization: TextCapitalization.words, + enabled: !_submitting, decoration: const InputDecoration( - labelText: 'Time zone', + labelText: 'Organization name', + hintText: 'e.g. Acme Inc.', border: OutlineInputBorder(), ), - items: _zones - .map( - (zone) => DropdownMenuItem( - value: zone.name, - child: Text( - zone.label, - overflow: TextOverflow.ellipsis, + validator: (value) { + final name = value?.trim() ?? ''; + if (name.isEmpty) return 'Give the organization a name'; + // Mirrors the server's `validate_name`, which is the rule + // that actually decides. This only saves a round trip. + if (RegExp( + r'''[~!@#$%^&*()+{}":;'/\[\]]''', + ).hasMatch(name)) { + return 'Letters, numbers, spaces, hyphens and dots only'; + } + return null; + }, + ), + const SizedBox(height: 20), + if (_loadingZones) + const Padding( + padding: EdgeInsets.symmetric(vertical: 8), + child: LinearProgressIndicator(), + ) + else + DropdownButtonFormField( + initialValue: _timezone, + isExpanded: true, + decoration: const InputDecoration( + labelText: 'Time zone', + border: OutlineInputBorder(), + ), + items: _zones + .map( + (zone) => DropdownMenuItem( + value: zone.name, + child: Text( + zone.label, + overflow: TextOverflow.ellipsis, + ), ), - ), - ) - .toList(), - onChanged: _submitting - ? null - : (value) { - if (value != null) { - setState(() => _timezone = value); - } - }, - ), - const SizedBox(height: 6), - Text( - 'Sets when a day starts here, so "due today" and "overdue" ' - 'mean what your team expects. You can change it later in ' - 'Settings.', - style: TextStyle(fontSize: 12, color: Colors.grey[600]), - ), - const SizedBox(height: 24), - SizedBox( - width: double.infinity, - child: FilledButton( - onPressed: _submitting ? null : _submit, - child: _submitting - ? const SizedBox( - height: 18, - width: 18, - child: CircularProgressIndicator(strokeWidth: 2), ) - : const Text('Create organization'), + .toList(), + onChanged: _submitting + ? null + : (value) { + if (value != null) { + setState(() => _timezone = value); + } + }, + ), + const SizedBox(height: 6), + Text( + 'Sets when a day starts here, so "due today" and "overdue" ' + 'mean what your team expects. You can change it later in ' + 'Settings.', + style: TextStyle(fontSize: 12, color: Colors.grey[600]), ), - ), - ], + const SizedBox(height: 24), + SizedBox( + width: double.infinity, + child: FilledButton( + onPressed: _submitting ? null : _submit, + child: _submitting + ? const SizedBox( + height: 18, + width: 18, + child: CircularProgressIndicator(strokeWidth: 2), + ) + : const Text('Create organization'), + ), + ), + ], + ), ), ), ), diff --git a/mobile/lib/screens/deals/deal_form_screen.dart b/mobile/lib/screens/deals/deal_form_screen.dart index 1987df7e..8f2e9230 100644 --- a/mobile/lib/screens/deals/deal_form_screen.dart +++ b/mobile/lib/screens/deals/deal_form_screen.dart @@ -9,6 +9,7 @@ import '../../providers/auth_provider.dart'; import '../../providers/deals_provider.dart'; import '../../providers/lookup_provider.dart'; import '../../widgets/common/common.dart'; +import '../../widgets/forms/unsaved_changes.dart'; import '../../widgets/forms/custom_fields_form.dart'; /// Deal Form Screen - Reusable for both Create and Edit @@ -282,35 +283,6 @@ class _DealFormScreenState extends ConsumerState { return true; } - Future _onWillPop() async { - if (!_hasUnsavedChanges) return true; - - final result = await showDialog( - context: context, - builder: (context) => AlertDialog( - title: const Text('Discard changes?'), - content: const Text( - 'You have unsaved changes. Are you sure you want to leave?', - ), - actions: [ - TextButton( - onPressed: () => Navigator.pop(context, false), - child: const Text('Cancel'), - ), - TextButton( - onPressed: () => Navigator.pop(context, true), - child: Text( - 'Discard', - style: TextStyle(color: AppColors.danger600), - ), - ), - ], - ), - ); - - return result ?? false; - } - Deal _buildDeal() { final amount = double.tryParse(_amountController.text.trim()) ?? 0.0; final probability = @@ -533,18 +505,9 @@ class _DealFormScreenState extends ConsumerState { @override Widget build(BuildContext context) { - return PopScope( - // While saving, swallow back navigation so the in-flight request doesn't - // get orphaned by the form unmounting. - canPop: !_hasUnsavedChanges && !_isLoading, - onPopInvokedWithResult: (didPop, result) async { - if (didPop) return; - if (_isLoading) return; - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - }, + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _isLoading, child: Scaffold( backgroundColor: AppColors.surface, appBar: AppBar( @@ -556,16 +519,8 @@ class _DealFormScreenState extends ConsumerState { icon: const Icon(LucideIcons.chevronLeft), onPressed: _isLoading ? null - : () async { - if (_hasUnsavedChanges) { - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - } else { - context.pop(); - } - }, + : () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), ), ), body: _buildBody(), diff --git a/mobile/lib/screens/leads/lead_form_screen.dart b/mobile/lib/screens/leads/lead_form_screen.dart index 791c7d1f..0c105949 100644 --- a/mobile/lib/screens/leads/lead_form_screen.dart +++ b/mobile/lib/screens/leads/lead_form_screen.dart @@ -10,6 +10,7 @@ import '../../providers/auth_provider.dart'; import '../../providers/leads_provider.dart'; import '../../providers/lookup_provider.dart'; import '../../widgets/common/common.dart'; +import '../../widgets/forms/unsaved_changes.dart'; // Salutation choices match the web form's hard-coded list and the backend's // LEAD_SALUTATION column (free-text, but the UI restricts to these values). @@ -614,8 +615,9 @@ class _LeadFormScreenState extends ConsumerState { ? _formatAmount(lead.opportunityAmount!) : ''; _currency = lead.currency ?? ref.read(selectedOrgProvider)?.defaultCurrency; - _probabilityController.text = - lead.probability != null ? lead.probability.toString() : ''; + _probabilityController.text = lead.probability != null + ? lead.probability.toString() + : ''; _closeDate = lead.closeDate; _addressLineController.text = lead.addressLine ?? ''; _cityController.text = lead.city ?? ''; @@ -676,8 +678,7 @@ class _LeadFormScreenState extends ConsumerState { ? _formatAmount(lead.opportunityAmount!) : '') || (_currency ?? '') != (lead.currency ?? '') || - _probabilityController.text != - (lead.probability?.toString() ?? '') || + _probabilityController.text != (lead.probability?.toString() ?? '') || _closeDate != lead.closeDate || _addressLineController.text != (lead.addressLine ?? '') || _cityController.text != (lead.city ?? '') || @@ -721,35 +722,6 @@ class _LeadFormScreenState extends ConsumerState { return true; } - Future _onWillPop() async { - if (!_hasUnsavedChanges) return true; - - final result = await showDialog( - context: context, - builder: (context) => AlertDialog( - title: const Text('Discard changes?'), - content: const Text( - 'You have unsaved changes. Are you sure you want to leave?', - ), - actions: [ - TextButton( - onPressed: () => Navigator.pop(context, false), - child: const Text('Cancel'), - ), - TextButton( - onPressed: () => Navigator.pop(context, true), - child: Text( - 'Discard', - style: TextStyle(color: AppColors.danger600), - ), - ), - ], - ), - ); - - return result ?? false; - } - Map _buildPayload() { final payload = { 'first_name': _firstNameController.text.trim(), @@ -801,8 +773,7 @@ class _LeadFormScreenState extends ConsumerState { final phone = _phoneController.text.trim(); if (phone.isNotEmpty) { payload['phone'] = phone; - } else if (widget.isEditMode && - (_existingLead?.phone ?? '').isNotEmpty) { + } else if (widget.isEditMode && (_existingLead?.phone ?? '').isNotEmpty) { // User cleared the phone in edit mode, null clears the column. payload['phone'] = null; } @@ -1016,18 +987,9 @@ class _LeadFormScreenState extends ConsumerState { @override Widget build(BuildContext context) { - return PopScope( - // While saving, swallow back navigation so the in-flight request - // doesn't get orphaned by the form unmounting. - canPop: !_hasUnsavedChanges && !_isLoading, - onPopInvokedWithResult: (didPop, result) async { - if (didPop) return; - if (_isLoading) return; - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - }, + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _isLoading, child: Scaffold( backgroundColor: AppColors.surface, appBar: AppBar( @@ -1039,16 +1001,8 @@ class _LeadFormScreenState extends ConsumerState { icon: const Icon(LucideIcons.chevronLeft), onPressed: _isLoading ? null - : () async { - if (_hasUnsavedChanges) { - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - } else { - context.pop(); - } - }, + : () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), ), ), body: _buildBody(), @@ -1347,9 +1301,8 @@ class _LeadFormScreenState extends ConsumerState { maxLength: 50, textCapitalization: TextCapitalization.words, textInputAction: TextInputAction.next, - validator: (v) => (v == null || v.trim().isEmpty) - ? 'Last name is required' - : null, + validator: (v) => + (v == null || v.trim().isEmpty) ? 'Last name is required' : null, ), ], ); @@ -1393,9 +1346,7 @@ class _LeadFormScreenState extends ConsumerState { keyboardType: TextInputType.phone, maxLength: 20, textInputAction: TextInputAction.next, - inputFormatters: [ - FilteringTextInputFormatter.allow(_phoneAllowed), - ], + inputFormatters: [FilteringTextInputFormatter.allow(_phoneAllowed)], onSubmitted: (_) => _normalizePhoneInPlace(), validator: (value) { final v = (value ?? '').trim(); @@ -1520,9 +1471,7 @@ class _LeadFormScreenState extends ConsumerState { _duplicateLeadId = null; _duplicateLeadLabel = null; }); - final match = await ref - .read(leadsProvider.notifier) - .findLeadByEmail(email); + final match = await ref.read(leadsProvider.notifier).findLeadByEmail(email); if (!mounted) return; setState(() { _checkingDuplicate = false; @@ -1718,9 +1667,9 @@ class _LeadFormScreenState extends ConsumerState { onTap: _isLoading ? null : () => _pickDate( - initial: _closeDate, - onSelect: (d) => setState(() => _closeDate = d), - ), + initial: _closeDate, + onSelect: (d) => setState(() => _closeDate = d), + ), onClear: _closeDate == null || _isLoading ? null : () => setState(() => _closeDate = null), @@ -1824,10 +1773,10 @@ class _LeadFormScreenState extends ConsumerState { onTap: _isLoading ? null : () => _pickDate( - initial: _lastContacted, - onSelect: (d) => setState(() => _lastContacted = d), - lastDate: DateTime.now(), - ), + initial: _lastContacted, + onSelect: (d) => setState(() => _lastContacted = d), + lastDate: DateTime.now(), + ), onClear: _lastContacted == null || _isLoading ? null : () => setState(() => _lastContacted = null), @@ -1841,9 +1790,9 @@ class _LeadFormScreenState extends ConsumerState { onTap: _isLoading ? null : () => _pickDate( - initial: _nextFollowUp, - onSelect: (d) => setState(() => _nextFollowUp = d), - ), + initial: _nextFollowUp, + onSelect: (d) => setState(() => _nextFollowUp = d), + ), onClear: _nextFollowUp == null || _isLoading ? null : () => setState(() => _nextFollowUp = null), @@ -1940,8 +1889,8 @@ class _LeadFormScreenState extends ConsumerState { onChanged: (v) { _customFieldValues[def.key] = def.fieldType == CustomFieldType.number - ? (v.trim().isEmpty ? null : v.trim()) - : v; + ? (v.trim().isEmpty ? null : v.trim()) + : v; }, ); case CustomFieldType.textarea: @@ -1968,11 +1917,11 @@ class _LeadFormScreenState extends ConsumerState { onTap: _isLoading ? null : () => _pickDate( - initial: dt, - onSelect: (d) => setState(() { - _customFieldValues[def.key] = _formatDate(d); - }), - ), + initial: dt, + onSelect: (d) => setState(() { + _customFieldValues[def.key] = _formatDate(d); + }), + ), onClear: dt == null || _isLoading ? null : () => setState(() => _customFieldValues[def.key] = null), @@ -1986,9 +1935,7 @@ class _LeadFormScreenState extends ConsumerState { return _PickerField( label: label, value: selected.value.isEmpty ? 'Select' : selected.label, - onTap: _isLoading - ? null - : () => _showCustomDropdownPicker(def), + onTap: _isLoading ? null : () => _showCustomDropdownPicker(def), ); case CustomFieldType.checkbox: final value = _customFieldValues[def.key] == true; @@ -2159,8 +2106,9 @@ class _LeadFormScreenState extends ConsumerState { color: isSelected ? Colors.white : AppColors.textSecondary, - fontWeight: - isSelected ? FontWeight.w600 : FontWeight.w500, + fontWeight: isSelected + ? FontWeight.w600 + : FontWeight.w500, ), ), ), @@ -2261,8 +2209,9 @@ class _LeadFormScreenState extends ConsumerState { } void _showSourcePicker() { - final sources = - LeadSource.values.where((s) => s != LeadSource.none).toList(); + final sources = LeadSource.values + .where((s) => s != LeadSource.none) + .toList(); showModalBottomSheet( context: context, backgroundColor: AppColors.surface, @@ -2496,10 +2445,7 @@ class _PickerField extends StatelessWidget { ), child: Row( children: [ - if (leading != null) ...[ - leading!, - const SizedBox(width: 12), - ], + if (leading != null) ...[leading!, const SizedBox(width: 12)], Expanded( child: Text( value, @@ -2609,8 +2555,7 @@ class _SearchablePickerSheet extends StatefulWidget { }); @override - State<_SearchablePickerSheet> createState() => - _SearchablePickerSheetState(); + State<_SearchablePickerSheet> createState() => _SearchablePickerSheetState(); } class _SearchablePickerSheetState extends State<_SearchablePickerSheet> { @@ -2622,8 +2567,8 @@ class _SearchablePickerSheetState extends State<_SearchablePickerSheet> { final filtered = q.isEmpty ? widget.options : widget.options - .where((o) => o.label.toLowerCase().contains(q)) - .toList(); + .where((o) => o.label.toLowerCase().contains(q)) + .toList(); return DraggableScrollableSheet( initialChildSize: 0.7, diff --git a/mobile/lib/screens/leads/leads_list_screen.dart b/mobile/lib/screens/leads/leads_list_screen.dart index 1a484079..5ea0b194 100644 --- a/mobile/lib/screens/leads/leads_list_screen.dart +++ b/mobile/lib/screens/leads/leads_list_screen.dart @@ -97,7 +97,7 @@ class _LeadsListScreenState extends ConsumerState { children: [ _buildSearchBar(), _buildFilterBar(), - _buildResultsCount(leads.length, data?.totalCount ?? 0), + _buildResultsCount(leadsAsync, leads.length, data?.totalCount ?? 0), Expanded(child: _buildLeadsList(leadsAsync, leads)), ], ), @@ -306,11 +306,25 @@ class _LeadsListScreenState extends ConsumerState { ); } - Widget _buildResultsCount(int loadedCount, int totalCount) { + Widget _buildResultsCount( + AsyncValue async, + int loadedCount, + int totalCount, + ) { + // Until the first page lands there is no count to state. Printing + // "0 leads" over a spinner reads as an empty org, which is what made a + // failed load take a minute to tell apart from an empty list. + if (async.value == null) { + return _countStrip(async.hasError ? '' : 'Loading'); + } final isFiltered = _filters.isActive; final label = isFiltered ? '$loadedCount of $totalCount lead${totalCount == 1 ? '' : 's'} (filtered)' : '$totalCount lead${totalCount == 1 ? '' : 's'}'; + return _countStrip(label); + } + + Widget _countStrip(String label) { return Container( width: double.infinity, color: AppColors.surfaceDim, diff --git a/mobile/lib/screens/settings/profile_screen.dart b/mobile/lib/screens/settings/profile_screen.dart index 73529203..7d2f4ab4 100644 --- a/mobile/lib/screens/settings/profile_screen.dart +++ b/mobile/lib/screens/settings/profile_screen.dart @@ -1,12 +1,12 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; -import 'package:go_router/go_router.dart'; import 'package:intl/intl.dart'; import 'package:lucide_icons_flutter/lucide_icons.dart'; import '../../core/theme/theme.dart'; import '../../data/models/profile.dart'; import '../../providers/profile_provider.dart'; import '../../widgets/common/common.dart'; +import '../../widgets/forms/unsaved_changes.dart'; /// Profile view/edit screen. /// @@ -38,6 +38,17 @@ class _ProfileScreenState extends ConsumerState { setState(() => _editing = true); } + /// Only counts while the fields are actually on screen. Outside edit mode + /// the controllers still hold whatever was last typed, and prompting about + /// that on the way out would be nonsense. + bool get _hasUnsavedChanges { + if (!_editing) return false; + final profile = ref.read(profileProvider).value; + if (profile == null) return false; + return _nameController.text.trim() != (profile.user.name ?? '') || + _phoneController.text.trim() != (profile.phone ?? ''); + } + void _cancelEdit() { FocusScope.of(context).unfocus(); setState(() => _editing = false); @@ -78,34 +89,41 @@ class _ProfileScreenState extends ConsumerState { @override Widget build(BuildContext context) { final async = ref.watch(profileProvider); - return Scaffold( - backgroundColor: AppColors.surfaceDim, - appBar: AppBar( - title: const Text('Profile'), - backgroundColor: AppColors.surface, - elevation: 0, - scrolledUnderElevation: 1, - leading: IconButton( - icon: const Icon(LucideIcons.chevronLeft), - onPressed: () => context.pop(), - ), - actions: [ - if (async.value != null && !_editing) - Padding( - padding: const EdgeInsets.only(right: 8), - child: IconButton( - onPressed: () => _enterEdit(async.value!), - icon: const Icon(LucideIcons.edit3, size: 20), - tooltip: 'Edit profile', - color: AppColors.primary600, + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _saving, + child: Scaffold( + backgroundColor: AppColors.surfaceDim, + appBar: AppBar( + title: const Text('Profile'), + backgroundColor: AppColors.surface, + elevation: 0, + scrolledUnderElevation: 1, + leading: IconButton( + icon: const Icon(LucideIcons.chevronLeft), + onPressed: _saving + ? null + : () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), + ), + actions: [ + if (async.value != null && !_editing) + Padding( + padding: const EdgeInsets.only(right: 8), + child: IconButton( + onPressed: () => _enterEdit(async.value!), + icon: const Icon(LucideIcons.edit3, size: 20), + tooltip: 'Edit profile', + color: AppColors.primary600, + ), ), - ), - ], - ), - body: async.when( - loading: () => const Center(child: CircularProgressIndicator()), - error: (e, _) => _buildError(e), - data: (profile) => _buildBody(profile), + ], + ), + body: async.when( + loading: () => const Center(child: CircularProgressIndicator()), + error: (e, _) => _buildError(e), + data: (profile) => _buildBody(profile), + ), ), ); } @@ -251,7 +269,9 @@ class _ProfileScreenState extends ConsumerState { _InfoRow( icon: LucideIcons.phone, label: 'Phone', - value: profile.phone?.trim().isNotEmpty == true ? profile.phone! : '—', + value: profile.phone?.trim().isNotEmpty == true + ? profile.phone! + : '—', ), _sectionHeader('Organization'), if (profile.org != null) diff --git a/mobile/lib/screens/tasks/task_form_screen.dart b/mobile/lib/screens/tasks/task_form_screen.dart index c3d69f3e..3d5515e7 100644 --- a/mobile/lib/screens/tasks/task_form_screen.dart +++ b/mobile/lib/screens/tasks/task_form_screen.dart @@ -11,6 +11,7 @@ import '../../providers/lookup_provider.dart'; import '../../providers/tasks_provider.dart'; import '../../providers/tickets_provider.dart'; import '../../widgets/common/common.dart'; +import '../../widgets/forms/unsaved_changes.dart'; import '../../widgets/forms/custom_fields_form.dart'; import '../../widgets/forms/multi_select_sheet.dart'; @@ -211,35 +212,6 @@ class _TaskFormScreenState extends ConsumerState { return true; } - Future _onWillPop() async { - if (!_hasUnsavedChanges) return true; - - final result = await showDialog( - context: context, - builder: (context) => AlertDialog( - title: const Text('Discard changes?'), - content: const Text( - 'You have unsaved changes. Are you sure you want to leave?', - ), - actions: [ - TextButton( - onPressed: () => Navigator.pop(context, false), - child: const Text('Cancel'), - ), - TextButton( - onPressed: () => Navigator.pop(context, true), - child: Text( - 'Discard', - style: TextStyle(color: AppColors.danger600), - ), - ), - ], - ), - ); - - return result ?? false; - } - Map _buildPayload() { final payload = { 'title': _titleController.text.trim(), @@ -302,15 +274,9 @@ class _TaskFormScreenState extends ConsumerState { @override Widget build(BuildContext context) { - return PopScope( - canPop: !_hasUnsavedChanges, - onPopInvokedWithResult: (didPop, result) async { - if (didPop) return; - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - }, + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _isLoading, child: Scaffold( backgroundColor: AppColors.surface, appBar: AppBar( @@ -320,16 +286,10 @@ class _TaskFormScreenState extends ConsumerState { scrolledUnderElevation: 1, leading: IconButton( icon: const Icon(LucideIcons.chevronLeft), - onPressed: () async { - if (_hasUnsavedChanges) { - final shouldPop = await _onWillPop(); - if (shouldPop && context.mounted) { - context.pop(); - } - } else { - context.pop(); - } - }, + onPressed: _isLoading + ? null + : () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), ), ), body: _buildBody(), @@ -434,16 +394,8 @@ class _TaskFormScreenState extends ConsumerState { // Cancel Button Center( child: GestureDetector( - onTap: () async { - if (_hasUnsavedChanges) { - final shouldPop = await _onWillPop(); - if (shouldPop && mounted) { - context.pop(); - } - } else { - context.pop(); - } - }, + onTap: () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), child: Text( 'Cancel', style: AppTypography.label.copyWith( diff --git a/mobile/lib/screens/tickets/ticket_form_screen.dart b/mobile/lib/screens/tickets/ticket_form_screen.dart index 3a7cd535..63b67427 100644 --- a/mobile/lib/screens/tickets/ticket_form_screen.dart +++ b/mobile/lib/screens/tickets/ticket_form_screen.dart @@ -10,6 +10,7 @@ import '../../providers/lookup_provider.dart'; import '../../widgets/common/common.dart'; import '../../widgets/forms/custom_fields_form.dart'; import '../../widgets/forms/multi_select_sheet.dart'; +import '../../widgets/forms/unsaved_changes.dart'; /// Shared create / edit form for tickets. class TicketFormScreen extends ConsumerStatefulWidget { @@ -44,9 +45,25 @@ class _TicketFormScreenState extends ConsumerState { @override void initState() { super.initState(); - if (widget.isEditMode) _fetchTicket(); + if (widget.isEditMode) { + _fetchTicket(); + } else { + _baseline = _snapshot(); + } } + /// The form as it looked when it finished loading. Comparing against the + /// payload the form would submit means a field added later is covered + /// automatically, unlike a hand-listed comparison that has to be remembered. + String? _baseline; + + /// `toString` rather than `jsonEncode`, which throws on the DateTime and + /// enum values these payloads can carry. Key order is stable because the + /// same builder produces every snapshot. + String _snapshot() => _buildPayload().toString(); + + bool get _hasUnsavedChanges => _baseline != null && _snapshot() != _baseline; + @override void dispose() { _nameController.dispose(); @@ -80,6 +97,9 @@ class _TicketFormScreenState extends ConsumerState { _fetchError = 'Failed to load ticket'; } }); + // After the fields hold the saved values, so an untouched edit form is + // clean rather than dirty against an empty baseline. + _baseline = _snapshot(); } Map _buildPayload() { @@ -111,8 +131,7 @@ class _TicketFormScreenState extends ConsumerState { Future _submit() async { if (!_formKey.currentState!.validate()) return; - if (!widget.isEditMode && - (_accountId == null || _accountId!.isEmpty)) { + if (!widget.isEditMode && (_accountId == null || _accountId!.isEmpty)) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: const Text('Please select an account.'), @@ -125,7 +144,9 @@ class _TicketFormScreenState extends ConsumerState { if (_status == TicketStatus.closed && _closedOn == null) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: const Text('Please set a closed-on date for closed tickets.'), + content: const Text( + 'Please set a closed-on date for closed tickets.', + ), backgroundColor: AppColors.danger600, behavior: SnackBarBehavior.floating, ), @@ -166,19 +187,26 @@ class _TicketFormScreenState extends ConsumerState { @override Widget build(BuildContext context) { - return Scaffold( - backgroundColor: AppColors.surface, - appBar: AppBar( - title: Text(widget.isEditMode ? 'Edit Ticket' : 'New Ticket'), + return UnsavedChangesGuard( + hasUnsavedChanges: () => _hasUnsavedChanges, + isSaving: _isLoading, + child: Scaffold( backgroundColor: AppColors.surface, - elevation: 0, - scrolledUnderElevation: 1, - leading: IconButton( - icon: const Icon(LucideIcons.chevronLeft), - onPressed: () => context.pop(), + appBar: AppBar( + title: Text(widget.isEditMode ? 'Edit Ticket' : 'New Ticket'), + backgroundColor: AppColors.surface, + elevation: 0, + scrolledUnderElevation: 1, + leading: IconButton( + icon: const Icon(LucideIcons.chevronLeft), + onPressed: _isLoading + ? null + : () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), + ), ), + body: _buildBody(), ), - body: _buildBody(), ); } @@ -307,7 +335,8 @@ class _TicketFormScreenState extends ConsumerState { const SizedBox(height: 16), Center( child: GestureDetector( - onTap: () => context.pop(), + onTap: () => + leaveForm(context, hasUnsavedChanges: _hasUnsavedChanges), child: Text( 'Cancel', style: AppTypography.label.copyWith( @@ -327,9 +356,7 @@ class _TicketFormScreenState extends ConsumerState { // Renders nothing if the org has no Case custom fields. return Consumer( builder: (context, ref, _) { - final asyncDefs = ref.watch( - customFieldDefinitionsProvider('Case'), - ); + final asyncDefs = ref.watch(customFieldDefinitionsProvider('Case')); final defs = asyncDefs.value ?? const []; if (defs.isEmpty) return const SizedBox.shrink(); return Column( @@ -497,8 +524,7 @@ class _TicketFormScreenState extends ConsumerState { spacing: 6, runSpacing: 4, children: [ - for (final l in selectedLabels) - LabelPill(label: l), + for (final l in selectedLabels) LabelPill(label: l), ], ), ), diff --git a/mobile/lib/screens/tickets/tickets_list_screen.dart b/mobile/lib/screens/tickets/tickets_list_screen.dart index 187b16d1..e79bf930 100644 --- a/mobile/lib/screens/tickets/tickets_list_screen.dart +++ b/mobile/lib/screens/tickets/tickets_list_screen.dart @@ -152,7 +152,11 @@ class _TicketsListScreenState extends ConsumerState { _buildSearchBar(), _buildQuickStatusChips(), _buildFilterBar(), - _buildResultsCount(tickets.length, data?.totalCount ?? 0), + _buildResultsCount( + ticketsAsync, + tickets.length, + data?.totalCount ?? 0, + ), Expanded(child: _buildList(ticketsAsync, tickets)), ], ), @@ -487,17 +491,31 @@ class _TicketsListScreenState extends ConsumerState { return 'Before ${fmt(before!)}'; } - Widget _buildResultsCount(int loadedCount, int totalCount) { + Widget _buildResultsCount( + AsyncValue async, + int loadedCount, + int totalCount, + ) { + // Until the first page lands there is no count to state. Printing + // "0 tickets" over a spinner reads as an empty org, which is what made a + // failed load take a minute to tell apart from an empty list. + if (async.value == null) { + return _countStrip(async.hasError ? '' : 'Loading'); + } final unit = totalCount == 1 ? 'ticket' : 'tickets'; final text = totalCount > 0 && loadedCount < totalCount ? '$loadedCount of $totalCount $unit' : '$totalCount $unit'; + return _countStrip(_filters.hasAny ? '$text (filtered)' : text); + } + + Widget _countStrip(String label) { return Container( width: double.infinity, color: AppColors.surfaceDim, padding: const EdgeInsets.fromLTRB(12, 6, 12, 8), child: Text( - _filters.hasAny ? '$text (filtered)' : text, + label, style: AppTypography.caption.copyWith(color: AppColors.textSecondary), ), ); diff --git a/mobile/lib/services/auth_service.dart b/mobile/lib/services/auth_service.dart index 1497ef27..50b392ea 100644 --- a/mobile/lib/services/auth_service.dart +++ b/mobile/lib/services/auth_service.dart @@ -7,6 +7,7 @@ import '../config/api_config.dart'; import '../data/models/auth_response.dart'; import 'api_service.dart'; import 'crash_reporting.dart'; +import 'token_storage.dart'; /// Authentication service for BottleCRM /// @@ -21,9 +22,12 @@ class AuthService { final GoogleSignIn _googleSignIn = GoogleSignIn.instance; final ApiService _apiService = ApiService(); + /// The two JWTs are the only things that do not live in shared preferences. + /// See `token_storage.dart` for why, and for the migration that empties the + /// old location on upgrade. + final TokenStorage _tokenStorage = TokenStorage(); + // Storage keys - static const String _accessTokenKey = 'jwt_token'; - static const String _refreshTokenKey = 'refresh_token'; static const String _userKey = 'user_data'; static const String _organizationsKey = 'organizations'; static const String _selectedOrgKey = 'selected_organization'; @@ -166,11 +170,10 @@ class AuthService { Future requestMagicCode(String email) async { try { debugPrint('AuthService: Requesting magic code for $email...'); - final response = await _apiService.post( - ApiConfig.magicLinkRequest, - {'email': email, 'delivery': 'code'}, - requiresAuth: false, - ); + final response = await _apiService.post(ApiConfig.magicLinkRequest, { + 'email': email, + 'delivery': 'code', + }, requiresAuth: false); return response.success; } catch (e) { debugPrint('AuthService: requestMagicCode error: $e'); @@ -189,11 +192,10 @@ class AuthService { }) async { try { debugPrint('AuthService: Verifying magic code for $email...'); - final response = await _apiService.post( - ApiConfig.magicLinkVerifyCode, - {'email': email, 'code': code}, - requiresAuth: false, - ); + final response = await _apiService.post(ApiConfig.magicLinkVerifyCode, { + 'email': email, + 'code': code, + }, requiresAuth: false); if (!response.success || response.data == null) { debugPrint( @@ -223,7 +225,8 @@ class AuthService { // The nested read is kept as a fallback in case an older build of the // API is on the other end. final orgsList = - (data['organizations'] ?? userData?['organizations']) as List?; + (data['organizations'] ?? userData?['organizations']) + as List?; _organizations = orgsList ?.map((org) => Organization.fromJson(org as Map)) .toList(); @@ -483,8 +486,9 @@ class AuthService { try { final prefs = await SharedPreferences.getInstance(); - _accessToken = prefs.getString(_accessTokenKey); - _refreshToken = prefs.getString(_refreshTokenKey); + final tokens = await _tokenStorage.read(); + _accessToken = tokens.access; + _refreshToken = tokens.refresh; final userJson = prefs.getString(_userKey); if (userJson != null) { @@ -524,12 +528,8 @@ class AuthService { try { final prefs = await SharedPreferences.getInstance(); - if (_accessToken != null) { - await prefs.setString(_accessTokenKey, _accessToken!); - } - if (_refreshToken != null) { - await prefs.setString(_refreshTokenKey, _refreshToken!); - } + await _tokenStorage.write(access: _accessToken, refresh: _refreshToken); + if (_currentUser != null) { await prefs.setString( _userKey, @@ -574,9 +574,9 @@ class AuthService { /// Clear all stored authentication data Future _clearStorage() async { try { + await _tokenStorage.clear(); + final prefs = await SharedPreferences.getInstance(); - await prefs.remove(_accessTokenKey); - await prefs.remove(_refreshTokenKey); await prefs.remove(_userKey); await prefs.remove(_organizationsKey); await prefs.remove(_selectedOrgKey); diff --git a/mobile/lib/services/token_storage.dart b/mobile/lib/services/token_storage.dart new file mode 100644 index 00000000..c4b244b7 --- /dev/null +++ b/mobile/lib/services/token_storage.dart @@ -0,0 +1,121 @@ +import 'package:flutter/foundation.dart'; +import 'package:flutter_secure_storage/flutter_secure_storage.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// Where the JWTs live. +/// +/// `mobile/CLAUDE.md` forbids putting tokens in `SharedPreferences` by name, +/// and until now that is exactly where both of them sat. On Android +/// `SharedPreferences` is a plain XML file in the app's data directory: +/// readable on a rooted device, and pullable through `adb backup` on any build +/// that has not turned backups off. A refresh token read out of it is good for +/// up to fourteen days, and no logout endpoint exists to cut it short. +/// +/// Secure storage means the Android Keystore and the iOS Keychain, so the +/// ciphertext on disk is useless without the hardware-backed key. +/// +/// Only the two tokens moved. The cached user, org list and selected org stay +/// in `SharedPreferences`: they are display data the UI already shows on +/// screen, and moving them would buy nothing while making every read slower. +class TokenStorage { + TokenStorage({FlutterSecureStorage? storage}) + : _storage = storage ?? const FlutterSecureStorage(); + + final FlutterSecureStorage _storage; + + static const String accessTokenKey = 'jwt_token'; + static const String refreshTokenKey = 'refresh_token'; + + /// Reads both tokens, migrating them out of `SharedPreferences` first if a + /// previous build left them there. + Future<({String? access, String? refresh})> read() async { + await _migrateLegacyTokens(); + return ( + access: await _read(accessTokenKey), + refresh: await _read(refreshTokenKey), + ); + } + + /// Writes both tokens. A null clears its key rather than leaving the + /// previous value behind, so a token can never outlive the session that + /// obtained it. + Future write({String? access, String? refresh}) async { + await _write(accessTokenKey, access); + await _write(refreshTokenKey, refresh); + } + + /// Clears both tokens, including any legacy copy, so signing out on a + /// device that upgraded mid-session does not leave a usable refresh token + /// in the old location. + Future clear() async { + await _write(accessTokenKey, null); + await _write(refreshTokenKey, null); + await _removeLegacyTokens(); + } + + /// Moves tokens written by a pre-secure-storage build, then deletes the + /// originals. + /// + /// The delete is the point of the exercise. Copying alone would leave the + /// readable plaintext on disk forever on every device that ever ran an + /// older build, so the upgrade would fix new installs and no one else. + /// + /// Migration is skipped once secure storage holds an access token, so the + /// common path is one read rather than three. + Future _migrateLegacyTokens() async { + if (await _read(accessTokenKey) != null) return; + + final SharedPreferences prefs; + try { + prefs = await SharedPreferences.getInstance(); + } catch (e) { + debugPrint('TokenStorage: legacy check skipped: $e'); + return; + } + + final legacyAccess = prefs.getString(accessTokenKey); + final legacyRefresh = prefs.getString(refreshTokenKey); + if (legacyAccess == null && legacyRefresh == null) return; + + await _write(accessTokenKey, legacyAccess); + await _write(refreshTokenKey, legacyRefresh); + await prefs.remove(accessTokenKey); + await prefs.remove(refreshTokenKey); + debugPrint('TokenStorage: migrated tokens out of shared preferences'); + } + + Future _removeLegacyTokens() async { + try { + final prefs = await SharedPreferences.getInstance(); + await prefs.remove(accessTokenKey); + await prefs.remove(refreshTokenKey); + } catch (e) { + debugPrint('TokenStorage: legacy clear skipped: $e'); + } + } + + /// Secure storage talks to the Keystore and can fail: a corrupt entry after + /// an OS restore, or a locked device on iOS. A throw here would take out + /// app startup, so a failed read is treated as "no token", which lands the + /// user on the sign-in screen instead of a crash. + Future _read(String key) async { + try { + return await _storage.read(key: key); + } catch (e) { + debugPrint('TokenStorage: read failed for $key: $e'); + return null; + } + } + + Future _write(String key, String? value) async { + try { + if (value == null) { + await _storage.delete(key: key); + } else { + await _storage.write(key: key, value: value); + } + } catch (e) { + debugPrint('TokenStorage: write failed for $key: $e'); + } + } +} diff --git a/mobile/lib/widgets/forms/unsaved_changes.dart b/mobile/lib/widgets/forms/unsaved_changes.dart new file mode 100644 index 00000000..b41af33f --- /dev/null +++ b/mobile/lib/widgets/forms/unsaved_changes.dart @@ -0,0 +1,100 @@ +import 'package:flutter/material.dart'; +import 'package:go_router/go_router.dart'; + +import '../../core/theme/theme.dart'; + +/// Leaving a form that has been filled in. +/// +/// THE TRAP THIS EXISTS TO CLOSE +/// Three screens already carried a discard prompt and it fired on none of +/// them. They wrote `canPop: !_hasUnsavedChanges`, which reads the flag once, +/// while `build` runs. Typing into a `TextFormField` rebuilds that field and +/// not its parent, so on a form that started empty `canPop` stays `true` for +/// the rest of its life and the system back gesture pops before +/// `onPopInvoked` is ever consulted. The dirty check was correct and simply +/// never ran. +/// +/// So `canPop` is always `false` here and the decision happens in the +/// callback, where the state being read is the state as it is now. A form +/// cannot silently lose its guard by gaining a field that nobody remembered +/// to rebuild on. +/// +/// The cost is the iOS interactive swipe-back and the Android predictive-back +/// preview on these five screens. Worth it: a form is exactly where an +/// accidental edge swipe is expensive. +/// +/// Everything routes through `context.pop()`, which is a direct pop and so +/// bypasses `PopScope` entirely. That is what lets the AppBar chevron, the +/// Cancel link and a successful save all leave without re-asking. +class UnsavedChangesGuard extends StatelessWidget { + const UnsavedChangesGuard({ + super.key, + required this.hasUnsavedChanges, + required this.child, + this.isSaving = false, + }); + + /// Read when back is pressed, not when this widget builds. Pass a closure + /// over the form's live state, never a captured bool. + final bool Function() hasUnsavedChanges; + + /// While a save is in flight, back does nothing. Popping here would orphan + /// the request and leave the user unsure whether the record was created. + final bool isSaving; + + final Widget child; + + @override + Widget build(BuildContext context) { + return PopScope( + canPop: false, + onPopInvokedWithResult: (didPop, result) async { + if (didPop || isSaving) return; + await leaveForm(context, hasUnsavedChanges: hasUnsavedChanges()); + }, + child: child, + ); + } +} + +/// Leaves the form, asking first if there is anything to lose. +/// +/// Used by the AppBar chevron and the Cancel link as well as by the guard, so +/// all three routes out of a form behave the same. Before this, a screen could +/// prompt on the back gesture and discard silently on its own Cancel button. +Future leaveForm( + BuildContext context, { + required bool hasUnsavedChanges, +}) async { + if (!hasUnsavedChanges) { + context.pop(); + return; + } + final discard = await confirmDiscard(context); + if (discard && context.mounted) context.pop(); +} + +/// The prompt itself. Defaults to keeping the work: dismissing the dialog by +/// tapping outside returns null, which is read as "do not discard". +Future confirmDiscard(BuildContext context) async { + final result = await showDialog( + context: context, + builder: (context) => AlertDialog( + title: const Text('Discard changes?'), + content: const Text( + 'You have unsaved changes. Are you sure you want to leave?', + ), + actions: [ + TextButton( + onPressed: () => Navigator.pop(context, false), + child: const Text('Cancel'), + ), + TextButton( + onPressed: () => Navigator.pop(context, true), + child: Text('Discard', style: TextStyle(color: AppColors.danger600)), + ), + ], + ), + ); + return result ?? false; +} diff --git a/mobile/pubspec.lock b/mobile/pubspec.lock index 17212a5e..9b4d5021 100644 --- a/mobile/pubspec.lock +++ b/mobile/pubspec.lock @@ -275,6 +275,54 @@ packages: url: "https://pub.dev" source: hosted version: "3.4.1" + flutter_secure_storage: + dependency: "direct main" + description: + name: flutter_secure_storage + sha256: "7686b1d6a29985dcbb808c59518226e603e3bfa7c0ddfd1a0d00e4cda77c868e" + url: "https://pub.dev" + source: hosted + version: "10.3.1" + flutter_secure_storage_darwin: + dependency: transitive + description: + name: flutter_secure_storage_darwin + sha256: "82329fa5cdf343773b1b6897dea959105a29f092454259edff92f9f6637e8149" + url: "https://pub.dev" + source: hosted + version: "0.3.2" + flutter_secure_storage_linux: + dependency: transitive + description: + name: flutter_secure_storage_linux + sha256: "76fa9c841b3b1619fc5b5bc36efc7d158fa2356f223b6caeb1d0c80a54168546" + url: "https://pub.dev" + source: hosted + version: "3.0.2" + flutter_secure_storage_platform_interface: + dependency: transitive + description: + name: flutter_secure_storage_platform_interface + sha256: "788060052712555182aba55ecb5f8b6e5cb9cfe8f776c83249a61fe3ce877db4" + url: "https://pub.dev" + source: hosted + version: "2.0.3" + flutter_secure_storage_web: + dependency: transitive + description: + name: flutter_secure_storage_web + sha256: "073a62b3aeb866ab4ce795f960413948e51e5a42a9b0c8333b6daf5bb3208a1c" + url: "https://pub.dev" + source: hosted + version: "2.1.1" + flutter_secure_storage_windows: + dependency: transitive + description: + name: flutter_secure_storage_windows + sha256: "471951813a97006d899db4948acc654a4f28c440083ea08178935ce20b173ec1" + url: "https://pub.dev" + source: hosted + version: "4.2.2" flutter_test: dependency: "direct dev" description: flutter diff --git a/mobile/pubspec.yaml b/mobile/pubspec.yaml index 80781a9d..c903569a 100644 --- a/mobile/pubspec.yaml +++ b/mobile/pubspec.yaml @@ -16,6 +16,9 @@ dependencies: http: ^1.1.0 google_sign_in: ^7.1.1 shared_preferences: ^2.5.5 + # Tokens live here, never in shared_preferences. Keystore-backed on Android, + # Keychain on iOS. See lib/services/token_storage.dart. + flutter_secure_storage: ^10.3.1 jwt_decoder: ^2.0.1 package_info_plus: ^10.1.0 diff --git a/mobile/test/screens/auth/org_create_screen_test.dart b/mobile/test/screens/auth/org_create_screen_test.dart index e7b74099..98b4d3e0 100644 --- a/mobile/test/screens/auth/org_create_screen_test.dart +++ b/mobile/test/screens/auth/org_create_screen_test.dart @@ -3,6 +3,7 @@ import 'package:bottle_crm/providers/auth_provider.dart'; import 'package:bottle_crm/screens/auth/org_create_screen.dart'; import 'package:bottle_crm/screens/auth/org_selection_screen.dart'; import 'package:flutter/material.dart'; +import 'package:go_router/go_router.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -17,7 +18,9 @@ void main() { testWidgets('a user with no organizations is given the action', ( tester, ) async { - await tester.pumpWidget(_host(const OrgSelectionScreen(), orgs: const [])); + await tester.pumpWidget( + _host(const OrgSelectionScreen(), orgs: const []), + ); await tester.pump(); expect(find.text('Create an organization'), findsOneWidget); @@ -66,6 +69,32 @@ void main() { ); }); + testWidgets('back after typing a name asks first', (tester) async { + final router = GoRouter( + initialLocation: '/picker', + routes: [ + GoRoute(path: '/picker', builder: (_, _) => const Text('the picker')), + GoRoute(path: '/new', builder: (_, _) => const OrgCreateScreen()), + ], + ); + await tester.pumpWidget( + ProviderScope( + overrides: [authProvider.overrideWith(() => _FakeAuth(const []))], + child: MaterialApp.router(routerConfig: router), + ), + ); + await tester.pumpAndSettle(); + router.push('/new'); + await tester.pumpAndSettle(); + + await tester.enterText(find.byType(TextFormField), 'Acme Inc.'); + tester.state(find.byType(Navigator).last).maybePop(); + await tester.pumpAndSettle(); + + expect(find.text('Discard changes?'), findsOneWidget); + expect(find.text('the picker'), findsNothing); + }); + testWidgets('accepts an ordinary name', (tester) async { await tester.pumpWidget(_host(const OrgCreateScreen())); await tester.pump(); @@ -85,10 +114,11 @@ void main() { }); } -Widget _host(Widget screen, {List orgs = const []}) => ProviderScope( - overrides: [authProvider.overrideWith(() => _FakeAuth(orgs))], - child: MaterialApp(home: screen), -); +Widget _host(Widget screen, {List orgs = const []}) => + ProviderScope( + overrides: [authProvider.overrideWith(() => _FakeAuth(orgs))], + child: MaterialApp(home: screen), + ); class _FakeAuth extends AuthNotifier { _FakeAuth(this.orgs); diff --git a/mobile/test/screens/leads/leads_count_header_test.dart b/mobile/test/screens/leads/leads_count_header_test.dart new file mode 100644 index 00000000..96d215fd --- /dev/null +++ b/mobile/test/screens/leads/leads_count_header_test.dart @@ -0,0 +1,71 @@ +import 'dart:async'; + +import 'package:bottle_crm/providers/leads_provider.dart'; +import 'package:bottle_crm/screens/leads/leads_list_screen.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// The header stated a number before it had one. For the whole of a slow load, +/// and for the whole of a failed one, the screen read "0 leads" over a spinner, +/// which is indistinguishable from an org with no leads. It took a minute to +/// tell an offline phone from an empty list, and I misread it myself. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + Future pump( + WidgetTester tester, + LeadsNotifier Function() notifier, + ) async { + await tester.pumpWidget( + ProviderScope( + overrides: [leadsProvider.overrideWith(notifier)], + child: const MaterialApp(home: LeadsListScreen()), + ), + ); + await tester.pump(); + } + + testWidgets('while the first page is in flight it does not claim zero', ( + tester, + ) async { + await pump(tester, _StillLoading.new); + + expect(find.text('0 leads'), findsNothing); + expect(find.text('Loading'), findsOneWidget); + }); + + testWidgets('once the page lands it states the real count', (tester) async { + await pump(tester, () => _Loaded(7)); + await tester.pump(); + + // The control. A header that always said "Loading" would pass the test + // above and tell nobody anything. + expect(find.text('7 leads'), findsOneWidget); + expect(find.text('Loading'), findsNothing); + }); + + testWidgets('an org that really has none still says so', (tester) async { + await pump(tester, () => _Loaded(0)); + await tester.pump(); + + // "0 leads" is the right answer here, and the fix must not have made it + // unreachable. + expect(find.text('0 leads'), findsOneWidget); + }); +} + +class _StillLoading extends LeadsNotifier { + @override + Future build() => Completer().future; +} + +class _Loaded extends LeadsNotifier { + _Loaded(this.total); + + final int total; + + @override + Future build() async => + LeadsListData(totalCount: total, hasMore: false); +} diff --git a/mobile/test/screens/settings/profile_screen_test.dart b/mobile/test/screens/settings/profile_screen_test.dart new file mode 100644 index 00000000..0d8f8b80 --- /dev/null +++ b/mobile/test/screens/settings/profile_screen_test.dart @@ -0,0 +1,86 @@ +import 'package:bottle_crm/data/models/auth_response.dart'; +import 'package:bottle_crm/data/models/profile.dart'; +import 'package:bottle_crm/providers/profile_provider.dart'; +import 'package:bottle_crm/screens/settings/profile_screen.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; + +/// The profile screen is an edit form too, and it had the same hole as the +/// other five: back threw the edits away without asking. Its rule differs from +/// theirs, because the fields only exist while edit mode is on. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + Future openProfile(WidgetTester tester) async { + final router = GoRouter( + initialLocation: '/more', + routes: [ + GoRoute(path: '/more', builder: (_, _) => const Text('the more sheet')), + GoRoute(path: '/profile', builder: (_, _) => const ProfileScreen()), + ], + ); + await tester.pumpWidget( + ProviderScope( + overrides: [profileProvider.overrideWith(() => _FakeProfile())], + child: MaterialApp.router(routerConfig: router), + ), + ); + await tester.pumpAndSettle(); + router.push('/profile'); + await tester.pumpAndSettle(); + } + + Future pressBack(WidgetTester tester) async { + tester.state(find.byType(Navigator).last).maybePop(); + await tester.pumpAndSettle(); + } + + testWidgets('back after changing the name asks first', (tester) async { + await openProfile(tester); + + await tester.tap(find.byTooltip('Edit profile')); + await tester.pumpAndSettle(); + await tester.enterText(find.byType(TextField).first, 'Ada King'); + await pressBack(tester); + + expect(find.text('Discard changes?'), findsOneWidget); + expect(find.text('the more sheet'), findsNothing); + }); + + testWidgets('back while just reading leaves straight away', (tester) async { + await openProfile(tester); + + await pressBack(tester); + + // The control. Most visits to this screen only look at it, and prompting + // those would train people to dismiss the dialog without reading it. + expect(find.text('Discard changes?'), findsNothing); + expect(find.text('the more sheet'), findsOneWidget); + }); + + testWidgets('opening edit and changing nothing is not a change', ( + tester, + ) async { + await openProfile(tester); + + await tester.tap(find.byTooltip('Edit profile')); + await tester.pumpAndSettle(); + await pressBack(tester); + + // The fields are pre-filled from the saved profile, so being in edit mode + // is not by itself something to lose. + expect(find.text('Discard changes?'), findsNothing); + expect(find.text('the more sheet'), findsOneWidget); + }); +} + +class _FakeProfile extends ProfileNotifier { + @override + Future build() async => const Profile( + id: 'p1', + user: AuthUser(id: 'u1', email: 'user@example.com', name: 'Ada Lovelace'), + phone: '+1 555 0100', + ); +} diff --git a/mobile/test/unit/token_storage_test.dart b/mobile/test/unit/token_storage_test.dart new file mode 100644 index 00000000..171eb790 --- /dev/null +++ b/mobile/test/unit/token_storage_test.dart @@ -0,0 +1,234 @@ +import 'package:bottle_crm/services/token_storage.dart'; +import 'package:flutter_secure_storage/flutter_secure_storage.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// `mobile/CLAUDE.md` forbids keeping tokens in `SharedPreferences` by name, +/// and both JWTs sat there until now. These tests pin the two halves of the +/// fix: new tokens go to secure storage, and tokens an older build already +/// wrote are moved out of the readable file rather than merely shadowed. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + /// The map handed to `setMockInitialValues` is the live backing store, so + /// holding a reference is how a test sees what really landed in the + /// keychain. + late Map secure; + + void useStores({ + Map secureSeed = const {}, + Map prefsSeed = const {}, + }) { + secure = Map.from(secureSeed); + FlutterSecureStorage.setMockInitialValues(secure); + SharedPreferences.setMockInitialValues(prefsSeed); + } + + group('where a token ends up', () { + test( + 'a written token is in secure storage and not in preferences', + () async { + useStores(); + final storage = TokenStorage(); + + await storage.write(access: 'access-jwt', refresh: 'refresh-jwt'); + + expect(secure[TokenStorage.accessTokenKey], 'access-jwt'); + expect(secure[TokenStorage.refreshTokenKey], 'refresh-jwt'); + + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString(TokenStorage.accessTokenKey), isNull); + expect(prefs.getString(TokenStorage.refreshTokenKey), isNull); + }, + ); + + test('what was written is what comes back', () async { + useStores(); + final storage = TokenStorage(); + + await storage.write(access: 'access-jwt', refresh: 'refresh-jwt'); + final tokens = await storage.read(); + + expect(tokens.access, 'access-jwt'); + expect(tokens.refresh, 'refresh-jwt'); + }); + + test('writing null clears rather than leaving the old value', () async { + useStores(); + final storage = TokenStorage(); + await storage.write(access: 'access-jwt', refresh: 'refresh-jwt'); + + // The session ended without a full clear. A token that survived here + // would outlive the session that obtained it. + await storage.write(access: null, refresh: null); + + expect(secure, isEmpty); + }); + }); + + group('upgrading from a build that used preferences', () { + test('legacy tokens are moved into secure storage', () async { + useStores( + prefsSeed: { + TokenStorage.accessTokenKey: 'old-access', + TokenStorage.refreshTokenKey: 'old-refresh', + }, + ); + + final tokens = await TokenStorage().read(); + + // The user stays signed in across the upgrade. + expect(tokens.access, 'old-access'); + expect(tokens.refresh, 'old-refresh'); + expect(secure[TokenStorage.accessTokenKey], 'old-access'); + }); + + test('and are deleted from preferences, which is the point', () async { + useStores( + prefsSeed: { + TokenStorage.accessTokenKey: 'old-access', + TokenStorage.refreshTokenKey: 'old-refresh', + }, + ); + + await TokenStorage().read(); + + // Copying alone would leave readable plaintext on disk forever on every + // device that ever ran an older build, so the upgrade would protect new + // installs and nobody else. + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString(TokenStorage.accessTokenKey), isNull); + expect(prefs.getString(TokenStorage.refreshTokenKey), isNull); + }); + + test('a legacy value never overwrites a newer secure one', () async { + useStores( + secureSeed: { + TokenStorage.accessTokenKey: 'current-access', + TokenStorage.refreshTokenKey: 'current-refresh', + }, + prefsSeed: {TokenStorage.accessTokenKey: 'stale-access'}, + ); + + final tokens = await TokenStorage().read(); + + // Migration is skipped once secure storage holds an access token. + // Without that guard a stale copy would sign the user back into an + // older session on the next launch. + expect(tokens.access, 'current-access'); + }); + + test('other preferences are left alone', () async { + useStores( + prefsSeed: { + TokenStorage.accessTokenKey: 'old-access', + 'user_data': '{"id":"u1"}', + 'selected_organization': '{"id":"o1"}', + }, + ); + + await TokenStorage().read(); + + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('user_data'), '{"id":"u1"}'); + expect(prefs.getString('selected_organization'), '{"id":"o1"}'); + }); + }); + + group('signing out', () { + test('clear empties secure storage', () async { + useStores( + secureSeed: { + TokenStorage.accessTokenKey: 'access-jwt', + TokenStorage.refreshTokenKey: 'refresh-jwt', + }, + ); + + await TokenStorage().clear(); + + expect(secure, isEmpty); + }); + + test('clear also empties a legacy copy left by an older build', () async { + // Signing out on a device that upgraded mid-session, before any read + // had a chance to migrate. Missing this leaves a refresh token good for + // fourteen days behind a UI that says you are signed out. + useStores( + prefsSeed: { + TokenStorage.accessTokenKey: 'old-access', + TokenStorage.refreshTokenKey: 'old-refresh', + }, + ); + + await TokenStorage().clear(); + + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString(TokenStorage.accessTokenKey), isNull); + expect(prefs.getString(TokenStorage.refreshTokenKey), isNull); + }); + }); + + group('when the keystore itself fails', () { + test( + 'a read failure signs the user out rather than crashing launch', + () async { + SharedPreferences.setMockInitialValues({}); + final storage = TokenStorage(storage: _BrokenSecureStorage()); + + final tokens = await storage.read(); + + expect(tokens.access, isNull); + expect(tokens.refresh, isNull); + }, + ); + }); +} + +/// A corrupt keychain entry after an OS restore, or a locked device on iOS. +/// Real, and a throw out of `read` would take out app startup. +class _BrokenSecureStorage implements FlutterSecureStorage { + @override + Future read({ + required String key, + dynamic iOptions, + dynamic aOptions, + dynamic lOptions, + dynamic webOptions, + dynamic mOptions, + dynamic wOptions, + }) async { + throw PlatformExceptionStub(); + } + + @override + Future write({ + required String key, + required String? value, + dynamic iOptions, + dynamic aOptions, + dynamic lOptions, + dynamic webOptions, + dynamic mOptions, + dynamic wOptions, + }) async { + throw PlatformExceptionStub(); + } + + @override + Future delete({ + required String key, + dynamic iOptions, + dynamic aOptions, + dynamic lOptions, + dynamic webOptions, + dynamic mOptions, + dynamic wOptions, + }) async { + throw PlatformExceptionStub(); + } + + @override + noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +class PlatformExceptionStub implements Exception {} diff --git a/mobile/test/widgets/unsaved_changes_test.dart b/mobile/test/widgets/unsaved_changes_test.dart new file mode 100644 index 00000000..fa1f5f02 --- /dev/null +++ b/mobile/test/widgets/unsaved_changes_test.dart @@ -0,0 +1,133 @@ +import 'package:bottle_crm/widgets/forms/unsaved_changes.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; + +/// Three form screens already carried a discard prompt and it fired on none of +/// them, because `canPop` was computed during `build` and typing into a field +/// never rebuilds its parent. The prompt was correct and simply unreachable. +/// +/// So the test that matters is the one that types and then presses back +/// WITHOUT any rebuild in between. Against the old code it walks straight out +/// of the form. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + /// Opens the form the way the app does: pushed on top of a list, so there is + /// somewhere to go back to. The form never calls setState when its field + /// changes, which is what every real form here does. + Future openForm( + WidgetTester tester, { + required TextEditingController controller, + bool isSaving = false, + }) async { + final router = GoRouter( + initialLocation: '/list', + routes: [ + GoRoute(path: '/list', builder: (_, _) => const Text('the list')), + GoRoute( + path: '/form', + builder: (_, _) => UnsavedChangesGuard( + hasUnsavedChanges: () => controller.text.isNotEmpty, + isSaving: isSaving, + child: Scaffold(body: TextField(controller: controller)), + ), + ), + ], + ); + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + router.push('/form'); + await tester.pumpAndSettle(); + } + + /// What the OS back gesture does. `maybePop` is the call that consults + /// PopScope, which a direct `pop` bypasses. + Future pressBack(WidgetTester tester) async { + final navigator = tester.state(find.byType(Navigator).last); + navigator.maybePop(); + await tester.pumpAndSettle(); + } + + testWidgets('back after typing asks before throwing the work away', ( + tester, + ) async { + final controller = TextEditingController(); + await openForm(tester, controller: controller); + + await tester.enterText( + find.byType(TextField), + 'Important Deal I do not Want to lose', + ); + await pressBack(tester); + + expect(find.text('Discard changes?'), findsOneWidget); + expect(find.text('the list'), findsNothing); + }); + + testWidgets('back on an untouched form leaves without a prompt', ( + tester, + ) async { + final controller = TextEditingController(); + await openForm(tester, controller: controller); + + await pressBack(tester); + + // The control. A guard that always prompted would pass the test above. + expect(find.text('Discard changes?'), findsNothing); + expect(find.text('the list'), findsOneWidget); + }); + + testWidgets('Cancel keeps the form and the typing', (tester) async { + final controller = TextEditingController(); + await openForm(tester, controller: controller); + await tester.enterText(find.byType(TextField), 'half-written note'); + await pressBack(tester); + + await tester.tap(find.text('Cancel')); + await tester.pumpAndSettle(); + + expect(find.text('the list'), findsNothing); + expect(controller.text, 'half-written note'); + }); + + testWidgets('Discard leaves', (tester) async { + final controller = TextEditingController(); + await openForm(tester, controller: controller); + await tester.enterText(find.byType(TextField), 'half-written note'); + await pressBack(tester); + + await tester.tap(find.text('Discard')); + await tester.pumpAndSettle(); + + expect(find.text('the list'), findsOneWidget); + }); + + testWidgets('dismissing the prompt keeps the form, it does not leave', ( + tester, + ) async { + final controller = TextEditingController(); + await openForm(tester, controller: controller); + await tester.enterText(find.byType(TextField), 'half-written note'); + await pressBack(tester); + + // Tapping the scrim returns null, which must read as "keep", never as + // "discard". + await tester.tapAt(const Offset(20, 20)); + await tester.pumpAndSettle(); + + expect(find.text('the list'), findsNothing); + }); + + testWidgets('back does nothing while a save is in flight', (tester) async { + final controller = TextEditingController(text: 'already typed'); + await openForm(tester, controller: controller, isSaving: true); + + await pressBack(tester); + + // Popping here would orphan the request and leave the user unsure whether + // the record was created. + expect(find.text('Discard changes?'), findsNothing); + expect(find.text('the list'), findsNothing); + }); +}