Add office-scoped user-list API and management UI - #1734
Conversation
|
Drafting until DB changes are in. Did one endpoint to start so i can check performance of the endpoints as I go. |
MikeNeilson
left a comment
There was a problem hiding this comment.
Seems like a reasonable start.
| crud("/users/{user-name}", new UsersController(metrics), adminRoles); | ||
| get("/roles", new GetRolesController(metrics), adminRoles); | ||
| get("/user/profile", new UserProfileController(metrics), userRoles); | ||
| get("/user/list/{user-list-id}/members", new UserListMembersController(metrics), adminRoles); |
There was a problem hiding this comment.
long term, I'd probably group these by "/user/list" rather that method -> "/user/..."
But that's just me.
|
Added the next user-list GET endpoint under /user/list: GET /user/list/{user-list-id}?office={office} Returns metadata for a single office-scoped user list Separate from the existing members endpoint. EDIT: Going to resolve conflicts at the end when it's ready for merge (not draft) |
|
Spoke with Mike N on 07/24/2026 Needs to be behind a feature flag (togglez) to make sure the DB has it. This will make it not show it in swagger. and a proper unsupported response. Minimum schema version enum. Also to go ahead and PR this to have all the GET/POST/DELETE/etc for one larger review. All the user stuff should have add user handlers/function for grouping so things are more clear. |
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
|
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil> # Conflicts: # cwms-data-api/src/main/java/cwms/cda/helpers/DatabaseHelpers.java
|
Related Batch Events follow-up: USACE-WaterManagement/cwms-batch-events#186 This draft consumes PR 1734's office-scoped CDA user-list API/UI so script administrators can select maintained recipient lists for failed-job email alerts. Batch Events resolves current membership at failure time instead of duplicating users locally. |
MikeNeilson
left a comment
There was a problem hiding this comment.
Otherwise looks good.
while i would like to see pages/user-lits/index.jsx broken down into smaller components, i wouldn't consider it super critical right now. but open an issue to track that work.
The test changes should be done, there already exists an annotation to do what you're doing with the schema version check.
| let detail = `${response.status} ${response.statusText}`.trim(); | ||
| try { | ||
| const payload = await response.json(); | ||
| detail = payload.message ?? payload.detail ?? detail; |
There was a problem hiding this comment.
what didn't return JSON? if it's the API itself something is wrong.
There was a problem hiding this comment.
or is this just a bit of paranoid just in case... which yeah, probably a good idea.
There was a problem hiding this comment.
bit of paranoid just in case
The gateway won't return a valid JSON if the API is down. It's a guard to make sure something can always render. I'm trying to be better at not assuming the API will always return valid...
There was a problem hiding this comment.
This should be broken up into smaller components, overall everything does seem fine, but it's difficult to follow the logical structure of what's rendering by code alone (err, harder than it should be.)
There was a problem hiding this comment.
Thanks, that was much easier to follow.
| public final class UserListDao { | ||
|
|
||
| private final DSLContext dsl; | ||
| private final Table<?> avUserListMembers = table(name("CWMS_20", "AV_USER_LIST_MEMBERS")).as("ulm"); |
There was a problem hiding this comment.
The table and field definition can be static.
Additionally, as they are reused in different places they should be declared in their own classes. I would follow a style at least similar to the JOOQ auto generated output so that transitioning to the next code gen will be a bit easier.
|
|
||
| @BeforeAll | ||
| static void ensureUserListSchema() throws SQLException { | ||
| if (!schemaSupportsUserLists()) { |
There was a problem hiding this comment.
Use the @MinimumSchema annotation on the class level
| @AuthType(user = TestAccounts.KeyUser.SPK_NORMAL2) | ||
| void test_user_lists_are_unsupported_on_older_schemas(String authType, | ||
| TestAccounts.KeyUser user, RequestSpecification authSpec) { | ||
| assumeTrue(getSchemaVersion() < V2026_07_16.numeric()); |
There was a problem hiding this comment.
use @MinimumSchema annotation at the class level.
There was a problem hiding this comment.
oh wait, for this we would need a maximum schema. this particular one is fine for the moment.
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
|
I put a TODO in there for the maximum comment. Do we have a pending issue to add that feature? |
There was a problem hiding this comment.
Thanks, that was much easier to follow.
|
|
||
| public static int getSchemaVersion() { | ||
| if (cwmsDb == null) { | ||
| // Class-level execution conditions run before the database container starts. |
There was a problem hiding this comment.
... I keep forgetting about that. But the fix for that is definitely beyond the scope of this PR.
We don't, I only just realized we needed it looking at your code. |
## Summary - Load deployed Keycloak settings at runtime from the OpenAPI document instead of eagerly reading build-time Vite variables. - Keep the CDA GUI available when OpenID configuration is missing or invalid, and show a controlled sign-in-unavailable state. - Add a true application-level React error boundary and correct the React Router error fallback to use `useRouteError()`. - Share OpenID/Keycloak parsing between the application auth provider and Swagger UI, with focused regression tests. ## Related Issue Follow-up to #1734. The user-list UI introduced an app-wide `createKeycloakAuthMethod()` call during `main.jsx` module evaluation. The deployable production-mode bundle does not define `VITE_AUTH_HOST`, so Groundwork received `host: undefined` and failed on `host.trim()` before React or the router error boundary could render. This left the CWBI dev UI on a white error page. ## User Impact CDA now derives authentication configuration from the deployment's own OpenAPI document. If the deployment does not advertise usable OpenID configuration, public CDA pages remain available and authenticated features explain that sign-in is unavailable instead of crashing the entire application. ## Validation - `npm run test` — 4 tests passed - `npm run lint` - `npm run build` using Vite production mode - Repeated test, lint, and production build in an isolated `npm ci --ignore-scripts` installation using the locked Groundwork 3.9.0 dependency - Loaded the production artifact locally in a browser with unavailable OpenID configuration; verified the CDA introduction page rendered, the sign-in-unavailable state appeared, the page was not blank, and the console had no errors ## Checklist - [x] AI tools used --------- Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Refs #1733.
Summary
Adds the complete, authenticated CDA user-list feature for office-scoped recipient groups:
Related Issue
Closes #1733
Database foundation: HydrologicEngineeringCenter/cwms-database#160. Two follow-up database commits for office scoping and CDA grants remain local for review before a separate database PR.
Validation
origin/developfetched and merged: already up to date:cwms-data-api:integrationTests --tests cwms.cda.api.users.UserListControllerTestIT: 5 passed:cwms-data-api:test --tests cwms.cda.features.CdaFeatureManagerProviderTest: 6 passed with the local runtime feature override isolatednpm run lint: passednpm run build: passedbuildcompleted compilation, WAR packaging, and the bundled UI build; its unit-test phase still reports unrelated existing Windows/JDK 21 fixture and Mockito failuresScreenshots
Checklist