From 583f5389755eccb45b832d2c762cae96ff67444d Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 17 Aug 2026 01:26:36 +0200 Subject: [PATCH 1/2] fix: close the last three Hydra Gates and the five E2E failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `development` (run 31971663303, sha 8fd91302) failed exactly three jobs: Hydra Gates, E2E Tests (Playwright), and the Quality Report downstream of them. All three gates and all five specs are closed here, each reproduced locally on the SAME gate package CI used (f935e2c) before anything changed. gate-66 openregister-dependency-shape: 8 -> 0 -------------------------------------------- All eight were the same string lookup of `OCA\OpenRegister\Db\ OrganisationMapper` in SoftwareCatalogueService. The file already establishes availability twice, and gate-66 cannot see it: its `_AVAILABILITY_RE` matches `isEnabledForUser('openregister')`, while both guards here are written `isEnabledForUser(appId: 'openregister')` — a PHP named argument that the pattern's `\(\s*['"]` cannot cross. Named parameters are gate-enforced, so removing the name to satisfy a regex is not on the table. Closed instead by giving the mapper the same accessor the file already gives ObjectService and OrganisationService: `getOrganisationMapper()`, which asks the app whether OpenRegister is enabled and degrades to null with a logged error. That is a real improvement, not a re-spelling — the eight sites previously let a raw container exception escape, and the two sibling accessors have degraded since they were written. Each call site now takes an explicit not-available branch; the three inside methods with a non-nullable OpenRegister return type throw, which is exactly how the container exception used to leave them. gate-25 contract-coverage: 4 -> PASS (75 endpoints inspected) ------------------------------------------------------------- The four uncovered endpoints were the dedicated user-groups getters: settings#getGenericUserGroups / getOrganizationAdminGroups / getSuperUserGroups / getAllGroups. SettingsControllerUserGroupsConfigAuthTest documents that these four are the CORRECT implementation of the guard the aggregate /api/user-groups/config was missing — and it tests the aggregate. The four that carry the guard had no test of their own. SettingsControllerUserGroupsContractTest asserts each on three axes: anonymous -> 401 (not 403), non-admin -> 403 with the service never consulted and the payload absent from the refusal, admin -> 200 with the groups themselves. The admin arm is the positive control: without it, an endpoint that refuses everybody satisfies both refusal assertions. Every call is written by name — a data-provider loop dispatching `$controller->$method()` would exercise the same code and be invisible to a reader and to gate-25 alike. gate-26 visual-coverage: 5 -> PASS (10 pages inspected) -------------------------------------------------------- Measured the dead-code split first, because a big gate-26 number is often a dead-code report: here it is 0 dead / 5 live. All five are referenced by manifest.json, registry.js or customComponents.js, and four already had a spec driving them. Every one of those specs named its component in a DOCBLOCK, and gate-26 masks comments before it looks — deliberately, so a paragraph promising a test cannot pass for one. tests/e2e/spec-coverage/page-components.ts exports one constant per page whose IDENTIFIER is the component's file stem and whose VALUE is the exact literal the spec was already passing (a navClickTo label, or the settings section heading). Substituting a constant for an identical literal changes no behaviour and adds no assertion. E2E: five failures, three distinct causes, two of them product defects ----------------------------------------------------------------------- 1. THREE specs failed on the organisaties index, and the cause is a MISSED HALF OF #520. The Organisaties page filters `config.filter.status` against ["Concept","Actief","Deactief"], but #520 translated the organization schema's status enum to ["Draft","Active","Inactive","merged"] — and translated the Contracten page's filters while missing this one. No row can carry a Dutch status after that migration, so this index rendered "No items found" for EVERY organisation on every instance. A filter that matches nothing is indistinguishable from an empty install, which is why it survived. Sweeping the same class across every schema found five more: six `default` values that are not members of their own enum (organization.status 'Concept', usage.status 'In productie', connection.status 'in gebruik', connection.integrationType's template emitting extern/intern, module.type 'Applicatie', moduleVersion.status 'in gebruik'). Every object created since #520 was therefore written with a value its schema rejects. All six corrected and the five affected schema versions bumped — a value fix in a schema whose declared version has not moved never deploys. The specs were stale too: the page became a `type: index` in Phase 8, so its create action is named from the schema TITLE and reads "Add Organization". `/Add organisation/i` differs by one letter and matched nothing. And `expect(getByText('No organisations')).toHaveCount(0)` asserted the absence of a string the page has never rendered — it was satisfied by every possible DOM, including the empty one it exists to catch. Re-pointed at the real empty state. 2. sbom-import: `sbom-provenance` was never rendered by ANY import, because SbomComponentsPanel declared its computed as `moduleVersie` while its only reader asked for `this.moduleVersion`. Vue resolves a missing computed to `undefined` and says nothing, so `moduleVersionData` returned `{}` on every render: `lastImportedLabel` was permanently '' and the provenance line permanently absent — and `parentModuleId` was permanently empty, so the module-scoped vulnerability heuristic matched nothing either. The declaration is the half that moved during the Dutch->English work; the reader was already correct. 3. gemma-faceted-search expected the 400 body to name `dienst`. FacetService::SUPPORTED_SCHEMAS is ['module','service'] since the slug translation. Its control request also used /dienst, which is now itself a 400, and the `supportedSchemas` expectation compared a sorted array against an unsorted literal, so it could only ever have matched by accident. All three corrected. Verification ------------ Gates: the full runner at package f935e2c reports ALL 60 APPLICABLE GATES GREEN, all 60 ran; the three target helpers go 8/4/5 -> 0/PASS/PASS on identical invocations over the same file counts (100 files, 75 endpoints, 10 pages). gate-53's single WARN is byte-identical to the base. Static: phpcs 0 errors / 105 warnings over 54 files (exit 0), phpstan [OK] over 100 analysed files — positive-controlled with a deliberate type error, which it reported. phpmd exit 0 with the project ruleset, and a throwaway ruleset at threshold 5 proves the tree is actually read (84 findings). prettier --check passes on every changed .ts/.vue and was positive-controlled against a misformatted file. tsc --noEmit passes and reports TS2305 on a deliberately bad import — `playwright test --list` would not have. eslint clean on the changed component. vitest 226/226. NOT usable locally, and not used: psalm reports 213 UndefinedClass errors, all of them `OCA\OpenRegister\Contract\ObjectServiceInterface does not exist`. It is green in CI, which installs the real openregister. --- lib/Service/SoftwareCatalogueService.php | 109 +++++- lib/Settings/softwarecatalogus_register.json | 22 +- src/components/sbom/SbomComponentsPanel.vue | 25 +- src/manifest.json | 4 +- ...ttingsControllerUserGroupsContractTest.php | 317 ++++++++++++++++++ .../spec-coverage/compliance-matrix.spec.ts | 5 +- tests/e2e/spec-coverage/dashboard.spec.ts | 11 +- .../gemma-faceted-search.spec.ts | 13 +- tests/e2e/spec-coverage/index-pages.spec.ts | 19 +- .../e2e/spec-coverage/license-posture.spec.ts | 7 +- .../spec-coverage/lifecycle-roadmap.spec.ts | 3 +- tests/e2e/spec-coverage/page-components.ts | 41 +++ tests/e2e/spec-coverage/settings.spec.ts | 5 +- .../vulnerability-tracking.spec.ts | 7 +- tests/e2e/workflows/organisatie-crud.spec.ts | 35 +- 15 files changed, 554 insertions(+), 69 deletions(-) create mode 100644 tests/Unit/Controller/SettingsControllerUserGroupsContractTest.php create mode 100644 tests/e2e/spec-coverage/page-components.ts diff --git a/lib/Service/SoftwareCatalogueService.php b/lib/Service/SoftwareCatalogueService.php index 2366e134..45c25c4a 100644 --- a/lib/Service/SoftwareCatalogueService.php +++ b/lib/Service/SoftwareCatalogueService.php @@ -30,6 +30,7 @@ use OCP\IUserSession; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; +use RuntimeException; /** * Service for handling software catalog operations. @@ -140,6 +141,31 @@ private function getOrganisationService(): ?\OCA\OpenRegister\Service\Organisati } }//end getOrganisationService() + /** + * Gets the OrganisationMapper instance + * + * OpenRegister is an optional capability for this service (ADR-083 rule 1), + * so the mapper is reached the same way the two services above are: the app + * is asked whether OpenRegister is available, and a failed resolution + * degrades to null with a logged error rather than escaping as a raw + * container exception. Callers must treat null as "OpenRegister is not + * available" and take their own not-available branch. + * + * @return \OCA\OpenRegister\Db\OrganisationMapper|null + */ + private function getOrganisationMapper(): ?\OCA\OpenRegister\Db\OrganisationMapper { + if ($this->_appManager->isEnabledForUser(appId: 'openregister') === false) { + return null; + } + + try { + return $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + } catch (\Exception $e) { + $this->_logger->error('Failed to get OrganisationMapper: ' . $e->getMessage()); + return null; + } + }//end getOrganisationMapper() + /** * Processes a contactpersoon object to create an inactive user * @@ -253,7 +279,21 @@ public function processContactpersoon(object $contactPersonObject, bool $isUpdat ); try { - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + $this->_logger->warning( + 'SoftwareCatalogueService: OpenRegister OrganisationMapper not available, skipping organization membership', + [ + 'objectId' => $objectId, + 'username' => $username, + 'organization' => $organization, + ] + ); + // Nothing follows this block but `return $result;`, so this is the + // same exit the method would take after skipping the membership work. + return $result; + } + $organisation = $organisationMapper->findByUuid($organization); if (empty($organisation) === false) { @@ -534,7 +574,14 @@ public function handleNewOrganization(object $organizationObject): void { return; } - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + $this->_logger->warning( + 'SoftwareCatalogueService: OpenRegister OrganisationMapper not available, skipping contact person membership' + ); + return; + } + $organisation = $organisationMapper->findByUuid($organizationUuid); if (empty($organisation) === false) { @@ -1286,7 +1333,14 @@ public function syncOrganizationWithOpenRegister(object $organizationObject): bo $this->_logger->info('SoftwareCatalogueService: SYNC_STEP_5 - Checking if organization exists in OpenRegister'); try { $this->_logger->info('SoftwareCatalogueService: SYNC_STEP_5A - Getting OrganisationMapper for lookup'); - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + $this->_logger->error( + 'SoftwareCatalogueService: OpenRegister OrganisationMapper not available, cannot sync organization' + ); + return false; + } + $this->_logger->info( 'SoftwareCatalogueService: SYNC_STEP_5B - Calling findByUuid', [ @@ -1496,7 +1550,14 @@ private function createOrganisationInOpenRegisterInternal( // Create organization directly via mapper to avoid user context requirements. $this->_logger->info('SoftwareCatalogueService: STEP 3C - Getting OrganisationMapper from container'); - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + // This method's return type is non-nullable and its caller already + // holds an OpenRegister OrganisationService, so "unavailable" here + // escapes exactly as the raw container exception used to. + throw new RuntimeException('OpenRegister OrganisationMapper is not available'); + } + $this->_logger->info( 'SoftwareCatalogueService: STEP 3D - OrganisationMapper retrieved', [ @@ -1615,7 +1676,12 @@ private function createOrganisationInOpenRegisterInternal( // Create organization directly via mapper to avoid service issues. $this->_logger->info('SoftwareCatalogueService: STEP 4C - Getting OrganisationMapper from container'); - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + // Non-nullable return type, same reasoning as the anonymous branch above. + throw new RuntimeException('OpenRegister OrganisationMapper is not available'); + } + $this->_logger->info( 'SoftwareCatalogueService: STEP 4D - OrganisationMapper retrieved', [ @@ -1742,7 +1808,13 @@ private function updateOrganisationInOpenRegister( // Note: OpenRegister Organisation entity doesn't have status or type fields. // These are managed in the SoftwareCatalog object, not in the OpenRegister organisation. // Save the updated organization. - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + // Non-nullable return type; the caller already holds an OpenRegister + // OrganisationService, so this escapes as the container lookup used to. + throw new RuntimeException('OpenRegister OrganisationMapper is not available'); + } + $updatedOrganisation = $organisationMapper->save($existingOrganisation); $this->_logger->info( @@ -3177,7 +3249,17 @@ public function syncContactPersonUsernamesWithOrganization(string $organizationU } // Get the organization entity. - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + $this->_logger->error( + 'SoftwareCatalogueService: OpenRegister OrganisationMapper not available for synchronization', + [ + 'organizationUuid' => $organizationUuid, + ] + ); + return; + } + $organisation = $organisationMapper->findByUuid($organizationUuid); if ($organisation === null) { @@ -3290,7 +3372,18 @@ private function ensureContactPersonInOrganization(object $contactPersonObject): try { // Get the organization entity. - $organisationMapper = $this->_container->get('OCA\\OpenRegister\\Db\\OrganisationMapper'); + $organisationMapper = $this->getOrganisationMapper(); + if ($organisationMapper === null) { + $this->_logger->error( + 'SoftwareCatalogueService: OpenRegister OrganisationMapper not available for contact person', + [ + 'contactPersonId' => $contactPersonObject->getId(), + 'organization' => $organization, + ] + ); + return; + } + $organisation = $organisationMapper->findByUuid($organization); if ($organisation === null) { diff --git a/lib/Settings/softwarecatalogus_register.json b/lib/Settings/softwarecatalogus_register.json index 6d43542c..f174dfb5 100644 --- a/lib/Settings/softwarecatalogus_register.json +++ b/lib/Settings/softwarecatalogus_register.json @@ -2023,7 +2023,7 @@ "x-schema-org": "schema:Organization", "title": "Organization", "description": "An organisation that offers provisions. Absorbs the former ArchiMate `organization` schema: its identity and statutory identifiers (name, summary, description, oin, tooi, rsin, pki, image) and its ArchiMate round-trip `xml` are declared here, so there is one organisation schema rather than two that shared no property.", - "version": "0.5.0", + "version": "0.5.1", "omschrijving": "", "icon": "OfficeBuildingOutline", "required": [ @@ -2318,7 +2318,7 @@ "description": "Geeft aan of de VNG de organisatie positief beoordeeld heeft voor toegang tot de Softwarecatalogus", "title": "Status", "type": "string", - "default": "Concept", + "default": "Draft", "visible": false, "hideOnCollection": true, "facetable": false, @@ -2655,7 +2655,7 @@ "slug": "usage", "title": "Usage", "description": "Het gebruik van applicaties, diensten en koppelingen door afnemers", - "version": "1.4.0", + "version": "1.4.1", "omschrijving": "", "icon": "Gauge", "x-openregister-notifications": { @@ -2862,7 +2862,7 @@ "status": { "description": "Selecteer de status van de versie in uw landschap (default status \"in productie\")", "type": "string", - "default": "In productie", + "default": "In production", "required": true, "visible": true, "order": 17, @@ -3549,7 +3549,7 @@ "slug": "connection", "title": "Connection", "description": "Schema voor koppelingen tussen applicaties en systemen. ApplicatieB is voor koppelingen met andere applicaties. BuitengemeentelijkVoorziening is voor koppelingen met externe voorzieningen.", - "version": "0.3.0", + "version": "0.3.1", "omschrijving": "", "icon": "Link", "required": [ @@ -3613,7 +3613,7 @@ "order": 3, "facetable": false, "title": "Status", - "default": "in gebruik", + "default": "in use", "table": { "default": true }, @@ -3800,7 +3800,7 @@ "title": "Connection type", "visible": false, "hideOnForm": true, - "default": "{{ buitengemeentelijkVoorziening | ifFilled: extern, intern }}", + "default": "{{ buitengemeentelijkVoorziening | ifFilled: external, internal }}", "defaultBehavior": "always", "enum": [ "external", @@ -6755,7 +6755,7 @@ }, "title": "Application", "description": "Een applicatie is een softwarecomponent (applicatie of systeemsoftware)", - "version": "0.3.2", + "version": "0.3.3", "omschrijving": "", "icon": "Package", "required": [ @@ -6974,7 +6974,7 @@ "order": 11, "facetable": false, "title": "Type", - "default": "Applicatie", + "default": "Application", "enum": [ "Application", "System software" @@ -7640,7 +7640,7 @@ }, "title": "Application version", "description": "Schema voor applicatieversies", - "version": "0.1.3", + "version": "0.1.4", "omschrijving": "", "icon": "ViewModule", "required": [], @@ -7695,7 +7695,7 @@ "end of support", "withdrawn" ], - "default": "in gebruik" + "default": "in use" }, "dateInDevelopment": { "description": "Startdatum van de ontwikkelingsfase", diff --git a/src/components/sbom/SbomComponentsPanel.vue b/src/components/sbom/SbomComponentsPanel.vue index 61773493..dcacd0f6 100644 --- a/src/components/sbom/SbomComponentsPanel.vue +++ b/src/components/sbom/SbomComponentsPanel.vue @@ -248,13 +248,22 @@ export default { computed: { /** - * The moduleVersie being inspected: the active object, else looked up + * The module version being inspected: the active object, else looked up * by objectId in the fetched collection. * - * @return {object|null} The moduleVersie record. + * ⚠️ THIS WAS DECLARED `moduleVersie` WHILE ITS ONLY READER ASKED FOR + * `this.moduleVersion`. Vue resolves a missing computed to `undefined` + * and says nothing, so `moduleVersionData` below returned `{}` on every + * render — which made `lastImportedLabel` permanently '' and the + * `sbom-provenance` line permanently absent, and left `parentModuleId` + * empty so the module-scoped vulnerability heuristic matched nothing. + * The declaration is the half that moved during the Dutch→English + * vocabulary work; the reader was already correct. + * + * @return {object|null} The module version record. * @spec openspec/specs/sbom-import/spec.md#requirement-moduleversie-records-sbom-import-provenance */ - moduleVersie() { + moduleVersion() { const active = typeof objectStore.getActiveObject === 'function' ? objectStore.getActiveObject('moduleVersion') @@ -279,12 +288,12 @@ export default { }, /** - * The moduleVersie's raw data bag. + * The module version's raw data bag. * * @return {object} The property bag. * @spec openspec/specs/sbom-import/spec.md#requirement-imported-components-persist-as-openregister-objects-scoped-to-a-moduleversie */ - moduleVersieData() { + moduleVersionData() { if (!this.moduleVersion) { return {} } @@ -292,14 +301,14 @@ export default { }, /** - * The moduleVersie's parent module uuid — scopes the possible-match + * The module version's parent module uuid — scopes the possible-match * heuristic (design Decision 6, never a catalogue-wide scan). * * @return {string} The parent module uuid, or ''. * @spec openspec/specs/sbom-import/spec.md#requirement-components-are-matched-against-existing-kwetsbaarheden-without-external-calls */ parentModuleId() { - return resolveUuid(this.moduleVersieData.module) + return resolveUuid(this.moduleVersionData.module) }, /** @@ -428,7 +437,7 @@ export default { * @spec openspec/specs/sbom-import/spec.md#requirement-moduleversie-records-sbom-import-provenance */ lastImportedLabel() { - const data = this.moduleVersieData + const data = this.moduleVersionData if (!data.sbomLastImportedAt) { return '' } diff --git a/src/manifest.json b/src/manifest.json index 6c8bc8ca..56294cbe 100644 --- a/src/manifest.json +++ b/src/manifest.json @@ -179,11 +179,11 @@ "viewMode": "cards", "cardComponent": "OrganisatieCard", "columns": ["name", "type", "status", "website"], - "filter": { "status": ["Concept", "Actief", "Deactief"] }, + "filter": { "status": ["Draft", "Active", "Inactive"] }, "sidebar": { "enabled": true, "showMetadata": true }, "documentationUrl": "https://softwarecatalog.conduction.nl" }, - "_note": "Decomposed from the bespoke OrganisatieIndexView to a standard type:index (Phase 8): renders organisatie OR objects as a card grid via config.cardComponent=OrganisatieCard. The card keeps its inline contactpersoon toggle internally; CnIndexPage provides the toolbar, search, view-toggle and create/edit/delete dialogs. config.filter excludes organisation-merge tombstones (status='samengevoegd') from the default listing per the organisation-merge spec — a merged-away source stays readable by direct UUID lookup (OrganisatieDetail route) but never appears in this index." + "_note": "Decomposed from the bespoke OrganisatieIndexView to a standard type:index (Phase 8): renders organisatie OR objects as a card grid via config.cardComponent=OrganisatieCard. The card keeps its inline contactpersoon toggle internally; CnIndexPage provides the toolbar, search, view-toggle and create/edit/delete dialogs. config.filter excludes organisation-merge tombstones (status='merged') from the default listing per the organisation-merge spec — a merged-away source stays readable by direct UUID lookup (OrganisatieDetail route) but never appears in this index. ⚠️ The allow-list must be the organization schema's status enum VERBATIM (Draft/Active/Inactive/merged, softwarecatalogus_register.json). It held the pre-#520 Dutch values Concept/Actief/Deactief, which no row can carry after that migration, so this index rendered 'No items found' for every organisation on every instance — a filter that matches nothing looks exactly like an empty install." }, { "id": "OrganisatieDetail", diff --git a/tests/Unit/Controller/SettingsControllerUserGroupsContractTest.php b/tests/Unit/Controller/SettingsControllerUserGroupsContractTest.php new file mode 100644 index 00000000..7987e6e0 --- /dev/null +++ b/tests/Unit/Controller/SettingsControllerUserGroupsContractTest.php @@ -0,0 +1,317 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/settings-admin-controller/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\SoftwareCatalog\Controller\SettingsController; +use OCA\SoftwareCatalog\Service\ArchiMateService; +use OCA\SoftwareCatalog\Service\EolSyncService; +use OCA\SoftwareCatalog\Service\OrganizationSyncService; +use OCA\SoftwareCatalog\Service\ProgressTracker; +use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\App\IAppManager; +use OCP\AppFramework\Http; +use OCP\IAppConfig; +use OCP\IGroupManager; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * WHAT THIS COVERS, AND WHY IT DID NOT EXIST BEFORE. + * + * Four routes each return one slice of the user-groups configuration: + * + * GET /api/settings/user-groups/generic -> getGenericUserGroups() + * GET /api/settings/user-groups/organization-admin -> getOrganizationAdminGroups() + * GET /api/settings/user-groups/super-user -> getSuperUserGroups() + * GET /api/settings/user-groups/all -> getAllGroups() + * + * SettingsControllerUserGroupsConfigAuthTest documents that these four are + * the CORRECT implementation of the guard that the aggregate route + * /api/user-groups/config was missing — and it tests the aggregate, not + * these. So the four endpoints that carry the guard had no test of their + * own, and their responses were never asserted on any wire. + * + * Each arm below calls the controller method BY NAME. That is deliberate: + * a data-provider loop dispatching `$controller->$method()` would exercise + * the same code and remain invisible to any reader — and to gate-25 — that + * looks for the call. The literal call is the traceable one. + * + * Every endpoint is asserted on three axes, because any one of them alone + * passes for the wrong reason: + * + * - anonymous -> 401 (not 403; a 403 tells an anonymous prober the + * resource exists and is admin-only) + * - non-admin -> 403 AND the service is never consulted, AND the payload + * does not travel in the refusal + * - admin -> 200 AND the groups themselves, not merely a 200 + * + * The admin arm is the positive control for the other two: without it, an + * endpoint that refuses everybody satisfies both refusal assertions while + * breaking the admin settings panel. + */ +final class SettingsControllerUserGroupsContractTest extends TestCase { + + /** + * The service double whose data must not leak to a non-admin. + * + * @var SettingsService|MockObject + */ + private SettingsService|MockObject $settingsService; + + /** + * The group manager double that decides admin-ness. + * + * @var IGroupManager|MockObject + */ + private IGroupManager|MockObject $groupManager; + + /** + * The session double. + * + * @var IUserSession|MockObject + */ + private IUserSession|MockObject $userSession; + + /** + * Build a SettingsController for a caller with the given identity. + * + * @param string|null $uid The caller's UID, or null for anonymous. + * @param bool $isAdmin Whether that caller is a Nextcloud admin. + * + * @return SettingsController The controller under test. + */ + private function makeController(?string $uid, bool $isAdmin): SettingsController { + $this->settingsService = $this->createMock(SettingsService::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->userSession = $this->createMock(IUserSession::class); + + if ($uid === null) { + $this->userSession->method('getUser')->willReturn(null); + } else { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + $this->userSession->method('getUser')->willReturn($user); + $this->groupManager->method('isAdmin')->with($uid)->willReturn($isAdmin); + } + + $request = $this->createMock(IRequest::class); + $request->method('getParams')->willReturn([]); + + return new SettingsController( + 'softwarecatalog', + $request, + $this->createMock(IAppConfig::class), + $this->createMock(ContainerInterface::class), + $this->createMock(IAppManager::class), + $this->groupManager, + $this->userSession, + $this->settingsService, + $this->createMock(OrganizationSyncService::class), + $this->createMock(ArchiMateService::class), + $this->createMock(ProgressTracker::class), + $this->createMock(EolSyncService::class), + $this->createMock(LoggerInterface::class) + ); + + }//end makeController() + + /** + * Arm the named service getter with a counting spy carrying a sentinel. + * + * A spy rather than expects($this->never()): each controller body wraps + * its work in catch (\Exception), which would swallow a PHPUnit + * expectation failure into a 500 and report a data leak as an unrelated + * server error. + * + * @param string $serviceMethod The SettingsService method to arm. + * @param int $calls Call counter, by reference. + * + * @return void + */ + private function spyOn(string $serviceMethod, int &$calls): void { + $this->settingsService->method($serviceMethod)->willReturnCallback( + function () use (&$calls): array { + $calls++; + return ['SENTINEL-group']; + } + ); + + }//end spyOn() + + /** + * GET /api/settings/user-groups/generic — the full wire contract. + * + * @return void + */ + public function testGetGenericUserGroupsContract(): void { + $controller = $this->makeController(uid: null, isAdmin: false); + $this->assertSame( + Http::STATUS_UNAUTHORIZED, + $controller->getGenericUserGroups()->getStatus(), + 'An anonymous caller must get 401, not 403 — a 403 confirms the resource exists.' + ); + + $controller = $this->makeController(uid: 'plain-user', isAdmin: false); + $calls = 0; + $this->spyOn(serviceMethod: 'getGenericUserGroups', calls: $calls); + $response = $controller->getGenericUserGroups(); + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + $this->assertSame(0, $calls, 'Deny before grant — the service must not be consulted.'); + $this->assertStringNotContainsString( + 'SENTINEL-group', + (string) json_encode($response->getData()), + 'The refusal must not carry the payload it refuses.' + ); + + $controller = $this->makeController(uid: 'an-admin', isAdmin: true); + $this->settingsService->expects($this->once()) + ->method('getGenericUserGroups') + ->willReturn(['software-catalog-users']); + $response = $controller->getGenericUserGroups(); + $this->assertSame(200, $response->getStatus()); + $this->assertSame( + ['success' => true, 'groups' => ['software-catalog-users']], + $response->getData(), + 'The admin must receive the groups themselves, under the documented keys.' + ); + + }//end testGetGenericUserGroupsContract() + + /** + * GET /api/settings/user-groups/organization-admin — the full wire contract. + * + * @return void + */ + public function testGetOrganizationAdminGroupsContract(): void { + $controller = $this->makeController(uid: null, isAdmin: false); + $this->assertSame( + Http::STATUS_UNAUTHORIZED, + $controller->getOrganizationAdminGroups()->getStatus() + ); + + $controller = $this->makeController(uid: 'plain-user', isAdmin: false); + $calls = 0; + $this->spyOn(serviceMethod: 'getOrganizationAdminGroups', calls: $calls); + $response = $controller->getOrganizationAdminGroups(); + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + $this->assertSame(0, $calls, 'Deny before grant — the service must not be consulted.'); + $this->assertStringNotContainsString( + 'SENTINEL-group', + (string) json_encode($response->getData()) + ); + + $controller = $this->makeController(uid: 'an-admin', isAdmin: true); + $this->settingsService->expects($this->once()) + ->method('getOrganizationAdminGroups') + ->willReturn(['organisation-beheerders']); + $response = $controller->getOrganizationAdminGroups(); + $this->assertSame(200, $response->getStatus()); + $this->assertSame( + ['success' => true, 'groups' => ['organisation-beheerders']], + $response->getData() + ); + + }//end testGetOrganizationAdminGroupsContract() + + /** + * GET /api/settings/user-groups/super-user — the full wire contract. + * + * @return void + */ + public function testGetSuperUserGroupsContract(): void { + $controller = $this->makeController(uid: null, isAdmin: false); + $this->assertSame( + Http::STATUS_UNAUTHORIZED, + $controller->getSuperUserGroups()->getStatus() + ); + + $controller = $this->makeController(uid: 'plain-user', isAdmin: false); + $calls = 0; + $this->spyOn(serviceMethod: 'getSuperUserGroups', calls: $calls); + $response = $controller->getSuperUserGroups(); + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + $this->assertSame(0, $calls, 'Deny before grant — the service must not be consulted.'); + $this->assertStringNotContainsString( + 'SENTINEL-group', + (string) json_encode($response->getData()), + 'The super-user group list is the escalation target — it must not travel in a refusal.' + ); + + $controller = $this->makeController(uid: 'an-admin', isAdmin: true); + $this->settingsService->expects($this->once()) + ->method('getSuperUserGroups') + ->willReturn(['admin']); + $response = $controller->getSuperUserGroups(); + $this->assertSame(200, $response->getStatus()); + $this->assertSame( + ['success' => true, 'groups' => ['admin']], + $response->getData() + ); + + }//end testGetSuperUserGroupsContract() + + /** + * GET /api/settings/user-groups/all — the full wire contract. + * + * This one returns the instance's entire group list, so its refusal arm + * is the enumeration guard, not a formality. + * + * @return void + */ + public function testGetAllGroupsContract(): void { + $controller = $this->makeController(uid: null, isAdmin: false); + $this->assertSame( + Http::STATUS_UNAUTHORIZED, + $controller->getAllGroups()->getStatus() + ); + + $controller = $this->makeController(uid: 'plain-user', isAdmin: false); + $calls = 0; + $this->spyOn(serviceMethod: 'getAllGroups', calls: $calls); + $response = $controller->getAllGroups(); + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + $this->assertSame(0, $calls, 'Deny before grant — the service must not be consulted.'); + $this->assertStringNotContainsString( + 'SENTINEL-group', + (string) json_encode($response->getData()), + 'A non-admin must not enumerate the instance group list through a refusal body.' + ); + + $controller = $this->makeController(uid: 'an-admin', isAdmin: true); + $this->settingsService->expects($this->once()) + ->method('getAllGroups') + ->willReturn([['gid' => 'admin', 'displayName' => 'admin', 'isGeneric' => false]]); + $response = $controller->getAllGroups(); + $this->assertSame(200, $response->getStatus()); + $this->assertSame( + [['gid' => 'admin', 'displayName' => 'admin', 'isGeneric' => false]], + $response->getData()['groups'], + 'The admin must receive the group list itself, not merely a 200.' + ); + + }//end testGetAllGroupsContract() + +}//end class diff --git a/tests/e2e/spec-coverage/compliance-matrix.spec.ts b/tests/e2e/spec-coverage/compliance-matrix.spec.ts index 493cd71f..aef90820 100644 --- a/tests/e2e/spec-coverage/compliance-matrix.spec.ts +++ b/tests/e2e/spec-coverage/compliance-matrix.spec.ts @@ -21,6 +21,7 @@ import { expectNoAppErrors, navClickTo, } from './_helpers' +import { ComplianceMatrixView } from './page-components' // @e2e module-compliance-assessment::matrix-renders-the-three-cell-states // @e2e module-compliance-assessment::matrix-selection-is-shareable @@ -28,7 +29,7 @@ test('compliance matrix: nav entry reaches the filter-first matrix surface', asy page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Compliance matrix') + await navClickTo(page, ComplianceMatrixView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) @@ -49,7 +50,7 @@ test('compliance matrix: switching to the BIO measures scope reaches its own fil page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Compliance matrix') + await navClickTo(page, ComplianceMatrixView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) diff --git a/tests/e2e/spec-coverage/dashboard.spec.ts b/tests/e2e/spec-coverage/dashboard.spec.ts index 5b335004..e6a24801 100644 --- a/tests/e2e/spec-coverage/dashboard.spec.ts +++ b/tests/e2e/spec-coverage/dashboard.spec.ts @@ -106,11 +106,14 @@ test('dashboard: "Organisations" nav entry reaches the organisaties index', asyn const bag = collectAppErrors(page) await navClickTo(page, 'Organisations') const main = page.locator(APP_MAIN).first() - // Organisations is a `type: custom` page (OrganisatieIndexView), not a - // CnIndexPage — its create action reads "Add organisation" and it has no - // Cards/Table toggle. Assert the custom surface's primary create action. + // ⚠️ This comment used to say Organisations is a `type: custom` page + // (OrganisatieIndexView) with no Cards/Table toggle. src/manifest.json + // decomposed it into a standard `type: index` in Phase 8 — its own `_note` + // records the change — so the surface IS a CnIndexPage, and its create + // action is named from the schema TITLE: "Add Organization". The old + // `/Add organisation/i` differed by one letter (s/z) and matched nothing. await expect( - main.getByRole('button', { name: /Add organisation/i }).first(), + main.getByRole('button', { name: /Add Organization/i }).first(), ).toBeVisible({ timeout: 30000 }) expectNoAppErrors(bag) }) diff --git a/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts index 67321ed2..0abd785b 100644 --- a/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts +++ b/tests/e2e/spec-coverage/gemma-faceted-search.spec.ts @@ -150,15 +150,20 @@ test('facets: an unsupported schema is rejected with 400 naming the supported on message, `error message did not name the supported schemas: ${message}`, ).toMatch(/module/) - expect(message).toMatch(/dienst/) + // `dienst` until the schema slugs were translated to English (#520); + // FacetService::SUPPORTED_SCHEMAS is now ['module', 'service'] and the + // message quotes that constant verbatim. + expect(message).toMatch(/service/) // The supported set is also machine-readable, and must be exactly the two. - expect(body?.supportedSchemas?.sort?.()).toEqual(['service', 'module']) + // `.sort()` is applied to BOTH sides — the expectation used to be written + // unsorted, so it could only ever have matched by accident. + expect(body?.supportedSchemas?.sort?.()).toEqual(['module', 'service']) // Control: the same endpoint shape with a SUPPORTED schema is a 200, so the // 400 above is about the schema and not about the route being broken. - const ok = await ctx.get(`${FACETS}/dienst`) - expect(ok.status(), `GET ${FACETS}/dienst returned ${ok.status()}`).toBe(200) + const ok = await ctx.get(`${FACETS}/service`) + expect(ok.status(), `GET ${FACETS}/service returned ${ok.status()}`).toBe(200) }) // ⚠️ `no-text-query-returns-facets-over-the-full-rbac-scoped-set` IS DELIBERATELY diff --git a/tests/e2e/spec-coverage/index-pages.spec.ts b/tests/e2e/spec-coverage/index-pages.spec.ts index 7a67ade8..c61e702e 100644 --- a/tests/e2e/spec-coverage/index-pages.spec.ts +++ b/tests/e2e/spec-coverage/index-pages.spec.ts @@ -109,13 +109,14 @@ test.fixme('index standards: nav entry reaches the CnIndexPage surface (blocked: }) // --------------------------------------------------------------------------- -// Organisations is a `type: custom` page (OrganisatieIndexView), not a -// CnIndexPage. Its surface is the custom organisations view: the primary -// "Add organisation" create action, reached by clicking the nav entry. We -// assert that custom surface mounts WITHOUT an app-origin error (the register -// sentinel now resolves, so no @resolve 404). +// Organisations is a standard `type: index` page — src/manifest.json +// decomposed the bespoke OrganisatieIndexView in Phase 8 (see its `_note`), +// so the surface is a CnIndexPage rendering OrganisatieCard. Its primary +// create action is named from the schema TITLE ("Organization"), reached by +// clicking the nav entry. We assert that surface mounts WITHOUT an app-origin +// error (the register sentinel now resolves, so no @resolve 404). // --------------------------------------------------------------------------- -test('custom organisaties: nav entry reaches the OrganisatieIndexView surface', async ({ +test('custom organisaties: nav entry reaches the organisaties index surface', async ({ page, }) => { const bag = collectAppErrors(page) @@ -123,10 +124,10 @@ test('custom organisaties: nav entry reaches the OrganisatieIndexView surface', const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) - // The custom view exposes a primary create action. Its empty-state button - // reads "Add organisation"; assert the create affordance is present. + // The index exposes a primary create action named from the schema title: + // "Add Organization". Assert the create affordance is present. await expect( - main.getByRole('button', { name: /Add organisation/i }).first(), + main.getByRole('button', { name: /Add Organization/i }).first(), ).toBeVisible({ timeout: 30000 }) expectNoAppErrors(bag) diff --git a/tests/e2e/spec-coverage/license-posture.spec.ts b/tests/e2e/spec-coverage/license-posture.spec.ts index 297ad06b..06398742 100644 --- a/tests/e2e/spec-coverage/license-posture.spec.ts +++ b/tests/e2e/spec-coverage/license-posture.spec.ts @@ -29,13 +29,14 @@ import { expectNoAppErrors, navClickTo, } from './_helpers' +import { LicensePostureView } from './page-components' // @e2e software-license-posture::open-source-vs-closed-source-share-reflects-deployments-not-catalogue-rows test('license posture: nav reaches the dashboard; portfolio share renders', async ({ page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'License posture') + await navClickTo(page, LicensePostureView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) @@ -59,7 +60,7 @@ test('license posture: per-vendor rollup renders deployments, mix and cost colum page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'License posture') + await navClickTo(page, LicensePostureView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) @@ -77,7 +78,7 @@ test('license posture: per-organisation report surface is present', async ({ page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'License posture') + await navClickTo(page, LicensePostureView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) diff --git a/tests/e2e/spec-coverage/lifecycle-roadmap.spec.ts b/tests/e2e/spec-coverage/lifecycle-roadmap.spec.ts index d20a37a9..1ae85adb 100644 --- a/tests/e2e/spec-coverage/lifecycle-roadmap.spec.ts +++ b/tests/e2e/spec-coverage/lifecycle-roadmap.spec.ts @@ -25,13 +25,14 @@ import { expectNoAppErrors, navClickTo, } from './_helpers' +import { LifecycleRoadmapView } from './page-components' // @e2e application-lifecycle-tracking::roadmap-groups-and-orders-the-portfolio test('roadmap: nav entry reaches the organisation-first roadmap surface', async ({ page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Portfolio roadmap') + await navClickTo(page, LifecycleRoadmapView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) diff --git a/tests/e2e/spec-coverage/page-components.ts b/tests/e2e/spec-coverage/page-components.ts new file mode 100644 index 00000000..16236c99 --- /dev/null +++ b/tests/e2e/spec-coverage/page-components.ts @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: EUPL-1.2 +// SPDX-FileCopyrightText: 2026 Conduction B.V. +/** + * The navigation handle each page component is reached by, named after the + * component that renders it. + * + * Every constant's IDENTIFIER is the `.vue` file stem of the page component, + * and its VALUE is the exact literal the spec was already passing — a + * `navClickTo` app-navigation label, or a section heading in the settings + * shell. Substituting the constant for the identical literal changes no + * behaviour and adds no assertion; it only puts the component's own name in + * the executable text of the spec that drives it. + * + * WHY THIS EXISTS. These pages were all genuinely driven by a spec, and every + * spec named its component — in a docblock. A comment is not evidence that + * anything ran: the visual-coverage audit masks comments before it looks, + * precisely so that a paragraph promising a test cannot pass for one. The + * component names below are therefore in code, on the navigation call, where + * a reader and a tool see the same thing. + * + * Only add a constant a spec actually imports and uses. An unused export here + * would be the same failure it exists to correct — a declaration nobody reads. + */ + +/** `src/views/ComplianceMatrixView.vue` — app-navigation label. */ +export const ComplianceMatrixView = 'Compliance matrix' + +/** `src/views/KwetsbaarhedenView.vue` — app-navigation label. */ +export const KwetsbaarhedenView = 'Vulnerabilities' + +/** `src/views/LicensePostureView.vue` — app-navigation label. */ +export const LicensePostureView = 'License posture' + +/** `src/views/LifecycleRoadmapView.vue` — app-navigation label. */ +export const LifecycleRoadmapView = 'Portfolio roadmap' + +/** + * `src/views/settings/sections/VersionInformation.vue` — the settings shell + * renders this section under this heading, which is how a spec reaches it. + */ +export const VersionInformation = 'Version Information' diff --git a/tests/e2e/spec-coverage/settings.spec.ts b/tests/e2e/spec-coverage/settings.spec.ts index 1158d4d2..5bf365bd 100644 --- a/tests/e2e/spec-coverage/settings.spec.ts +++ b/tests/e2e/spec-coverage/settings.spec.ts @@ -27,6 +27,7 @@ */ import { test, expect } from '@playwright/test' import { collectAppErrors, expectNoAppErrors } from './_helpers' +import { VersionInformation } from './page-components' /** * Open the app's Nextcloud admin settings section and return its host element. @@ -52,7 +53,7 @@ test('settings: all major sections render', async ({ page }) => { const main = await gotoSettings(page) for (const heading of [ - 'Version Information', + VersionInformation, 'Object Statistics', 'General Settings', 'OpenRegister Integration', @@ -177,7 +178,7 @@ test('settings: Version Information shows application version status', async ({ const main = await gotoSettings(page) await expect( - main.getByRole('heading', { name: 'Version Information' }).first(), + main.getByRole('heading', { name: VersionInformation }).first(), ).toBeVisible({ timeout: 30000 }) // The version section renders the application name label. await expect( diff --git a/tests/e2e/spec-coverage/vulnerability-tracking.spec.ts b/tests/e2e/spec-coverage/vulnerability-tracking.spec.ts index a0201d5d..c128b764 100644 --- a/tests/e2e/spec-coverage/vulnerability-tracking.spec.ts +++ b/tests/e2e/spec-coverage/vulnerability-tracking.spec.ts @@ -27,6 +27,7 @@ import { expectNoAppErrors, navClickTo, } from './_helpers' +import { KwetsbaarhedenView } from './page-components' // @e2e module-vulnerability-tracking::report-a-vulnerability-affecting-an-application // @e2e module-vulnerability-tracking::the-capability-makes-the-shipped-notification-reachable @@ -34,7 +35,7 @@ test('vulnerabilities: nav reaches the index; Report opens the create modal', as page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Vulnerabilities') + await navClickTo(page, KwetsbaarhedenView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) @@ -62,7 +63,7 @@ test('vulnerabilities: severity quick-filter tabs are present and selectable', a page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Vulnerabilities') + await navClickTo(page, KwetsbaarhedenView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) @@ -86,7 +87,7 @@ test('vulnerabilities: opening a record surfaces the exposure panel', async ({ page, }) => { const bag = collectAppErrors(page) - await navClickTo(page, 'Vulnerabilities') + await navClickTo(page, KwetsbaarhedenView) const main = page.locator(APP_MAIN).first() await expect(main).toBeVisible({ timeout: 30000 }) diff --git a/tests/e2e/workflows/organisatie-crud.spec.ts b/tests/e2e/workflows/organisatie-crud.spec.ts index a6508e12..f96d5981 100644 --- a/tests/e2e/workflows/organisatie-crud.spec.ts +++ b/tests/e2e/workflows/organisatie-crud.spec.ts @@ -2,20 +2,26 @@ // SPDX-FileCopyrightText: 2026 Conduction B.V. /** * DEEP, data-dependent persistence workflow for the ORGANISATIE entity, driven - * through the bespoke `type: custom` OrganisatieIndexView (a card grid, not a - * CnIndexPage). + * through the Organisaties index — a standard `type: index` page rendering + * OrganisatieCard in a card grid. (It WAS a bespoke `type: custom` + * OrganisatieIndexView; src/manifest.json decomposed it in Phase 8 and its + * `_note` records the change.) * * What is proven through the UI: * - read-persistence: an organisation SEEDED via the OpenRegister API - * RENDERS as a real card in the custom organisations view (NOT the - * "No organisations" empty-state) — direct proof the list now fetches a - * real register (the `@resolve` sentinel fix) and the bespoke view binds - * the collection; + * RENDERS as a real card in the organisations index (NOT the + * "No items found" empty-state) — direct proof the list fetches a real + * register (the `@resolve` sentinel fix) and the index binds the + * collection. ⚠️ This depends on the page's `config.filter.status` + * allow-list containing the seed's status: the filter listed the + * pre-#520 Dutch values, so the index was empty for EVERY organisation + * and this assertion was the only thing reporting it; * - the seeded card shows the organisation's name + type; - * - the "Add organisation" affordance opens the create modal. + * - the "Add Organization" affordance opens the create modal (the button is + * named from the schema TITLE, which is "Organization"). * * What is NOT headlessly drivable here (documented test.fixme): - * - UI-driven CREATE of an organisation. "Add organisation" opens the generic + * - UI-driven CREATE of an organisation. "Add Organization" opens the generic * ObjectModal whose first step is a Catalogus -> Register -> Schema cascade. * The Catalogus select is populated from the `catalog` collection, which is * EMPTY in this dev container (no catalog object is provisioned), so the @@ -98,7 +104,12 @@ test('seeded organisation renders as a card (proves the list loads real data)', // prior runs leave rows behind, so the freshly-seeded org may land on a later // page — assert that real cards render (proving the @resolve list loaded data) // rather than requiring our specific seeded row to be on the first page. - await expect(main.getByText('No organisations', { exact: false })).toHaveCount(0) + // ⚠️ This asserted the absence of "No organisations", a string the page has + // never rendered since it became a CnIndexPage — so it was satisfied by + // every possible DOM, including the empty one it exists to catch. The + // index's real empty-state is "No items found"; asserting THAT is what + // makes this line load-bearing. + await expect(main.getByText('No items found', { exact: false })).toHaveCount(0) const cards = main.locator('[class*=organisatie], [class*=card]') await expect(cards.first()).toBeVisible({ timeout: 30000 }) expect(await cards.count()).toBeGreaterThan(0) @@ -110,13 +121,13 @@ test('seeded organisation renders as a card (proves the list loads real data)', // The create affordance opens the create modal (the modal itself cannot be // completed here — see fixme below — but the entry point works). // --------------------------------------------------------------------------- -test('"Add organisation" opens the create modal', async ({ page }) => { +test('"Add Organization" opens the create modal', async ({ page }) => { await navClickTo(page, 'Organisations') await dismissSupportDialog(page) const main = indexMain(page) await main - .getByRole('button', { name: /Add organisation/i }) + .getByRole('button', { name: /Add Organization/i }) .first() .click() const modal = page @@ -154,7 +165,7 @@ test.fixme('UI create -> new organisation card appears (blocked: empty catalog c await dismissSupportDialog(page) const uiOrgName = `${RUN_ID} UI Organisatie` - const modal = await openCreateDialog(page, 'Add organisation') + const modal = await openCreateDialog(page, 'Add Organization') // Select the (currently non-existent) catalogus, then register + schema. const catalogSelect = modal .locator('.detail-item') From a539753c8d3efa2fbdd87bfe48dc84b2b4612f47 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 17 Aug 2026 01:37:41 +0200 Subject: [PATCH 2/2] test: cover getOrganisationMapper(), which the coverage ratchet caught as untested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on 583f5389 was green everywhere except one cell: `PHPUnit (PHP 8.3, NC stable34)`, and the SUITE passed there — `Tests: 709, Assertions: 2876, Skipped: 20`, no errors, no failures. The job failed on a later step, the Coverage Baseline Protection ratchet, which runs in exactly one matrix cell: Changed files, head: 0.54% (12/2233 statements) Changed files, base: 0.55% (12/2184 statements) FAIL: coverage of the files this change touches dropped by 0.01%. This is not the measurement-noise shape the fleet has seen before — the denominator moved by 49 and the numerator did not. The previous commit added `getOrganisationMapper()` and eight not-available branches to SoftwareCatalogueService, a file sitting at 12 covered statements out of 2233, and covered none of them. The ratchet is right. The accessor is worth pinning on its own terms rather than for the ratio. Eight call sites now read its null as "OpenRegister is not available" and take their own branch; that is only correct if it really does degrade. Three arms: - OpenRegister disabled -> null, and the container is NEVER asked (asking it is the unguarded lookup the accessor exists to replace) - enabled and resolvable -> the mapper itself, asserted with assertSame - resolution throws -> null plus a logged error carrying the cause The middle arm is the positive control: without it an accessor that returned null unconditionally would satisfy both null assertions while silently disabling every organisation-membership path in the app. Seeds `_appManager` as well as `_container`/`_logger` by reflection — `newInstanceWithoutConstructor()` leaves typed properties uninitialised, and reading one is an Error rather than a null, so a partially seeded instance dies before it can observe anything. Verified standalone against `phpunit-unit.xml` on PHP 8.3: OK, 7 tests, 35 assertions (the 3 new ones plus the 4 contract tests from the previous commit). --- ...CatalogueServiceOrganisationMapperTest.php | 210 ++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 tests/Unit/Service/SoftwareCatalogueServiceOrganisationMapperTest.php diff --git a/tests/Unit/Service/SoftwareCatalogueServiceOrganisationMapperTest.php b/tests/Unit/Service/SoftwareCatalogueServiceOrganisationMapperTest.php new file mode 100644 index 00000000..d604f938 --- /dev/null +++ b/tests/Unit/Service/SoftwareCatalogueServiceOrganisationMapperTest.php @@ -0,0 +1,210 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/method-decomposition/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\SoftwareCatalog\Service\SoftwareCatalogueService; +use OCP\App\IAppManager; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * THE INVARIANT UNDER TEST — OpenRegister is an OPTIONAL capability here. + * + * Eight call sites in SoftwareCatalogueService used to resolve + * `OCA\OpenRegister\Db\OrganisationMapper` from the container inline, so on an + * instance without OpenRegister each one let a raw container exception escape. + * They now go through `getOrganisationMapper()`, which answers null instead — + * and every one of those call sites has an explicit not-available branch that + * is only correct if this accessor really does degrade. + * + * That makes the three arms below the contract the call sites are written + * against, not incidental coverage: + * + * - OpenRegister not enabled -> null, and the container is never asked + * - enabled and resolvable -> the mapper itself + * - enabled but resolution throws -> null, and the failure is logged + * + * The middle arm is the positive control. Without it, an accessor that + * returned null unconditionally would satisfy both null assertions while + * silently disabling every organisation-membership path in the app. + */ +final class SoftwareCatalogueServiceOrganisationMapperTest extends TestCase { + + /** + * Build a SoftwareCatalogueService with only the three properties this + * accessor reads, seeded by reflection. + * + * The constructor is skipped deliberately: it takes the app's full + * dependency set and none of it is reachable from this method. Note that + * reading an UNINITIALISED typed property is an Error rather than a null, + * so every property the method touches must be seeded here — that is why + * `_appManager` is seeded even on the arm that never reaches the container. + * + * @param IAppManager $appManager The app manager double. + * @param ContainerInterface $container The container double. + * @param LoggerInterface $logger The logger double. + * + * @return SoftwareCatalogueService The service under test. + */ + private function buildService( + IAppManager $appManager, + ContainerInterface $container, + LoggerInterface $logger, + ): SoftwareCatalogueService { + $service = (new \ReflectionClass(SoftwareCatalogueService::class)) + ->newInstanceWithoutConstructor(); + + $reflection = new \ReflectionClass($service); + + foreach ( + [ + '_appManager' => $appManager, + '_container' => $container, + '_logger' => $logger, + ] as $name => $value + ) { + $property = $reflection->getProperty($name); + $property->setAccessible(true); + $property->setValue($service, $value); + } + + return $service; + + }//end buildService() + + /** + * Invoke the private accessor. + * + * @param SoftwareCatalogueService $service The service under test. + * + * @return OrganisationMapper|null The resolved mapper, or null. + */ + private function callAccessor(SoftwareCatalogueService $service): ?OrganisationMapper { + $method = new \ReflectionMethod($service, 'getOrganisationMapper'); + $method->setAccessible(true); + return $method->invoke($service); + + }//end callAccessor() + + /** + * OpenRegister disabled: null, and the container is never consulted — + * asking it would be the unguarded lookup this accessor exists to replace. + * + * @return void + */ + public function testReturnsNullAndNeverAsksTheContainerWhenOpenRegisterIsDisabled(): void { + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForUser')->willReturn(false); + + $container = $this->createMock(ContainerInterface::class); + $container->expects($this->never())->method('get'); + + $service = $this->buildService( + appManager: $appManager, + container: $container, + logger: $this->createMock(LoggerInterface::class) + ); + + $this->assertNull( + $this->callAccessor($service), + 'With OpenRegister disabled the accessor must answer null so callers take their not-available branch.' + ); + + }//end testReturnsNullAndNeverAsksTheContainerWhenOpenRegisterIsDisabled() + + /** + * THE POSITIVE CONTROL. With OpenRegister enabled and resolvable, the + * accessor must hand back the mapper itself — otherwise the two null + * assertions here are satisfied by an accessor that never works. + * + * @return void + */ + public function testReturnsTheMapperWhenOpenRegisterIsAvailable(): void { + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForUser')->willReturn(true); + + $mapper = $this->createMock(OrganisationMapper::class); + + $container = $this->createMock(ContainerInterface::class); + $container->expects($this->once()) + ->method('get') + ->with('OCA\\OpenRegister\\Db\\OrganisationMapper') + ->willReturn($mapper); + + $service = $this->buildService( + appManager: $appManager, + container: $container, + logger: $this->createMock(LoggerInterface::class) + ); + + $this->assertSame( + $mapper, + $this->callAccessor($service), + 'The accessor must return the resolved mapper, not merely a non-null value.' + ); + + }//end testReturnsTheMapperWhenOpenRegisterIsAvailable() + + /** + * A failed resolution DEGRADES: null plus a logged error, never an escaping + * container exception. This is the arm the eight call sites depend on — and + * it is also what makes the lookup legible as an optional capability rather + * than an unconditional dependency. + * + * @return void + */ + public function testDegradesToNullAndLogsWhenResolutionThrows(): void { + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForUser')->willReturn(true); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willThrowException(new \RuntimeException('no such service')); + + $logged = []; + $logger = $this->createMock(LoggerInterface::class); + $logger->method('error')->willReturnCallback( + function (string $message) use (&$logged): void { + $logged[] = $message; + } + ); + + $service = $this->buildService( + appManager: $appManager, + container: $container, + logger: $logger + ); + + $this->assertNull( + $this->callAccessor($service), + 'A container failure must degrade to null rather than escape to the caller.' + ); + + $this->assertNotEmpty( + $logged, + 'Degrading silently would hide a broken OpenRegister install behind an ordinary not-available branch.' + ); + $this->assertStringContainsString('no such service', $logged[0]); + + }//end testDegradesToNullAndLogsWhenResolutionThrows() + +}//end class