refactor(admin-roles): migrate from server

[shikanime]

Issues liées

Issues numéro:

• Closes [NestJS] Harmoniser les permissions par défaut legacy/server-nestjs pour éviter des échecs client #2351

Quel est le comportement actuel ?

Le module admin-role (rôles d'administration, permissions, positions) est servi par l'ancienne application Fastify apps/server.

Quel est le nouveau comportement ?

Migration du module admin-role vers apps/server-nestjs :

• AdminRoleController : routes GET /, POST /, PUT /, DELETE /:roleId.
• AdminRoleService : create, list, patch, delete + émission des hooks adminRole.upsert / adminRole.delete via AppEventsService.
• AdminRoleQueriesUtils / AdminRoleTestingUtils : sélections Prisma typées et fabriques faker.
• Enregistrement du module dans main.module.ts.

Parité vérifiée contre apps/server/src/resources/admin-role/business.ts :

• Garde d'intégrité de position (business.ts:39) reproduite — code mort par conception (longueurs toujours égales), signalé en revue.
• adminRole.delete émis dans la transaction (business.ts:94) — parité préservée.
• ListRoles volontairement non gardé (comportement legacy-client) — conforme au router legacy.

Cette PR introduit-elle un breaking change ?

Non.

[github-actions]

🤖 Hey !

The security scan report for the current pull request is available here.

[github-actions]

🤖 Hey !

The security scan report for the current pull request is available here.

[github-actions]

🤖 Hey !

The security scan report for the current pull request is available here.

[shikanime]

Verdict: REQUEST CHANGES

Migration of admin-roles from the legacy Fastify server into server-nestjs is well-structured (typecheck clean, admin-role.service.spec.ts passes, eslint clean). One blocker must be resolved before the legacy router is cut over.

Blocker — emitted adminRole.* events are never consumed (silent Keycloak/GitLab sync regression).
The service emits adminRole.upsert / adminRole.delete (service.ts:45, :124, :169) via EventEmitter2, but no @OnEvent('adminRole.upsert'|'adminRole.delete') handler exists in server-nestjs and @cpn-console/hooks is not imported by the module. In the legacy server, those same flows call hook.adminRole.upsert / hook.adminRole.delete (business.ts:42,66,94), which drive the Keycloak OIDC-group sync and GitLab admin/auditor group sync. Every other migrated entity bridges its event into the plugin hook system via an @OnEvent handler (e.g. keycloak.service.ts:28, gitlab.service.ts:63). Without that bridge, once the legacy route is removed, admin-role CRUD will silently stop syncing member groups. Add the @OnEvent bridge handlers (mirroring the project bridge) or explicitly defer with a tracked follow-up before cutover.

Warnings (parity-preserving, confirm): dead patch position-integrity guard (service.ts:92) — only reachable in the legacy-incompatible all-roles path; adminRole.delete emitted before the row delete inside the transaction (service.ts:169 vs :185) — legacy does the same.

Minor: unused testing-utils exports (AdminRoleContract, AdminRoleResponse, makeAdminRoleMember in admin-role-testing.utils.ts:5,6,27).

Full detail in the inline comments.

[github-actions]

🤖 Hey !

The security scan report for the current pull request is available here.

[github-actions]

🤖 Hey !

The security scan report for the current pull request is available here.

[StephaneTrebel]

Rien qui me choque, mais les questions évoquées me semblent pertinentes

[shikanime]

Review: PR #2371 — verdict REQUEST CHANGES

I reviewed the actual diff (34 commits / 206 files). CI is green (unit-tests, lint, build, SonarQube, Trivy, CodeQL all pass) and the branch is mergeable with no conflicts.

The quality of the code is high: the deployment valueSources feature uses a clean parse-don't-validate boundary, the config→nestjs migration is coherent, and the admin-role migration is faithful to legacy. But a few issues should be fixed before merge. (Note: GitHub won't let me set a formal "Request changes" state on my own PR, so this is posted as a comment — please treat the blockers as required.)

Blockers

1. Misleading title/scope — titled refactor(admin-roles): migrate from server but actually covers deployment value-sources, the full config→nestjs migration, conditional module activation, health-check refactors, CI changes, and admin-role. Either split into stacked PRs or correct the title/description.
2. deleteAllDeploymentsByProjectId unhandled rejection — apps/server-nestjs/src/modules/deployment/deployment.service.ts:88-90 calls emitProjectEvent with no .catch, while the 3 sibling methods route through reconcileProject() which catches. Add a .catch for parity.
3. admin-role patch untested — only create has a spec (admin-role.service.spec.ts). patch (position guard), delete, list, memberCounts are uncovered. Add at least one patch spec for the position-coherence branch.

Warnings (follow-up)

• Position guard quirk carried from legacy (admin-role.service.ts:92 ≈ legacy business.ts:39): a partial PATCH including position on a subset throws 400. Faithful, but worth a follow-up issue to relax to "unique/contiguous among provided roles."
• listAdminRoles intentionally unauthenticated (admin-role.controller.ts:18 TODO) — matches legacy + client need; resolve the TODO later.

Nits

• getDotenvPaths() (utils/dotenv.utils.ts) returns relative filenames; assumes process.cwd() is the app root. Fine today, noted.

[shikanime]

Review: #2371 — refactor(admin-roles): migrate from server

Verdict: REQUEST CHANGES (reviewer agent) — blocker still open since b2642e08.

[blocker] Emitted adminRole.upsert / adminRole.delete events (admin-role.service.ts:45, :124, :169 via EventEmitter2) are never consumed. There is no @OnEvent('adminRole.*') handler and admin-role.module.ts does not import @cpn-console/hooks. server-nestjs has no generic event→hook bridge (only project.upsert has explicit per-entity handlers). In the legacy server, adminRole.upsert/delete drive the Keycloak OIDC-group + GitLab admin/auditor group sync. Without the bridge, admin-role CRUD will silently stop syncing member groups after legacy-route cutover. Add the @OnEvent bridge handlers (mirror project.service) or defer with a tracked follow-up before cutover.

Warnings (parity-preserving): dead patch position-integrity guard (admin-role.service.ts:92); adminRole.delete emitted before the row delete inside the transaction (legacy does the same). Minor: unused testing-utils exports.

Cannot approve until the blocker is resolved.

[cloud-pi-native-sonarqube]

Quality Gate passed

Issues

• 3 New Issues
• 0 Fixed Issues
• 0 Accepted Issues

Measures

• 0 Security Hotspots
• 1.10% Coverage (50.40% Estimated after merge)
• 0.00% Duplicated Code (4.00% Estimated after merge)

Project ID: cloud-pi-native_console_AYsa46O31ebDtzs-pYyZ

View in SonarQube