Skip to content

feat(erli,web): Allegro credentials checkbox + offer wizard category/parameters steps#1401

Merged
norbert-kulus-blockydevs merged 4 commits into
epic/1381-erli-allegro-category-catalogfrom
1384-erli-fe-category-wizard
Jul 7, 2026
Merged

feat(erli,web): Allegro credentials checkbox + offer wizard category/parameters steps#1401
norbert-kulus-blockydevs merged 4 commits into
epic/1381-erli-allegro-category-catalogfrom
1384-erli-fe-category-wizard

Conversation

@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator

Summary

Part of #1381, sub-task 3 of 4 (depends on #1382/#1383, both merged; see also #1387 for the reuse-existing-connection follow-up).

  • ErliCredentialsPanel (apps/web/src/plugins/erli/components/erli-credentials-panel.tsx): adds the "Browse Allegro categories when creating Erli offers" checkbox, which reveals masked Client ID / Client Secret fields with a show/hide toggle. Client-side Zod-equivalent validation blocks submit when only one of the pair is filled, or when enabling the checkbox with both fields blank.
  • ErliCreateOfferWizard (apps/web/src/features/listings/components/erli/erli-create-offer-wizard.tsx): when connection.config.allegroCategoryAccessEnabled === true, renders the reused Allegro CategoryPicker and CategoryParametersStep (fed by the existing useCategoryParametersQuery) as two new steps ("Category", "Category parameters"), with dynamic per-category Zod validation gating "Next" — mirrors AllegroCreateOfferWizard. Falls back to today's plain-text field plus a hint linking to the connection's edit page when the flag is absent.
  • erli-create-offer.schema.ts gains a parameters slice (z.record(...).default({}), z.input/z.output split like Allegro's createOfferFieldsSchema); submit serializes it into overrides.parameters via the existing categoryParametersToOfferParameters helper — no backend change needed (OfferBuilderService.buildOfferParameters already merges it).
  • ERLI_STEP_LABELS / needsProductParameters are now capability-conditional.

Per ADR-031's correction, the gating signal is connection.config.allegroCategoryAccessEnablednot connection.supportedCapabilities, which is a static, per-adapterKey manifest value that cannot reflect per-connection Allegro credential configuration.

Atomicity design decision (the trickiest part of this issue)

The backend has no single request that can write both the Allegro credential pair and config.allegroCategoryAccessEnabled together — PUT /connections/:id/credentials and PATCH /connections/:id (config) are independent, generic, single-purpose routes with zero cross-write coupling for any platform (confirmed by reading ConnectionController/ConnectionService; the #1399 commit message that landed #1383 explicitly defers this write-path to #1384's scope).

Given that, ErliCredentialsPanel's single "Save" click sequences two existing mutations rather than inventing a new backend endpoint:

  1. useUpdateConnectionCredentialsMutation — writes apiKey (if changed) and allegroClientId/allegroClientSecret (only when the checkbox is checked and both are filled; omitted entirely, not sent as empty strings, when unchecked).
  2. useUpdateConnectionMutation — patches config.allegroCategoryAccessEnabled to match the checkbox, but only if it actually changed (avoids a needless request on every unrelated apiKey rotation) or new Allegro credentials were just written.

Ordering is deliberate and fails safe: credentials write first, config-flag write second.

  • If the credentials write fails, the config patch never runs — the flag stays untouched, so a connection can never end up advertising category access that the backend can't actually serve (the CategoryPicker/parameters steps would render but every fetch would error).
  • If the config patch fails after the credentials write succeeded, the flag simply stays at its prior value. Worst case, the wizard keeps showing today's safe plain-text fallback a beat longer than intended — never a broken experience. The entered field values are not cleared on failure, so a retry re-sends both mutations; the credentials write is idempotent (same values), and the retry naturally re-attempts the config patch.
  • Both mutation errors render as separate inline Alerts so the operator knows exactly which step to retry.

I considered doing this the other way (config flag first, then credentials) and rejected it — that ordering can end up with the flag flipped to true while the credential write is still in flight or fails, which is the unsafe direction (advertises access before the backend can serve it).

Test plan

  • pnpm --filter @openlinker/web type-check — clean
  • pnpm --filter @openlinker/web lint — 0 errors (pre-existing warnings elsewhere, none in touched files)
  • pnpm --filter @openlinker/web test — 239 files / 2038 tests pass, including:
    • erli-credentials-panel.test.tsx (extended: checkbox reveal, both-or-neither validation, enable-without-credentials validation, secret show/hide, sequenced mutation ordering for both enable and disable directions)
    • erli-create-offer-wizard.test.tsx (extended: fallback hint when unconfigured; Category + Category-parameters steps, required-parameter blocking, overrides.parameters submit payload when configured)
    • bulk-edit-modal.test.tsx ([BUG] Frontend — bulk offer wizard hides category-parameter step for Allegro (required "Stan" unsettable) #1367 bulk-wizard gate) — passes unmodified, confirming no regression
  • Full repo pre-commit hook (pnpm -r lint && pnpm check:invariants) passed

🤖 Generated with Claude Code

https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu

…ard category/parameters steps

Adds the operator-facing half of the Erli-owned Allegro category-catalog
feature (#1384, part of epic #1381, depends on #1382/#1383):

- ErliCredentialsPanel gains a "Browse Allegro categories when creating
  Erli offers" checkbox that reveals Client ID / Client Secret fields
  (masked, show/hide toggle). Saving sequences two existing mutations from
  one click: useUpdateConnectionCredentialsMutation (credentials pair,
  merged with apiKey) then useUpdateConnectionMutation (config patch for
  allegroCategoryAccessEnabled) — credentials first so a failure never
  leaves the flag advertising access the backend can't serve.

- ErliCreateOfferWizard renders the reused Allegro CategoryPicker +
  CategoryParametersStep (fed by the existing useCategoryParametersQuery)
  as dedicated Category / Category-parameters steps when
  connection.config.allegroCategoryAccessEnabled is true — per ADR-031's
  correction, this per-connection-instance config flag is the FE gating
  signal, not the static, per-adapterKey connection.supportedCapabilities.
  Falls back to today's plain-text field + a link to the connection's edit
  page otherwise. Stepper labels and needsProductParameters become
  capability-conditional; erliCreateOfferSchema gains a parameters slice
  serialized into overrides.parameters on submit (no BE change needed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Code review findings

Scope: pure frontend change (apps/web) - adds an Allegro-credentials checkbox to ErliCredentialsPanel and two new wizard steps to ErliCreateOfferWizard, gated on config.allegroCategoryAccessEnabled. Overall the implementation mirrors the existing Allegro wizard patterns well and is thoroughly tested.

IMPORTANT

  • apps/web/src/plugins/erli/components/erli-credentials-panel.tsx:816-821, 861-869, 880-891 - canSubmit is computed from apiKey.trim() || idFilled || secretFilled || allegroEnabled !== initialAllegroEnabled, without gating idFilled/secretFilled on allegroEnabled. Repro: check the box, type a Client ID, uncheck the box, click Save. The Save button is enabled, but every mutation branch inside onSave is gated on allegroEnabled (now false), so both the credentials write and the config patch are skipped - yet the code still shows "Credentials saved" and closes the panel. This is a false-positive success toast for an operation that silently did nothing, and the typed secret is discarded without warning. Suggest gating idFilled/secretFilled on allegroEnabled in canSubmit, or clearing the ID/secret state when the checkbox is unchecked.

SUGGESTION

  • apps/web/src/plugins/erli/components/erli-credentials-panel.tsx:931 - inline style={{ accentColor: 'var(--accent)' }} references a token that doesn't exist (tokens.ts/index.css only define --accent-primary, etc.). index.css:755 already applies accent-color: var(--accent-primary) globally to checkboxes, so this override is both redundant and invalid at compute time - it silently falls back away from the brand accent color. Recommend removing the inline style or fixing it to var(--accent-primary).
  • apps/web/src/features/listings/components/erli/create-erli-offer-request-to-form-values.ts:9-15 - retry-prefill resets parameters: {}, so retrying a failed offer-creation record drops previously entered category-parameter values (operator has to re-enter them; required-field validation re-blocks submission rather than silently accepting stale data). Already called out in a code comment as an accepted simplification - just flagging it as a tracked follow-up candidate if retry volume matters.

Not found: no architecture-boundary violations, no any/console.log, no security issues, test coverage for the new branches is solid.

- Gate `canSubmit` in ErliCredentialsPanel on `allegroEnabled` so
  unchecking the Allegro-access box after typing (but not saving)
  Client ID/Secret disables Save instead of showing a false-positive
  "Credentials saved" toast while discarding the typed secret.
- Drop the checkbox's invalid inline `accentColor: var(--accent)`
  (token doesn't exist) — `index.css` already applies
  `accent-color: var(--accent-primary)` globally.
- Restore category-parameter values on Erli offer retry by reusing
  the Allegro retry mapper's `readParameters` heuristic (exported)
  instead of always prefilling an empty `parameters` object — both
  platforms persist the same neutral `overrides.parameters` shape.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Addressed all findings from the review (commit be20988):

IMPORTANT

  • erli-credentials-panel.tsx - canSubmit now gates idFilled/secretFilled on allegroEnabled, so unchecking the box after typing a Client ID (without saving) disables Save instead of showing a false "Credentials saved" toast while silently discarding both mutations and the typed secret. Added a regression test covering exactly that repro (check -> type Client ID -> uncheck -> Save is disabled, neither mutation fires).

SUGGESTION

  • Removed the invalid inline style={{ accentColor: 'var(--accent)' }} on the checkbox - index.css already applies accent-color: var(--accent-primary) globally, so the override was both redundant and silently invalid.
  • Category-parameter values are now restored on Erli offer retry: exported the Allegro retry mapper's readParameters heuristic from create-offer-request-to-form-values.ts and reused it in createErliOfferRequestToFormValues instead of always prefilling an empty object - both platforms persist the same neutral overrides.parameters wire shape, so no duplicated logic was needed. Added a test asserting parameter values round-trip through retry.

Verified: pnpm --filter @openlinker/web test (2040/2040 passing, incl. 2 new tests), lint (0 errors, only pre-existing warnings), type-check (clean), full repo pre-commit hook (pnpm -r lint && pnpm check:invariants) passed.

@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Second review (focused on state coverage vs. the approved mockup)

Compared this PR against the approved UI mockup (linked from #1384) and the plan's acceptance criteria. Found two confirmed state-handling gaps.

🔴 Blocking: rotating the plain Erli API key silently wipes stored Allegro credentials

ErliCredentialsPanel.onSave only includes the fields the operator actually touched in the credentials payload it sends to rotate.mutateAsync. The Allegro Client ID/Client Secret inputs always start blank (they're secrets, never rehydrated from the server), so if an operator opens the panel on a connection that already has Allegro category access configured and only rotates the plain apiKey (leaving the Allegro fields empty, which is the natural thing to do), validation passes cleanly and the mutation is called with just { apiKey: '...' }.

ConnectionService.updateCredentials -> IntegrationCredentialRepository.update does a full replace of credentialsJson (existing.credentialsCiphertext = encrypt(JSON.stringify(patch.credentialsJson))), not a merge. So this call overwrites the entire encrypted blob, permanently deleting the previously stored allegroClientId/allegroClientSecret.

Because allegroEnabled === initialAllegroEnabled (the checkbox wasn't touched) and no new Allegro credentials were submitted, the second updateConfig mutation never fires either, so config.allegroCategoryAccessEnabled stays true. Result: the wizard keeps showing the Category/Category-parameters steps (gated on that stale flag), but ErliAdapterFactory.buildAllegroCategoryCatalog now returns undefined since the credentials are gone, so every subsequent offer creation on this connection breaks with no warning at the point the API key was rotated.

This is a very ordinary operator action (rotate the API key on a fully configured connection) and there's no test covering it - the closest existing test (unchecking a previously-enabled connection ... without resending credentials) only covers the config-only path, not "apiKey-only rotate while Allegro creds already exist."

Suggested fix: either (a) have the credentials panel fetch/preserve the existing Allegro-configured flag and refuse to rotate apiKey alone without re-entering the Allegro pair when they're configured, or (b) make updateCredentials merge into the existing credentialsJson instead of replacing it wholesale.

🟠 Important: category selection isn't enforced when Allegro access is enabled

In AllegroCreateOfferWizard, categoryId is part of the validated STEP_FIELDS for its category step, so the operator can't advance without picking a category. In this PR's Erli wizard, STEP_FIELDS[categoryStepIndex] is [] and categoryId: z.string().optional() stays optional even when allegroCategoryAccessEnabled is true. An operator can click "Next" through the new Category step without touching CategoryPicker at all, sail through Category-parameters (which shows "No additional parameters required" since the query is disabled without a categoryId), and submit an offer with no category chosen.

The EAN-based auto-resolve (useResolveCategoryQuery) partially masks this when it succeeds, but when it doesn't (e.g. no existing mapping yet, which is exactly when an operator would want to browse manually) nothing blocks the empty submission. This breaks the "mirrors AllegroCreateOfferWizard" parity goal stated in the plan, and there's no test exercising "click Next without selecting a category."

🟡 Minor: mockup copy not fully carried over

The approved mockup includes specific helper text under each field ("From an Allegro app you register at apps.developer.allegro.pl... never to sign in as a seller or place offers" under Client ID, "Stored encrypted. You won't see it again after saving" under Client Secret). Only a shortened version of the first line made it into the checkbox's own hint text; neither field has its own helper text in the shipped panel. Not a functional issue (the plan explicitly says to reuse the existing Alert/error pattern rather than reinvent the mockup's live-validation badge), just flagging the copy gap in case it matters for the design sign-off.

Everything else checked out well: the sequenced credentials-then-config write (and its fail-safe ordering), the enable/disable/re-enable checkbox flows, the stale-category-schema handling, and the loading/error/empty states on the parameters step all match the plan and are covered by tests.

…ick (#1401 second review)

- ConnectionService.updateCredentials now merges the submitted payload onto
  the existing stored credentialsJson instead of replacing it wholesale, so
  rotating the plain Erli apiKey no longer silently wipes a previously
  configured allegroClientId/allegroClientSecret pair (and enabling Allegro
  access without touching apiKey no longer fails shape validation).
- ErliCreateOfferWizard now blocks Next on the Category step until a
  category is actually selected, mirroring AllegroCreateOfferWizard's
  required categoryId field, instead of silently falling through to
  Category-parameters/Review with an empty category.
- Carried over the approved mockup's helper copy under the Allegro Client ID
  / Client Secret fields and restored the checkbox's original hint text.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Addressed all findings from the second review (commit f421551):

Blocking

  • ConnectionService.updateCredentials now merges the submitted patch onto the existing stored credentialsJson instead of replacing it wholesale, then validates and persists the merged object. Rotating just the plain Erli apiKey no longer wipes a previously-configured allegroClientId/allegroClientSecret pair, and this also fixes a related latent bug: enabling Allegro access without touching apiKey would previously fail the shape validator's "non-empty apiKey required" rule since only the partial payload was validated. Added a regression test (should merge onto existing stored fields instead of replacing the whole blob) and updated credentials.getByRef mocking across the existing updateCredentials spec.

Important

  • ErliCreateOfferWizard.goNext now blocks advancing past the Category step until categoryId is actually set, mirroring AllegroCreateOfferWizard's required categoryId field (Category is a static [] STEP_FIELDS entry since CategoryPicker has no RHF-registered input, so this couldn't be enforced via form.trigger). Renders the error message under the picker with proper aria-describedby wiring. Added a regression test asserting Next is blocked and the Category-parameters step never renders when no category was picked.

Minor

  • Carried over the mockup's exact helper copy: a hint under Client ID ("From an Allegro app you register at apps.developer.allegro.pl...") and under Client Secret ("Stored encrypted. You won't see it again after saving."), and restored the checkbox's original mockup hint text (the shipped version had accidentally duplicated the Client-ID copy onto the checkbox instead).

Verified: pnpm --filter @openlinker/web test (2041/2041 passing, incl. 2 new tests), pnpm --filter @openlinker/api test -- integrations (115/115 passing), lint/type-check clean on both packages, full repo pre-commit hook (pnpm -r lint && pnpm check:invariants) passed.

@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Reviewed via /tech-review + /pr-review, focused on comparing the implementation against the approved design mockup and verifying state coverage. No blocking issues, but two things worth closing before merge and two smaller notes.

Worth closing before merge:

  1. Missing failure-path tests for the two-mutation sequencing. The PR description calls the credentials-then-config-flag ordering "the trickiest part of this issue," but every new test in erli-credentials-panel.test.tsx uses mockResolvedValue - none exercise the rejection paths (credentials write fails -> config write must never fire; config write fails after credentials succeeded -> flag stays at prior value + inline error). The happy-path logic reads correctly on inspection, but the claims about safe failure behavior aren't actually verified by the test suite. Worth adding both rejection-path tests before merge given how safety-critical this ordering is by the PR's own framing.

  2. ADR-031 documentation drift. The Decision section states allegroCategoryAccessEnabled is "written/cleared by the backend in the same operation" as the credential pair. The shipped implementation instead does two independently-sequenced FE-orchestrated mutations (correctly, since no single backend endpoint exists for both writes) - but the ADR was never updated to reflect that, unlike the capability-signal deviation earlier in the same document, which did get an explicit "Correction" paragraph. Suggest adding a second Correction note so the ADR doesn't contradict the code it's cited to justify.

Smaller notes:

  1. State-coverage gap vs. the approved mockup: the mockup shows a credential-verification status indicator (badge cycling "Not verified yet" -> "Verification failed", aria-invalid on the secret field on rejection). Not implemented here - traces to a real backend gap (no synchronous Allegro credential verification at save time exists yet), so a reasonable scope cut, but worth confirming explicitly / tracking as a fast-follow rather than leaving implicit.

  2. Wizard step topology deviates from the mockup when unconfigured: the mockup's stepper is always 5 steps, with the Category step itself rendering the plain-text fallback when off. This PR instead collapses to a 3-step shape, folding the category field into "Offer details." Functionally fine and well-tested, just an unstated deviation from the approved design.

Everything else checked out cleanly: checkbox reveal/hide, masked-secret show/hide toggle, both-or-neither client-side validation, category-step/fallback gating with the correct edit-page link, required/optional parameter grouping (inherited unchanged from the shared category-parameters-step.tsx), correct reuse of CategoryPicker/useCategoryParametersQuery from the shared features/listings/components/ location (no plugin/feature boundary violation, confirmed against frontend-architecture.md), correct stepper index math across both step-array shapes, parameters Zod z.input/z.output split mirroring the Allegro precedent exactly, zero regression risk in the shared create-offer-request-to-form-values.ts (additive-only export change), correct MissingCategoryParameterSectionError/staleSchemaError handling (mirrors Allegro's wizard), and the bundled backend fix in connection.service.ts (merge-not-replace on credential rotation) is a genuinely good catch for a real pre-existing bug, well tested.

…031 documentation gaps (#1401 third review)

- erli-credentials-panel.test.tsx: add regression tests asserting the config
  patch never fires when the credentials write rejects, and that a config-patch
  rejection after a successful credentials write leaves the flag/fields intact
  with a retryable inline error - the two claims the PR description makes about
  "the trickiest part of this issue" were previously unverified by the suite.
- ADR-031: add a second Correction noting the sequencing is two FE-orchestrated
  mutations, not a single backend write, since no endpoint accepts both the
  credential pair and the config flag together; also record the wizard's
  3-step-vs-5-step topology and the deferred credential-verification indicator
  as explicit, confirmed scope cuts rather than implicit deviations from the
  approved mockup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Addressed all findings from the third review (commit 4e75b30):

Worth closing before merge

  • Added the two missing failure-path tests for the credentials-then-config sequencing: one asserting the config patch never fires when the credentials write rejects, one asserting a config-patch rejection after a successful credentials write leaves allegroCategoryAccessEnabled at its prior value, keeps the entered fields intact for a retry, and surfaces the retryable inline error. Both claims in the PR description's "trickiest part of this issue" section are now actually verified by the suite, not just correct on inspection.
  • ADR-031 previously stated the config flag is "written/cleared by the backend in the same operation" as the credential pair - added a second Correction paragraph clarifying it's actually two independently-sequenced FE-orchestrated mutations (no backend endpoint accepts both writes together), matching the code and the earlier capability-signal Correction already in the same doc.

Smaller notes - now explicitly confirmed rather than left implicit

  • Added a Cons entry to ADR-031 confirming the deferred credential-verification status indicator (mockup's "Not verified yet"/"Verification failed" badge + aria-invalid) as an accepted scope cut: there's no synchronous Allegro credential-verification endpoint today (the client_credentials exchange only happens lazily on first category-catalog fetch), so the FE has nothing to poll/await at save time. Flagged as a fast-follow candidate once such an endpoint exists.
  • Added a Cons entry confirming the 3-step (vs. the mockup's constant 5-step) wizard topology is a deliberate, tested simplification - folding category entry into "Offer details" when Allegro access is off, rather than rendering an in-place fallback inside a step that changes shape.

Verified: pnpm --filter @openlinker/web test (2043/2043 passing, incl. 2 new tests), lint (0 errors, only pre-existing warnings elsewhere), type-check (clean).

@norbert-kulus-blockydevs

Copy link
Copy Markdown
Collaborator Author

Final review pass (independent /pr-review + /tech-review over the full diff)

Ran both /pr-review and /tech-review fresh against the full diff (epic/1381-erli-allegro-category-catalog...HEAD), after the three prior review rounds (all addressed) plus commit 4e75b300 (the failure-path tests + ADR-031 documentation corrections). Read every changed file directly rather than relying only on the skills' generic checklists, and cross-checked the claims that mattered most.

Concretely verified:

  • apps/api/src/integrations/application/services/connection.service.ts - the credentials-merge fix (getByRef + spread merge, from the second review round) is correct: getByRef throws CredentialNotFoundException on a missing ref rather than returning null, so there's no unhandled-null path in the merge.
  • erli-credentials-panel.test.tsx's two new failure-path tests actually match the component's real branching, not just superficially present - traced both through onSave's try/catch and confirmed the assertions (update never called when updateCredentials rejects; fields/flag stay intact and the inline error renders when update rejects after credentials succeeded).
  • erli-create-offer-wizard.tsx - the category-selection enforcement added in the second review round (goNext blocking on categoryId for the Category step) is still correctly wired, with its regression test intact.
  • ADR-031's two corrections (two independent FE-orchestrated mutations, not one backend write) checked against the real backend routes in connection.controller.ts: confirmed PATCH /connections/:id and PUT /connections/:id/credentials are in fact two separate endpoints with no combined-write route - the ADR's correction is factually accurate, not just plausible.
  • Full verification run: pnpm --filter @openlinker/web test - 2043/2043 passing; pnpm --filter @openlinker/web lint - 0 errors (only pre-existing warnings in untouched files); pnpm --filter @openlinker/web type-check - clean.

Verdict: READY TO MERGE - no new blocking or important findings.

Everything found in this pass either restates or extends what the three prior rounds already fixed and documented (false-success toast, invalid CSS token, retry-parameter-loss, credential-rotation wiping Allegro creds, missing category-selection enforcement, mockup copy gaps, missing failure-path tests, ADR documentation drift, verification-indicator and wizard-topology scope cuts).

Non-blocking nitpicks (not blocking merge, just noting for future reference):

  1. erli-credentials-panel.tsx uses inline style={{...}} for layout (flex/gap) rather than a dedicated CSS class - consistent with existing precedent in bulk-edit-modal.tsx / bulk-config-step.tsx, so not asking for a change here, just flagging it as part of a batch worth revisiting if the team ever tightens the "dedicated component classes" convention.
  2. The Erli wizard's Category step duplicates a small amount of manual field-error plumbing (form.setError('categoryId', ...)) that mirrors AllegroCreateOfferWizard almost verbatim. This matches the codebase's stated philosophy of small per-platform duplication over premature shared abstraction, so no change requested - just noting it for any future cross-wizard consolidation pass.

@norbert-kulus-blockydevs norbert-kulus-blockydevs merged commit 3559e0b into epic/1381-erli-allegro-category-catalog Jul 7, 2026

@norbert-kulus-blockydevs norbert-kulus-blockydevs left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated tech-review + pr-review (two independent passes, retrospective)

This PR was already merged (3559e0bd/4e75b300, epic branch) by the time both reviews started. Both agents reviewed the code exactly as it landed, in isolated worktrees, and ran the full quality gate themselves rather than trusting the PR description.

Quality gate — both passes green independently

  • pnpm -r --filter "./libs/**" build — clean
  • pnpm --filter @openlinker/web lint / type-check — 0 errors (76 pre-existing warnings elsewhere, none in touched files)
  • pnpm --filter @openlinker/web test — 239/239 files, 2043/2043 tests pass, including bulk-edit-modal.test.tsx (#1367 gate) — confirmed untouched by this diff and passing
  • pnpm --filter @openlinker/api test -- connection.service.spec — 66-67/67 pass, including the new credentials-merge test

Independently verified as correct

  • Gating signal: connection.config.allegroCategoryAccessEnabled is the only signal used anywhere in the Erli FE surface — grep sweep confirms zero live supportedCapabilities.includes('CategoryBrowser') checks remain (only comments/fixtures documenting why it's not used, per ADR-031's correction).
  • Atomicity design: credentials write first, config patch second (only when the flag or credentials actually changed), second write skipped entirely on first-write failure, fields never cleared on failure so retry resends both. Matches the stated design exactly and is directly exercised by dedicated regression tests for both failure directions.
  • overrides.parameters wiring: z.record(...).default({}) slice with the same z.input/z.output split as Allegro's schema; readParameters reused verbatim (not reimplemented) for the retry/prefill path — good reuse discipline.
  • Required-category enforcement: CategoryPicker has no static RHF-trigger field, so a manual form.setError guard was added at step-advance — a real gap the author caught and fixed in a self-review round before this review even started.
  • Reuse vs duplication: CategoryPicker, CategoryParametersStep, buildParametersZodSchema, categoryParametersToOfferParameters are imported from the sibling Allegro files under the same feature folder — same-feature relative imports, no cross-feature/plugin boundary violation.

Findings

[IMPORTANT] Issue #1384's literal acceptance criterion ("the backend code path... MUST, in the same request, set config.allegroCategoryAccessEnabled") is not satisfied by the shipped design — it's two sequenced FE mutations, not an atomic backend write. This is a reasonable, well-tested trade-off (verified: no backend endpoint accepts both payloads together; ordering fails safe in both directions, now covered by explicit regression tests) and ADR-031 was proactively corrected to document it — but issue #1384's text itself was never edited to stop contradicting the shipped design. Suggest a follow-up edit to the issue body pointing at ADR-031's "Second correction," per this repo's own convention of resolving review threads in the body rather than leaving stale text.

[IMPORTANT] The ConnectionService.updateCredentials fix (merge onto existing stored JSON instead of wholesale replace) is a genuine, correctly-scoped, well-tested bug fix — but it's cross-cutting: it changes shared backend semantics for every platform's credential-rotation flow (PrestaShop, Allegro, WooCommerce, KSeF, inFakt, Erli), not just Erli's, and this isn't called out in the PR description's scope. No evidence any platform relied on the old "omit a field to clear it" semantics, and the new behavior is strictly more conservative (harder to lose data). Suggest a short follow-up sweep/note confirming no other platform's credential-rotation UI depends on clear-by-omission.

[SUGGESTION] The config PATCH half of the save flow is a whole-object client-side merge with no version/ETag check (ConnectionRepository.update replaces existing.config wholesale; the panel only compensates by spreading the last-fetched config). A concurrent edit to baseUrl/allegroEnvironment/etc. in another tab between fetch and save would be silently lost. Pre-existing pattern, not introduced by this PR, and not addressed by the PR's atomicity documentation (which only covers credentials-vs-flag ordering) — worth a doc note or follow-up issue for optimistic concurrency on config PATCH generally.

[SUGGESTION] Unchecking a previously-enabled connection flips the flag to false but leaves allegroClientId/allegroClientSecret dormant in the credentials blob indefinitely (re-enabling doesn't require re-entering the secret). Reasonable choice, consistent with the flag being the sole gating signal, but undocumented — worth a doc note or a future "clear credentials" affordance if dormant-secret retention becomes a concern.

[SUGGESTION] The checkbox+label markup is now duplicated verbatim across erli-credentials-panel.tsx, erli-create-offer-wizard.tsx, and pre-existing Allegro wizard code. No rule forces extraction yet; a third occurrence would be a good trigger for a shared CheckboxField primitive.

Verdict

Approve (retrospective) — no blocking or correctness defects found by either independent pass. The two IMPORTANT items are documentation/scope-hygiene follow-ups (reconcile issue #1384's text with ADR-031, confirm the cross-platform blast radius of the credentials-merge fix), not code changes.

norbert-kulus-blockydevs added a commit that referenced this pull request Jul 8, 2026
…parameters steps (#1401)

* feat(erli,web): checkbox-reveal Allegro credentials panel + offer wizard category/parameters steps

Adds the operator-facing half of the Erli-owned Allegro category-catalog
feature (#1384, part of epic #1381, depends on #1382/#1383):

- ErliCredentialsPanel gains a "Browse Allegro categories when creating
  Erli offers" checkbox that reveals Client ID / Client Secret fields
  (masked, show/hide toggle). Saving sequences two existing mutations from
  one click: useUpdateConnectionCredentialsMutation (credentials pair,
  merged with apiKey) then useUpdateConnectionMutation (config patch for
  allegroCategoryAccessEnabled) — credentials first so a failure never
  leaves the flag advertising access the backend can't serve.

- ErliCreateOfferWizard renders the reused Allegro CategoryPicker +
  CategoryParametersStep (fed by the existing useCategoryParametersQuery)
  as dedicated Category / Category-parameters steps when
  connection.config.allegroCategoryAccessEnabled is true — per ADR-031's
  correction, this per-connection-instance config flag is the FE gating
  signal, not the static, per-adapterKey connection.supportedCapabilities.
  Falls back to today's plain-text field + a link to the connection's edit
  page otherwise. Stepper labels and needsProductParameters become
  capability-conditional; erliCreateOfferSchema gains a parameters slice
  serialized into overrides.parameters on submit (no BE change needed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli,web): address PR #1401 review findings

- Gate `canSubmit` in ErliCredentialsPanel on `allegroEnabled` so
  unchecking the Allegro-access box after typing (but not saving)
  Client ID/Secret disables Save instead of showing a false-positive
  "Credentials saved" toast while discarding the typed secret.
- Drop the checkbox's invalid inline `accentColor: var(--accent)`
  (token doesn't exist) — `index.css` already applies
  `accent-color: var(--accent-primary)` globally.
- Restore category-parameter values on Erli offer retry by reusing
  the Allegro retry mapper's `readParameters` heuristic (exported)
  instead of always prefilling an empty `parameters` object — both
  platforms persist the same neutral `overrides.parameters` shape.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli,connections): merge credential rotation + enforce category pick (#1401 second review)

- ConnectionService.updateCredentials now merges the submitted payload onto
  the existing stored credentialsJson instead of replacing it wholesale, so
  rotating the plain Erli apiKey no longer silently wipes a previously
  configured allegroClientId/allegroClientSecret pair (and enabling Allegro
  access without touching apiKey no longer fails shape validation).
- ErliCreateOfferWizard now blocks Next on the Category step until a
  category is actually selected, mirroring AllegroCreateOfferWizard's
  required categoryId field, instead of silently falling through to
  Category-parameters/Review with an empty category.
- Carried over the approved mockup's helper copy under the Allegro Client ID
  / Client Secret fields and restored the checkbox's original hint text.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* test(erli,docs): cover mutation-sequencing failure paths + close ADR-031 documentation gaps (#1401 third review)

- erli-credentials-panel.test.tsx: add regression tests asserting the config
  patch never fires when the credentials write rejects, and that a config-patch
  rejection after a successful credentials write leaves the flag/fields intact
  with a retryable inline error - the two claims the PR description makes about
  "the trickiest part of this issue" were previously unverified by the suite.
- ADR-031: add a second Correction noting the sequencing is two FE-orchestrated
  mutations, not a single backend write, since no endpoint accepts both the
  credential pair and the config flag together; also record the wizard's
  3-step-vs-5-step topology and the deferred credential-verification indicator
  as explicit, confirmed scope cuts rather than implicit deviations from the
  approved mockup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

---------

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
piotrswierzy pushed a commit that referenced this pull request Jul 8, 2026
…t a required Allegro connection (#1407)

* feat(erli): Erli-owned Allegro client_credentials category-catalog client (#1388)

* feat(erli): add Erli-owned Allegro client_credentials category-catalog client

Adds AllegroCategoryCatalogClient, a self-contained HTTP client that lets
an Erli connection browse Allegro's public /sale/categories and
/sale/categories/{id}/parameters catalog via an Allegro app's
grant_type=client_credentials token, with no dependency on
@openlinker/integrations-allegro (per ADR-030) and no requirement for the
operator to own a real Allegro seller connection.

Extends ErliConnectionConfig with an optional allegroEnvironment field and
ErliCredentials with optional allegroClientId/allegroClientSecret fields
to carry the Allegro app credentials alongside Erli's own apiKey.

Part of #1381, sub-task 1 of 3 (see #1383, #1384).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli): address PR #1388 review findings on AllegroCategoryCatalogClient

- Extract inline wire-shape types to allegro-category-catalog-client.types.ts,
  matching the erli-http-client.ts / erli-http-client.types.ts split.
- Add a single-flight guard around token acquisition so two concurrent
  fetchCategories/fetchCategoryParameters calls on a cold cache issue one
  grant_type=client_credentials request, not two.
- Treat a token response missing expires_in as already-expired instead of
  caching it indefinitely.
- Add tests for the 401/403 branch on a data call (categories/parameters),
  distinct from the token-endpoint rejection branch already covered.
- Add a cross-plugin mapper parity test suite that runs Allegro's real
  sandbox category-parameters fixture through this client's copy of
  toNeutralCategoryParameter, mirroring assertions from
  allegro-category-parameter.mapper.spec.ts, with cross-referencing comments
  in both spec files so a future mapper change prompts updating both.
- Replace the inline 'sandbox' | 'production' union with an as const +
  runtime-array AllegroCatalogEnvironment type in erli-connection.types.ts,
  matching AllegroConnectionConfig's own convention.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

---------

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

* feat(erli): wire per-connection Allegro category-catalog capability + credential/config validation (#1399)

* feat(erli): wire per-connection Allegro category-catalog capability + credential/config validation (#1383)

ErliOfferManagerAdapter now wires fetchCategories/fetchCategoryParameters as
optional instance properties (never a static implements clause) so
isCategoryBrowser/isCategoryParametersReader reflect whether a specific Erli
connection has configured a valid Allegro app credential pair, per ADR-031.
ErliAdapterFactory constructs the shared AllegroCategoryCatalogClient only
when both allegroClientId and allegroClientSecret resolve to non-empty
strings, resolving config.allegroEnvironment (default 'production').

The credentials shape validator now enforces "both or neither" for the
Allegro credential pair, and the config shape validator restricts
allegroEnvironment to 'sandbox' | 'production'.

Verified CategoriesCacheService and ListingsController need no changes: both
already resolve capabilities generically via getCapabilityAdapter + is* type
guards, so a misconfigured Erli connection behaves like today's
"adapter doesn't implement this capability" case.

Added an integration test exercising the real ErliAdapterFactory +
ErliOfferManagerAdapter + AllegroCategoryCatalogClient (fetch stubbed) through
the production adapter-resolution seam and the real HTTP endpoint, covering
configured / unconfigured / partially-configured connections. Confirmed the
existing #1367 bulk-wizard capability-gate test passes unmodified.

Part of #1381, sub-task 2 of 4 (depends on #1382, merged; see also #1384, #1387).

Signed-off-by: Norbert Kulus <42zeroo@gmail.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli): add allegroCategoryAccessEnabled config flag as the FE-visible signal

connection.supportedCapabilities turned out to be a static, per-adapterKey
manifest value rather than computed per-connection-instance, so it can't
distinguish an Erli connection with configured Allegro app credentials from
one without. Add a non-secret ErliConnectionConfig.allegroCategoryAccessEnabled
boolean for the frontend to read instead (see ADR-031 "Correction"); the
write path that sets allegroClientId/allegroClientSecret must set/clear this
flag atomically (tracked in #1384's scope).

Also fixes stale ADR-030 references (renumbered to ADR-031 to avoid a
collision with #1307) in allegro-category-catalog-client.ts and
erli-connection.types.ts.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli): hoist Allegro category-token cache to host.cache + dedupe credential resolve (#1399 review)

AllegroCategoryCatalogClient built a fresh in-memory token cache per adapter
instance, but ErliAdapterFactory builds a fresh instance per
getCapabilityAdapter call — so fetchCategoryParameters paid a full
client_credentials OAuth round-trip on every request. The client now
persists the acquired token in the optional CachePort (host.cache), keyed by
clientId, with a TTL matching the token's remaining lifetime, so it survives
across per-request instances and processes.

Also collapses ErliAdapterFactory.createAdapters's two credentialsResolver.get
calls (one for apiKey, one for the Allegro pair) into one shared resolve.

Addresses both IMPORTANT findings from the PR #1399 review round (Piotr's
token-cache note and the self-review's double-resolve finding).

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

---------

Signed-off-by: Norbert Kulus <42zeroo@gmail.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* feat(erli,web): Allegro credentials checkbox + offer wizard category/parameters steps (#1401)

* feat(erli,web): checkbox-reveal Allegro credentials panel + offer wizard category/parameters steps

Adds the operator-facing half of the Erli-owned Allegro category-catalog
feature (#1384, part of epic #1381, depends on #1382/#1383):

- ErliCredentialsPanel gains a "Browse Allegro categories when creating
  Erli offers" checkbox that reveals Client ID / Client Secret fields
  (masked, show/hide toggle). Saving sequences two existing mutations from
  one click: useUpdateConnectionCredentialsMutation (credentials pair,
  merged with apiKey) then useUpdateConnectionMutation (config patch for
  allegroCategoryAccessEnabled) — credentials first so a failure never
  leaves the flag advertising access the backend can't serve.

- ErliCreateOfferWizard renders the reused Allegro CategoryPicker +
  CategoryParametersStep (fed by the existing useCategoryParametersQuery)
  as dedicated Category / Category-parameters steps when
  connection.config.allegroCategoryAccessEnabled is true — per ADR-031's
  correction, this per-connection-instance config flag is the FE gating
  signal, not the static, per-adapterKey connection.supportedCapabilities.
  Falls back to today's plain-text field + a link to the connection's edit
  page otherwise. Stepper labels and needsProductParameters become
  capability-conditional; erliCreateOfferSchema gains a parameters slice
  serialized into overrides.parameters on submit (no BE change needed).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli,web): address PR #1401 review findings

- Gate `canSubmit` in ErliCredentialsPanel on `allegroEnabled` so
  unchecking the Allegro-access box after typing (but not saving)
  Client ID/Secret disables Save instead of showing a false-positive
  "Credentials saved" toast while discarding the typed secret.
- Drop the checkbox's invalid inline `accentColor: var(--accent)`
  (token doesn't exist) — `index.css` already applies
  `accent-color: var(--accent-primary)` globally.
- Restore category-parameter values on Erli offer retry by reusing
  the Allegro retry mapper's `readParameters` heuristic (exported)
  instead of always prefilling an empty `parameters` object — both
  platforms persist the same neutral `overrides.parameters` shape.

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* fix(erli,connections): merge credential rotation + enforce category pick (#1401 second review)

- ConnectionService.updateCredentials now merges the submitted payload onto
  the existing stored credentialsJson instead of replacing it wholesale, so
  rotating the plain Erli apiKey no longer silently wipes a previously
  configured allegroClientId/allegroClientSecret pair (and enabling Allegro
  access without touching apiKey no longer fails shape validation).
- ErliCreateOfferWizard now blocks Next on the Category step until a
  category is actually selected, mirroring AllegroCreateOfferWizard's
  required categoryId field, instead of silently falling through to
  Category-parameters/Review with an empty category.
- Carried over the approved mockup's helper copy under the Allegro Client ID
  / Client Secret fields and restored the checkbox's original hint text.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

* test(erli,docs): cover mutation-sequencing failure paths + close ADR-031 documentation gaps (#1401 third review)

- erli-credentials-panel.test.tsx: add regression tests asserting the config
  patch never fires when the credentials write rejects, and that a config-patch
  rejection after a successful credentials write leaves the flag/fields intact
  with a retryable inline error - the two claims the PR description makes about
  "the trickiest part of this issue" were previously unverified by the suite.
- ADR-031: add a second Correction noting the sequencing is two FE-orchestrated
  mutations, not a single backend write, since no endpoint accepts both the
  credential pair and the config flag together; also record the wizard's
  3-step-vs-5-step topology and the deferred credential-verification indicator
  as explicit, confirmed scope cuts rather than implicit deviations from the
  approved mockup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

---------

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

* feat(erli): reuse an existing Allegro connection's credentials for category access (#1387) (#1405)

Extends the #1384 Erli credentials panel with a reuse-vs-manual choice: when
the operator already has an Allegro connection, its clientId/clientSecret can
be copied server-side into the Erli connection instead of registering a
second Allegro app. The raw clientSecret never round-trips through the
browser.

The credential-copy logic is dispatched through a new, platform-neutral
ConnectionCredentialsRewriterPort + registry (mirroring the existing
ConnectionConfigShapeValidatorPort/ConnectionCredentialsShapeValidatorPort
pair) so the generic ConnectionService.updateCredentials stays unaware that
Allegro or Erli exist. The Erli-specific resolution lives entirely in
ErliAllegroCredentialsRewriterAdapter, registered from a companion
ErliCredentialsRewriterModule (mirrors ErliWebhookProvisioningModule) since
it needs ConnectionPort, which is intentionally outside the plugin-neutral
HostServices bag.

Closes #1387

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

---------

Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
Signed-off-by: Norbert Kulus <42zeroo@gmail.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant