# Auto-pay polymorphic payment-method cleanup — proposal

**Status:** NOT executed. Author recommendation: do this before Live cutover.
**Approval needed from:** admin@vmwebs.com

## The problem

Today `student_auto_pay_agreements.payment_method_id` is interpreted two different ways
depending on `provider`:

| provider      | payment_method_id references                       |
|---------------|----------------------------------------------------|
| `bambora`     | `payment_methods.id` (card)                        |
| `gocardless`  | `parent_bank_mandates.id` (bank mandate)           |

This was a deliberate shortcut taken in Phase 2 to avoid a schema migration mid-build.
It works today because the branching happens in exactly one place
([ProcessAutoPayments.php](../app/Console/Commands/ProcessAutoPayments.php#L106-L140)),
but it has three real risks:

1. **Silent cross-type ID collision.** `payment_methods.id = 12` and
   `parent_bank_mandates.id = 12` can both exist. A future join written against the
   wrong table (likely via a developer who doesn't know the convention) will quietly
   charge the wrong thing or report wrong data.
2. **No referential integrity.** There is no FK — the column can't FK to either table
   because the correct target depends on another column. A deleted mandate or card
   doesn't trigger any constraint; the agreement row keeps pointing at a dead id.
3. **Readability.** `payment_method_id` is a card-shaped identifier name. Reading
   agreement rows for GoCardless and seeing `payment_method_id=42` is misleading.

## Proposed change (one migration + one service edit)

### Schema

```sql
ALTER TABLE student_auto_pay_agreements
  ADD COLUMN payment_method_kind VARCHAR(16) NULL
    AFTER payment_method_id COMMENT 'card | bank — route for payment_method_id',
  ADD COLUMN bank_mandate_id BIGINT UNSIGNED NULL
    AFTER payment_method_kind
    COMMENT 'parent_bank_mandates.id when payment_method_kind=bank',
  ADD KEY sapa_bank_mandate_idx (bank_mandate_id);

-- payment_method_id stays non-null; its meaning becomes:
--   kind=card → references payment_methods.id (as today)
--   kind=bank → nullable; the real reference is bank_mandate_id

ALTER TABLE student_auto_pay_agreements
  MODIFY COLUMN payment_method_id BIGINT UNSIGNED NULL,
  ADD CONSTRAINT sapa_bank_mandate_fk FOREIGN KEY (bank_mandate_id)
    REFERENCES parent_bank_mandates(id) ON DELETE RESTRICT ON UPDATE CASCADE;

-- Backfill existing rows:
UPDATE student_auto_pay_agreements
   SET payment_method_kind = CASE provider
     WHEN 'bambora'    THEN 'card'
     WHEN 'gocardless' THEN 'bank'
     ELSE 'card'
   END;

-- Move the GoCardless rows' overloaded payment_method_id into bank_mandate_id.
UPDATE student_auto_pay_agreements
   SET bank_mandate_id   = payment_method_id,
       payment_method_id = NULL
 WHERE provider = 'gocardless' AND bank_mandate_id IS NULL;

-- Enforce the invariant.
ALTER TABLE student_auto_pay_agreements
  ADD CONSTRAINT sapa_route_chk CHECK (
    (payment_method_kind = 'card' AND payment_method_id IS NOT NULL AND bank_mandate_id IS NULL)
 OR (payment_method_kind = 'bank' AND bank_mandate_id IS NOT NULL)
  );
```

### Code

- [ProcessAutoPayments.php](../app/Console/Commands/ProcessAutoPayments.php): branch on
  `payment_method_kind` instead of `provider`; read `bank_mandate_id` for bank path.
- [StudentAutoPayAgreementService](../app/Payments/Services/StudentAutoPayAgreementService.php):
  `createAgreement` + `changePaymentMethod` take an explicit kind + reference id.
- Admin UI `AutoPaySetupDialog` / `AutoPayManageDialog`: expose the bank-mandate
  picker when the parent has an active mandate, mutually exclusive with the card
  picker.

### Rollout

1. Migration is additive — new columns + backfill, no data loss.
2. Deploy code that writes the new columns alongside the old ones for one release.
3. Flip readers to use the new columns.
4. In a second migration, drop the overloaded meaning entirely (make
   `payment_method_id` NULL when `kind=bank`, add CHECK).

## Why not do it right now

Three reasons:

- No live GoCardless auto-pay agreements exist yet — the cleanup is zero-risk later
  but adds schema churn right as you're trying to finish sandbox verification.
- The project convention is to attach a verbatim user-quoted approval to any
  schema-altering migration ([feedback_migration_approval_header](../../memory/feedback_migration_approval_header.md)).
  You asked for the design but not for the migration — I don't have an approval
  quotation yet.
- Multi-DB hygiene ([feedback_multi_db_migration_hygiene](../../memory/feedback_multi_db_migration_hygiene.md))
  — this needs to land in `stepuplivedb`, `stepupdemodb`, `stepup_test_db` same batch.

## When to execute

Immediately **before** Live cutover (step 5 of §13 in the previous final report), so
the cleanup lands on an empty-of-bank-agreements prod DB and no backfill decision has
to be made for GoCardless rows.