[icelessgo]

Behaviour and scope

Clients configure shared catalogue products from their gallery or while booking included/optional products on a quote. Quotes keep their offered definitions and prices; acceptance freezes the chosen specification for contracts and invoices. A later gallery choice becomes a new billable product item.

Implements the direction agreed with Luca-Timo in #2222 (comment) and contributes to #1924. Gallery and CRM work remain together in this draft. Album layout/proofing/export, payment collection and supplier ordering remain separate.

• One editable option/image library, structured width × height + mm/cm/in sizes, basic colours, admin-defined product types and a shared picker. Attributes support required/optional choices, multiple selection and one prerequisite choice; invalid references and cycles are refused.
• CRM catalogue items own base prices and option surcharges. Direct items, package children and published templates reuse the shared product. Authors can pre-fill choices and lock a complete agreed specification. Server totals apply native quantities, discounts, rounding, VAT and shipping.
• With CRM billing enabled, locking a gallery choice creates an audited customer/project item at the current catalogue price. Exactly one compatible unissued, unarmed project invoice receives a separate frozen line; otherwise it remains open for explicit billing. Issued invoices require native cancellation/Storno. Gallery price display defaults off and is informational until lock.
• Frozen specifications and images survive catalogue changes. Existing storage adapters, backups and portable transfers cover the image objects. Normal and archived gallery deletion erase client choices/history explicitly while financial records survive with their gallery link cleared.

Review fixes and decisions

• 26 October follow-up: existing ordinary quote lines retain their authored price and plain status after their catalogue preset is linked to a product. Published templates freeze the absence of configuration too, including package children; new catalogue lines and explicitly republished templates still use current definitions/prices. Ten new regressions cover these boundaries.

• Percentage promotions on unissued invoices are documented as being recalculated when gallery products are appended. The route-scanner comment now describes only locally declared named routers. Accepted-quote amendment screenshots are embedded below: desktop, phone.

• Archived ordinary catalogue entries continue to work in existing quotes and previously published templates, including with the configurator disabled. Editing an unchanged product association no longer revalidates a disabled product/option. New associations, option-price changes and new definitions still require an active product and the feature; configurable catalogue items retain fixed pricing.

• Gallery ownership now guards lock/unlock independently of the billing flag, protecting retained financial items after billing is disabled. Assigned admins do not receive owner billing controls. Public/portal quotes expose the configuration lock; gallery client reads/writes omit internal customer/catalogue IDs.

• Identical acceptances and concurrent identical amendments produce one selection-history entry, PDF and relevant email. Malformed configuration entries return 400 at HTTP and domain boundaries. Conditional invoice-item stamps must affect one row or roll back with 409.

• Invoice appends use the shared totals calculation, including percentage discounts, rounding and shipping. Configured package parent totals are persisted consistently with quote PDFs and contract snapshots.

• The route inventory includes all six event subrouter routes; its scanner now detects named routers. Backup-path expectations, editable seeded catalogue classification and affected overview test fixtures are updated. Catalogue viewers get a complete read-only view; legacy controls use shared labelled inputs/switches and retry buttons.

• Template prices freeze at publication. New quotes from a published version keep its product base prices and surcharges. Publishing a new version picks up current catalogue prices.

• Admins configure newly booked optional products in the existing accepted-quote add-on amendment screen, including package children. Saving recomputes the offered price, records history, retains another accepted PDF and emails the updated quote. Existing accepted specifications remain read-only; changing one requires reissue. Studio-locked prefilled offers retain their agreed specification while unbooked. Picker instances have independent radio groups.

• Existing public/portal quote images remain available with the configurator flag off, behind normal document verification/customer ownership checks.

Migrations

Rebased onto main 191acb36 (including #2246 and #2256). The later 94acb7bb main update changes release metadata/version labels only; this branch remains conflict-free. Unreleased migrations are now 320–322, avoiding 316 in #2232, 317–318 in #2233 and 319 in #2249:

• 320: catalogue/choice storage, disabled albumProducts flag and final products.view, products.manage, products.configure permissions.
• 321: shared library/types and CRM configuration fields.
• 322: quote locks, gallery pricing/customer links, audited billable items, retained image references and invoice revision guard. Schema/trigger creation and backfill retries tolerate interrupted SQLite upgrades; PostgreSQL uses guarded trigger/function creation.

Existing workflow documentation records these decisions. No additional prose documentation was introduced in this review follow-up.

Validation

Rebase verification on 8830b383: full GitHub CI is green, including the backend/frontend suites, browser smoke tests, AMD64/ARM64 image builds, clean install and upgrade checks. Local rebase checks also passed: 170 affected backend tests, 31 frontend tests, full TypeScript checking, production build and frontend lint with existing warnings. All eleven PR commits were retained. The quote-page conflict was resolved by preserving main's position-based parent handling; migrations 320–322 remain available. There are no unresolved review threads. Draft/merge state is left to the owner and maintainers.

Database checks use real core migrations, not stub schemas, and synthetic fixtures.

26 October follow-up (2e8c6501): 170 SQLite tests across nine suites and 49 PostgreSQL 15 CRM tests pass under Node 22.23.3. Nine new preservation cases first failed on e2e5dce6; a tenth positive-control case confirms new catalogue lines and republished templates still adopt current definitions/prices. Backend and touched-test lint pass with one existing no-console warning in unrelated source; whitespace and staged credential checks pass. No frontend or migration changes in this follow-up, and no live deployment.

Earlier validation (before this backend follow-up):

• SQLite: 182 tests passed across 14 suites. productConfiguratorCrm (39), albumProducts (13), migration recovery (2), plus 128 related quote/add-on/template/reissue, API/access, backup and privacy checks. Eleven initial regression cases were also run against the original PR head and failed there before the fixes.
• PostgreSQL 15: 54 tests passed on fresh isolated databases: productConfiguratorCrm (39), albumProducts (13), migration recovery (2). Includes deterministic concurrent duplicate submissions, stamp rollback, financial permissions, public/portal projection and partial-backfill retry.
• The full backend run in a Node 22 container passed 10,059 tests in 866 suites; four suites were skipped by their normal guards. The remaining SSH suite initially could not start because the container lacked ssh; after installing OpenSSH in the local validation image, all 56 SSH tests passed. Final focused runs above cover subsequent amendment changes.
• The full frontend run passed 3,076 tests in 448 files, followed by 31 final focused tests in six files covering the amendment picker, independent product instances, catalogue/client flows, permission controls and public quotes. The affected overview fixture also passes its six tests.
• Backend/frontend lint and the production frontend build pass with existing warnings. Full TypeScript checking now passes after integrating fix: restore the frontend TypeScript check #2246.
• Chromium checked the actual UI with synthetic API fixtures at 1440px and 390px: required choices block saving, the amendment sends the frozen definition version and selected options, no page errors/overflow, and catalogue viewers cannot save. Backend HTTP tests separately exercise native authentication and migrated databases.

GitHub checks for the updated head are separate from these local results. The PR remains draft for maintainer review.

Screenshots

New accepted-quote add-on booking, with required choices and offered option surcharges:

Phone layout and read-only catalogue

Earlier gallery lock/billing and quote authoring views:

[Luca-Timo]

Thanks a lot for picking this up, and for the care that went into it! The revision and retry handling, the PNG re-encode path, the event scoping on the admin routes and the backup coverage are really solid, and the docs page made the PR easy to follow. I ran the branch locally end to end (catalogue → assign to a gallery → client submits → lock → client sees it read-only) to get a feel for the flow.

Your draft also helped us think through the open questions in #1924, and in doing so we realised we need something broader than the issue described. So before you spend more time polishing this shape, here's where we'd like to take it. Very happy to discuss!

Where we'd like to take #1924

What we need is a generic product configurator, not one tailored to albums. Products with several attributes and optional add-ons should be modelled freely by the admin. To illustrate, one example among many: a large-format print in 60×40 / 90×60 / 120×80, with an optional wood frame (oak / walnut / black) and an optional passe-partout. A photobook with size, cover, paper and page count would be another. Neither fits exactly one model and one colour per product, and the design shouldn't assume any particular product. So:

1. Shared option library instead of per-product lists. Admins maintain standard values once (colours, sizes, frames, materials, papers, …) and products pick from them. Today every product re-types its colours and re-uploads their images.
2. Standard sizes / formats with editable presets. Sizes should be structured (width × height + unit), not free-text model names. They should ship with a preset set (common photobook and print formats, a basic colour range) that the admin can change, extend or deactivate.
3. Generic products with their own specs. Albums are one case, not the model for all. A product type declares which attributes it uses and whether each is required, optional or multi-select; the admin defines both the types and the attributes. A new kind of product should be configuration, not code.
4. Add-ons on a product. Beyond pick-one attributes, a product needs optional extras the customer can select, each with its own options and an optional surcharge. Some will depend on another choice (in the example above, a passe-partout only with a frame).
5. Works with the CRM catalogue and presets. The same library and products should be usable in the quote catalogue: as a catalogue item or package line, with the option values and presets coming from the same source rather than a parallel list. Prices and per-option surcharges live there.
6. "Offer as add-on" must be supported. A configured product should work as an optional quote line ("Als Zusatzleistung anbieten") that the customer books on the quote page and configures while booking. The accepted quote then freezes the chosen spec, and contract and invoice lines carry it (for the example above: "Print 90×60, oak frame, passe-partout").

We understand keeping it separate from quotes was deliberate, because historical quote contents must not change. Snapshot-on-accept covers that: the accepted quote keeps a frozen copy, the same way your choice history already does. Choosing in the gallery after the photo selection can stay as an additional entry point that uses the same picker and the same snapshot.

Things to fix regardless of direction

Blockers

1. Migration number collides with feat(projects): new inquiry, and a project page that runs the job #2216. backend/migrations/core/315_album_product_configurator.js and feat(projects): new inquiry, and a project page that runs the job #2216's 315_project_inquiry_fields.js share 315, and feat(projects): new inquiry, and a project page that runs the job #2216 is cleared to merge. Please rebase once it lands and renumber to the next free number, updating anything that cites the path.
2. Client submit fails on PostgreSQL.
   • backend/src/services/albumProducts/service.js:279-292 inserts into activity_logs directly with actor_id: String(actor.id || '').
   • For a client, actor.id is the 64-hex session digest (routes/gallery/albumProducts.js:67-78), but activity_logs.actor_id is an integer column (src/database/db.js:523).
   • So Postgres rejects the insert, the transaction rolls back, and every client submit returns 500.
   • Neither test run can catch this: SQLite accepts the string, and the domain suite's stub schema declares actor_id as a string (__tests__/integration/albumProducts.test.js:27).
   • Fix: go through logActivity(…, tx), which coerces non-integer ids to null and keeps the name. Run the suite against the real core migrations on Postgres (DATABASE_CLIENT=pg) rather than a hand-built schema.
3. Client panel renders outside the gallery layout.
   • GalleryView.tsx:1729 and :1960 mount GalleryProductPanel as a sibling before <GalleryLayout>, so it sits above the gallery header (title, Download, Logout).
   • On a phone, the client scrolls about two screens before the first photo. In the Story layout the fixed nav overlaps it.
   • It belongs inside the layout's content area.
4. Deleting a gallery leaves its choices behind on SQLite.
   • The event delete in routes/adminEvents/helpers.js (~L216-255) removes each child table explicitly, because SQLite runs without PRAGMA foreign_keys.
   • event_album_products and album_product_choice_history aren't on that list, and they hold the client-typed name and session digest. Those outlive the gallery, although docs/album-products.md says deletion cascades.
   • Fix: add hasTable-guarded deletes there (history first), with a test.

Concerns

5. Buttons have no padding. GalleryProductPanel.tsx uses bare btn btn-primary / btn btn-secondary classes. .btn (index.css:85) carries no size, so "Save draft" and "Submit choice" render edge-to-edge and nearly touch. Use the shared <Button>.
6. Lock is blocked by any edit to the product, and the client is never told.
   • Lock re-validates against a hash of the whole product (service.js:247-258, hash(product) in validation.js).
   • So editing even the description after a submit makes "Lock choice" fail with "The product options changed" until the client resubmits, while the client still sees "Submitted".
   • Either hash only the chosen options, or tell the client to review their choice.
7. Submitted → draft is silent. A client re-saving with submit: false moves a submitted choice back to draft with no activity entry (service.js:265, :277). The bell still says "submitted", and a later lock returns 409. Log the retraction or refuse it.
8. SQLite boolean no-op check. service.js:270 compares current[key] === value. SQLite returns enabled as 1/0, so on SQLite only, re-sending the same value bumps the revision and writes a history row. Normalise with normalizeBoolean and write with formatBoolean.
9. Unused permissions are seeded and granted. Migration 315 seeds albums.view and albums.export and grants them to admin roles, but nothing references them. Please drop them until a feature uses them.
10. Only client access sees the picker. The picker appears for the separate client-access link + PIN or an assigned portal account, not for the normal gallery password. That may be the right call. But the portal dashboard and event page never mention the album, so a portal customer has no hint it exists. Worth stating as a decision and adding a pointer in the portal.
11. Notification is generic. The bell shows "System activity: album product submitted" with no gallery or choice, although the metadata carries both.

UI/UX guide (frontend/STYLING.md, frontend/UX.md)

• Client picker: options should be selectable tiles with a selected state (UX §6) rather than bare radios. The name field should be <Input themed>. Use border-border-token for borders and rounded-xl / rounded-lg for corners.
• Admin components: use <Switch> instead of raw checkboxes for Active, and <Input label> / <Button variant="ghost"> instead of raw elements. Show Draft/Submitted/Locked as a <Badge>. Use text-danger-text or <Notice tone="danger"> for errors.
• Admin page states and actions: add loading, error and empty states (UX §4), keep one primary action per view, and give the product name field a visible label.
• Page header: use SectionPageHeader feature=… so the experimental badge shows. Drop albumProducts from TABS_WITH_OWN_HEADER and add it to TAB_FEATURE.
• Read-only access: products.view holders get a bare line instead of a read-only catalogue (UX §3).
• Invalid token: ProductImage.tsx:17 uses bg-panel-subtle, which isn't a token.
• Small cleanups: model.ts:44 hard-codes 'Please try again.', and EventProductsCard fetches with useState/useEffect instead of TanStack Query.
• i18n: common.name, common.description, albumProducts.featureTitle and albumProducts.featureDescription are missing from both locales. The common.retry fallback says "Try again" where en.json says "Retry".

Migrations

• The 109 edit: the album-products row added to 109_add_backup_paths.js follows the existing pattern, since boot reseeds defaults from 109. That makes the backup_paths insert in 315 redundant; keep one of them.
• Missing guard: in 315, guard the roles query with hasTable.
• down(): in 315, remove the backup_paths row.

PR body

• Test counts: the body says "54 focused backend checks", "9 Postgres" and "10 frontend checks". I count 14 tests in the two new backend suites (44 including the touched walker suite), 10 in the domain suite and 4 frontend tests. Please list the suites with their counts, and say that the Postgres run used a stub schema.
• Unstated decisions: please name them in the body: migration 315 (four tables, a flag row, a backup path), the 109 edit, the new albums permission category, and where the client panel mounts.

Happy to talk through the direction on #1924 before you put more time into this shape.

[icelessgo]

Thanks for the detailed review. I agree with the broader direction and am revising this draft accordingly. CRM integration is in scope now, alongside the standalone gallery entry point.

Proposed shared contract:

• One admin-maintained option library (including images and structured width/height/unit sizes), editable/deactivatable presets, and configurable product types. Product attributes reference library values; each declares required/optional and single/multiple selection. Products can include optional add-ons with options and acyclic dependencies on other choices. No album-specific assumptions in the new picker or validation.
• The CRM catalogue item references that product and owns its base price and option/add-on surcharges. Package lines and quote presets reuse the same reference. Gallery choices do not require CRM activation and do not charge a customer.
• Quotes copy the offered configuration definition and CRM prices into their line when authored. Customers configure both included and optional lines on the existing verified quote page; only booked lines require a complete configuration. The server validates dependencies, recomputes all surcharges and the quote total, and checks the total the customer confirmed.
• Acceptance atomically freezes the selected specification (IDs, labels, dimensions, images, and price breakdown) on each booked line, using the same selection snapshot as the gallery. Contracts and invoices carry that frozen specification. Later library/price edits never rewrite sent offers or accepted documents. The existing response-window and conversion locks remain authoritative.
• The gallery picker remains restricted to primary-client/assigned portal access, within gallery content, with a portal pointer. Gallery drafts/submissions/photographer locks remain separate from a legally accepted CRM specification; changing one cannot silently amend the other.

I will first land the four blockers and regression coverage (including actual core migrations on PostgreSQL), then the shared catalogue/picker and CRM path in reviewable commits on this draft. The album layout/proofing/export work remains separate from this product foundation. I will also address the native UI, notification, permission and migration concerns you listed.

Does this match the intended ownership and snapshot boundaries? I am proceeding with those boundaries while keeping the PR draft so adjustments remain reviewable.

[icelessgo]

The four blockers are fixed in 389447c, rebased onto fb6702f (including the inquiry change):

• Renumbered to 316_product_configurator.js; removed unused view/export permissions, redundant backup-path insertion, added role-table guards and rollback cleanup.
• Client submission uses native logActivity(..., tx) with the real integer actor column. The domain suite now boots all core migrations rather than a hand-built schema.
• The normal gallery entry is inside GalleryLayout; full-page layouts place it after their photo content. The picker is a compact collapsed entry until opened. Main actions use shared Buttons and a themed Input, with selected tiles.
• Native gallery deletion explicitly removes choice history then offers, including names/session digests when SQLite foreign keys are off.

Also covered: description-only changes do not block locking; submitted choices cannot silently return to draft; boolean no-op writes do not create fake revisions.

Validation for this commit: albumProducts.test.js 12 tests with the actual core migrations on SQLite and PostgreSQL (DATABASE_CLIENT=pg); albumProducts.access.test.js 4 tests on the real SQLite schema. The deletion test also confirms a different gallery's history survives. This replaces the earlier PostgreSQL result based on a stub schema.

The shared generic catalogue/CRM design is proposed in the comment above. I have kept the wider implementation out of this blocker commit so this milestone can be reviewed independently; the PR remains draft. Updated screenshot attachments and PR-body corrections follow.

[icelessgo]

Updated client picker after the blocker fixes (synthetic illustrations and client name only). The entry is collapsed until opened; selected tiles, padded shared buttons and the themed name input are shown here.

[Luca-Timo]

Thanks for the quick turnaround! I checked out 1b995a8d4 and went through the blocker commit:

• Migration: renumbered to 316 after the feat(projects): new inquiry, and a project page that runs the job #2216 merge. Unused permissions are dropped, the roles guard is in, and the down() cleanup works.
• Activity log: submit now goes through logActivity(…, tx).
• Gallery layout: the panel is inside the gallery content, and the picker has selected tiles and padded buttons (thanks for the screenshot).
• Gallery deletion: removes choices and history in deleteClaimedEvent.
• Lock / draft / no-op: the lock re-check only covers the chosen options, a submitted choice can no longer silently return to draft, and repeating a boolean no longer bumps the revision.

I also ran albumProducts.test.js on PostgreSQL 15 with the real core migrations (12/12) and both suites on SQLite (16/16), and usageCoverageInventory still passes. Nice work.

One remaining gap of the same kind as blocker 4:

• Deleting an archived gallery skips the new tables. backend/src/routes/adminArchives.js (~L986-1021) has its own delete transaction. It clears selection_submissions and the other child tables explicitly, but not album_product_choice_history / event_album_products. On SQLite (foreign keys off), deleting an archived gallery therefore still leaves the client names and session digests behind. Please add the same hasTable-guarded deletes there, with a test like the one for deleteClaimedEvent.

On the shared design

Yes, this matches the ownership and snapshot boundaries we had in mind: one library, the CRM item owning prices, quotes copying the definition when authored, server-side recompute against the confirmed total, and a frozen spec on acceptance that contracts and invoices carry. docs/product-configurator-direction.md can drop "confirmation pending". A few additions and adjustments:

1. Gallery choices should become billable. In practice the gallery usually exists after the quote is accepted and locked, so a choice made there can't go back into the quote. When the photographer locks a gallery choice (and the CRM is enabled), it should become an open billable item for that customer/project, priced from the CRM catalogue at lock time.

   • This works the same way open hour entries and re-bills already do (customerHoursService, combinedBillingService: unbilled → stamped with its invoice line).
   • If the project's invoice hasn't been issued yet, the item goes onto it as its own line with the frozen spec.
   • If that invoice is already issued (the usual case, since the gallery comes after the job), the photographer bills the open product items as a separate, second invoice for just those items.
   • An issued invoice is never changed. If the choice is unlocked before it is billed, the open item goes away with it. Once it's billed, a change goes through the normal correction path (Storno), not a silent edit.

   This keeps your boundary intact, since a gallery lock never amends an accepted document; it creates a new billable item instead.

2. Prices in the gallery picker: an admin toggle. Off by default, the picker shows options only. When it's on (and the CRM is enabled), it shows base price and surcharges from the CRM catalogue, as information only. The binding price is the one on the open item at lock time.

3. Pre-filled configuration on quote lines. When authoring a quote, the photographer can pre-fill a line's configuration and lock it, for a spec already agreed with the client (e.g. "album 30×30, linen"). The customer then sees it read-only. Unlocked or partly filled lines stay customer-configurable, as you proposed.

4. VAT on surcharges follows the line's VAT code from the shared vat_codes registry, not a separate rate.

5. Images referenced by frozen specs (accepted quotes, contracts, invoices, billed items) must never be removed, including by any later cleanup of unused images.

6. Keep dependencies simple for v1. "Option B is only available with option A" covers the real cases. Rejecting cycles is fine, but a general dependency graph doesn't need to be built yet.

7. Inputs and money: width/height and price fields need to accept German decimal commas (parseLocaleDecimal / DecimalInput), and amounts should go through the existing money helpers (formatMoneyMinor, utils/money).

8. One PR, please. When the CRM part on codex/shared-product-configurator-crm is ready, bring it into this PR instead of opening a separate one. It's easier to review the configurator as a whole, and keeping it in separate commits as you planned works well for that.

Thanks again for taking this on!

[icelessgo]

Thanks, Luca — I agree with the adjustments and have brought the CRM work into this PR. The shared design document now records the agreement.

The updates cover the archived-gallery deletion blocker, the shared option/image library, frozen CRM configurations, gallery billing, quote pre-fill and locked choices, retained document images, and native VAT, decimal-input and money handling. Gallery prices default to hidden, and attribute dependencies remain limited to one prerequisite per attribute.

One deliberate billing safeguard: automatic append is limited to exactly one compatible unissued, unarmed project invoice. Ambiguous, armed or incompatible invoices leave the item open for photographer review; issued invoices are never changed. Unlock removes an unbilled item, while billed choices require the native cancellation/Storno path. A revision check also prevents issuing a stale PDF if a product is appended during rendering.

Validation passed: 194 SQLite tests across 15 related suites, 19 CRM and 13 gallery tests on PostgreSQL 15 using real core migrations, and 16 frontend tests, plus focused lint and the production frontend build. The PR description includes updated synthetic UI screenshots and the existing unrelated TypeScript limitations. The PR remains draft pending upstream review and workflow approval.

All of this is included in #2222. codex/shared-product-configurator-crm is now a development/reference branch with the same final file contents; no second PR is needed.

[the-luap]

This is a review of the CRM part (307f23d71, c935eb252) and the migrations, on head c935eb252. Luca's earlier rounds covered the gallery-only shape, and I did not re-review that. I approved the fork workflow runs; most checks are green, and two were still running when I posted this.

🔴 Blockers

1. Archiving a catalogue item breaks every draft quote that contains it, with albumProducts off too

backend/src/services/albumProducts/crm.js:131-141:

if (!line.catalogue_preset_id) return finish(out);
...
const preset = await db('quote_line_item_presets')
  .where({ id: line.catalogue_preset_id })
  .first();
if (!preset || !isTruthyFlag(preset.is_active))
  throw new AppError('The catalogue item is unavailable', 400);
if (!preset.product_id) return finish(out);

• In quote mode, LineItemsTable now stamps cataloguePresetId on every line inserted from the catalogue, package sub-items included (cataloguePresetId: mode === 'quote' ? preset.id : undefined). So this hits ordinary catalogue items, not only configurable products.
• deleteLineItemPreset is a soft delete (is_active = false, quoteService.js:2744).
• So once a studio archives a catalogue item, a draft quote that contains such a line can no longer be saved: it returns 400, "The catalogue item is unavailable". Reissue, template publish and quote-from-template go through the same prepareLine and fail the same way.
• This is a regression for every CRM user, whether or not the configurator is on.

Fix: return finish(out) for a preset without product_id before the active check. Only reject an inactive preset when a configurable line is being newly added.

2. Migrations 316–318 collide with open PRs #2232 and #2233

#2232 ships 316_add_agb_cms_page.js, and #2233 ships 317_photo_sales.js / 318_photo_sales_email_template.js. Whichever lands second has to renumber. This is not something you did wrong, only a merge-order coordination point. Please rebase and renumber to the next free numbers right before this leaves draft, and update every path that cites them.

🟡 Concerns

1. Route inventory misses six routes. adminAlbumProducts.js:114-199 registers GET /, GET /billing, POST /billing, POST /offers, GET /offers/:id/history and PUT /offers/:id on the events sub-router (events.get/post/put). The gate in usageCoverageInventory.test.js:20 only matches router.<verb>(, so CI cannot see that they are absent from docs/usage-coverage.v6.json. Please add them under adminAlbumProducts.js. Ideally, also widen the regex, or register them as router.<verb>('/events/:eventId/…').
2. With CRM billing off, unlock skips the owner check. At adminAlbumProducts.js:213-216, if (!on) return next(); drops both requireEventOwner and bills.manage. But billing.unlock (billing.js:97-136) runs no matter what the flag says, and auditedDeletes the open product_billable_items. Items created while billing was on therefore survive the flag being turned off, and a gallery-assigned admin with products.configure can then delete them. Fix: always require the owner for lock and unlock, or have unlock refuse while open items exist and billing is off.
3. The public quote never sends configurationLocked. publicDocumentViews.js:147-149 projects configurationDefinition and configurationSnapshot only. QuoteResponsePage.tsx checks !li.configurationLocked, so a locked line renders as an editable picker, and the client only learns otherwise from the server's 409 CONFIGURATION_LOCKED. Add configurationLocked: isTruthyFlag(li.configuration_locked) there, and check the portal projection for the same gap.
4. Re-accepting an unchanged configurable quote counts as a change. In quoteService.js:1612, the !configurable && guard skips the "same choice" early return for any quote with a configurable line. Every identical re-accept inside the response window then goes through the full path again, and so does every double-click. Inferred from that path: another business mail, another -accepted-N PDF and an empty selection_changes entry. Compare the configuration snapshot as well, and return null when nothing changed.
5. Gallery clients receive internal CRM ids. service.js:137-149 shape() includes customerId (billing_customer_id) and cataloguePresetId, and clientOffers spreads ...row into the client response. Strip them on the client path.
6. Invoice totals are summed by hand in append (billing.js:279-293). It adds the line into net and recomputes VAT from it. As far as I read it, that bypasses the shared total helper, the crm_invoice_round_total setting and percentage discount lines on the target invoice. That is inferred from reading, not reproduced. Reload the lines and run the same total function that the invoice editor uses.
7. Package parent totals may go stale when a configured sub-item's price changes on accept. writeSnapshots (crm.js:255-286) rewrites only the configured lines. Inferred, not reproduced: please check what the quote PDF and contract render for a package with a configured sub-item.
8. Product-linked catalogue items become uneditable once the flag is off or the product or one of its options is deactivated. presetColumns (crm.js:51-73) re-runs catalogueProduct on every PUT that carries product_id, and the UI always sends it, which gives 403/400. The same 403 hits adding such an item to a quote, or publishing a template that has one (crm.js:37-38). Only validate when product_id actually changes.
9. Unchecked stamp writes. At billing.js:268-274 and 408-414, the billable item is stamped with .whereNull('invoice_id'), but the affected row count is never checked. If it is 0, an invoice line is created without a link back. Throw a 409 on 0.
10. configurations entries are not validated. At crm.js:170, c.position is read on raw body elements, so a null entry gives a 500. Add body('configurations.*').isObject().
11. Migration 318 has no idempotency guards. createTable, alterTable and CREATE FUNCTION/CREATE TRIGGER all run without hasTable/hasColumn and without IF NOT EXISTS / OR REPLACE. On PostgreSQL the runner wraps each migration in a transaction (run-migrations.js:36), but on SQLite it does not. So if the backfill loop (the contract retention pass) fails half-way, the next boot fails on table already exists and stays down. Please guard it the way 316/317 are guarded.
12. Template prices are frozen at publish time (quoteTemplateService.js:422-428). Quotes created from a template later carry the CRM base and surcharge prices from when the template was published. If that is intended, please say so in docs/album-products.md.

🟢 Nits

• billing.js:291 passes updated_at: new Date() to knex. The repo convention is .toISOString() (see CLAUDE.md, Jest + SQLite).
• Migration 316 seeds albums.edit, and 317 renames it to products.configure. Neither migration has been released, so 316 could seed the final names directly, and the rename step in 317 could go.
• The public and portal product-images routes are not behind albumProducts. That is reasonable for frozen documents, but worth a sentence in the docs.
• An admin changeAddOns on an accepted quote can't book a configured add-on that has no snapshot ("Choose X"), and the accepted quote can't be edited to fix that.

✅ Verified

Directly inspected:

• Migrations 316/317/318 and the 109 edit: read in full. 316/317 are guarded, 318 is not (concern 11). The PG trigger and the SQLite RAISE(ABORT) trigger both protect referenced images. The backfill only retains snapshots from accepted or converted quotes.
• server.js: one mount, /api/admin/album-products, behind adminAuth plus requireFeatureFlag('albumProducts') at the router level.
• Fork CI: the PR touches no .github/, package or Docker files, and I approved its workflow runs.
• The blocker, concerns 1–5, the bypass in concern 2 and the unlock delete path: each re-read at the cited line on c935eb252.

Checked by a second reviewer pass and spot-read by me, but not traced end to end:

• Price recompute: the unit price comes from the stored definition plus definition surcharges. Option ids are validated against the frozen product, and the client cannot set quantity, discount or unit price. expectedTotalMinor is compared as an exact integer. The response window, assertQuoteNotConverted and the conditional status update are intact.
• Freeze on accept: it happens in the same transaction as acceptance, and the invoice PUT refuses line or VAT changes on frozen lines.
• Lock path: the catalogue and offer rows and the candidate invoice are taken FOR UPDATE, with unique(offer_id, offer_revision) and the product_items_revision check in sendInvoiceOnce.
• Image upload: PNG magic bytes, sharp metadata and a re-encode, with server-generated album-products/<uuid>.png keys.
• Tenancy: billable items are scoped to their creator, and portal and public routes stay behind ownedDocument / hasValidGrant.

Not inspected: customer erasure with open product items, the frontend components outside the quote pages, and the test suites themselves (194 + 32 + 16 trusted at PR-body level).

Verdict

Close blocker 1, and concerns 1–5 and 11, before this leaves draft. Renumber the migrations against whatever has landed by then. The remaining concerns need a fix or a clear answer.

[icelessgo]

Updated this draft in 1811a27, ec623f5 and e2e5dce, rebased onto main af52e31f.

The review follow-up covers:

• Archived ordinary catalogue items and unchanged product associations remain editable without requiring an active configurator/product. Fresh associations and option-price changes retain validation.
• Owner checks apply to lock/unlock even with billing off; assigned-admin controls match that boundary. Public/portal quotes expose the lock, and client offer responses omit internal CRM IDs.
• Identical acceptances/amendments, including concurrent requests, avoid duplicate PDFs, mail and history. Malformed configuration entries return 400.
• Invoice appends use shared discounts/rounding/VAT/shipping calculations; package parent amounts persist to PDFs/contracts. Both conditional billing stamps now refuse and roll back on zero affected rows.
• All six subrouter routes are inventoried, the scanner detects named routers, and the backup/seeded-table/overview fixtures are fixed.
• Migrations are now 320–322, avoiding feat(legal): add an AGB page next to Impressum and Datenschutz #2232/feat(gallery): sell photos from priced galleries through Snipcart #2233/feat(gallery): opt into client PIN entry #2249. The first seeds final product permissions; the retention migration recovers interrupted schema/backfill work on SQLite and is repeatable on PostgreSQL. Billing timestamps use ISO strings.
• Catalogue viewers get the full read-only view; remaining legacy controls use shared UI components. Existing quote images remain authorized and available with the feature off.

Two decisions are now explicit in the existing workflow docs and covered by tests:

1. Published templates freeze base prices and surcharges. Republish to offer current catalogue prices.
2. Admins configure newly booked optional products in the accepted quote's existing add-on amendment screen, including package children. Saving records history, stores another accepted PDF and emails the customer. Existing accepted specifications stay read-only and need reissue to change; unbooked studio-locked offers keep their agreed specification.

Final focused validation: 182 SQLite tests / 14 suites, 54 PostgreSQL tests (39 CRM + 13 gallery + 2 migration), and 31 frontend tests / 6 files passed. Production build and lint pass with existing warnings. Chromium checks passed at 1440px/390px; new screenshots are in the PR body. The body also records the broader suite runs and the seven existing TypeScript diagnostics addressed by #2246.

Checks for the new head are running; schema drift has passed. Keeping this draft for maintainer review.

[the-luap]

I re-verified 1811a27b, ec623f5c and e2e5dce6 on head e2e5dce6. 13 of the 14 points are closed at file:line, and the remaining one is a narrower form of blocker 1. Thanks also for widening the inventory scanner instead of only listing the routes.

Blockers

	Status	Evidence on e2e5dce6
1. Archived catalogue item breaks drafts	⚠️ partly	crm.js:146-150 now returns finish(out) for a preset without product_id before the active check. Lines that already carry a definition return early at :117-135, and reissue/duplicate stop at :114. The gap left is below.
2. Migration numbers	✅	320–322. No 31[678]_product reference is left, and the tests require the new paths.

Remaining gap in 1: an existing plain line is upgraded on its next save

crm.js:117-153: if configuration_source_line_id resolves to a source line without a definition, the code falls through to the preset lookup, exactly as for a brand-new line. Scenario:

• A draft has an ordinary "Album" line from the catalogue.
• The studio then links that catalogue item to a product. That is allowed, since C8 now permits editing.
• On the next draft save, the existing line silently becomes configurable, and its unit price is reset to baseUnitPriceMinor, overwriting any price the photographer typed.
• If the item is archived by then, the save returns 400 again. If albumProducts is off, catalogueProduct returns 403.

Quote-from-template takes the same path. As far as I read it, toServiceLine passes the preset id without a definition, so the line picks up the current catalogue price, against the frozen-template-price decision you documented. I inferred this from the code and did not trace it end to end.

Fix: when a source line resolves without a definition, return finish(out) without consulting the preset. Only a line that is newly added from the catalogue (no source line) should be upgraded. A regression test would be: save a draft → link the preset to a product → save again → the line stays plain at its old price.

Concerns — closed

	Status	Evidence
1. Route inventory	✅	usageCoverageInventory.test.js:21-22 collects every const X = (express.)Router( name. Only adminAlbumProducts.js changes (6 → 12), and it matches usage-coverage.v6.json. Small nit: the comment mentions "routers passed into modules", which the regex does not detect.
2. Owner check on lock/unlock	✅	adminAlbumProducts.js:215-220: requireEventOwner always runs first, and bills.manage is added only when billing is on.
3. configurationLocked	✅	publicDocumentViews.js:148. The portal uses the same buildQuoteView.
4. Identical re-accept	✅	quoteService.js:1631-1638 returns null, and writeAcceptedSelection re-checks under forUpdate (:1450-1455). PDF and mail are gated on selectionChanged.
5. Internal ids to clients	✅	service.js:151-156 clientShape covers clientOffers and all non-admin mutate returns.
6. Hand-summed invoice totals	✅	billing.js:278-305 uses computeTotals with crm_invoice_round_total.
7. Package parent totals	✅	crm.js:274-279 writes line_total_minor for lines without a definition.
8. Product-linked items uneditable	✅	crm.js:64-67 skips validation when the link and the surcharges are unchanged.
9. Unchecked stamp writes	✅	billing.js:274-275, :428-429: stamped !== 1 returns 409.
10. configurations.*	✅	crm.js:178, plus isObject() on all four routes and on admin /add-ons.
11. Migration guards	✅	322: hasTable/hasColumn throughout. PG uses CREATE OR REPLACE FUNCTION and a pg_trigger existence check, SQLite uses CREATE TRIGGER IF NOT EXISTS, and retention uses onConflict().ignore().
12. Template price freeze	✅	Documented in album-products.md. 320 seeds the final products.* permissions, and the rename is gone. The billing timestamp is an ISO string.

New in e2e5dce6: add-on configuration on an accepted quote

This looks right. It requires quotes.manage, calls assertQuoteNotConverted, requires status accepted and checks the definition version. The frozen check (quoteService.js:2063-2073) lets through only add-ons that are newly booked or being removed. Every other configured line that differs gets 409 CONFIGURATION_FROZEN. I found no path that silently changes an accepted spec.

🟢 Nits

• Discounts on appended products: append now re-resolves percentage promotion lines through computeTotals, so a quote's percentage promotion also discounts gallery products appended later. That follows from what I asked for, but invoice/create.js does not re-resolve promotions, so please state it in album-products.md.
• Screenshots: the amendment screen for accepted-quote add-ons is new UI and has no screenshot yet.

✅ Verified

• Read directly: the full diff of the three fix commits, crm.js, migration 322, the permission block of 320 and the guards of 321, resolveCustomerSelection / writeAcceptedSelection, adminChangeAddOns, computeTotals in append, and clientShape.
• Scanner: the old and new inventory regexes were run over the whole route tree.
• CI is green on e2e5dce6.
• Trusted at PR-body level: the 182 SQLite, 54 PostgreSQL and 31 frontend tests.

Verdict

Close the remaining gap in blocker 1 (an existing plain line must never be upgraded) with a test, add the amendment screenshot, and from my side this can leave draft.

[icelessgo]

Fixed the remaining plain-line preservation gap in 2e8c6501.

• An existing source line without a configuration definition now returns without consulting the current preset, preserving its authored price and ordinary status.
• Published templates explicitly preserve the absence of configuration, including package children. New catalogue lines and explicitly republished templates still adopt current product definitions/prices.
• Added ten regression cases. The nine preservation cases first failed against e2e5dce6; the positive control verifies that newly authored products still work.
• Documented the percentage-promotion recalculation on appended gallery products and narrowed the inventory-scanner comment to what its regex detects.

Validation under Node 22.23.3: 170 SQLite tests / nine suites, 49 PostgreSQL 15 CRM tests, and backend/touched-test lint pass (one existing unrelated console warning). Both database runs use real core migrations and synthetic fixtures.

The accepted-quote add-on amendment screenshots are embedded in the PR description; direct links for convenience: desktop, phone. Those assets are present on pr-assets; this follow-up makes no UI change.

The PR description now separates this follow-up's validation from the earlier broader runs. Keeping it draft while the updated head's GitHub checks run.

[the-luap]

Re-verified 2e8c650. The remaining part of blocker 1 is closed, and both nits from the last round are done.

Blocker 1 — closed

Item	Where	Status
An existing plain line was upgraded or repriced on its next save	albumProducts/crm.js:137-138: a source line without a definition now returns finish(out) before the catalogue lookup, so its authored price and ordinary status survive a later product link, archive or disable.	✅
Templates re-resolving plain lines	quoteTemplateService.js:481 always passes configuration_definition (null for a plain line), and crm.js:115 treats any defined value, including null, as a service-owned snapshot. New catalogue lines still go through the preset lookup, and the positive-control test covers that.	✅

Nits — closed

• Accepted-quote add-on amendment screenshots (desktop and phone) are in the description.
• docs/album-products.md now documents the percentage-promotion recalculation when products are appended, and the plain-line rule.

Still needed

• Rebase. The PR conflicts with main again (mergeStateStatus: DIRTY), so GitHub runs no pull_request workflows on this head. Only the 4 always-on checks have reported. Please rebase, and re-check the migration numbers (320–322) against main while you do.

✅ Verified

• Directly inspected: the crm.js and quoteTemplateService.js diffs and the doc addition.
• Trusted at description level: the ten new regression cases and the SQLite/PG runs. CI has not run them on this head yet.

Verdict

Once it is rebased and CI is green, and you take it out of draft, merge cleared from my side.

[icelessgo]

Updated in 2e8c6501: existing plain quote lines and published templates now retain their saved prices and plain status after catalogue relinking. Added 10 regression tests, documented percentage discounts on appended products, and clarified the amendment screenshot links in the PR description.

Validation: 170 SQLite tests and 49 PostgreSQL tests passed. Lint has no errors, with one existing unrelated warning. No deployment changes.

[icelessgo]

Rebased onto main 191acb36; current head is 8830b383. The quote-page conflict preserves upstream's position-based parent handling, and migrations 320–322 remain available.

Full CI is green, including backend/frontend tests, browser smoke tests, AMD64/ARM64 builds, clean install and upgrade checks. No unresolved review threads remain. The subsequent main commit 94acb7bb changes release metadata only; the PR remains mergeable. Draft status is unchanged.

[the-luap]

Checked the rebased head 8830b38. git range-diff against the reviewed commits shows all three as identical (=), the PR is out of draft, and CI is green.

I trial-merged this PR together with #2249, #2260 and #2262 onto main. tsc -p tsconfig.app.json is clean, and the product-configurator, album-product, slideshow, gallery client-mode and usage-coverage suites pass on the combined tree (176 tests). Migrations 319–323 apply in order.

The one overlap is GalleryView.tsx, where #2249's {clientAccessDialog} and this PR's <GalleryProductPanel> land on the same line. Both lines stay. #2249 merges first, so I will push a merge commit with main to this branch that resolves it that way, and merge once CI is green on it.

Merge cleared.

[the-luap]

Merge cleared, see the review thread.