Protect the seed-admin to work with multiple projects #1807
Conversation
| import { projectService } from '../../project/project-service'; | ||
|
|
||
| export async function getAdminProject(user: User): Promise<Project | null> { | ||
| return projectService.getOneForUser(user); |
There was a problem hiding this comment.
This behaviour will be different in the other repository.
MarceloRGonc
changed the title
| import { userService } from '../user/user-service'; | ||
| import { getAdminProject } from './seeds/get-admin-project'; | ||
|
|
||
| // TODO: Change the place where this method is used to not rely on the admin user |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
This PR refactors the seed-admin functionality to support multiple projects by eliminating assumptions that users have only one workspace, database, or token in OpenOps Tables. The key changes enable reusing existing workspace/database/token context when seeding admin users.
Key Changes:
- Modified
createDefaultWorkspaceAndDatabaseto accept optional existing workspace context and verify/reuse it rather than always creating new resources - Refactored seed-admin logic to check for existing project data and pass it to workspace/database creation
- Extracted
getAdminProjecthelper function for consistent project retrieval
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/server/api/src/app/openops-tables/default-workspace-database.ts |
Refactored workspace/database creation to support optional existing context and verification of existing resources |
packages/server/api/src/app/database/seeds/seed-admin.ts |
Updated admin seeding to resolve existing organization/project context and pass it to workspace creation |
packages/server/api/src/app/database/seeds/get-admin-project.ts |
Added new helper function to retrieve admin user's project |
packages/server/api/src/app/database/get-default-user-db-token.ts |
Updated to use new getAdminProject helper and added TODO comment |
packages/server/api/test/unit/openops-tables/default-workspace-database.test.ts |
Added test coverage for existing workspace/database scenario and updated existing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/server/api/src/app/openops-tables/default-workspace-database.ts
Outdated
Show resolved
Hide resolved
packages/server/api/test/unit/openops-tables/default-workspace-database.test.ts
Outdated
Show resolved
Hide resolved
| databaseId: number, | ||
| workspaceId: number, | ||
| databaseToken: string, | ||
| ): Promise<Project> { |
There was a problem hiding this comment.
Why are these checks no longer necessary?
There was a problem hiding this comment.
I'm validating this directly on the Tables validation. Example:
async function ensureTablesWorkspaceExists(
token: string,
workspaceId?: number,
): Promise<number> {
if (workspaceId) {
return getWorkspaceId(token, workspaceId);
}
const workspace = await openopsTables.createWorkspace(
OPENOPS_DEFAULT_WORKSPACE_NAME,
token,
);
return workspace.id;
}
async function getWorkspaceId(
token: string,
workspaceId: number,
): Promise<number> {
const workspaces = await openopsTables.listWorkspaces(token);
const exists = workspaces.some((w) => w.id === workspaceId);
if (!exists) {
throw new Error(
`Workspace ${workspaceId} was not found in OpenOps Tables.`,
);
}
return workspaceId;
}
| return {}; | ||
| } | ||
|
|
||
| async function ensureOpenOpsTablesWorkspaceAndDatabaseExist( |
There was a problem hiding this comment.
So what happens exactly when params is undefined? I can't quite understand the flow in that scenario.
There was a problem hiding this comment.
When params is undefined, is because we don't have a project on the DB so we will create a new workspace, and database
|
|
||
| const user = await ensureUserExists(email, password); | ||
|
|
||
| const { organization, project } = await resolveUserOrganizationContext( |
There was a problem hiding this comment.
There's room for some debug logs for transparency around the params and findings.
There was a problem hiding this comment.
I added some debug logs
| ); | ||
| }); | ||
|
|
||
| it('should successfully fetch the existing workspace and database', async () => { |
There was a problem hiding this comment.
How about a few more tests for different failures?
|
Fixes OPS-3340.