# Research: Customer Management Re-execution (SP-04)

**Date**: 2026-06-07
**Spec**: [./spec.md](./spec.md)
**Plan**: [./plan.md](./plan.md)

This document records the research that resolved every "NEEDS CLARIFICATION" or open question before implementation. The re-execution guidelines are unusually prescriptive — most questions are already answered by the constitution, the BRD, the database schema, and the API manifest. The only decisions required at the planning level concern **how** to express the mandate in code, not **what** behavior to implement.

---

## R-1. Where the `Client` model should live

**Question**: Constitution Section 4.7 mandates a domain layout (`app/Domains/{Domain}/Models/`) but the existing project has `Client` at the top-level `app/Models/Client.php`. Should the rewritten model move into the domain?

**Decision**: Move the `Client` model to `src/app/Domains/Customer/Models/Client.php`. The Client entity is referenced by all customer-domain code, and placing it in the domain is consistent with the rest of the structure. References from other domains (Agent, Contract) will need updating, but the move is the right architectural call because:
1. The domain owns the entity's behavior (immutability protection, flag helpers).
2. Other domains that need `Client` can still import it (cross-domain imports are not forbidden by the constitution; only the routing/exception patterns are domain-isolated).

**Rationale**: Constitution Section 4.7 shows the canonical model location inside the domain. The previous top-level placement was an artifact of the earlier architecture where a single `User` table existed.

**Alternatives considered**:
- **Keep at `app/Models/Client.php`**: rejected because it would split ownership of the model between the domain (Services/Controllers referencing it) and the global namespace, and would violate the visual scanability of the mandated layout.

---

## R-2. How to model the `customer_listing_mv` materialization

**Question**: Should `customer_listing_mv` be a Model, a DBAL query, or a custom Repository? (Repository is forbidden by the constitution.)

**Decision**: Create a dedicated Eloquent Model `src/app/Domains/Customer/Models/CustomerListingMv.php` that points to the `customer_listing_mv` view, with the casts from the constitution's example and a `scopeSearch`, `scopeFirstContractFrom`, `scopeFirstContractTo`, and `scopeOrderByNextDue` set of Scopes for the searchable/filterable fields.

**Rationale**: The constitution itself gives this exact pattern in Section 14. Using a Model + Scopes satisfies the "Model Scopes for search and filtering" rule (Section 2.I) and avoids the forbidden Repository pattern.

**Alternatives considered**:
- **Raw `DB::table('customer_listing_mv')` queries in the Service**: rejected — would violate "no raw queries in Controller" and push query logic out of the reusable Scopes area.
- **Repository wrapping the MV**: rejected — Repository pattern is forbidden.

---

## R-3. The shape of the live financial summary query

**Question**: The spec says "compute the financial summary at request time via a live query on `installments` joined to `contracts` where `contracts.status IN ('active', 'completed')`". Should the query live in the Service, the Installment Model, the Contract Model, or as a Scope on Installment?

**Decision**: Live as a single Eloquent query inside `CustomerService::getCustomerDetail()`. No Scope on Installment, because:
- A financial summary is a derived read-only projection; it's not a reusable filter the way `scopePending` etc. are.
- Moving it to a Scope would require a Scope that takes a `client_id` and returns aggregates, which is awkward.
- Keeping it in the Service is consistent with the constitution's "Service = business logic" rule.

**Rationale**: A single `DB::table('installments')->join('contracts', ...)->selectRaw(...)` query (or Eloquent equivalent) inside the Service is the cleanest expression of the requirement.

**Alternatives considered**:
- **Add a `scopeFinancialSummaryFor($clientId)` on Installment**: rejected — Scopes should not return aggregate projections; they should be composable filters.
- **Add a static helper on a Contract repo**: rejected — Repository pattern is forbidden.

---

## R-4. How to apply `permissions:customers.<action>` middleware

**Question**: The project uses `spatie/laravel-permission`. Does its middleware need configuration to work with the `admin` guard?

**Decision**: Use Spatie's middleware exactly as `permissions:customers.view` etc. on each route. The middleware reads the user's permissions via the guard configured in `config/permission.php` (the project already uses `guard_name = 'admin'` per `DB_Schema.md` Section 2).

**Rationale**: This is the standard Spatie middleware and the project already wires it (per existing Auth routes that I'll verify during implementation). No additional setup required beyond `auth:sanctum` first, then the permission middleware.

**Alternatives considered**:
- **Custom middleware**: rejected — would be redundant with the standard Spatie middleware.

---

## R-5. How to register the domain Service Provider

**Question**: The existing `CustomerServiceProvider` only loads routes; it does not register in `bootstrap/providers.php`. Where should the new version go?

**Decision**: Add `App\Domains\Customer\CustomerServiceProvider::class` to the `providers` array in `src/bootstrap/providers.php`. The Service Provider's `boot()` method calls `Route::middleware('api')->prefix('api/v1')->group(__DIR__ . '/Routes/v1/api.php')` (note: Laravel 11 applies the `api` middleware group automatically when loading routes from a service provider that calls `$this->loadRoutesFrom()`, so the explicit `Route::prefix(...)` is the alternative pattern shown in the constitution).

**Rationale**: Constitution Section 4.7 mandates this exact pattern. Verification during implementation will confirm whether the existing `Route::prefix('api/v1')` syntax works inside the Service Provider or whether `$this->loadRoutesFrom(__DIR__ . '/Routes/v1/api.php')` is required.

**Alternatives considered**:
- **Auto-discovery via `composer.json` extra section**: rejected — not configured in this project; the manual `bootstrap/providers.php` registration is the established pattern.

---

## R-6. How to shape the `ApiResponseHelper` to match the constitution

**Question**: The existing `ApiResponseHelper` returns `status` (not `success`) and wraps error `message` as an array. The constitution requires `success: true|false` and a string `message` (Section 4.1). What is the minimal rewrite?

**Decision**: Modify the `_response()` function in `src/app/Helpers/ApiResponseHelper.php` to:
- Replace the key `status` with `success`.
- Keep `message` as a string for both success and error.
- For `error()`: pass the string through directly (no array wrap).
- Keep all named helper functions unchanged in signature so callers don't break.

**Rationale**: The constitution's response format is explicit and is the highest-priority reference (Priority 1). The current code violates it. The fix is small and surgical.

**Alternatives considered**:
- **Create a new helper alongside the old one**: rejected — the constitution requires a single `ApiResponseHelper` source of truth; coexistence would violate "ALL responses via ApiResponseHelper" (Section 4.5).
- **Wrap the helper to convert `status` → `success` at the boundary**: rejected — pushes complexity into every caller; better to fix the source.

---

## R-7. How to wire `ExceptionMappings.php` into Laravel 11

**Question**: The constitution mandates `ExceptionMappings::map()` registered via `bootstrap/app.php`'s `withExceptions()`. The existing project uses a custom `ExceptionHandler.php` and a `config/ExceptionClassToMethod.php`. What is the migration path?

**Decision**:
1. Create `src/app/Exceptions/ExceptionMappings.php` with a `map()` static method that returns the array described in the constitution.
2. Edit `src/bootstrap/app.php` to add:
   ```php
   ->withExceptions(function (Exceptions $exceptions) {
       foreach (\App\Exceptions\ExceptionMappings::map() as $class => $handler) {
           $exceptions->map($class, $handler);
       }
   })
   ```
3. Delete `src/app/Exceptions/ExceptionHandler.php`, `src/app/Exceptions/ApiResponseException.php`, `src/app/Exceptions/ApiRateLimitException.php`, `src/app/Exceptions/ApiConnectionException.php`, and the entire `src/app/Exceptions/Custom/` directory.
4. Delete `src/app/Helpers/CostumErrorResponse.php` and `src/config/ExceptionClassToMethod.php`.

**Rationale**: This is the explicit pattern in constitution Section 12. The deletions remove forbidden files and reduce the exception surface to a single source of truth.

**Alternatives considered**:
- **Keep the custom ExceptionHandler as a fallback**: rejected — the constitution is explicit that this is a Laravel 11 standard, no custom layer.
- **Gradual migration (keep some Custom exceptions, replace others)**: rejected — would leave inconsistent error response shapes.

---

## R-8. The order of operations for `markAsCustomer()` and `save()`

**Question**: The spec says "use `save()` and not `saveQuietly()`". The `Client` model needs to add the `customer` flag in the same transaction as the create. How should the Service coordinate this?

**Decision**: In `CustomerService::createCustomer()`:
```php
DB::transaction(function () use ($request) {
    $client = Client::create([
        'name' => $request->input('name'),
        'phone' => $request->input('phone'),
        'description' => $request->input('description'),
        'client_type_flags' => ['customer'],  // direct set in create
    ]);
    if ($request->hasFile('avatar')) {
        $path = $request->file('avatar')->store('avatars', 'public');
        $client->avatar = $path;
        $client->save();  // fires events
    }
    return $client;
});
```

For the helper `markAsCustomer()` (used by other domains in the future, e.g., when a contract is created for an existing Client who isn't yet a customer):
```php
public function markAsCustomer(): void
{
    $flags = $this->client_type_flags ?? [];
    if (!in_array('customer', $flags, true)) {
        $flags[] = 'customer';
        $this->client_type_flags = $flags;
        $this->save();  // NOT saveQuietly — events must fire
    }
}
```

**Rationale**: Setting the flag directly in the `create()` call is one query and one transaction. Using the helper for future cross-domain use is consistent with the constitution's intent that flag changes happen "in the same database transaction" (Section 3.2).

**Alternatives considered**:
- **Create the Client without the flag, then call `markAsCustomer()`**: rejected — adds an extra query without benefit when the create flow already knows the new row is a customer.

---

## R-9. Test isolation with the Materialized View

**Question**: The list endpoint reads from `customer_listing_mv`, which is a pre-computed view. In tests, how do we ensure the list reflects newly created customers?

**Decision**: In Feature tests, after seeding test data, call `Artisan::call('db:seed', ['--class' => 'RefreshCustomerListingMv'])` OR use a `REFRESH MATERIALIZED VIEW customer_listing_mv` SQL command directly via `DB::statement()` in a test helper. The Constitution's Section 14 schedule (every 5 minutes) is for production; tests must refresh on demand.

**Rationale**: The MV is a deliberate performance choice and tests must not bypass it. The standard practice in Laravel projects with MVs is to either (a) call the refresh command in `setUp()` of feature tests, or (b) wrap tests in a database transaction (Laravel's `RefreshDatabase` trait). For the list endpoint specifically, (a) is required because reading from a stale MV in a test would defeat the assertion.

**Alternatives considered**:
- **Use `RefreshDatabase` only**: rejected — would still hit the stale MV.
- **Skip the MV in tests and read from a live query**: rejected — would not match production behavior, violating SC-002 (performance) and SC-003 (filter works through MV).

---

## R-10. The data seeder for Admin permissions

**Question**: The spec says "all three customer permissions must be assigned to the existing Admin user". What's the convention for identifying "the current Admin"?

**Decision**: In `CustomerPermissionsSeeder::run()`:
```php
$admin = \App\Domains\Auth\Models\Admin::first();  // the first admin ever created
$permissions = ['customers.view', 'customers.create', 'customers.update'];
foreach ($permissions as $p) {
    Permission::firstOrCreate(['name' => $p, 'guard_name' => 'admin']);
}
$admin->syncPermissions($permissions);  // idempotent: ensures exactly these 3
```

**Rationale**: "The current Admin" in the re-execution context means the single pre-existing admin created by SP-01. Using `Admin::first()` matches the one-admin-per-system convention implied by the constitution (Section 1: "Admin (System Manager)"). The seeder is idempotent so re-running it is safe.

**Alternatives considered**:
- **Find by hard-coded email**: rejected — would couple the seeder to a specific seed value.
- **Loop over all admins**: rejected — constitution implies a single admin; future role expansion is SP-01's scope.

---

## R-11. Avatar file cleanup safety

**Question**: When updating a customer with a new avatar, the old file should be deleted from storage. What if the new upload succeeds but the DB update fails?

**Decision**: Delete the old file **only after** the database row update succeeds. If the DB update fails, the new file is left as an orphan in storage (acceptable trade-off; storage cleanup is not transactional with the DB). This is the standard Laravel approach.

**Rationale**: Trying to make storage cleanup transactional with the DB is over-engineering for this feature. The constitution does not require transactional storage. The simpler "delete old on successful update" is sufficient.

**Alternatives considered**:
- **Upload new file to a temp location, then move on DB success**: rejected — adds complexity not required by the spec.
- **Use Laravel's `Storage::delete()` before the update (delete first, then update)**: rejected — if the update fails, the user has lost their old avatar with no replacement.

---

## R-12. Response shape for the list endpoint with cursor pagination

**Question**: The spec says "Response contains each customer's data + `total_remaining_amount`" and "use cursor pagination". What is the wire format?

**Decision**: Use Laravel's `->cursorPaginate(20)` which returns a `CursorPaginator`. Pass it through `ApiResponseHelper::success()` with:
```json
{
  "success": true,
  "message": "...",
  "data": [ { "id": ..., "name": ..., "phone": ..., "avatar": ..., "first_contract_date": ..., "next_due_date": ..., "total_remaining_amount": ... }, ... ],
  "meta": {
    "path": "...",
    "per_page": 20,
    "next_cursor": "eyJ...",  // opaque base64-encoded cursor
    "prev_cursor": null
  }
}
```

**Rationale**: Standard Laravel cursor pagination shape, wrapped in the constitution's `data` + `meta` envelope. No `total` (which is the whole point of cursor pagination — cheaper at scale).

**Alternatives considered**:
- **Return the `CursorPaginator` raw and let the resource do the wrapping**: rejected — the controller must use `ApiResponseHelper`.
- **Custom `meta` shape**: rejected — Laravel's built-in cursor metadata is well-understood by API consumers.

---

## R-13. Phone number uniqueness rule conflict with own record

**Question**: When updating a customer, the phone uniqueness check should ignore the current record. The form-request validation must compare against `clients.phone` excluding the current `id`.

**Decision**: In `UpdateCustomerRequest::rules()`:
```php
return [
    'phone' => ['sometimes', 'string', 'regex:/^0?9\d{8}$/', Rule::unique('clients', 'phone')->ignore($this->route('id'))],
    // ...
];
```

**Rationale**: Standard Laravel pattern for "unique except this row". The `regex` enforces the Syrian local format from clarification Q1.

**Alternatives considered**:
- **Validate uniqueness only when the phone has changed**: rejected — extra logic in the Service, not idiomatic; the `Rule::unique()->ignore()` approach handles it automatically.

---

## R-14. Where to find the Admin model for the permission seeder

**Question**: The seeder needs to attach permissions to the Admin. The existing project has `src/app/Domains/Auth/Models/Admin.php`.

**Decision**: Import `App\Domains\Auth\Models\Admin` in the seeder. The seeder lives at `src/database/seeders/CustomerPermissionsSeeder.php` and is registered in `DatabaseSeeder.php`.

**Rationale**: The Admin model already exists in the Auth domain (from SP-01). Cross-domain model imports for shared entities are acceptable per the constitution (the model itself is `Clients` which is shared; only routes/responses are domain-isolated).

**Alternatives considered**:
- **Use `Spatie\Permission\Models\Role` to create an "admin" role and attach permissions to the role**: rejected — the constitution describes a single Admin user with permissions directly attached, not a role-based system (RBAC is in scope of SP-01, not SP-04).

---

## R-15. The order of cleanup operations in the `update` flow

**Question**: The `update` flow does (1) update Client, (2) handle avatar. What's the precise order?

**Decision**:
```php
$client = Client::findOrFail($id);  // throws ModelNotFoundException → 404 via helper

DB::transaction(function () use ($client, $request) {
    $oldAvatar = $client->avatar;

    $client->fill($request->only(['name', 'phone', 'description']));
    // client_type_flags is NEVER modified here
    $client->save();

    if ($request->hasFile('avatar')) {
        $path = $request->file('avatar')->store('avatars', 'public');
        $client->avatar = $path;
        $client->save();

        if ($oldAvatar) {
            Storage::disk('public')->delete($oldAvatar);
        }
    }
});
```

**Rationale**: Fill then save (single save when no avatar change); if avatar is replaced, a second save updates the column, then the old file is deleted. Wrapping in a DB transaction ensures the data update is atomic; the storage delete is a best-effort after-success operation.

**Alternatives considered**:
- **Use `$client->update([...])` directly**: rejected — `update()` doesn't fire all the same events; `fill() + save()` is more explicit and aligned with FR-022 (`save()` not `saveQuietly()`).

---

## R-16. Acceptance of risk: 5-minute MV staleness in tests vs production

**Question**: SC-002 says "first page within 1s for 10,000 customers" — but tests with only 10 records will complete much faster. Is this a problem?

**Decision**: No. SC-002 is a production scalability target; tests assert correctness (order, pagination, filter) not raw performance. The MV index `idx_customer_listing_mv_next_due` is the production safeguard.

**Rationale**: Constitution Section 2.V defines testing for behavior, not performance. Performance tests are typically a separate concern (k6, JMeter) and are out of scope for SP-04.

**Alternatives considered**:
- **Add a `ClockMock` or large dataset test**: rejected — adds complexity for no concrete validation benefit at this stage.

---

## R-17. How to ensure the "no delete endpoint" rule is verifiable

**Question**: The spec is explicit that no DELETE route should exist. How is this verified at code review / CI?

**Decision**: The route file contains only `GET`, `POST`, `PUT` — by inspection. A simple `grep` for `Route::delete.*customers` in the `Routes/v1/api.php` would yield zero matches. No additional test required (the route file is small enough to inspect).

**Rationale**: The constraint is structural, not behavioral. A negative test ("DELETE returns 404") would be misleading because the absence of the route IS the requirement.

**Alternatives considered**:
- **Add a feature test that hits `DELETE /api/v1/customers/1` and asserts 404**: rejected — would create a route to test, but the route is supposed to not exist; Laravel's auto-generated 405 for missing routes is the implicit verification.

---

## Summary

All 17 research questions resolved. No unresolved "NEEDS CLARIFICATION" items remain. The implementation can proceed to Phase 1 (data-model.md, contracts, quickstart.md) and then Phase 2 (tasks.md, generated by `/speckit.tasks`).