From ab27ea85fdc101178af49751e04996b5eae81f84 Mon Sep 17 00:00:00 2001 From: Ashwin Date: Fri, 7 Aug 2026 10:05:19 +0530 Subject: [PATCH] Refactor dashboard API response to reduce payload size and improve performance - Removed serialization of full account, contact, lead, and opportunity objects from the dashboard API response. - Added new fields for urgent counts, pipeline by stage, revenue metrics, hot leads, tasks, activities, and goal summary. - Updated the context building logic to only include counts and essential data. Enhance JWT signing key validation in settings - Added a check to ensure the SECRET_KEY is at least 32 bytes for HS256 JWT signing. - Introduced warnings for development environments when the key is shorter than required. Update environment variable documentation - Clarified the requirements for the SECRET_KEY in the environment variables documentation, emphasizing the minimum length for security. Revise mobile app API configuration for self-hosting - Updated the mobile app to allow self-hosting by using `--dart-define` for API_BASE_URL and GOOGLE_SERVER_CLIENT_ID. - Ensured that the app enforces HTTPS in release builds to secure JWT transmission. Implement entity lookup for task linking in mobile app - Created a new EntityLookup model to handle task attachments to accounts, leads, opportunities, and tickets. - Refactored task form screen to utilize the new lookup mechanism, improving the user experience by providing real-time loading states and error handling. Add unit tests for new features and configurations - Implemented unit tests for API configuration, entity lookup, and task link picker to ensure functionality and reliability. --- backend/README.md | 4 +- backend/common/tests/test_dashboard.py | 68 +++++ .../common/tests/test_signing_key_strength.py | 103 ++++++++ backend/common/views/dashboard_views.py | 26 +- backend/crm/settings.py | 25 +- docs/reference/environment-variables.md | 2 +- mobile/README.md | 42 +++- mobile/lib/config/api_config.dart | 61 ++++- mobile/lib/data/models/lookup_models.dart | 62 +++++ mobile/lib/providers/lookup_provider.dart | 132 +++++++++- .../lib/screens/tasks/task_form_screen.dart | 232 +++++++----------- mobile/lib/services/auth_service.dart | 7 +- .../screens/tasks/task_link_picker_test.dart | 105 ++++++++ mobile/test/unit/api_config_test.dart | 102 ++++++++ mobile/test/unit/entity_lookup_test.dart | 168 +++++++++++++ 15 files changed, 960 insertions(+), 179 deletions(-) create mode 100644 backend/common/tests/test_signing_key_strength.py create mode 100644 mobile/test/screens/tasks/task_link_picker_test.dart create mode 100644 mobile/test/unit/api_config_test.dart create mode 100644 mobile/test/unit/entity_lookup_test.dart diff --git a/backend/README.md b/backend/README.md index fbfe3176..56302fd6 100644 --- a/backend/README.md +++ b/backend/README.md @@ -103,6 +103,8 @@ Create a `.env` file in the `backend/` directory: ```env # Django +# At least 32 bytes: this key also signs the JWTs, and HS256 needs one that long. +# python -c "import secrets; print(secrets.token_urlsafe(48))" SECRET_KEY=your-secret-key-here ENV_TYPE=dev @@ -423,7 +425,7 @@ uv lock --upgrade | Variable | Description | |----------|-------------| -| `SECRET_KEY` | Django secret key | +| `SECRET_KEY` | Django secret key, and the JWT signing key. At least 32 bytes | | `ENV_TYPE` | Environment type (`dev` or `prod`) | | `DBNAME` | PostgreSQL database name | | `DBUSER` | PostgreSQL username | diff --git a/backend/common/tests/test_dashboard.py b/backend/common/tests/test_dashboard.py index e9e27b03..98a710ef 100644 --- a/backend/common/tests/test_dashboard.py +++ b/backend/common/tests/test_dashboard.py @@ -315,3 +315,71 @@ class TestDashboardScopedMetrics: from common.views.dashboard_views import ApiHomeView assert HasOrgContext in ApiHomeView.permission_classes + + +@pytest.mark.django_db +class TestDashboardSendsOnlyWhatItRenders: + """The dashboard used to serialize four whole tables nobody read. + + Measured against the seeded org: 384 KB of response, of which + ``opportunities`` (162 KB), ``accounts`` (86 KB), ``leads`` (84 KB) and + ``contacts`` (41 KB) were 97%. The only caller is the mobile dashboard, and + its model reads counts, ``urgent_counts``, ``pipeline_by_stage``, + ``revenue_metrics``, ``hot_leads``, ``tasks``, ``activities`` and + ``goal_summary``. It never touched the four lists. + + They were also unpaged, so the payload grew with the org forever. + """ + + url = "/api/dashboard/" + + # Note the trap here: ``hot_leads`` is a list on the response AND a count + # inside ``urgent_counts``. Only the top-level list survives. + DROPPED = ("accounts", "contacts", "leads", "opportunities") + + KEPT = ( + "accounts_count", + "contacts_count", + "leads_count", + "opportunities_count", + "urgent_counts", + "pipeline_by_stage", + "revenue_metrics", + "hot_leads", + "tasks", + "activities", + "goal_summary", + ) + + def test_the_four_unread_lists_are_gone(self, admin_client, org_a): + Account.objects.create(name="Acc1", org=org_a) + Contact.objects.create(first_name="Con", last_name="Tact", org=org_a) + Lead.objects.create( + first_name="Lead", last_name="One", email="lead1@test.com", org=org_a + ) + Opportunity.objects.create(name="Deal", org=org_a, stage="PROSPECTING") + + response = admin_client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + for key in self.DROPPED: + assert key not in response.data, f"{key} is back in the response" + + def test_everything_a_client_reads_is_still_there(self, admin_client, org_a): + """The other direction. Trimming is only safe while this passes.""" + response = admin_client.get(self.url) + + assert response.status_code == status.HTTP_200_OK + for key in self.KEPT: + assert key in response.data, f"{key} disappeared" + + def test_the_counts_still_count_the_rows_that_were_dropped( + self, admin_client, org_a + ): + """The querysets stayed; only the serialization went.""" + Account.objects.create(name="Acc1", org=org_a) + Account.objects.create(name="Acc2", org=org_a) + + response = admin_client.get(self.url) + + assert response.data["accounts_count"] == 2 diff --git a/backend/common/tests/test_signing_key_strength.py b/backend/common/tests/test_signing_key_strength.py new file mode 100644 index 00000000..c78d5c84 --- /dev/null +++ b/backend/common/tests/test_signing_key_strength.py @@ -0,0 +1,103 @@ +"""``SECRET_KEY`` signs every JWT, so its length is a security control. + +``SIMPLE_JWT["SIGNING_KEY"]`` is ``SECRET_KEY``, and the algorithm is HS256. +RFC 7518 section 3.2 requires an HMAC key at least as long as the hash function +output, which is 32 bytes for SHA-256. PyJWT does not refuse a shorter key; it +emits a warning, once per call, into a log nobody reads. The local ``.env`` here +carried a 20-byte key for the life of the project and every token this server +issued was signed with it. + +``crm/settings.py`` now raises on a short key outside dev and warns inside it. +These tests import the settings module in a subprocess, because the check runs at +import time and reloading it inside the test process would rewrite settings for +everything that runs afterwards. +""" + +import os +import subprocess +import sys +from pathlib import Path + +BACKEND_DIR = Path(__file__).resolve().parents[2] + +# 32 bytes exactly, the floor. Not a real key: it never signs anything, the +# subprocess only imports the module and exits. +KEY_AT_FLOOR = "k" * 32 +KEY_BELOW_FLOOR = "k" * 31 + + +def _import_settings(secret_key, env_type): + """Import ``crm.settings`` in a clean subprocess and report how it went. + + ``load_dotenv()`` does not override variables already in the environment, so + what is passed here wins over the developer's own ``.env``. + """ + env = { + key: value + for key, value in os.environ.items() + if key not in ("SECRET_KEY", "ENV_TYPE") + } + env["SECRET_KEY"] = secret_key + env["ENV_TYPE"] = env_type + return subprocess.run( + [sys.executable, "-W", "always", "-c", "import crm.settings"], + cwd=BACKEND_DIR, + env=env, + capture_output=True, + text=True, + ) + + +class TestAShortSigningKeyIsRefusedOutsideDev: + def test_a_31_byte_key_fails_the_import(self): + result = _import_settings(KEY_BELOW_FLOOR, "production") + + assert result.returncode != 0 + assert "31 bytes" in result.stderr + assert "at least 32" in result.stderr + + def test_the_error_names_the_command_that_produces_a_good_key(self): + """A refusal that does not say what to do next is a support ticket.""" + result = _import_settings(KEY_BELOW_FLOOR, "production") + + assert "secrets.token_urlsafe" in result.stderr + + def test_a_32_byte_key_is_accepted(self): + """The other direction: the check has to be able to pass.""" + result = _import_settings(KEY_AT_FLOOR, "production") + + assert result.returncode == 0, result.stderr + + def test_a_long_key_is_accepted(self): + result = _import_settings("k" * 64, "production") + + assert result.returncode == 0, result.stderr + + +class TestDevIsWarnedRatherThanBlocked: + def test_a_short_key_warns_in_dev_and_still_imports(self): + """Blocking dev would mean every existing checkout stops running.""" + result = _import_settings(KEY_BELOW_FLOOR, "dev") + + assert result.returncode == 0, result.stderr + assert "31 bytes" in result.stderr + + def test_no_warning_in_dev_once_the_key_is_long_enough(self): + result = _import_settings(KEY_AT_FLOOR, "dev") + + assert result.returncode == 0, result.stderr + assert "SECRET_KEY" not in result.stderr + + +class TestTheKeyIsMeasuredInBytesNotCharacters: + def test_a_31_character_key_of_multibyte_characters_passes(self): + """31 characters, 62 bytes. The RFC counts bytes, and so does HMAC. + + Measuring `len(SECRET_KEY)` instead would reject this key, and would + accept a 32-character key that was somehow shorter in bytes. Python + strings cannot be shorter in bytes than in characters, so only the + false rejection is reachable, which is the one this pins. + """ + result = _import_settings("é" * 31, "production") + + assert result.returncode == 0, result.stderr diff --git a/backend/common/views/dashboard_views.py b/backend/common/views/dashboard_views.py index 3e98de5d..35b2de93 100644 --- a/backend/common/views/dashboard_views.py +++ b/backend/common/views/dashboard_views.py @@ -10,19 +10,15 @@ from rest_framework.response import Response from rest_framework.views import APIView from accounts.models import Account -from accounts.serializer import AccountSerializer from cases.models import Case from common import serializer, swagger_params from common.models import Activity from common.permissions import HasOrgContext, is_org_admin from common.utils import STAGES from contacts.models import Contact -from contacts.serializer import ContactSerializer from invoices.models import UNPAID_STATUSES, Invoice from leads.models import Lead -from leads.serializer import LeadSerializer from opportunity.models import Opportunity, StageAgingConfig -from opportunity.serializer import OpportunitySerializer from opportunity.workflow import DEFAULT_STAGE_EXPECTED_DAYS, ROTTEN_MULTIPLIER from tasks.models import Task from tasks.serializer import TaskSerializer @@ -94,10 +90,13 @@ class ApiHomeView(APIView): "contacts_count": serializers.IntegerField(), "leads_count": serializers.IntegerField(), "opportunities_count": serializers.IntegerField(), - "accounts": AccountSerializer(many=True), - "contacts": ContactSerializer(many=True), - "leads": LeadSerializer(many=True), - "opportunities": OpportunitySerializer(many=True), + "urgent_counts": serializers.DictField(), + "pipeline_by_stage": serializers.DictField(), + "revenue_metrics": serializers.DictField(), + "hot_leads": serializers.ListField(), + "tasks": TaskSerializer(many=True), + "activities": serializer.DashboardActivitySerializer(many=True), + "goal_summary": serializers.ListField(), }, ) }, @@ -126,16 +125,17 @@ class ApiHomeView(APIView): opportunities = _owned_or_assigned(opportunities, profile) tasks = _owned_or_assigned(tasks, profile) - # Build base context (existing) + # Counts only. This used to serialize every account, contact, lead and + # opportunity in the org in full beside them: 372 KB of a 384 KB + # response, measured against the seeded org, none of which any caller + # read. The screens below want counts, the pipeline, the urgent numbers, + # ten hot leads and ten tasks. Whoever needs a list calls its own + # endpoint, which pages; these four never did. context = {} context["accounts_count"] = accounts.count() context["contacts_count"] = contacts.count() context["leads_count"] = leads.count() context["opportunities_count"] = opportunities.count() - context["accounts"] = AccountSerializer(accounts, many=True).data - context["contacts"] = ContactSerializer(contacts, many=True).data - context["leads"] = LeadSerializer(leads, many=True).data - context["opportunities"] = OpportunitySerializer(opportunities, many=True).data # NEW: Urgent counts for Focus Bar overdue_tasks = tasks.filter( diff --git a/backend/crm/settings.py b/backend/crm/settings.py index 578d7a15..af7bc35e 100644 --- a/backend/crm/settings.py +++ b/backend/crm/settings.py @@ -1,4 +1,5 @@ import os +import warnings from datetime import timedelta from corsheaders.defaults import default_headers @@ -12,10 +13,32 @@ load_dotenv() # SECURITY WARNING: keep the secret key used in production secret! SECRET_KEY = os.environ.get("SECRET_KEY", "django-insecure-dev-key-please-change-in-production") +IS_DEV_ENV = os.environ.get("ENV_TYPE", "dev") == "dev" + if not SECRET_KEY or SECRET_KEY.startswith("django-insecure"): - if os.environ.get("ENV_TYPE", "dev") != "dev": + if not IS_DEV_ENV: raise ValueError("SECRET_KEY must be set to a secure value in non-dev environments") +# This key also signs every JWT (see SIMPLE_JWT["SIGNING_KEY"] below). RFC 7518 +# section 3.2 requires an HMAC key at least as long as the hash it produces, so +# HS256 needs 32 bytes. PyJWT only warns about a shorter one, on every call, and +# a warning nobody reads is not a control. +# +# Raising here means a deploy fails rather than quietly signing weak tokens. +# Lengthening the key invalidates every access and refresh token already issued, +# so everyone is signed out once when it changes. +JWT_MIN_SIGNING_KEY_BYTES = 32 + +if len(SECRET_KEY.encode()) < JWT_MIN_SIGNING_KEY_BYTES: + message = ( + f"SECRET_KEY is {len(SECRET_KEY.encode())} bytes. It signs every JWT, " + f"and HS256 needs at least {JWT_MIN_SIGNING_KEY_BYTES}. Generate one with " + "`python -c \"import secrets; print(secrets.token_urlsafe(48))\"`." + ) + if not IS_DEV_ENV: + raise ValueError(message) + warnings.warn(message, RuntimeWarning, stacklevel=2) + # SECURITY WARNING: don't run with debug turned on in production! DEBUG = os.environ.get("DEBUG", "False").lower() == "true" diff --git a/docs/reference/environment-variables.md b/docs/reference/environment-variables.md index fec03133..af561846 100644 --- a/docs/reference/environment-variables.md +++ b/docs/reference/environment-variables.md @@ -50,7 +50,7 @@ itself, so they aren't in the table below: | Variable | Default | Required | Purpose | | --- | --- | --- | --- | -| `SECRET_KEY` | `django-insecure-dev-key-please-change-in-production` | Yes, once `ENV_TYPE` is anything other than `dev` (see above) | Django's cryptographic signing key. It also *is* the JWT `SIGNING_KEY`: `SIMPLE_JWT["SIGNING_KEY"]` is set to `SECRET_KEY` directly, so there is no separate JWT signing key setting anywhere in this codebase, and rotating `SECRET_KEY` invalidates every outstanding access and refresh token. See [Architecture: Authentication](../architecture/authentication.md#token-model) and [Security hardening](../self-hosting/security-hardening.md#secrets). | +| `SECRET_KEY` | `django-insecure-dev-key-please-change-in-production` | Yes, once `ENV_TYPE` is anything other than `dev` (see above) | Django's cryptographic signing key. It also *is* the JWT `SIGNING_KEY`: `SIMPLE_JWT["SIGNING_KEY"]` is set to `SECRET_KEY` directly, so there is no separate JWT signing key setting anywhere in this codebase, and rotating `SECRET_KEY` invalidates every outstanding access and refresh token. **It must be at least 32 bytes.** HS256 signs the JWTs, and RFC 7518 section 3.2 requires an HMAC key at least as long as the hash output; a shorter one raises at import time outside `dev` and warns inside it. Generate one with `python -c "import secrets; print(secrets.token_urlsafe(48))"`. See [Architecture: Authentication](../architecture/authentication.md#token-model) and [Security hardening](../self-hosting/security-hardening.md#secrets). | | `DEBUG` | `False` | No | Django debug mode. Also the sole gate on the `devlogin` management command. See [Management commands](management-commands.md#devlogin). | | `ALLOWED_HOSTS` | `localhost,127.0.0.1` | Yes, once serving traffic under a real hostname | Comma-separated list, split on `,` into Django's `ALLOWED_HOSTS`. | | `TRUST_PROXY_SSL_HEADER` | `False` | Yes, if TLS is terminated by a proxy in front of Django | Set to `true` (case-insensitive) to set Django's `SECURE_PROXY_SSL_HEADER` to `("HTTP_X_FORWARDED_PROTO", "https")`. Without it, a request arriving from nginx or an ALB over plain HTTP makes `request.is_secure()` false, and `SecurityMiddleware` then emits **no** `Strict-Transport-Security` header at all, so the `SECURE_HSTS_SECONDS` / `INCLUDE_SUBDOMAINS` / `PRELOAD` settings are silently inert. Only turn it on when that proxy is the sole route to the app **and** it overwrites `X-Forwarded-Proto` rather than passing through a client-supplied one, because otherwise any caller reaching Django directly can make `request.is_secure()` return true. The http-to-https redirect belongs on the proxy; `SECURE_SSL_REDIRECT` is deliberately not set. | diff --git a/mobile/README.md b/mobile/README.md index 4b185e95..6b613cdd 100644 --- a/mobile/README.md +++ b/mobile/README.md @@ -134,11 +134,26 @@ BottleCRM Mobile is designed to streamline your sales and customer management pr 4. Configure OAuth consent screen #### Environment Configuration -The app automatically switches between environments: -- **Debug builds**: Uses development API (ngrok URL) -- **Release builds**: Uses production API (bottlecrm.io) +With no build flags, the app switches on build mode: +- **Debug builds**: development API +- **Release builds**: production API (bottlecrm.io) -You can modify URLs in `lib/config/api_config.dart` +Self-hosting points a build at your own server without editing source: + +```bash +flutter build apk --release \ + --dart-define=API_BASE_URL=https://crm.example.com \ + --dart-define=GOOGLE_SERVER_CLIENT_ID=.apps.googleusercontent.com +``` + +`API_BASE_URL` must start with `https://` in a release build. The app refuses to +start on a plain-HTTP address rather than send its JWTs unencrypted; `http://` is +allowed in debug, where a local Django lives. A trailing slash is trimmed. + +Both settings are compile-time (`String.fromEnvironment`). Deliberately so: a +runtime setting would let anything that can write the app's storage point every +request, and every token, at a server of its choosing. The defaults still live in +`lib/config/api_config.dart`. ## 🛠 Development @@ -312,17 +327,17 @@ For detailed architectural guidelines, see [CLAUDE.md](CLAUDE.md) and [.github/c ### API Configuration -The application uses environment-based API URLs configured in `lib/config/api_config.dart`: +Every endpoint is built from one host, resolved in `lib/config/api_config.dart`: -```dart -// Development (debug builds) -static const String _developmentUrl = 'https://b2ad5166b831.ngrok-free.app'; - -// Production (release builds) -static const String _productionUrl = 'https://api.bottlecrm.io'; +```bash +# Point a build anywhere. Release builds must be https. +flutter run --dart-define=API_BASE_URL=http://10.0.2.2:8000 ``` -**Environment Detection**: Automatic switching based on `kDebugMode` flag +With no `API_BASE_URL`, the host comes from `kDebugMode`: the development server +in debug, `https://api.bottlecrm.io` in release. See +[Environment Configuration](#environment-configuration) above for the release +build flags and the plain-HTTP rule. ### Authentication Setup @@ -474,7 +489,8 @@ flutter doctor -v #### API Connection Issues ```bash # Problem: API requests failing in debug mode -# Solution: Check ngrok tunnel status and update URL in api_config.dart +# Solution: Point the build at a server you control, no source edit needed +flutter run --dart-define=API_BASE_URL=http://10.0.2.2:8000 # Problem: 401 Unauthorized errors # Solution: Clear app data and re-authenticate diff --git a/mobile/lib/config/api_config.dart b/mobile/lib/config/api_config.dart index a48d8a14..df954baa 100644 --- a/mobile/lib/config/api_config.dart +++ b/mobile/lib/config/api_config.dart @@ -20,8 +20,65 @@ class ApiConfig { /// which would drift the moment either is edited. static const String marketingSite = 'https://bottlecrm.io'; - /// Get the current base URL based on build mode - static String get baseUrl => kDebugMode ? _developmentUrl : _productionUrl; + /// Build-time override, so somebody self-hosting points a build at their own + /// server without editing this file: + /// + /// flutter build apk --release --dart-define=API_BASE_URL=https://crm.example.com + /// + /// Compile-time on purpose. A runtime setting would mean anything able to + /// write the app's storage could redirect every request, and every JWT with + /// it, to a server of its own choosing. + static const String _baseUrlOverride = String.fromEnvironment('API_BASE_URL'); + + /// Google's Web OAuth client, the audience the backend's `GOOGLE_CLIENT_ID` + /// verifies against. Overridable the same way, because a self-hoster signs in + /// against their own Google project: + /// + /// --dart-define=GOOGLE_SERVER_CLIENT_ID=.apps.googleusercontent.com + /// + /// Not a secret. A client id is public by design; the backend is what decides + /// whether an ID token is acceptable. + static const String googleServerClientId = String.fromEnvironment( + 'GOOGLE_SERVER_CLIENT_ID', + defaultValue: + '1072513761792-p59rct7b1c3go7l58e51r3geuqff2tfl.apps.googleusercontent.com', + ); + + /// The API host this build talks to. + static final String baseUrl = resolveBaseUrl( + override: _baseUrlOverride, + isDebug: kDebugMode, + ); + + /// The override rules, separated from the build constants so both branches + /// can be tested. `flutter test` runs one build, so a `--dart-define` the + /// suite does not pass is a branch no test could otherwise reach. + @visibleForTesting + static String resolveBaseUrl({ + required String override, + required bool isDebug, + }) { + if (override.isEmpty) { + return isDebug ? _developmentUrl : _productionUrl; + } + // A trailing slash would produce `https://host//api`, and the resulting 404 + // on every request says nothing about the cause. + final trimmed = override.endsWith('/') + ? override.substring(0, override.length - 1) + : override; + // A release build talking plain HTTP puts every JWT on the wire in the + // clear. Refusing loudly beats falling back to the default, which would + // leave a self-hoster looking at somebody else's server and no explanation. + // `http://` stays allowed in debug: that is how anyone runs against a local + // Django. + if (!isDebug && !trimmed.startsWith('https://')) { + throw StateError( + 'API_BASE_URL must start with https:// in a release build. ' + 'Got "$trimmed", which would send every token unencrypted.', + ); + } + return trimmed; + } /// API base path static String get apiBaseUrl => '$baseUrl/api'; diff --git a/mobile/lib/data/models/lookup_models.dart b/mobile/lib/data/models/lookup_models.dart index 2c31de6f..d5f18e62 100644 --- a/mobile/lib/data/models/lookup_models.dart +++ b/mobile/lib/data/models/lookup_models.dart @@ -191,3 +191,65 @@ class TagLookup { @override int get hashCode => id.hashCode; } + +/// A record a task can be attached to: one id, one label, nothing else. +/// +/// The task form used to source these from the leads, deals and tickets list +/// providers, which hold whatever page the corresponding screen last loaded +/// under whatever filter it last applied. On a cold app those are empty, so the +/// picker said "Nothing to pick" for an org with twenty leads. +class EntityLookup { + final String id; + final String label; + + const EntityLookup({required this.id, required this.label}); + + /// A record with one naming field. [labelKeys] are tried in order and the + /// first non-empty one wins. + factory EntityLookup.fromJson( + Map json, { + required List labelKeys, + }) { + return EntityLookup( + id: json['id']?.toString() ?? '', + label: _firstNonEmpty(json, labelKeys) ?? 'Untitled', + ); + } + + /// A person, whose name is two optional fields rather than one. Neither is + /// required on a lead, so [fallbackKeys] is what keeps an unnamed row + /// selectable instead of blank. + factory EntityLookup.person( + Map json, { + List fallbackKeys = const [], + }) { + final name = [ + json['first_name']?.toString().trim() ?? '', + json['last_name']?.toString().trim() ?? '', + ].where((part) => part.isNotEmpty).join(' '); + return EntityLookup( + id: json['id']?.toString() ?? '', + label: name.isNotEmpty + ? name + : (_firstNonEmpty(json, fallbackKeys) ?? 'Unnamed'), + ); + } + + static String? _firstNonEmpty(Map json, List keys) { + for (final key in keys) { + final value = json[key]?.toString().trim() ?? ''; + if (value.isNotEmpty) return value; + } + return null; + } + + @override + bool operator ==(Object other) => + identical(this, other) || + other is EntityLookup && + runtimeType == other.runtimeType && + id == other.id; + + @override + int get hashCode => id.hashCode; +} diff --git a/mobile/lib/providers/lookup_provider.dart b/mobile/lib/providers/lookup_provider.dart index bb304544..65d2ef2b 100644 --- a/mobile/lib/providers/lookup_provider.dart +++ b/mobile/lib/providers/lookup_provider.dart @@ -1,3 +1,4 @@ +import 'package:flutter/foundation.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../config/api_config.dart'; import '../data/api_envelope.dart'; @@ -15,6 +16,26 @@ import '../services/api_service.dart'; final ApiService _apiService = ApiService(); +/// How many rows a picker asks for. +/// +/// Every paginated list endpoint here uses DRF's `LimitOffsetPagination` with +/// `PAGE_SIZE = 10`, so a lookup that sent no `limit` offered ten rows and no +/// hint that there were more. Confirmed against the seeded org: `/api/contacts/` +/// answered `contacts_count: 15` beside ten rows, and five contacts could not be +/// picked on any form in the app. Tags were 10 of 14. +/// +/// 500 is a cap rather than a fix. An org past it still cannot pick everything, +/// and the real answer is a searching picker backed by the server. This number +/// is what makes the gap rare instead of routine. +const int lookupPageLimit = 500; + +/// The URL a lookup actually requests. Public only so a test can assert the +/// limit is on it: without one every picker in the app silently offers ten rows. +@visibleForTesting +String lookupUrl(String url) => Uri.parse( + url, +).replace(queryParameters: {'limit': '$lookupPageLimit'}).toString(); + /// Accounts class AccountsLookupNotifier extends AsyncNotifier> { @override @@ -26,7 +47,7 @@ class AccountsLookupNotifier extends AsyncNotifier> { } Future> _fetch() async { - final response = await _apiService.get(ApiConfig.accounts); + final response = await _apiService.get(lookupUrl(ApiConfig.accounts)); if (!response.success || response.data == null) { throw Exception(response.message ?? 'Failed to load accounts'); } @@ -59,7 +80,7 @@ class ContactsLookupNotifier extends AsyncNotifier> { } Future> _fetch() async { - final response = await _apiService.get(ApiConfig.contacts); + final response = await _apiService.get(lookupUrl(ApiConfig.contacts)); if (!response.success || response.data == null) { throw Exception(response.message ?? 'Failed to load contacts'); } @@ -142,7 +163,7 @@ class TagsLookupNotifier extends AsyncNotifier> { } Future> _fetch() async { - final response = await _apiService.get(ApiConfig.tags); + final response = await _apiService.get(lookupUrl(ApiConfig.tags)); if (!response.success || response.data == null) { throw Exception(response.message ?? 'Failed to load tags'); } @@ -158,6 +179,79 @@ final tagsLookupProvider = TagsLookupNotifier.new, ); +/// The three records a task can be attached to that are not accounts. +/// +/// Each has a full module provider already, and the task form used to read +/// those. It could not: they page (20 rows) and they carry whatever search and +/// filters their list screen last applied, so a task form opened after filtering +/// deals to Won offered only won deals, and a task form opened on a cold app +/// offered nothing at all with the message "Open this section on the web or +/// create one first." +class _EntityLookupNotifier extends AsyncNotifier> { + _EntityLookupNotifier({ + required this.url, + required this.paths, + required this.parse, + required this.what, + }); + + final String url; + final List paths; + final EntityLookup Function(Map) parse; + final String what; + + @override + Future> build() => _fetch(); + + Future refresh() async { + state = const AsyncValue.loading(); + state = await AsyncValue.guard(_fetch); + } + + Future> _fetch() async { + final response = await _apiService.get(lookupUrl(url)); + if (!response.success || response.data == null) { + throw Exception(response.message ?? 'Failed to load $what'); + } + return listFromEnvelope(response.data!, paths).map(parse).toList(); + } +} + +/// Leads. Rows are nested under `open_leads.open_leads`, the same shape trap the +/// accounts lookup hit. Closed leads sit under `close_leads` and are not offered. +final leadsLookupProvider = + AsyncNotifierProvider<_EntityLookupNotifier, List>( + () => _EntityLookupNotifier( + url: ApiConfig.leads, + paths: const ['open_leads.open_leads'], + parse: (json) => EntityLookup.person( + json, + fallbackKeys: const ['email', 'company_name'], + ), + what: 'leads', + ), + ); + +final opportunitiesLookupProvider = + AsyncNotifierProvider<_EntityLookupNotifier, List>( + () => _EntityLookupNotifier( + url: ApiConfig.opportunities, + paths: const ['opportunities'], + parse: (json) => EntityLookup.fromJson(json, labelKeys: const ['name']), + what: 'deals', + ), + ); + +final ticketsLookupProvider = + AsyncNotifierProvider<_EntityLookupNotifier, List>( + () => _EntityLookupNotifier( + url: ApiConfig.tickets, + paths: const ['cases'], + parse: (json) => EntityLookup.fromJson(json, labelKeys: const ['name']), + what: 'tickets', + ), + ); + /// Convenience wrappers. Return the resolved list (or empty during /// load/error). Forms that don't care about loading state watch these. /// Picker options, resolved. Named for what they are rather than for the @@ -183,6 +277,38 @@ final teamsProvider = Provider>((ref) { return ref.watch(teamsLookupProvider).value ?? const []; }); +/// The four kinds a task can be attached to, in one shape. +/// +/// These keep the `AsyncValue` rather than flattening to a list like the +/// wrappers above, because the picker that reads them has to tell "still +/// loading" apart from "this org has none". Flattening is what made a cold app +/// claim there was nothing to link to. +final accountEntityOptionsProvider = Provider>>(( + ref, +) { + return ref + .watch(accountsLookupProvider) + .whenData( + (accounts) => [ + for (final account in accounts) + EntityLookup(id: account.id, label: account.name), + ], + ); +}); + +final leadEntityOptionsProvider = Provider>>( + (ref) => ref.watch(leadsLookupProvider), +); + +final opportunityEntityOptionsProvider = + Provider>>( + (ref) => ref.watch(opportunitiesLookupProvider), + ); + +final ticketEntityOptionsProvider = Provider>>( + (ref) => ref.watch(ticketsLookupProvider), +); + /// Custom field definitions, keyed by target_model (Case, Lead, ...). /// /// Each form watches the family for its entity; results are cached per-target diff --git a/mobile/lib/screens/tasks/task_form_screen.dart b/mobile/lib/screens/tasks/task_form_screen.dart index 05305df5..58400764 100644 --- a/mobile/lib/screens/tasks/task_form_screen.dart +++ b/mobile/lib/screens/tasks/task_form_screen.dart @@ -4,11 +4,8 @@ import 'package:go_router/go_router.dart'; import 'package:lucide_icons_flutter/lucide_icons.dart'; import '../../core/theme/theme.dart'; import '../../data/models/models.dart'; -import '../../providers/deals_provider.dart'; -import '../../providers/leads_provider.dart'; 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'; @@ -134,41 +131,38 @@ class _TaskFormScreenState extends ConsumerState { } } - // Resolve the label for the currently-linked entity from cached lookup - // providers. The TaskSerializer returns FKs as plain UUIDs (not nested - // objects), so we have no embedded name on the edit fetch. We look it up - // in the same provider used by the picker. Returns null if the cache - // doesn't have it yet (provider not loaded); the UI falls back to a - // generic "Selected" hint until the provider warms up. + /// The options for one kind, as a still-loading-aware value. + /// + /// These are the lookup providers, not the leads/deals/tickets list + /// providers this screen used to read. Those hold one page of whatever the + /// corresponding list screen last fetched, under whatever filter it last + /// applied, so linking a task to a deal was only possible if the deals list + /// happened to be showing it. + Provider>> _optionsOf(_RelatedKind kind) { + switch (kind) { + case _RelatedKind.account: + return accountEntityOptionsProvider; + case _RelatedKind.lead: + return leadEntityOptionsProvider; + case _RelatedKind.opportunity: + return opportunityEntityOptionsProvider; + case _RelatedKind.ticket: + return ticketEntityOptionsProvider; + } + } + + // Resolve the label for the currently-linked entity. The TaskSerializer + // returns these FKs as plain UUIDs rather than nested objects, so the edit + // fetch carries no name. Returns null while the lookup is still in flight; + // the UI shows a generic "Selected" until then. String? _resolveRelatedLabel() { final id = _relatedId; final kind = _relatedKind; if (id == null || kind == null) return null; - switch (kind) { - case _RelatedKind.account: - for (final a in ref.watch(accountOptionsProvider)) { - if (a.id == id) return a.name; - } - return null; - case _RelatedKind.lead: - for (final l in ref.watch(leadsListProvider)) { - if (l.id == id) { - final n = '${l.firstName} ${l.lastName}'.trim(); - return n.isEmpty ? l.email : n; - } - } - return null; - case _RelatedKind.opportunity: - for (final d in ref.watch(dealsListProvider)) { - if (d.id == id) return d.title; - } - return null; - case _RelatedKind.ticket: - for (final t in ref.watch(ticketsListProvider)) { - if (t.id == id) return t.name; - } - return null; + for (final option in ref.watch(_optionsOf(kind)).value ?? const []) { + if (option.id == id) return option.label; } + return null; } /// The form as it looked when it finished loading, against the payload it @@ -817,80 +811,21 @@ class _TaskFormScreenState extends ConsumerState { } Future _pickEntityForKind(_RelatedKind kind) async { - switch (kind) { - case _RelatedKind.account: - final accounts = ref.read(accountOptionsProvider); - final pick = await _pickFromList( - title: 'Select Account', - items: accounts, - labelOf: (a) => a.name, - icon: LucideIcons.building2, - ); - if (pick != null) { - setState(() { - _relatedKind = kind; - _relatedId = pick.id; - }); - } - break; - case _RelatedKind.lead: - final leads = ref.read(leadsListProvider); - final pick = await _pickFromList( - title: 'Select Lead', - items: leads, - labelOf: (l) { - final n = '${l.firstName} ${l.lastName}'.trim(); - return n.isEmpty ? l.email : n; - }, - icon: LucideIcons.user, - ); - if (pick != null) { - setState(() { - _relatedKind = kind; - _relatedId = pick.id; - }); - } - break; - case _RelatedKind.opportunity: - final deals = ref.read(dealsListProvider); - final pick = await _pickFromList( - title: 'Select Opportunity', - items: deals, - labelOf: (d) => d.title, - icon: LucideIcons.trendingUp, - ); - if (pick != null) { - setState(() { - _relatedKind = kind; - _relatedId = pick.id; - }); - } - break; - case _RelatedKind.ticket: - final tickets = ref.read(ticketsListProvider); - final pick = await _pickFromList( - title: 'Select Ticket', - items: tickets, - labelOf: (t) => t.name, - icon: LucideIcons.lifeBuoy, - ); - if (pick != null) { - setState(() { - _relatedKind = kind; - _relatedId = pick.id; - }); - } - break; - } + final pick = await _pickFromList(kind); + if (pick == null) return; + setState(() { + _relatedKind = kind; + _relatedId = pick.id; + }); } - Future _pickFromList({ - required String title, - required List items, - required String Function(T) labelOf, - required IconData icon, - }) { - return showModalBottomSheet( + /// The sheet watches the lookup rather than being handed a list, so it can + /// say "loading" and "that failed" instead of showing the empty state for + /// all three. The empty state used to read "Open this section on the web or + /// create one first", which on a cold app was simply untrue. + Future _pickFromList(_RelatedKind kind) { + final provider = _optionsOf(kind); + return showModalBottomSheet( context: context, backgroundColor: AppColors.surface, isScrollControlled: true, @@ -915,50 +850,63 @@ class _TaskFormScreenState extends ConsumerState { ), Padding( padding: const EdgeInsets.all(16), - child: Text(title, style: AppTypography.h3), + child: Text('Select ${kind.label}', style: AppTypography.h3), ), Expanded( - child: items.isEmpty - ? const Center( - child: EmptyState( - icon: LucideIcons.inbox, - title: 'Nothing to pick', - description: - 'Open this section on the web or create one first.', + child: Consumer( + builder: (_, ref, _) => ref + .watch(provider) + .when( + loading: () => + const Center(child: CircularProgressIndicator()), + error: (_, _) => Center( + child: EmptyState( + icon: LucideIcons.alertCircle, + title: 'Could not load ${kind.label.toLowerCase()}s', + description: + 'Close this and try again once you are back online.', + ), ), - ) - : ListView.builder( - controller: controller, - itemCount: items.length, - itemBuilder: (_, i) { - final item = items[i]; - return InkWell( - onTap: () => Navigator.pop(ctx, item), - child: Padding( - padding: const EdgeInsets.symmetric( - horizontal: 16, - vertical: 14, - ), - child: Row( - children: [ - Icon( - icon, - size: 18, - color: AppColors.textSecondary, - ), - const SizedBox(width: 12), - Expanded( - child: Text( - labelOf(item), - style: AppTypography.body, + data: (items) => items.isEmpty + ? Center( + child: EmptyState( + icon: LucideIcons.inbox, + title: 'No ${kind.label.toLowerCase()}s yet', + description: + 'Create one first, then link this task to it.', + ), + ) + : ListView.builder( + controller: controller, + itemCount: items.length, + itemBuilder: (_, i) => InkWell( + onTap: () => Navigator.pop(ctx, items[i]), + child: Padding( + padding: const EdgeInsets.symmetric( + horizontal: 16, + vertical: 14, + ), + child: Row( + children: [ + Icon( + kind.icon, + size: 18, + color: AppColors.textSecondary, + ), + const SizedBox(width: 12), + Expanded( + child: Text( + items[i].label, + style: AppTypography.body, + ), + ), + ], ), ), - ], + ), ), - ), - ); - }, ), + ), ), ], ), diff --git a/mobile/lib/services/auth_service.dart b/mobile/lib/services/auth_service.dart index dbe3d935..ed88fafe 100644 --- a/mobile/lib/services/auth_service.dart +++ b/mobile/lib/services/auth_service.dart @@ -69,11 +69,12 @@ class AuthService { // Initialize Google Sign-In (required in v7.1.1). // serverClientId is the Web OAuth client (client_type: 3 in google-services.json) //. This is the audience the Django backend's GOOGLE_CLIENT_ID verifies against, - // and it's what makes Android return a usable ID token. + // and it's what makes Android return a usable ID token. It lives in + // ApiConfig so a self-hoster can point a build at their own Google project + // with --dart-define instead of editing this file. try { await _googleSignIn.initialize( - serverClientId: - '1072513761792-p59rct7b1c3go7l58e51r3geuqff2tfl.apps.googleusercontent.com', + serverClientId: ApiConfig.googleServerClientId, ); debugPrint('AuthService: Google Sign-In initialized successfully'); } catch (e) { diff --git a/mobile/test/screens/tasks/task_link_picker_test.dart b/mobile/test/screens/tasks/task_link_picker_test.dart new file mode 100644 index 00000000..127eb6fa --- /dev/null +++ b/mobile/test/screens/tasks/task_link_picker_test.dart @@ -0,0 +1,105 @@ +import 'package:bottle_crm/core/theme/theme.dart'; +import 'package:bottle_crm/data/models/lookup_models.dart'; +import 'package:bottle_crm/providers/lookup_provider.dart'; +import 'package:bottle_crm/screens/tasks/task_form_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'; + +/// "Link to a lead" read the leads *list* provider, which holds one page of +/// whatever the leads screen last fetched under whatever filter it last +/// applied. Open a fresh app, create a task, tap Lead: nothing, under the +/// heading "Nothing to pick" and the advice "Open this section on the web or +/// create one first". The org had twenty leads. +/// +/// The picker now reads a lookup of its own and, because it watches rather than +/// reads, can tell the three states apart. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + Future openLeadPicker( + WidgetTester tester, + AsyncValue> leads, + ) async { + final router = GoRouter( + initialLocation: '/new', + routes: [ + GoRoute(path: '/new', builder: (_, _) => const TaskFormScreen()), + ], + ); + await tester.pumpWidget( + ProviderScope( + overrides: [leadEntityOptionsProvider.overrideWithValue(leads)], + child: MaterialApp.router(theme: AppTheme.light, routerConfig: router), + ), + ); + await tester.pumpAndSettle(); + + // The Linked Record row sits below the fold on a test-sized screen. + await tester.ensureVisible(find.text('Not linked')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Not linked')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Lead')); + await tester.pump(); + await tester.pump(const Duration(milliseconds: 400)); + } + + testWidgets('a lookup still in flight shows loading, not an empty org', ( + tester, + ) async { + await openLeadPicker(tester, const AsyncValue.loading()); + + expect(find.byType(CircularProgressIndicator), findsOneWidget); + expect(find.textContaining('No Leads yet'), findsNothing); + }); + + testWidgets('leads that loaded are offered by name', (tester) async { + await openLeadPicker( + tester, + const AsyncValue.data([ + EntityLookup(id: 'l1', label: 'Jill Shaffer'), + EntityLookup(id: 'l2', label: 'Cruz Group'), + ]), + ); + + expect(find.text('Jill Shaffer'), findsOneWidget); + expect(find.text('Cruz Group'), findsOneWidget); + }); + + testWidgets('picking one puts it on the form', (tester) async { + await openLeadPicker( + tester, + const AsyncValue.data([EntityLookup(id: 'l1', label: 'Jill Shaffer')]), + ); + + await tester.tap(find.text('Jill Shaffer')); + await tester.pumpAndSettle(); + + expect(find.text('Lead: Jill Shaffer'), findsOneWidget); + }); + + testWidgets('an org with no leads is told that, and nothing else', ( + tester, + ) async { + await openLeadPicker(tester, const AsyncValue.data([])); + + expect(find.text('No leads yet'), findsOneWidget); + // The old copy sent people to the web for a list that was simply not + // fetched yet. An org with no leads has nothing to open there either. + expect(find.textContaining('on the web'), findsNothing); + }); + + testWidgets('a failed lookup says so instead of claiming the org is empty', ( + tester, + ) async { + await openLeadPicker( + tester, + AsyncValue.error(Exception('offline'), StackTrace.empty), + ); + + expect(find.text('Could not load leads'), findsOneWidget); + expect(find.text('No leads yet'), findsNothing); + }); +} diff --git a/mobile/test/unit/api_config_test.dart b/mobile/test/unit/api_config_test.dart new file mode 100644 index 00000000..a41ac219 --- /dev/null +++ b/mobile/test/unit/api_config_test.dart @@ -0,0 +1,102 @@ +import 'package:bottle_crm/config/api_config.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// The API host was two literals in Dart, so anyone self-hosting had to edit +/// source and rebuild. It is now a `--dart-define`, and the branches that +/// define adds are what these tests cover. +/// +/// `flutter test` compiles once, without any `--dart-define`, so the override +/// path is unreachable through `ApiConfig.baseUrl` itself. `resolveBaseUrl` +/// takes the two inputs as arguments for exactly that reason. +void main() { + group('with no override, which is every build today', () { + test('debug and release resolve to different hosts', () { + final debug = ApiConfig.resolveBaseUrl(override: '', isDebug: true); + final release = ApiConfig.resolveBaseUrl(override: '', isDebug: false); + + expect(debug, isNot(release)); + expect(release, 'https://api.bottlecrm.io'); + }); + + test('the shipped default is what the app actually uses', () { + // Pins the wiring, not just the helper: a `baseUrl` that stopped calling + // `resolveBaseUrl` would leave every test above green and every request + // pointed somewhere else. + expect( + ApiConfig.baseUrl, + ApiConfig.resolveBaseUrl(override: '', isDebug: true), + ); + }); + }); + + group('with an override', () { + test('it wins over both built-in hosts', () { + expect( + ApiConfig.resolveBaseUrl( + override: 'https://crm.example.com', + isDebug: false, + ), + 'https://crm.example.com', + ); + }); + + test('a trailing slash is dropped rather than doubled into the path', () { + // `https://host//api` 404s on every call and says nothing about why. + final resolved = ApiConfig.resolveBaseUrl( + override: 'https://crm.example.com/', + isDebug: false, + ); + + expect(resolved, 'https://crm.example.com'); + expect('$resolved/api', 'https://crm.example.com/api'); + }); + + test('plain HTTP is refused in a release build', () { + // The whole point of the guard: before this knob existed nobody could + // ship a plaintext build without editing source. Now it is one flag away, + // and every JWT would travel in the clear. + expect( + () => ApiConfig.resolveBaseUrl( + override: 'http://crm.example.com', + isDebug: false, + ), + throwsA(isA()), + ); + }); + + test('the refusal names the address, so the mistake is findable', () { + expect( + () => ApiConfig.resolveBaseUrl( + override: 'http://crm.example.com', + isDebug: false, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains('http://crm.example.com'), + ), + ), + ); + }); + + test('plain HTTP is allowed in debug, where localhost lives', () { + expect( + ApiConfig.resolveBaseUrl( + override: 'http://10.0.2.2:8000', + isDebug: true, + ), + 'http://10.0.2.2:8000', + ); + }); + }); + + group('the Google client id', () { + test('has a default, so an unconfigured build still signs in', () { + expect( + ApiConfig.googleServerClientId, + endsWith('.apps.googleusercontent.com'), + ); + }); + }); +} diff --git a/mobile/test/unit/entity_lookup_test.dart b/mobile/test/unit/entity_lookup_test.dart new file mode 100644 index 00000000..8275c13c --- /dev/null +++ b/mobile/test/unit/entity_lookup_test.dart @@ -0,0 +1,168 @@ +import 'package:bottle_crm/config/api_config.dart'; +import 'package:bottle_crm/data/api_envelope.dart'; +import 'package:bottle_crm/data/models/lookup_models.dart'; +import 'package:bottle_crm/providers/lookup_provider.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// Every picker in this app was capped at ten rows, and none of them said so. +/// +/// The endpoints use DRF's `LimitOffsetPagination` with `PAGE_SIZE = 10`. A +/// lookup that sent no `limit` got ten. Confirmed live against the seeded org: +/// `/api/contacts/` answers `contacts_count: 15` beside ten rows, so five +/// contacts could not be attached to anything from the phone. There was no +/// error, no truncation notice, and no way to tell from the screen. +void main() { + group('the lookup request', () { + test('asks for more than one page', () { + final url = lookupUrl(ApiConfig.contacts); + + expect(url, contains('limit=$lookupPageLimit')); + expect(lookupPageLimit, greaterThan(10)); + }); + + test('keeps the path it was given', () { + expect(lookupUrl(ApiConfig.tags), startsWith(ApiConfig.tags)); + }); + }); + + group('the envelope each new lookup reads', () { + test('leads are two levels down, under the open group', () { + // Same shape trap as accounts. `results` is absent, and a missed path + // reads as "this org has no leads" rather than as a failure. + final body = { + 'open_leads': { + 'open_leads': [ + {'id': 'l1', 'first_name': 'Jill', 'last_name': 'Shaffer'}, + ], + 'leads_count': 20, + }, + 'close_leads': { + 'close_leads': [ + {'id': 'l9', 'first_name': 'Gone', 'last_name': 'Away'}, + ], + }, + }; + + expect(listFromEnvelope(body, const ['results']), isEmpty); + expect( + listFromEnvelope(body, const ['open_leads.open_leads']), + hasLength(1), + ); + }); + + test('deals and tickets sit at the top level under their own key', () { + expect( + listFromEnvelope( + { + 'opportunities': [ + {'id': 'o1', 'name': 'Renewal'}, + ], + }, + const ['opportunities'], + ), + hasLength(1), + ); + expect( + listFromEnvelope( + { + 'cases': [ + {'id': 'c1', 'name': 'Login fails'}, + ], + }, + const ['cases'], + ), + hasLength(1), + ); + }); + }); + + group('a labelled record', () { + test('a deal is named by its name field, not its title field', () { + // `title` exists on the row and is null on every deal the API returned. + // Reading it would label every option "Untitled". + final deal = EntityLookup.fromJson( + {'id': 'o1', 'name': 'Stand-alone throughput Deal', 'title': null}, + labelKeys: const ['name'], + ); + + expect(deal.label, 'Stand-alone throughput Deal'); + }); + + test('a row with nothing to show still carries its id', () { + final deal = EntityLookup.fromJson( + {'id': 'o2'}, + labelKeys: const ['name'], + ); + + // Selectable, and visibly odd. An empty label would render a blank row + // that looks like a rendering bug and cannot be tapped with confidence. + expect(deal.id, 'o2'); + expect(deal.label, 'Untitled'); + }); + }); + + group('a person', () { + test('is named first and last', () { + final lead = EntityLookup.person({ + 'id': 'l1', + 'first_name': 'Jill', + 'last_name': 'Shaffer', + 'email': 'jill@example.test', + }); + + expect(lead.label, 'Jill Shaffer'); + }); + + test('with only one name part does not carry a stray space', () { + final lead = EntityLookup.person({ + 'id': 'l1', + 'first_name': 'Jill', + 'last_name': '', + }); + + expect(lead.label, 'Jill'); + }); + + test('with no name falls back in order, email before company', () { + final lead = EntityLookup.person( + { + 'id': 'l1', + 'first_name': '', + 'last_name': '', + 'email': 'agarcia@example.test', + 'company_name': 'Cruz Group', + }, + fallbackKeys: const ['email', 'company_name'], + ); + + expect(lead.label, 'agarcia@example.test'); + }); + + test('with no name and no email falls through to the company', () { + final lead = EntityLookup.person( + {'id': 'l1', 'company_name': 'Cruz Group'}, + fallbackKeys: const ['email', 'company_name'], + ); + + expect(lead.label, 'Cruz Group'); + }); + + test('with nothing at all is still tappable', () { + final lead = EntityLookup.person({'id': 'l1'}); + + expect(lead.id, 'l1'); + expect(lead.label, 'Unnamed'); + }); + }); + + group('identity', () { + test('two rows with the same id are the same option', () { + // The picker compares options to mark the current selection. Comparing + // by label would tie two people who share a name. + expect( + const EntityLookup(id: 'x', label: 'One'), + const EntityLookup(id: 'x', label: 'Another'), + ); + }); + }); +}