From c0c97d1d1f4d8ef2654fe6935d25d9f82b289bf3 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 24 May 2026 20:57:35 +0200 Subject: [PATCH] retrofit: extend method-decomposition with SettingsController public API (REQ-DECOMP-013..017) Extends method-decomposition capability with 5 retrofit REQs covering 18 SettingsController public methods that fall outside REQ-DECOMP-001's named scope. Groupings: settings CRUD / config bootstrap+status / sync orchestration / cache+heartbeat diagnostics / progress streaming. Notes flag 3 concrete bugs spotted during reverse-spec: empty-if blocks inverting HTTP status codes in performSync (always 500 on incremental success) and resetAutoConfig (always 400 on success), plus missing @NoAdminRequired on the general config endpoints. 23 remaining SettingsController _misc_ methods + Service-side misc clusters deferred to future retrofit passes. Refs #285 --- lib/Controller/SettingsController.php | 36 ++++++ .../design.md | 30 +++++ .../proposal.md | 33 +++++ .../specs/method-decomposition/spec.md | 120 ++++++++++++++++++ .../tasks.md | 7 + 5 files changed, 226 insertions(+) create mode 100644 openspec/changes/retrofit-2026-05-24-method-decomposition/design.md create mode 100644 openspec/changes/retrofit-2026-05-24-method-decomposition/proposal.md create mode 100644 openspec/changes/retrofit-2026-05-24-method-decomposition/specs/method-decomposition/spec.md create mode 100644 openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 0cabfdce..28545d1c 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -142,6 +142,8 @@ public function getConfigurationService(): ?ConfigurationService * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function index(): JSONResponse { @@ -177,6 +179,8 @@ public function index(): JSONResponse * @SuppressWarnings(PHPMD.CyclomaticComplexity) * @SuppressWarnings(PHPMD.NPathComplexity) * @SuppressWarnings(PHPMD.ExcessiveMethodLength) + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function create(): JSONResponse { @@ -285,6 +289,8 @@ function ($key) { * @NoCSRFRequired * * @return JSONResponse General configuration + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function getGeneralConfig(): JSONResponse { @@ -322,6 +328,8 @@ public function getGeneralConfig(): JSONResponse * @NoCSRFRequired * * @return JSONResponse Update result + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function updateGeneralConfig(): JSONResponse { @@ -365,6 +373,8 @@ public function updateGeneralConfig(): JSONResponse * @NoCSRFRequired * * @return JSONResponse Sync configuration + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function getSyncConfig(): JSONResponse { @@ -402,6 +412,8 @@ public function getSyncConfig(): JSONResponse * @NoCSRFRequired * * @return JSONResponse Update result + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-1 */ public function updateSyncConfig(): JSONResponse { @@ -445,6 +457,8 @@ public function updateSyncConfig(): JSONResponse * @return JSONResponse JSON response containing the settings. * * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-2 */ public function load(): JSONResponse { @@ -463,6 +477,8 @@ public function load(): JSONResponse * @return JSONResponse JSON response containing the initialization results * * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-2 */ public function initialize(): JSONResponse { @@ -487,6 +503,8 @@ public function initialize(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-2 */ public function status(): JSONResponse { @@ -582,6 +600,8 @@ public function autoConfigure(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-4 */ public function stats(): JSONResponse { @@ -617,6 +637,8 @@ public function stats(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-4 */ public function debug(): JSONResponse { @@ -688,6 +710,8 @@ public function sendTestEmail(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-3 */ public function getSyncStatus(int $minutesBack=10): JSONResponse { @@ -703,6 +727,8 @@ public function getSyncStatus(int $minutesBack=10): JSONResponse * @return JSONResponse JSON response containing sync results * * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-3 */ public function performSync(int $minutesBack=0): JSONResponse { @@ -763,6 +789,8 @@ public function performSync(int $minutesBack=0): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-4 */ public function heartbeat(): JSONResponse { @@ -811,6 +839,8 @@ public function heartbeat(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-2 */ public function getVersionInfo(): JSONResponse { @@ -884,6 +914,8 @@ public function resetAutoConfig(): JSONResponse * @return JSONResponse JSON response containing cache clear results. * * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-4 */ public function clearCache(): JSONResponse { @@ -1101,6 +1133,8 @@ public function consolidatedAutoConfigure(): JSONResponse * * @NoAdminRequired * @NoCSRFRequired + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-5 */ public function getProgress(string $operationId): JSONResponse { @@ -1156,6 +1190,8 @@ public function getProgress(string $operationId): JSONResponse * * @SuppressWarnings(PHPMD.ExcessiveMethodLength) * @SuppressWarnings(PHPMD.UnusedFormalParameter) + * + * @spec openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md#task-5 */ public function streamProgress(string $operationId): Response { diff --git a/openspec/changes/retrofit-2026-05-24-method-decomposition/design.md b/openspec/changes/retrofit-2026-05-24-method-decomposition/design.md new file mode 100644 index 00000000..27c6355d --- /dev/null +++ b/openspec/changes/retrofit-2026-05-24-method-decomposition/design.md @@ -0,0 +1,30 @@ +# Design — Retrofit method-decomposition (SettingsController public API) + +Retrofit change. Tasks describe retroactive annotation, not new implementation work. + +## Context + +`method-decomposition` is a refactoring/quality spec, not a feature spec — its existing REQ-DECOMP-001..012 describe target-state handler classes that don't yet exist (see Bucket 3b in the coverage report). The current code is the *subject* of decomposition. + +The coverage scan flagged 23 SettingsController methods as `_misc_*` aggregate rows under Bucket 2a — methods in REQ-DECOMP-001's file scope but outside its named-method scope (`syncSoftwareCatalogue`, `registerModules`, `syncOrganizations`, `configureArchiMate`). They describe observable HTTP behaviour, not refactoring targets, but they live in a file the existing capability already owns. + +## Decisions + +- **`--extend method-decomposition`** — the methods are in a file already tied to this capability. Minting a new capability for "SettingsController public API" would fork ownership of the same file across two specs. +- **REQ-DECOMP-013..017 (5 REQs)** — within the 5-REQ-per-run cap. Future runs add 014+ for the remaining files. +- **REQ language describes the contract.** Three bugs (empty-if blocks that invert HTTP status codes in `performSync` + `resetAutoConfig`, missing `@NoAdminRequired` on general config endpoints) were spotted while reading. They are flagged in Notes, not silently encoded as REQ behaviour. +- **Frontmatter uses block-YAML `retrofit_extensions`** per skill convention. Five entries, one per REQ. +- **17 methods annotated, 6 groupings** — methods within a REQ that share a behavioural cluster (e.g. `getGeneralConfig` + `updateGeneralConfig` both under REQ-DECOMP-013) get the same `@spec` tag pointing at the same task. + +## Out of scope + +- Fixing the empty-if bugs (`performSync`, `resetAutoConfig`). +- Revisiting `@NoAdminRequired` on `getGeneralConfig` / `updateGeneralConfig`. +- The remaining 14 SettingsController methods + 6 service files in Bucket 2a `_misc_*`. + +## References + +- Umbrella: ConductionNL/softwarecatalog#285 +- Coverage report: openspec/coverage-report.md (2026-05-24) +- Sibling retrofit (progress-tracking): PR #288 — same empty-if bug pattern +- Source: lib/Controller/SettingsController.php diff --git a/openspec/changes/retrofit-2026-05-24-method-decomposition/proposal.md b/openspec/changes/retrofit-2026-05-24-method-decomposition/proposal.md new file mode 100644 index 00000000..83c0db9d --- /dev/null +++ b/openspec/changes/retrofit-2026-05-24-method-decomposition/proposal.md @@ -0,0 +1,33 @@ +# Retrofit — method-decomposition (extend with SettingsController public API) + +Extends the existing `method-decomposition` capability with 5 new REQs that retro-describe the observable contract of `SettingsController` public methods that are NOT covered by REQ-DECOMP-001's scope (which only names `syncSoftwareCatalogue`, `registerModules`, `syncOrganizations`, `configureArchiMate`). Bucket 2a in the coverage report flagged 23 SettingsController methods as `_misc_*` aggregate rows — this run captures the first 5 behavioural groupings (settings CRUD, configuration status/version, sync orchestration, cache + heartbeat diagnostics, progress streaming). + +Code already exists — this change retroactively specifies it. + +## Scope (this run) + +5 REQs covering 18 SettingsController public methods: + +- **REQ-DECOMP-013 — Settings CRUD endpoints** (index, create, getGeneralConfig, updateGeneralConfig, getSyncConfig, updateSyncConfig) +- **REQ-DECOMP-014 — Configuration bootstrap + status endpoints** (load, initialize, status, getVersionInfo) +- **REQ-DECOMP-015 — Sync orchestration endpoints** (getSyncStatus, performSync) +- **REQ-DECOMP-016 — Cache + heartbeat diagnostics** (clearCache, heartbeat, stats, debug) +- **REQ-DECOMP-017 — Progress snapshot + SSE streaming** (getProgress, streamProgress) + +## Out of scope (future runs) + +23 methods remain in Bucket 2a `_misc_` — split into future PRs of 5 REQs each: + +- **SettingsController remainder** (autoConfigure, resetAutoConfig, manualImport, forceUpdate, consolidatedAutoConfigure, importArchiMate, exportArchiMate, exportOrgArchiMate, downloadArchiMate, sendTestEmail, testEmailConnection, getEmailSettings, updateEmailSettings, render — 14 methods) +- **SettingsService _misc_ 50+ methods** +- **SoftwareCatalogueService _misc_ 40+ methods** +- **ArchiMate Import/Export/Service _misc_ ~170 methods combined** +- **ContactPersonHandler _misc_ 30+ methods** +- 7 other services with `_misc_methods` aggregate rows + +## Approach + +- For each REQ group: describe observed contract (request shape, success envelope, status codes, exception path), flag any observed-but-buggy behaviour. +- Notes section captures three concrete bugs visible while reading the code (empty-if blocks that invert success/failure status codes in `performSync` and `resetAutoConfig`, mirroring the `progress-tracking#calculateOverallPercentage` bug from PR #288). + +Source: openspec/coverage-report.md generated 2026-05-24. Umbrella: ConductionNL/softwarecatalog#285. diff --git a/openspec/changes/retrofit-2026-05-24-method-decomposition/specs/method-decomposition/spec.md b/openspec/changes/retrofit-2026-05-24-method-decomposition/specs/method-decomposition/spec.md new file mode 100644 index 00000000..96563f05 --- /dev/null +++ b/openspec/changes/retrofit-2026-05-24-method-decomposition/specs/method-decomposition/spec.md @@ -0,0 +1,120 @@ +--- +retrofit_extensions: + - REQ-DECOMP-013 + - REQ-DECOMP-014 + - REQ-DECOMP-015 + - REQ-DECOMP-016 + - REQ-DECOMP-017 +--- + +## ADDED Requirements + +### REQ-DECOMP-013: SettingsController Settings CRUD endpoints + +The SettingsController MUST expose JSON endpoints to read and write the four primary settings blocks — full settings tree, configuration + selected register, general config (`catalogLocation`), and sync config — plus a unified `create` endpoint that routes by request-body shape. + +**Endpoints**: `index()`, `create()`, `getGeneralConfig()`, `updateGeneralConfig()`, `getSyncConfig()`, `updateSyncConfig()`. + +**Common contract**: Each endpoint MUST catch every `\Exception`, log it via `LoggerInterface::error`, and return `{error: }` with HTTP 500. Successful reads MUST return either the raw settings tree (`index`) or `{success: true, config: {...}}` (the granular getters). Successful writes MUST return `{success: true, data: ..., message: ...}` (for `create`) or `{success: true, message: ..., config: {...}}` (for the granular setters). + +**`create()` routing**: It MUST inspect `request.params` and dispatch on the presence of `configuration` / `selectedRegister` → `SettingsService::updateSettings`; `userGroups.generic` / `.organizationAdmin` / `.superUser` → validate each via `validateGroups`, return HTTP 400 with the validation envelope on any `invalid` entries, otherwise persist via the matching setter; `emailSettings` → `updateEmailSettings`. Multiple sections in one body MUST be processed in sequence and reported under a combined `data` map. + +#### Scenario: index attaches openRegisters availability + isAdmin +- GIVEN the OpenRegister app is installed and the current user is in the `admin` group +- WHEN `GET /settings` is invoked +- THEN the response body MUST include `openRegisters: true` and `isAdmin: true` alongside the settings tree + +#### Scenario: create with invalid generic group returns 400 +- GIVEN `request.params = { userGroups: { generic: ['no-such-group'] } }` +- AND `validateGroups` flags `'no-such-group'` as invalid +- WHEN `POST /settings` is invoked +- THEN the response status MUST be `400` +- AND the body MUST equal `{error: "Invalid generic group names provided", validation: {...}}` + +#### Scenario: getGeneralConfig surfaces catalogLocation +- WHEN `GET /settings/general` is invoked +- THEN the response body MUST equal `{success: true, config: {catalogLocation: }}` + +### REQ-DECOMP-014: Configuration bootstrap + status endpoints + +The SettingsController MUST expose four endpoints that surface app readiness and version information for the admin UI: `load()` (initial UI bootstrap payload), `initialize()` (idempotent first-run setup), `status()` (current configuration health), `getVersionInfo()` (app version + cache-busting timestamp). + +`getVersionInfo()` MUST attach a `timestamp` field (Unix seconds at response time) to every response — including error responses — for cache-busting on the frontend. + +`status()` MUST aggregate the configuration health summary from `SettingsService` and return it as a JSON envelope without wrapping; consumers depend on the raw shape. + +#### Scenario: getVersionInfo attaches timestamp on success +- WHEN `GET /settings/version` is invoked successfully +- THEN the response body MUST include `timestamp: ` matching `time()` at response time + +#### Scenario: getVersionInfo attaches timestamp on error +- GIVEN `SettingsService::getVersionInfo()` throws +- WHEN the endpoint is invoked +- THEN the response status MUST be `500` +- AND the body MUST include `{error: , timestamp: }` + +### REQ-DECOMP-015: Sync orchestration endpoints + +The SettingsController MUST expose two endpoints for organisation synchronisation: `getSyncStatus(minutesBack=10)` (read-only sync status with error handling) and `performSync(minutesBack=0)` (trigger sync). + +`performSync` MUST branch on `minutesBack`: `0` → full optimized sync via `OrganizationSyncService::performOptimizedManualSync(maxRounds: 15, batchSize: 75)` returning `{success: true, results, message, isOptimized: true}`; non-zero → incremental sync via `performManualSync($minutesBack)` returning the service result. + +`getSyncStatus` MUST delegate without try/catch — the underlying service method already wraps errors into the response shape. + +#### Scenario: Full sync invokes optimized path +- WHEN `POST /settings/sync` is called with `minutesBack=0` +- THEN `performOptimizedManualSync` MUST be invoked with `maxRounds: 15` and `batchSize: 75` +- AND the response body MUST contain `isOptimized: true` + +#### Scenario: Sync exception maps to 500 with success: false +- GIVEN the underlying sync service throws +- WHEN the endpoint is invoked +- THEN the response status MUST be `500` +- AND the body MUST equal `{success: false, message: "Synchronization failed: ", error: }` + +### REQ-DECOMP-016: Cache, heartbeat, and diagnostic endpoints + +The SettingsController MUST expose lightweight endpoints for ops + diagnostics: `clearCache()` (force schema/register cache reload), `heartbeat()` (keep-alive for long-running browser-side operations), `stats()` (catalog statistics), `debug()` (diagnostic dump). + +`heartbeat()` MUST accept an optional `timestamp` query parameter (defaulting to `time() * 1000`), echo it back alongside the server's current timestamp, and respond with `{success: true, message: "Heartbeat received", timestamp, server_time}`. Both timestamps MUST be in milliseconds. + +`heartbeat()` is the only endpoint in this group annotated `@NoAdminRequired`; the rest require admin (default for `@NoCSRFRequired` without `@NoAdminRequired` is admin-required per ADR-005). + +#### Scenario: Heartbeat echoes timestamp in ms +- GIVEN the client sends `timestamp=1716576000000` +- WHEN `POST /settings/heartbeat` is invoked +- THEN the response body MUST contain `timestamp: 1716576000000` and `server_time: ` + +#### Scenario: Heartbeat default timestamp uses server time +- GIVEN no `timestamp` parameter is provided +- WHEN the endpoint is invoked +- THEN the response `timestamp` MUST equal the server's current `time() * 1000` + +### REQ-DECOMP-017: Progress snapshot + SSE streaming endpoints + +The SettingsController MUST expose two progress-related endpoints that consume the `ProgressTracker` service (see `progress-tracking#REQ-005`): `getProgress(operationId)` (one-shot JSON snapshot) and `streamProgress(operationId)` (Server-Sent Events stream). + +`getProgress` MUST delegate to `ProgressTracker::getProgress($operationId)` and return `{success: true, progress: }` with HTTP 200 when a snapshot exists, or `{success: false, error: "Operation not found"}` with HTTP 404 when it does not. + +`streamProgress` MUST return an `OCP\AppFramework\Http\Response` subclass that streams `text/event-stream` events for the operation until the operation reaches phase `completed` or the client disconnects. The response MUST set `Content-Type: text/event-stream`, `Cache-Control: no-cache`, and `Connection: keep-alive`. + +#### Scenario: getProgress returns 404 for unknown id +- GIVEN no operation with id `import_xyz` exists in the session +- WHEN `GET /settings/progress/import_xyz` is invoked +- THEN the response status MUST be `404` +- AND the body MUST contain `{success: false, error: "Operation not found"}` + +#### Scenario: streamProgress sets SSE headers +- WHEN `GET /settings/stream-progress/import_abc` is invoked +- THEN the response Content-Type MUST be `text/event-stream` +- AND `Cache-Control` MUST be `no-cache` + +## Notes + +These REQs describe the *intended contract* of the endpoints. Three concrete bugs were spotted while reverse-spec'ing — flagged here for follow-up rather than silently captured as REQ behaviour: + +- **`performSync()` empty-if inverts incremental-sync status code.** Lines 732-735: `if ($result['success'] === true) { }` empty block followed by an unconditional `return new JSONResponse($result, 500);`. Successful incremental syncs (any non-zero `minutesBack` value) return HTTP 500 with the success envelope. The intended branch was probably `return new JSONResponse($result, 200);` inside the if-block. Mirrors the `progress-tracking#calculateOverallPercentage` bug pattern from PR #288. +- **`resetAutoConfig()` has the same empty-if pattern.** Lines 865-868: `if ($result['success'] === true) { }` empty followed by `return new JSONResponse($result, 400);` (indented inside the function but outside the if). Successful resets return HTTP 400. Same fix shape as performSync. +- **`getGeneralConfig()` / `updateGeneralConfig()` lack `@NoAdminRequired`.** They are decorated only with `@NoCSRFRequired`. Per ADR-005 + the Nextcloud framework defaults, this routes them through admin-required middleware. The intent is unclear — REQ-DECOMP-013's contract describes successful responses irrespective of the auth posture; revisiting the annotations is a separate concern. + +The 14 remaining SettingsController public methods (autoConfigure, resetAutoConfig, manualImport, forceUpdate, consolidatedAutoConfigure, importArchiMate, exportArchiMate, exportOrgArchiMate, downloadArchiMate, sendTestEmail, testEmailConnection, getEmailSettings, updateEmailSettings, render) are intentionally deferred — they form behavioural groups (ArchiMate import/export, email transport, auto-config orchestration) that warrant their own REQ groupings in a subsequent retrofit pass. diff --git a/openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md b/openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md new file mode 100644 index 00000000..31998ac9 --- /dev/null +++ b/openspec/changes/retrofit-2026-05-24-method-decomposition/tasks.md @@ -0,0 +1,7 @@ +# Tasks + +- [x] task-1: method-decomposition#REQ-DECOMP-013 — SettingsController Settings CRUD endpoints (retroactive annotation; 6 methods: index, create, getGeneralConfig, updateGeneralConfig, getSyncConfig, updateSyncConfig) +- [x] task-2: method-decomposition#REQ-DECOMP-014 — Configuration bootstrap + status endpoints (retroactive annotation; 4 methods: load, initialize, status, getVersionInfo) +- [x] task-3: method-decomposition#REQ-DECOMP-015 — Sync orchestration endpoints (retroactive annotation; 2 methods: getSyncStatus, performSync) +- [x] task-4: method-decomposition#REQ-DECOMP-016 — Cache, heartbeat, and diagnostic endpoints (retroactive annotation; 4 methods: clearCache, heartbeat, stats, debug) +- [x] task-5: method-decomposition#REQ-DECOMP-017 — Progress snapshot + SSE streaming endpoints (retroactive annotation; 2 methods: getProgress, streamProgress)