fix(admin-features): make admin writes use the field names consumers actually read - #11

app/server/services/admin-feature-migration.ts (2)

161-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider covering the name and active legacy spellings.

listServices and listServiceExtras in app/server/services/catalog-service.ts (lines 71-90 and 126-142) already fall back to d.name and d.active. Documents that carry those keys are therefore not broken for customers, but the migration does not add them to knownKeys. Such documents count as unrecognised in the runner summary, which makes the census harder to read. Run --census first and confirm whether name and active exist in the real collections.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/server/services/admin-feature-migration.ts` around lines 161 - 187, Check
the real collections with --census to confirm whether legacy name and active
keys exist, then update migrateServiceDefinition and migrateAddOn to map those
spellings to title and isAvailable and include them in knownKeys handling,
matching the existing display_name and is_active mappings.

---

205-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

discount_type is flagged even when the canonical value already exists.

If a document has both discount_type: 'buy_one_get_one' and discountType: 'PERCENT', line 208 still pushes discount_type into flagged. The runner then counts that document under "needs human decision" at line 202. Nothing would be written for that field in either case, because the canonical value is present and always wins. The report asks an operator to act on a document that needs no action.

Consider flagging only when discountType is absent.

♻️ Proposed change

   if ('discount_type' in doc) {
     const normalized = normalizeLegacyDiscountType(doc.discount_type)
-    if (normalized === null) {
+    if (normalized === null) {
+      if (!('discountType' in doc)) flagForHumanReview(doc, 'discount_type', ctx)
+    } else if ('discountType' in doc) {

Note: app/tests/admin-feature-migration.test.ts lines 146-156 pin the current behavior, so update that test together with this change.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/server/services/admin-feature-migration.ts` around lines 205 - 222,
Update the discount_type handling around normalizeLegacyDiscountType so an
invalid legacy value is flagged only when the canonical discountType field is
absent; when discountType exists, preserve conflict detection for differing
values and avoid adding the document to human review. Update the corresponding
admin-feature-migration test expectations to reflect this behavior.

app/tests/admin-feature-migration.test.ts (1)

26-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a conflict assertion to these two cases.

Both cases produce a conflict in mapField (lines 114-119 of app/server/services/admin-feature-migration.ts), not a silent skip. The tests assert only that patch stays empty. A regression that drops conflict reporting for service definitions would still pass.

💚 Proposed extra assertions

   it('maps notes to description only when description is absent', () => {
     expect(migrateServiceDefinition({ notes: 'n' }).patch.description).toBe('n')
-    expect(migrateServiceDefinition({ notes: 'n', description: 'd' }).patch.description).toBeUndefined()
+    const withBoth = migrateServiceDefinition({ notes: 'n', description: 'd' })
+    expect(withBoth.patch.description).toBeUndefined()
+    expect(withBoth.conflicts).toHaveLength(1)
   })
 
   it('never clobbers an existing canonical title', () => {
-    const { patch } = migrateServiceDefinition({ display_name: 'Legacy', title: 'Canonical' })
+    const { patch, conflicts } = migrateServiceDefinition({ display_name: 'Legacy', title: 'Canonical' })
     expect(patch.title).toBeUndefined()
+    expect(conflicts).toHaveLength(1)
   })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/tests/admin-feature-migration.test.ts` around lines 26 - 34, Add
assertions in both migration tests for the conflict reported by mapField when
notes and description, or display_name and title, conflict. Verify the
migrateServiceDefinition result preserves the expected conflict entry while
retaining the existing patch assertions.

app/server/scripts/migrate-admin-feature-fields.ts (2)

92-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider isolating per-document write failures.

A single updateOne rejection propagates out of runCollection, out of main, and into the catch at line 230. The process then exits with code 1. The summary for the collections already processed is never printed, and the operator cannot tell how far the run got. The migration is idempotent, so a partial run is recoverable, but only if the operator knows what happened.

Catch the error per document, record the _id in a failedIds list, and report it in the summary.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/server/scripts/migrate-admin-feature-fields.ts` around lines 92 - 125,
Isolate per-document failures in the updateOne call within the runCollection
loop so one rejected write does not abort processing or suppress the completed
summary. Catch the error for each document, record its _id in a failedIds list,
and extend the summary/reporting output to include those failed document IDs
while preserving existing migration counters.

---

183-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Report the _ids for invalid documents.

conflicting documents are listed by _id at line 188, so an operator can find and fix them. invalid documents get only a count. Line 96 discards result.invalid, which holds the offending legacy key names. The operator cannot locate a document with a malformed numeric field, so those documents stay unmigrated on every subsequent run with no way to act on the report.

Record the _id and the invalid field names, then print them like the conflict list.

♻️ Proposed reporting change

 interface CollectionSummary {
   ...
   conflictIds: string[]
+  /** _id -> legacy keys whose converted value was non-finite. */
+  invalidByDocId: Record<string, string[]>

-    if (result.invalid.length > 0) summary.invalid += 1
+    if (result.invalid.length > 0) {
+      summary.invalid += 1
+      summary.invalidByDocId[String(doc._id)] = result.invalid
+    }

     if (s.conflictIds.length > 0) {
       console.log(`    conflicting _ids: ${s.conflictIds.join(', ')}`)
     }
+    for (const [id, keys] of Object.entries(s.invalidByDocId)) {
+      console.log(`    invalid _id ${id}: non-numeric ${keys.join(', ')}`)
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/server/scripts/migrate-admin-feature-fields.ts` around lines 183 - 186,
Update the invalid-document handling in the migration flow to retain each
invalid document’s _id and offending legacy field names instead of discarding
result.invalid. Extend the summary/reporting logic near the existing
conflicting-document list to print the invalid entries in the same actionable
format, while preserving the current invalid count.

app/tests/admin-payload-reaches-consumers.test.ts (1)

67-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Give this block an explicit removal trigger.

These tests assert defects as expected behavior. Line 78 asserts that a service marked is_active: false is still shown to customers. The fixtures are hardcoded, so the block stays green after the migration repairs the real data. Nothing signals that the block is obsolete.

Two consequences:

1. The block never fails, so it cannot tell anyone when the damage is gone.
2. If someone later adds a display_name or is_active fallback to listServices in app/server/services/catalog-service.ts, line 71 and line 78 fail. A reader without context may revert the fallback instead of deleting the test.

Add a comment that names the tracking issue and the condition for deleting the block. State that the fix belongs in the migration, not in a consumer fallback.

Do you want me to open a follow-up issue to remove this block after --apply runs in production?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/tests/admin-payload-reaches-consumers.test.ts` around lines 67 - 86, Add
a clear comment immediately before the legacy admin payload test block
identifying the tracking issue and stating that the block must be removed after
the migration’s --apply run completes in production. Clarify that the corrective
fix belongs in the migration, not in consumer fallbacks such as listServices, so
future failures are interpreted correctly.

app/server/schemas/admin-features.ts (1)

103-126: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a cross-field check tying priceUnit to the matching price field.

ServiceDefinitionCreate lets priceUnit be 'FLAT' or 'HOURLY' while basePrice and hourlyRate stay fully independent optional fields. Nothing stops a client from creating a FLAT service with no basePrice, or an HOURLY service with no hourlyRate. The docstring says this schema exists specifically because services previously "rendered as 'Service' with no price," so this gap can reproduce the same symptom through the new canonical schema.

Confirm with pricing-service.ts whether it tolerates a missing price for the selected unit, or add a refine to require the matching field.

♻️ Proposed refine to require the matching price field

 export const ServiceDefinitionCreate = z
   .object({
     title: z.string().min(1),
     description: z.string().optional(),
     basePrice: z.number().nonnegative().optional(),
     hourlyRate: z.number().nonnegative().optional(),
     minimumHours: z.number().positive().optional(),
     maximumHours: z.number().positive().optional(),
     hourIncrement: z.number().positive().optional(),
     priceUnit: AdminPriceUnit.optional(),
     currency: z.string().min(1).optional(),
     isAvailable: z.boolean().optional(),
     checklist: z.array(z.string()).optional(),
     service_key: z.string().optional(),
   })
+  .refine((v) => v.priceUnit !== 'FLAT' || v.basePrice != null, {
+    message: 'basePrice is required when priceUnit is FLAT',
+    path: ['basePrice'],
+  })
+  .refine((v) => v.priceUnit !== 'HOURLY' || v.hourlyRate != null, {
+    message: 'hourlyRate is required when priceUnit is HOURLY',
+    path: ['hourlyRate'],
+  })
   .openapi('ServiceDefinitionCreate')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/server/schemas/admin-features.ts` around lines 103 - 126, Add a
cross-field validation refinement to ServiceDefinitionCreate that requires
basePrice when priceUnit is FLAT and hourlyRate when priceUnit is HOURLY, using
the pricing-service.ts contract to confirm the expected behavior. Ensure the
validation targets the relevant price field and preserves existing validation
for all other combinations, including omitted priceUnit.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes