retrofit: extend method-decomposition with SettingsController public API (5 REQs / 18 methods)

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
     {

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

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.

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: <message>}` 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: <value>}}`
+
+### 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: <int>` 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: <message>, timestamp: <int>}`
+
+### 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: <msg>", error: <msg>}`
+
+### 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: <current-ms>`
+
+#### 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: <snapshot>}` 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.