diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index d323108028e..b5cb4e5c715 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -27,7 +27,7 @@ export { ResolvePasswordTokenQueryDto } from './password-reset/resolve-password- export { ChangePasswordRequestDto } from './password-reset/change-password-request.dto'; export { CreateProjectDto } from './project/create-project.dto'; -export { UpdateProjectDto } from './project/update-project.dto'; +export { UpdateProjectDto, UpdateProjectWithRelationsDto } from './project/update-project.dto'; export { DeleteProjectDto } from './project/delete-project.dto'; export { AddUsersToProjectDto } from './project/add-users-to-project.dto'; export { ChangeUserRoleInProject } from './project/change-user-role-in-project.dto'; diff --git a/packages/@n8n/api-types/src/dto/project/__tests__/update-project.dto.test.ts b/packages/@n8n/api-types/src/dto/project/__tests__/update-project.dto.test.ts index 72a839ff466..44cd1bae4ef 100644 --- a/packages/@n8n/api-types/src/dto/project/__tests__/update-project.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/project/__tests__/update-project.dto.test.ts @@ -1,6 +1,6 @@ -import { UpdateProjectDto } from '../update-project.dto'; +import { UpdateProjectWithRelationsDto } from '../update-project.dto'; -describe('UpdateProjectDto', () => { +describe('UpdateProjectWithRelationsDto', () => { describe('Valid requests', () => { test.each([ { @@ -70,7 +70,7 @@ describe('UpdateProjectDto', () => { }, }, ])('should pass validation for $name', ({ request }) => { - const result = UpdateProjectDto.safeParse(request); + const result = UpdateProjectWithRelationsDto.safeParse(request); expect(result.success).toBe(true); }); }); @@ -137,7 +137,7 @@ describe('UpdateProjectDto', () => { expectedErrorPath: ['description'], }, ])('should fail validation for $name', ({ request, expectedErrorPath }) => { - const result = UpdateProjectDto.safeParse(request); + const result = UpdateProjectWithRelationsDto.safeParse(request); expect(result.success).toBe(false); diff --git a/packages/@n8n/api-types/src/dto/project/update-project.dto.ts b/packages/@n8n/api-types/src/dto/project/update-project.dto.ts index de282f7b791..c6ec09a6dca 100644 --- a/packages/@n8n/api-types/src/dto/project/update-project.dto.ts +++ b/packages/@n8n/api-types/src/dto/project/update-project.dto.ts @@ -8,9 +8,15 @@ import { projectRelationSchema, } from '../../schemas/project.schema'; -export class UpdateProjectDto extends Z.class({ +const updateProjectShape = { name: projectNameSchema.optional(), icon: projectIconSchema.optional(), description: projectDescriptionSchema.optional(), +}; + +export class UpdateProjectDto extends Z.class(updateProjectShape) {} + +export class UpdateProjectWithRelationsDto extends Z.class({ + ...updateProjectShape, relations: z.array(projectRelationSchema).optional(), }) {} diff --git a/packages/@n8n/permissions/src/index.ts b/packages/@n8n/permissions/src/index.ts index 64743f7fa73..0388b333599 100644 --- a/packages/@n8n/permissions/src/index.ts +++ b/packages/@n8n/permissions/src/index.ts @@ -1,4 +1,4 @@ -export type * from './types.ee'; +export * from './types.ee'; export * from './constants.ee'; export * from './roles/scopes/global-scopes.ee'; diff --git a/packages/@n8n/permissions/src/types.ee.ts b/packages/@n8n/permissions/src/types.ee.ts index c3e2d4edd2e..3fb4ea0413b 100644 --- a/packages/@n8n/permissions/src/types.ee.ts +++ b/packages/@n8n/permissions/src/types.ee.ts @@ -12,6 +12,7 @@ import type { workflowSharingRoleSchema, assignableProjectRoleSchema, } from './schemas.ee'; +import { PROJECT_OWNER_ROLE_SLUG } from './constants.ee'; import { ALL_API_KEY_SCOPES } from './scope-information'; export type ScopeInformation = { @@ -62,6 +63,18 @@ export type TeamProjectRole = z.infer; export type ProjectRole = z.infer; export type AssignableProjectRole = z.infer; +/** + * Type guard for assignable project role slugs. + * + * Custom project roles are supported. We consider any slug that: + * - starts with the `project:` prefix, and + * - is not the personal owner role + * to be an assignable project role. + */ +export function isAssignableProjectRoleSlug(slug: string): slug is AssignableProjectRole { + return slug.startsWith('project:') && slug !== PROJECT_OWNER_ROLE_SLUG; +} + /** Union of all possible role types in the system */ export type AllRoleTypes = GlobalRole | ProjectRole | WorkflowSharingRole | CredentialSharingRole; diff --git a/packages/cli/src/controllers/__tests__/project.controller.test.ts b/packages/cli/src/controllers/__tests__/project.controller.test.ts new file mode 100644 index 00000000000..9249d190f82 --- /dev/null +++ b/packages/cli/src/controllers/__tests__/project.controller.test.ts @@ -0,0 +1,188 @@ +import type { AuthenticatedRequest, ProjectRepository } from '@n8n/db'; +import { mock } from 'jest-mock-extended'; + +import type { EventService } from '@/events/event.service'; +import type { Response } from 'express'; +import { ProjectController } from '@/controllers/project.controller'; +import type { ProjectService } from '@/services/project.service.ee'; +import type { UserManagementMailer } from '@/user-management/email'; + +describe('ProjectController (members endpoints)', () => { + const eventService = mock(); + const projectsService = mock(); + const projectRepository = mock(); + const userManagementMailer = mock(); + + const controller = new ProjectController( + projectsService as unknown as ProjectService, + projectRepository as unknown as ProjectRepository, + eventService as unknown as EventService, + userManagementMailer as unknown as UserManagementMailer, + ); + + const makeRes = () => { + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + } as unknown as Response; + return res; + }; + + const req: AuthenticatedRequest = { + user: { id: 'actor-user', role: { slug: 'global:owner' } } as any, + } as AuthenticatedRequest; + + beforeEach(() => { + jest.resetAllMocks(); + }); + + it('emits team-project-updated with full members list on addProjectUsers', async () => { + // Arrange + const projectId = 'p1'; + const payload = { relations: [{ userId: 'u2', role: 'project:viewer' as const }] }; + + (projectsService.addUsersWithConflictSemantics as jest.Mock).mockResolvedValue({ + project: { id: projectId, name: 'Project' }, + added: payload.relations, + conflicts: [], + }); + + (projectsService.getProjectRelations as jest.Mock).mockResolvedValue([ + { userId: 'u1', role: { slug: 'project:admin' } }, + { userId: 'u2', role: { slug: 'project:viewer' } }, + ]); + + const res = makeRes(); + + // Act + await controller.addProjectUsers(req, res, projectId, payload as any); + + // Assert + expect(eventService.emit).toHaveBeenCalledWith('team-project-updated', { + userId: 'actor-user', + role: 'global:owner', + members: [ + { userId: 'u1', role: 'project:admin' }, + { userId: 'u2', role: 'project:viewer' }, + ], + projectId, + }); + + // Verify mailer called for new sharees + expect(userManagementMailer.notifyProjectShared).toHaveBeenCalledWith({ + sharer: req.user, + newSharees: payload.relations, + project: { id: projectId, name: 'Project' }, + }); + }); + + it('emits team-project-updated on changeProjectUserRole and returns 204', async () => { + // Arrange + const projectId = 'p2'; + (projectsService.getProjectRelations as jest.Mock).mockResolvedValue([ + { userId: 'u1', role: { slug: 'project:admin' } }, + { userId: 'u2', role: { slug: 'project:editor' } }, + ]); + + const res = makeRes(); + + // Act + await controller.changeProjectUserRole(req, res, projectId, 'u2', { + role: 'project:editor', + } as any); + + // Assert + expect(eventService.emit).toHaveBeenCalledWith('team-project-updated', { + userId: 'actor-user', + role: 'global:owner', + members: [ + { userId: 'u1', role: 'project:admin' }, + { userId: 'u2', role: 'project:editor' }, + ], + projectId, + }); + expect(res.status).toHaveBeenCalledWith(204); + }); + + it('emits team-project-updated on deleteProjectUser and returns 204', async () => { + // Arrange + const projectId = 'p3'; + (projectsService.getProjectRelations as jest.Mock).mockResolvedValue([ + { userId: 'u1', role: { slug: 'project:admin' } }, + { userId: 'u3', role: { slug: 'project:viewer' } }, + ]); + + const res = makeRes(); + + // Act + await controller.deleteProjectUser(req, res, projectId, 'u2'); + + // Assert + expect(eventService.emit).toHaveBeenCalledWith('team-project-updated', { + userId: 'actor-user', + role: 'global:owner', + members: [ + { userId: 'u1', role: 'project:admin' }, + { userId: 'u3', role: 'project:viewer' }, + ], + projectId, + }); + expect(res.status).toHaveBeenCalledWith(204); + }); + + it('returns 201 with conflicts body when some users added and some conflicted', async () => { + // Arrange + const projectId = 'p4'; + const added = [{ userId: 'u4', role: 'project:viewer' as const }]; + const conflicts = [ + { + userId: 'u5', + currentRole: 'project:viewer' as const, + requestedRole: 'project:editor' as const, + }, + ]; + + (projectsService.addUsersWithConflictSemantics as jest.Mock).mockResolvedValue({ + project: { id: projectId, name: 'Project' }, + added, + conflicts, + }); + + (projectsService.getProjectRelations as jest.Mock).mockResolvedValue([ + { userId: 'u1', role: { slug: 'project:admin' } }, + { userId: 'u4', role: { slug: 'project:viewer' } }, + { userId: 'u5', role: { slug: 'project:viewer' } }, + ]); + + const res = makeRes(); + + // Act + await controller.addProjectUsers(req, res, projectId, { + relations: [...added, { userId: 'u5', role: 'project:editor' }], + } as any); + + // Assert: 201 with conflicts body + expect(res.status).toHaveBeenCalledWith(201); + expect(res.json).toHaveBeenCalledWith({ conflicts }); + + // Mailer is called for newly added sharees + expect(userManagementMailer.notifyProjectShared).toHaveBeenCalledWith({ + sharer: req.user, + newSharees: added, + project: { id: projectId, name: 'Project' }, + }); + + // Telemetry event has full members list + expect(eventService.emit).toHaveBeenCalledWith('team-project-updated', { + userId: 'actor-user', + role: 'global:owner', + members: [ + { userId: 'u1', role: 'project:admin' }, + { userId: 'u4', role: 'project:viewer' }, + { userId: 'u5', role: 'project:viewer' }, + ], + projectId, + }); + }); +}); diff --git a/packages/cli/src/controllers/project.controller.ts b/packages/cli/src/controllers/project.controller.ts index 316d5ccd816..5a7b355ac6a 100644 --- a/packages/cli/src/controllers/project.controller.ts +++ b/packages/cli/src/controllers/project.controller.ts @@ -1,4 +1,10 @@ -import { CreateProjectDto, DeleteProjectDto, UpdateProjectDto } from '@n8n/api-types'; +import { + CreateProjectDto, + DeleteProjectDto, + UpdateProjectDto, + AddUsersToProjectDto, + ChangeUserRoleInProject, +} from '@n8n/api-types'; import type { Project } from '@n8n/db'; import { AuthenticatedRequest, ProjectRepository } from '@n8n/db'; import { @@ -210,44 +216,106 @@ export class ProjectController { @Patch('/:projectId') @ProjectScope('project:update') async updateProject( - req: AuthenticatedRequest, + _req: AuthenticatedRequest, _res: Response, @Body payload: UpdateProjectDto, @Param('projectId') projectId: string, ) { - const { name, icon, relations, description } = payload; - if (name || icon || description) { - await this.projectsService.updateProject(projectId, { name, icon, description }); - } - if (relations) { - try { - const { project, newRelations } = await this.projectsService.syncProjectRelations( - projectId, - relations, - ); + await this.projectsService.updateProject(projectId, payload); + } - // Send email notifications to new sharees + @Post('/:projectId/users') + @ProjectScope('project:update') + async addProjectUsers( + req: AuthenticatedRequest, + res: Response, + @Param('projectId') projectId: string, + @Body payload: AddUsersToProjectDto, + ) { + try { + const { added, conflicts, project } = + await this.projectsService.addUsersWithConflictSemantics(projectId, payload.relations); + + if (added.length > 0) { await this.userManagementMailer.notifyProjectShared({ sharer: req.user, - newSharees: newRelations, + newSharees: added, project: { id: project.id, name: project.name }, }); - } catch (e) { - if (e instanceof UnlicensedProjectRoleError) { - throw new BadRequestError(e.message); - } - throw e; } + const relations = await this.projectsService.getProjectRelations(projectId); this.eventService.emit('team-project-updated', { userId: req.user.id, role: req.user.role.slug, - members: relations, + members: relations.map((r) => ({ userId: r.userId, role: r.role.slug })), projectId, }); + + // Response semantics: + // - If at least one user was added, return 201. When there are also conflicts, include them in the body. + // - If no users were added but conflicts exist, return 409 with conflicts. + if (added.length > 0) { + return conflicts.length > 0 ? res.status(201).json({ conflicts }) : res.status(201).send(); + } + if (conflicts.length > 0) return res.status(409).json({ conflicts }); + return res.status(200).send(); + } catch (e) { + if (e instanceof UnlicensedProjectRoleError) { + throw new BadRequestError(e.message); + } + throw e; } } + @Patch('/:projectId/users/:userId') + @ProjectScope('project:update') + async changeProjectUserRole( + req: AuthenticatedRequest, + res: Response, + @Param('projectId') projectId: string, + @Param('userId') userId: string, + @Body body: ChangeUserRoleInProject, + ) { + try { + await this.projectsService.changeUserRoleInProject(projectId, userId, body.role); + await this.projectsService.clearCredentialCanUseExternalSecretsCache(projectId); + const relations = await this.projectsService.getProjectRelations(projectId); + this.eventService.emit('team-project-updated', { + userId: req.user.id, + role: req.user.role.slug, + members: relations.map((r) => ({ userId: r.userId, role: r.role.slug })), + projectId, + }); + return res.status(204).send(); + } catch (e) { + if (e instanceof UnlicensedProjectRoleError) { + throw new BadRequestError(e.message); + } + throw e; + } + } + + @Delete('/:projectId/users/:userId') + @ProjectScope('project:update') + async deleteProjectUser( + req: AuthenticatedRequest, + res: Response, + @Param('projectId') projectId: string, + @Param('userId') userId: string, + ) { + await this.projectsService.deleteUserFromProject(projectId, userId); + await this.projectsService.clearCredentialCanUseExternalSecretsCache(projectId); + const relations = await this.projectsService.getProjectRelations(projectId); + this.eventService.emit('team-project-updated', { + userId: req.user.id, + role: req.user.role.slug, + members: relations.map((r) => ({ userId: r.userId, role: r.role.slug })), + projectId, + }); + return res.status(204).send(); + } + @Delete('/:projectId') @ProjectScope('project:delete') async deleteProject( diff --git a/packages/cli/src/public-api/v1/handlers/projects/projects.handler.ts b/packages/cli/src/public-api/v1/handlers/projects/projects.handler.ts index 8f1df7cef24..50c1c7be583 100644 --- a/packages/cli/src/public-api/v1/handlers/projects/projects.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/projects/projects.handler.ts @@ -3,7 +3,7 @@ import { ChangeUserRoleInProject, CreateProjectDto, DeleteProjectDto, - UpdateProjectDto, + UpdateProjectWithRelationsDto, } from '@n8n/api-types'; import type { AuthenticatedRequest } from '@n8n/db'; import { ProjectRepository } from '@n8n/db'; @@ -42,7 +42,7 @@ export = { isLicensed('feat:projectRole:admin'), apiKeyHasScopeWithGlobalScopeFallback({ scope: 'project:update' }), async (req: AuthenticatedRequest<{ projectId: string }>, res: Response) => { - const payload = UpdateProjectDto.safeParse(req.body); + const payload = UpdateProjectWithRelationsDto.safeParse(req.body); if (payload.error) { return res.status(400).json(payload.error.errors[0]); } diff --git a/packages/cli/src/services/project.service.ee.ts b/packages/cli/src/services/project.service.ee.ts index 529eeeb3215..9279ee723b9 100644 --- a/packages/cli/src/services/project.service.ee.ts +++ b/packages/cli/src/services/project.service.ee.ts @@ -15,10 +15,10 @@ import { Container, Service } from '@n8n/di'; import { hasGlobalScope, type Scope, - type ProjectRole, AssignableProjectRole, PROJECT_OWNER_ROLE_SLUG, PROJECT_ADMIN_ROLE_SLUG, + isAssignableProjectRoleSlug, } from '@n8n/permissions'; // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import import type { FindOptionsWhere, EntityManager } from '@n8n/typeorm'; @@ -42,7 +42,7 @@ export class TeamProjectOverQuotaError extends UserError { } export class UnlicensedProjectRoleError extends UserError { - constructor(role: ProjectRole | AssignableProjectRole) { + constructor(role: AssignableProjectRole) { super(`Your instance is not licensed to use role "${role}".`); } } @@ -353,6 +353,68 @@ export class ProjectService { ); } + /** + * Add users with conflict semantics: + * - Adds users that are not already members + * - No-ops for users already in the project with the same role + * - Reports conflicts for users already in the project with a different role (no change) + */ + async addUsersWithConflictSemantics( + projectId: string, + relations: Array<{ userId: string; role: AssignableProjectRole }>, + ): Promise<{ + project: Project; + added: Array<{ userId: string; role: AssignableProjectRole }>; + conflicts: Array<{ + userId: string; + currentRole: AssignableProjectRole; + requestedRole: AssignableProjectRole; + }>; + }> { + const project = await this.getTeamProjectWithRelations(projectId); + this.checkRolesLicensed(project, relations); + + // Validate roles exist + await this.roleService.checkRolesExist( + relations.map((r) => r.role), + 'project', + ); + + const existingByUserId = new Map(project.projectRelations.map((r) => [r.userId, r])); + const added: Array<{ userId: string; role: AssignableProjectRole }> = []; + const conflicts: Array<{ + userId: string; + currentRole: AssignableProjectRole; + requestedRole: AssignableProjectRole; + }> = []; + + for (const rel of relations) { + const existing = existingByUserId.get(rel.userId); + if (!existing) continue; // will be inserted below + const current = existing.role?.slug; + if (current && current !== rel.role && isAssignableProjectRoleSlug(current)) { + conflicts.push({ userId: rel.userId, currentRole: current, requestedRole: rel.role }); + } + } + + // Insert only non-existing users + const toInsert = relations.filter((rel) => !existingByUserId.has(rel.userId)); + if (toInsert.length > 0) { + // Use insert to avoid accidental upsert of different role + await this.projectRelationRepository.insert( + toInsert.map((v) => ({ + projectId: project.id, + userId: v.userId, + role: { slug: v.role }, + })), + ); + added.push(...toInsert); + } + + await this.clearCredentialCanUseExternalSecretsCache(projectId); + return { project, added, conflicts }; + } + private async getTeamProjectWithRelations(projectId: string) { const project = await this.projectRepository.findOne({ where: { id: projectId, type: 'team' }, @@ -411,6 +473,13 @@ export class ProjectService { throw new ProjectNotFoundError(projectId); } + // License check: only allow change to roles that are licensed + const currentRelation = project.projectRelations.find((r) => r.userId === userId); + const currentRole = currentRelation?.role?.slug; + if (currentRole !== role && !this.roleService.isRoleLicensed(role)) { + throw new UnlicensedProjectRoleError(role); + } + await this.projectRelationRepository.update({ projectId, userId }, { role: { slug: role } }); } diff --git a/packages/cli/test/integration/project.api.test.ts b/packages/cli/test/integration/project.api.test.ts index 5943813d5f9..b5422d429a0 100644 --- a/packages/cli/test/integration/project.api.test.ts +++ b/packages/cli/test/integration/project.api.test.ts @@ -143,6 +143,97 @@ describe('GET /projects/', () => { }); }); +describe('Project members endpoints', () => { + test('POST /projects/:projectId/users adds a member and emits telemetry', async () => { + const owner = await createOwner(); + const member = await createUser(); + const project = await createTeamProject('Team Project', owner); + + const ownerAgent = testServer.authAgentFor(owner); + const res = await ownerAgent + .post(`/projects/${project.id}/users`) + .send({ relations: [{ userId: member.id, role: 'project:viewer' }] }); + + expect(res.status).toBe(201); + const relations = await getProjectRelations({ projectId: project.id }); + expect(relations.some((r) => r.userId === member.id && r.role.slug === 'project:viewer')).toBe( + true, + ); + }); + + test('POST /projects/:projectId/users returns 409 for existing member with different role', async () => { + const owner = await createOwner(); + const member = await createUser(); + const project = await createTeamProject('Team Project', owner); + + // First add as viewer + await linkUserToProject(member, project, 'project:viewer'); + + const ownerAgent = testServer.authAgentFor(owner); + const res = await ownerAgent + .post(`/projects/${project.id}/users`) + .send({ relations: [{ userId: member.id, role: 'project:editor' }] }); + + expect(res.status).toBe(409); + const relations = await getProjectRelations({ projectId: project.id }); + expect(relations.some((r) => r.userId === member.id && r.role.slug === 'project:viewer')).toBe( + true, + ); + }); + + test('POST /projects/:projectId/users returns 200 when member already exists with same role (no-op)', async () => { + const owner = await createOwner(); + const member = await createUser(); + const project = await createTeamProject('Team Project', owner); + + // First add as viewer + await linkUserToProject(member, project, 'project:viewer'); + + const ownerAgent = testServer.authAgentFor(owner); + const res = await ownerAgent + .post(`/projects/${project.id}/users`) + .send({ relations: [{ userId: member.id, role: 'project:viewer' }] }); + + expect(res.status).toBe(200); + const relations = await getProjectRelations({ projectId: project.id }); + expect(relations.some((r) => r.userId === member.id && r.role.slug === 'project:viewer')).toBe( + true, + ); + }); + + test("PATCH /projects/:projectId/users/:userId changes a member's role", async () => { + const owner = await createOwner(); + const member = await createUser(); + const project = await createTeamProject('Team Project', owner); + await linkUserToProject(member, project, 'project:viewer'); + + const ownerAgent = testServer.authAgentFor(owner); + const res = await ownerAgent + .patch(`/projects/${project.id}/users/${member.id}`) + .send({ role: 'project:editor' }); + + expect(res.status).toBe(204); + const relations = await getProjectRelations({ projectId: project.id }); + expect(relations.some((r) => r.userId === member.id && r.role.slug === 'project:editor')).toBe( + true, + ); + }); + + test('DELETE /projects/:projectId/users/:userId removes a member', async () => { + const owner = await createOwner(); + const member = await createUser(); + const project = await createTeamProject('Team Project', owner); + await linkUserToProject(member, project, 'project:viewer'); + + const ownerAgent = testServer.authAgentFor(owner); + const res = await ownerAgent.delete(`/projects/${project.id}/users/${member.id}`); + + expect(res.status).toBe(204); + const relations = await getProjectRelations({ projectId: project.id }); + expect(relations.some((r) => r.userId === member.id)).toBe(false); + }); +}); + describe('GET /projects/count', () => { test('should return correct number of projects', async () => { const [firstUser] = await Promise.all([ @@ -576,10 +667,9 @@ describe('PATCH /projects/:projectId', () => { const memberAgent = testServer.authAgentFor(testUser1); const deleteSpy = jest.spyOn(Container.get(CacheService), 'deleteMany'); - const resp = await memberAgent.patch(`/projects/${teamProject1.id}`).send({ - name: teamProject1.name, + // Add two members to teamProject1 + const addResp = await memberAgent.post(`/projects/${teamProject1.id}/users`).send({ relations: [ - { userId: testUser1.id, role: 'project:admin' }, { userId: testUser3.id, role: 'project:editor' }, { userId: ownerUser.id, role: 'project:viewer' }, ] as Array<{ @@ -587,8 +677,9 @@ describe('PATCH /projects/:projectId', () => { role: ProjectRole; }>, }); - expect(resp.status).toBe(200); + expect(addResp.status).toBe(201); + // External secrets cache must be cleared for credentials owned by teamProject1 expect(deleteSpy).toBeCalledWith([`credential-can-use-secrets:${credential1.id}`]); deleteSpy.mockClear(); @@ -687,13 +778,9 @@ describe('PATCH /projects/:projectId', () => { await testServer .authAgentFor(projectAdmin) - .patch(`/projects/${teamProject.id}`) + .post(`/projects/${teamProject.id}/users`) .send({ - name: teamProject.name, - relations: [ - { userId: projectAdmin.id, role: 'project:admin' }, - { userId: userToBeInvited.id, role }, - ] as Array<{ + relations: [{ userId: userToBeInvited.id, role }] as Array<{ userId: string; role: ProjectRole; }>, @@ -727,17 +814,9 @@ describe('PATCH /projects/:projectId', () => { const memberAgent = testServer.authAgentFor(testUser2); - const resp = await memberAgent.patch(`/projects/${teamProject.id}`).send({ - name: teamProject.name, - relations: [ - { userId: testUser2.id, role: 'project:admin' }, - { userId: testUser1.id, role: 'project:editor' }, - { userId: testUser3.id, role: 'project:editor' }, - ] as Array<{ - userId: string; - role: ProjectRole; - }>, - }); + const resp = await memberAgent + .patch(`/projects/${teamProject.id}/users/${testUser1.id}`) + .send({ role: 'project:editor' }); expect(resp.status).toBe(400); const tpRelations = await getProjectRelations({ projectId: teamProject.id }); @@ -764,18 +843,14 @@ describe('PATCH /projects/:projectId', () => { const memberAgent = testServer.authAgentFor(testUser2); - const resp = await memberAgent.patch(`/projects/${teamProject.id}`).send({ - name: teamProject.name, - relations: [ - { userId: testUser1.id, role: 'project:viewer' }, - { userId: testUser2.id, role: 'project:admin' }, - { userId: testUser3.id, role: 'project:admin' }, - ] as Array<{ - userId: string; - role: ProjectRole; - }>, - }); - expect(resp.status).toBe(200); + const resp1 = await memberAgent + .patch(`/projects/${teamProject.id}/users/${testUser2.id}`) + .send({ role: 'project:admin' }); + expect(resp1.status).toBe(204); + const resp2 = await memberAgent + .patch(`/projects/${teamProject.id}/users/${testUser3.id}`) + .send({ role: 'project:admin' }); + expect(resp2.status).toBe(204); const tpRelations = await getProjectRelations({ projectId: teamProject.id }); expect(tpRelations.length).toBe(3); @@ -795,11 +870,8 @@ describe('PATCH /projects/:projectId', () => { const memberAgent = testServer.authAgentFor(testUser1); - const resp = await memberAgent.patch(`/projects/${personalProject.id}`).send({ - relations: [ - { userId: testUser1.id, role: PROJECT_OWNER_ROLE_SLUG }, - { userId: testUser2.id, role: 'project:admin' }, - ] as Array<{ + const resp = await memberAgent.post(`/projects/${personalProject.id}/users`).send({ + relations: [{ userId: testUser2.id, role: 'project:admin' }] as Array<{ userId: string; role: ProjectRole; }>, diff --git a/packages/cli/test/integration/public-api/projects.test.ts b/packages/cli/test/integration/public-api/projects.test.ts index 1b2953b0c8f..f0b675c8e27 100644 --- a/packages/cli/test/integration/public-api/projects.test.ts +++ b/packages/cli/test/integration/public-api/projects.test.ts @@ -675,6 +675,9 @@ describe('Projects in Public API', () => { beforeEach(() => { testServer.license.setQuota('quota:maxTeamProjects', -1); testServer.license.enable('feat:projectRole:admin'); + // Enable role licenses required for role change operations + testServer.license.enable('feat:projectRole:viewer'); + testServer.license.enable('feat:projectRole:editor'); }); it('should reject with 400 if the role do not exist', async () => { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index d6d7b91d6ba..c42e62c9061 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3111,10 +3111,11 @@ "projects.menu.personal": "Personal", "projects.menu.addFirstProject": "Add project", "projects.settings": "Project settings", + "projects.settings.info": "Project info", "projects.settings.newProjectName": "My project", "projects.settings.iconPicker.button.tooltip": "Choose project icon", - "projects.settings.name": "Project icon and name", - "projects.settings.description": "Project description", + "projects.settings.name": "Icon and name", + "projects.settings.description": "Description", "projects.settings.projectMembers": "Project members", "projects.settings.message.unsavedChanges": "You have unsaved changes", "projects.settings.danger.message": "When deleting a project, you can also choose to move all workflows and credentials to another project.", @@ -3155,6 +3156,7 @@ "projects.settings.memberRole.update.error.title": "An error occurred while updating member role", "projects.settings.member.removed.title": "Member removed successfully", "projects.settings.member.remove.error.title": "An error occurred while removing member", + "projects.settings.member.added.title": "Member added successfully", "projects.sharing.noMatchingProjects": "There are no available projects", "projects.sharing.noMatchingUsers": "No matching users or projects", "projects.sharing.select.placeholder": "Select project or user", diff --git a/packages/frontend/editor-ui/src/api/projects.api.ts b/packages/frontend/editor-ui/src/api/projects.api.ts index 61516f8d515..79e5d4fb481 100644 --- a/packages/frontend/editor-ui/src/api/projects.api.ts +++ b/packages/frontend/editor-ui/src/api/projects.api.ts @@ -2,6 +2,7 @@ import type { IRestApiContext } from '@n8n/rest-api-client'; import { makeRestApiRequest } from '@n8n/rest-api-client'; import type { Project, ProjectListItem, ProjectsCount } from '@/types/projects.types'; import type { CreateProjectDto, UpdateProjectDto } from '@n8n/api-types'; +import type { AssignableProjectRole } from '@n8n/permissions'; export const getAllProjects = async (context: IRestApiContext): Promise => { return await makeRestApiRequest(context, 'GET', '/projects'); @@ -50,3 +51,28 @@ export const deleteProject = async ( export const getProjectsCount = async (context: IRestApiContext): Promise => { return await makeRestApiRequest(context, 'GET', '/projects/count'); }; + +export const addProjectMembers = async ( + context: IRestApiContext, + projectId: string, + relations: Array<{ userId: string; role: AssignableProjectRole }>, +): Promise => { + await makeRestApiRequest(context, 'POST', `/projects/${projectId}/users`, { relations }); +}; + +export const updateProjectMemberRole = async ( + context: IRestApiContext, + projectId: string, + userId: string, + role: AssignableProjectRole, +): Promise => { + await makeRestApiRequest(context, 'PATCH', `/projects/${projectId}/users/${userId}`, { role }); +}; + +export const deleteProjectMember = async ( + context: IRestApiContext, + projectId: string, + userId: string, +): Promise => { + await makeRestApiRequest(context, 'DELETE', `/projects/${projectId}/users/${userId}`); +}; diff --git a/packages/frontend/editor-ui/src/components/Projects/ProjectHeader.vue b/packages/frontend/editor-ui/src/components/Projects/ProjectHeader.vue index a5d92c1a770..6c9fa6cbb40 100644 --- a/packages/frontend/editor-ui/src/components/Projects/ProjectHeader.vue +++ b/packages/frontend/editor-ui/src/components/Projects/ProjectHeader.vue @@ -381,7 +381,7 @@ const onSelect = (action: string) => { display: flex; align-items: flex-start; justify-content: space-between; - padding-bottom: var(--spacing-m); + padding-bottom: var(--spacing-l); min-height: var(--spacing-3xl); } diff --git a/packages/frontend/editor-ui/src/stores/projects.store.test.ts b/packages/frontend/editor-ui/src/stores/projects.store.test.ts index 8de35cd14ec..0355e428650 100644 --- a/packages/frontend/editor-ui/src/stores/projects.store.test.ts +++ b/packages/frontend/editor-ui/src/stores/projects.store.test.ts @@ -15,6 +15,9 @@ vi.mock('vue-router', () => ({ vi.mock('@/api/projects.api', () => ({ updateProject: vi.fn(), getProject: vi.fn(), + addProjectMembers: vi.fn(), + updateProjectMemberRole: vi.fn(), + deleteProjectMember: vi.fn(), })); // Typed mocked facade for the API module @@ -93,14 +96,71 @@ describe('useProjectsStore.updateProject (partial payloads)', () => { expect(mockedProjectsApi.getProject).not.toHaveBeenCalled(); }); - it('refetches project when relations are provided and does not touch name/icon/description', async () => { + it('addMember calls API and refreshes current project', async () => { const store = makeStoreWithProject(); - mockedProjectsApi.updateProject.mockResolvedValue(undefined); + mockedProjectsApi.addProjectMembers.mockResolvedValue(undefined); const now = new Date().toISOString(); const serverProject: Project = { id: 'p1', - name: 'SERVER', - description: 'SERVER', + name: 'A', + description: 'desc', + icon: { type: 'icon', value: 'layers' }, + type: ProjectTypes.Team, + createdAt: now, + updatedAt: now, + relations: [ + { id: 'u1', email: 'x@y.z', firstName: 'X', lastName: 'Y', role: 'project:editor' }, + { id: 'u2', email: 'a@b.c', firstName: 'A', lastName: 'B', role: 'project:viewer' }, + ], + scopes: ['project:read' as Scope], + }; + mockedProjectsApi.getProject.mockResolvedValue(serverProject); + + await store.addMember('p1', { userId: 'u2', role: 'project:viewer' }); + expect(mockedProjectsApi.addProjectMembers).toHaveBeenCalledWith(expect.anything(), 'p1', [ + { userId: 'u2', role: 'project:viewer' }, + ]); + expect(mockedProjectsApi.getProject).toHaveBeenCalledWith(expect.anything(), 'p1'); + expect(store.currentProject?.relations.length).toBe(2); + }); + + it('updateMemberRole calls API and refreshes current project', async () => { + const store = makeStoreWithProject(); + mockedProjectsApi.updateProjectMemberRole.mockResolvedValue(undefined); + const now = new Date().toISOString(); + const serverProject: Project = { + id: 'p1', + name: 'A', + description: 'desc', + icon: { type: 'icon', value: 'layers' }, + type: ProjectTypes.Team, + createdAt: now, + updatedAt: now, + relations: [ + { id: 'u1', email: 'x@y.z', firstName: 'X', lastName: 'Y', role: 'project:viewer' }, + ], + scopes: ['project:read' as Scope], + }; + mockedProjectsApi.getProject.mockResolvedValue(serverProject); + + await store.updateMemberRole('p1', 'u1', 'project:viewer'); + expect(mockedProjectsApi.updateProjectMemberRole).toHaveBeenCalledWith( + expect.anything(), + 'p1', + 'u1', + 'project:viewer', + ); + expect(mockedProjectsApi.getProject).toHaveBeenCalledWith(expect.anything(), 'p1'); + }); + + it('removeMember calls API and refreshes current project', async () => { + const store = makeStoreWithProject(); + mockedProjectsApi.deleteProjectMember.mockResolvedValue(undefined); + const now = new Date().toISOString(); + const serverProject: Project = { + id: 'p1', + name: 'A', + description: 'desc', icon: { type: 'icon', value: 'layers' }, type: ProjectTypes.Team, createdAt: now, @@ -110,18 +170,13 @@ describe('useProjectsStore.updateProject (partial payloads)', () => { }; mockedProjectsApi.getProject.mockResolvedValue(serverProject); - await store.updateProject('p1', { relations: [{ userId: 'u2', role: 'project:viewer' }] }); - - // Ensure only relations were sent - expect(mockedProjectsApi.updateProject).toHaveBeenCalledWith(expect.anything(), 'p1', { - relations: [{ userId: 'u2', role: 'project:viewer' }], - }); - // Refetch invoked + await store.removeMember('p1', 'u1'); + expect(mockedProjectsApi.deleteProjectMember).toHaveBeenCalledWith( + expect.anything(), + 'p1', + 'u1', + ); expect(mockedProjectsApi.getProject).toHaveBeenCalledWith(expect.anything(), 'p1'); - // Local name/description remain unchanged eagerly; currentProject then replaced by getProject - expect(store.myProjects[0].name).toBe('A'); - expect(store.myProjects[0].description).toBe('desc'); - expect(store.currentProject?.name).toBe('SERVER'); - expect(store.currentProject?.description).toBe('SERVER'); + expect(store.currentProject?.relations.length).toBe(0); }); }); diff --git a/packages/frontend/editor-ui/src/stores/projects.store.ts b/packages/frontend/editor-ui/src/stores/projects.store.ts index 5fc299e7a5e..4b10736ed8d 100644 --- a/packages/frontend/editor-ui/src/stores/projects.store.ts +++ b/packages/frontend/editor-ui/src/stores/projects.store.ts @@ -125,25 +125,48 @@ export const useProjectsStore = defineStore(STORES.PROJECTS, () => { }; const updateProject = async (id: Project['id'], projectData: UpdateProjectDto): Promise => { - await projectsApi.updateProject(rootStore.restApiContext, id, projectData); + const { name, icon, description } = projectData; + const payload: UpdateProjectDto = {}; + if (name !== undefined) payload.name = name; + if (icon !== undefined) payload.icon = icon; + if (description !== undefined) payload.description = description; + await projectsApi.updateProject(rootStore.restApiContext, id, payload); const projectIndex = myProjects.value.findIndex((p) => p.id === id); - const { name, icon, description, relations } = projectData; + const { name: nm, icon: ic, description: desc } = { name, icon, description }; if (projectIndex !== -1) { - if (typeof name !== 'undefined') myProjects.value[projectIndex].name = name; - if (typeof icon !== 'undefined') myProjects.value[projectIndex].icon = icon; - if (typeof description !== 'undefined') - myProjects.value[projectIndex].description = description; + if (nm !== undefined) myProjects.value[projectIndex].name = nm; + if (ic !== undefined) myProjects.value[projectIndex].icon = ic; + if (desc !== undefined) myProjects.value[projectIndex].description = desc; } if (currentProject.value) { - if (typeof name !== 'undefined') currentProject.value.name = name; - if (typeof icon !== 'undefined') currentProject.value.icon = icon; - if (typeof description !== 'undefined') currentProject.value.description = description; - } - if (relations) { - await getProject(id); + if (nm !== undefined) currentProject.value.name = nm; + if (ic !== undefined) currentProject.value.icon = ic; + if (desc !== undefined) currentProject.value.description = desc; } }; + const addMember = async ( + projectId: string, + { userId, role }: { userId: string; role: string }, + ): Promise => { + await projectsApi.addProjectMembers(rootStore.restApiContext, projectId, [{ userId, role }]); + await getProject(projectId); + }; + + const updateMemberRole = async ( + projectId: string, + userId: string, + role: string, + ): Promise => { + await projectsApi.updateProjectMemberRole(rootStore.restApiContext, projectId, userId, role); + await getProject(projectId); + }; + + const removeMember = async (projectId: string, userId: string): Promise => { + await projectsApi.deleteProjectMember(rootStore.restApiContext, projectId, userId); + await getProject(projectId); + }; + const deleteProject = async (projectId: string, transferId?: string): Promise => { await projectsApi.deleteProject(rootStore.restApiContext, projectId, transferId); await getProjectsCount(); @@ -286,6 +309,9 @@ export const useProjectsStore = defineStore(STORES.PROJECTS, () => { getProject, createProject, updateProject, + addMember, + updateMemberRole, + removeMember, deleteProject, getProjectsCount, setProjectNavActiveIdByWorkflowHomeProject, diff --git a/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts b/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts index 6069544f0bc..0158bb4a79f 100644 --- a/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts +++ b/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts @@ -312,17 +312,16 @@ describe('ProjectSettings', () => { }); it('renders core form elements and initializes state', async () => { - const { getByTestId } = renderComponent(); + const { getByTestId, queryByTestId } = renderComponent(); await nextTick(); expect(getByTestId('project-settings-container')).toBeInTheDocument(); const nameInput = getByTestId('project-settings-name-input'); const descriptionInput = getByTestId('project-settings-description-input'); - const saveButton = getByTestId('project-settings-save-button'); - const cancelButton = getByTestId('project-settings-cancel-button'); expect(nameInput).toBeInTheDocument(); expect(descriptionInput).toBeInTheDocument(); - expect(saveButton).toBeDisabled(); - expect(cancelButton).toBeDisabled(); + // Save/Cancel are not rendered until form is dirty + expect(queryByTestId('project-settings-save-button')).toBeNull(); + expect(queryByTestId('project-settings-cancel-button')).toBeNull(); const actualName = getInput(nameInput); const actualDesc = getTextarea(descriptionInput); expect(actualName.value).toBe('Test Project'); @@ -332,18 +331,19 @@ describe('ProjectSettings', () => { describe('Form interactions', () => { it('marks dirty, cancels reset, and saves via Enter and button', async () => { const updateSpy = vi.spyOn(projectsStore, 'updateProject').mockResolvedValue(undefined); - const { getByTestId } = renderComponent(); + const { getByTestId, queryByTestId } = renderComponent(); const nameInput = getByTestId('project-settings-name-input'); - const saveButton = getByTestId('project-settings-save-button'); - const cancelButton = getByTestId('project-settings-cancel-button'); const actualInput = getInput(nameInput); // Dirty then cancel await userEvent.type(actualInput, ' Extra'); - expect(cancelButton).toBeEnabled(); - await userEvent.click(cancelButton); + const cancelBtn1 = getByTestId('project-settings-cancel-button'); + expect(cancelBtn1).toBeEnabled(); + await userEvent.click(cancelBtn1); expect(actualInput.value).toBe('Test Project'); - expect(cancelButton).toBeDisabled(); + // Buttons disappear when clean + expect(queryByTestId('project-settings-cancel-button')).toBeNull(); + expect(queryByTestId('project-settings-save-button')).toBeNull(); // Save via Enter await userEvent.type(actualInput, ' - Updated'); @@ -361,8 +361,9 @@ describe('ProjectSettings', () => { // Save via button await userEvent.type(actualInput, ' Again'); - expect(saveButton).toBeEnabled(); - await userEvent.click(saveButton); + const saveBtn = getByTestId('project-settings-save-button'); + expect(saveBtn).toBeEnabled(); + await userEvent.click(saveBtn); await nextTick(); expect(updateSpy).toHaveBeenCalledTimes(2); expect(mockShowMessage).toHaveBeenCalledTimes(2); @@ -375,7 +376,6 @@ describe('ProjectSettings', () => { projectsStore.updateProject.mockRejectedValue(error); const { getByTestId } = renderComponent(); const nameInput = getByTestId('project-settings-name-input'); - const saveButton = getByTestId('project-settings-save-button'); const actualInput = getInput(nameInput); await userEvent.type(actualInput, ' Updated'); @@ -387,8 +387,9 @@ describe('ProjectSettings', () => { projectsStore.updateProject.mockClear(); await userEvent.clear(actualInput); - expect(saveButton).toBeDisabled(); - await userEvent.click(saveButton); + const saveButton2 = getByTestId('project-settings-save-button'); + expect(saveButton2).toBeDisabled(); + await userEvent.click(saveButton2); expect(projectsStore.updateProject).not.toHaveBeenCalled(); }); }); @@ -396,28 +397,28 @@ describe('ProjectSettings', () => { describe('Save state and validation', () => { it('maintains state after save and validation toggles', async () => { const updateSpy = vi.spyOn(projectsStore, 'updateProject').mockResolvedValue(undefined); - const { getByTestId } = renderComponent(); + const { getByTestId, queryByTestId } = renderComponent(); const nameInput = getByTestId('project-settings-name-input'); - const cancelButton = getByTestId('project-settings-cancel-button'); - const saveButton = getByTestId('project-settings-save-button'); const actualInput = getInput(nameInput); await userEvent.clear(actualInput); + const saveButton = getByTestId('project-settings-save-button'); expect(saveButton).toBeDisabled(); await userEvent.type(actualInput, 'Valid Project Name'); expect(saveButton).toBeEnabled(); - expect(cancelButton).toBeEnabled(); + expect(getByTestId('project-settings-cancel-button')).toBeEnabled(); await userEvent.click(saveButton); await nextTick(); expect(updateSpy).toHaveBeenCalled(); - expect(cancelButton).toBeDisabled(); - expect(saveButton).toBeDisabled(); + // Buttons removed when clean again + expect(queryByTestId('project-settings-cancel-button')).toBeNull(); + expect(queryByTestId('project-settings-save-button')).toBeNull(); }); }); describe('Members table and role updates', () => { - it('adds a member and saves with telemetry', async () => { - const updateSpy = vi.spyOn(projectsStore, 'updateProject').mockResolvedValue(undefined); + it('adds a member immediately and emits telemetry', async () => { + const addSpy = vi.spyOn(projectsStore, 'addMember').mockResolvedValue(undefined); const { getByTestId } = renderComponent(); await nextTick(); @@ -428,14 +429,8 @@ describe('ProjectSettings', () => { emitters.n8nUserSelect.emit('update:model-value', '2'); await nextTick(); expect(getByTestId('members-count').textContent).toBe('2'); - - // Ensure form valid + dirty; name keystroke triggers validate and enables save - const nameInput = getByTestId('project-settings-name-input'); - await userEvent.type(nameInput.querySelector('input')!, ' '); - await userEvent.click(getByTestId('project-settings-save-button')); - await nextTick(); - - expect(updateSpy).toHaveBeenCalled(); + expect(addSpy).toHaveBeenCalledWith('123', expect.objectContaining({ userId: '2' })); + expect(mockShowMessage).toHaveBeenCalled(); expect(mockTrack).toHaveBeenCalledWith( 'User added member to project', expect.objectContaining({ project_id: '123', target_user_id: '2' }), @@ -460,20 +455,13 @@ describe('ProjectSettings', () => { }); it('inline role change saves immediately with telemetry', async () => { - projectsStore.updateProject.mockResolvedValue(undefined); + const roleSpy = vi.spyOn(projectsStore, 'updateMemberRole').mockResolvedValue(undefined); renderComponent(); await nextTick(); emitters.projectMembersTable.emit('update:role', { userId: '1', role: 'project:editor' }); await nextTick(); - expect(projectsStore.updateProject).toHaveBeenCalledWith( - '123', - expect.objectContaining({ - relations: expect.arrayContaining([ - expect.objectContaining({ userId: '1', role: 'project:editor' }), - ]), - }), - ); + expect(roleSpy).toHaveBeenCalledWith('123', '1', 'project:editor'); expect(mockShowMessage).toHaveBeenCalled(); expect(mockTrack).toHaveBeenCalledWith( 'User changed member role on project', @@ -483,26 +471,28 @@ describe('ProjectSettings', () => { it('rolls back role on save error', async () => { // First, inline update fails and rolls back - projectsStore.updateProject.mockRejectedValueOnce(new Error('fail')); - const utils = renderComponent(); + vi.spyOn(projectsStore, 'updateMemberRole').mockRejectedValueOnce(new Error('fail')); + const wrapper = renderComponent(); await nextTick(); emitters.projectMembersTable.emit('update:role', { userId: '1', role: 'project:viewer' }); await nextTick(); expect(mockShowError).toHaveBeenCalled(); - // Next form save should contain original role (admin) due to rollback - const nameInput = utils.getByTestId('project-settings-name-input'); + // Next form save persists only name/description and succeeds + const nameInput = wrapper.getByTestId('project-settings-name-input'); await userEvent.type(getInput(nameInput), ' touch'); - await userEvent.click(utils.getByTestId('project-settings-save-button')); + await userEvent.click(wrapper.getByTestId('project-settings-save-button')); await nextTick(); const lastCall = projectsStore.updateProject.mock.calls.pop(); - expect(lastCall?.[1].relations).toEqual( - expect.arrayContaining([expect.objectContaining({ userId: '1', role: 'project:admin' })]), + const payload = lastCall?.[1]; + expect(payload).toEqual( + expect.objectContaining({ name: expect.any(String), description: expect.any(String) }), ); + expect(payload).not.toHaveProperty('relations'); }); it('removes member immediately and shows success toast with telemetry', async () => { - const updateSpy = vi.spyOn(projectsStore, 'updateProject').mockResolvedValue(undefined); + const removeSpy = vi.spyOn(projectsStore, 'removeMember').mockResolvedValue(undefined); const { getByTestId, queryByTestId } = renderComponent(); await nextTick(); expect(getByTestId('members-count').textContent).toBe('1'); @@ -513,44 +503,45 @@ describe('ProjectSettings', () => { // Members list container should now be hidden expect(queryByTestId('members-count')).toBeNull(); - expect(updateSpy).toHaveBeenCalled(); - const payload = updateSpy.mock.calls[0][1]; - expect(payload.relations).toEqual([]); + expect(removeSpy).toHaveBeenCalledWith('123', '1'); expect(mockShowMessage).toHaveBeenCalled(); expect(mockTrack).toHaveBeenCalledWith( 'User removed member from project', expect.objectContaining({ project_id: '123', target_user_id: '1' }), ); - // Save should not re-add removed user - await userEvent.click(getByTestId('project-settings-save-button')); - await nextTick(); - expect(queryByTestId('members-count')).toBeNull(); + // Save button is not shown (no form edits) + expect(queryByTestId('project-settings-save-button')).toBeNull(); }); - it('prevents saving when invalid role selected', async () => { - // Set invalid role and try to save - const utils = renderComponent(); + it('saves only name and description with Save button', async () => { + const wrapper = renderComponent(); await nextTick(); - emitters.projectMembersTable.emit('update:role', { - userId: '1', - role: 'project:personalOwner', - }); + + // Edit name and description + const nameInput = wrapper.getByTestId('project-settings-name-input'); + await userEvent.type(nameInput.querySelector('input')!, ' New'); + const descInput = wrapper.getByTestId('project-settings-description-input'); + await userEvent.type(descInput.querySelector('textarea')!, 'New description'); + + // Save + await userEvent.click(wrapper.getByTestId('project-settings-save-button')); await nextTick(); - // Clear prior success toast from inline update (if any) - mockShowMessage.mockClear(); - // Mark form dirty so save is enabled - const nameInput = utils.getByTestId('project-settings-name-input'); - await userEvent.type(nameInput.querySelector('input')!, ' '); - await userEvent.click(utils.getByTestId('project-settings-save-button')); - await nextTick(); - expect(mockShowError).toHaveBeenCalled(); - // Should not show success on invalid role - expect(mockShowMessage).not.toHaveBeenCalled(); + + expect(projectsStore.updateProject).toHaveBeenCalled(); + const lastCall = projectsStore.updateProject.mock.calls.pop(); + const [id, payload] = lastCall as unknown as [string, Record]; + expect(id).toBe('123'); + expect(payload).toEqual( + expect.objectContaining({ name: expect.any(String), description: expect.any(String) }), + ); + expect(payload).not.toHaveProperty('relations'); + expect(payload).not.toHaveProperty('icon'); + expect(mockShowMessage).toHaveBeenCalled(); }); it('resets pagination to first page on search', async () => { - const utils = renderComponent(); + const wrapper = renderComponent(); await nextTick(); emitters.projectMembersTable.emit('update:options', { page: 2, @@ -558,14 +549,14 @@ describe('ProjectSettings', () => { sortBy: [], }); await nextTick(); - const searchContainer = utils.getByTestId('project-members-search'); + const searchContainer = wrapper.getByTestId('project-members-search'); const searchInput = searchContainer.querySelector('input')!; await userEvent.type(searchInput, 'john'); await new Promise((r) => setTimeout(r, 350)); // unmount first to avoid duplicate elements - utils.unmount(); - const utils2 = renderComponent(); - expect(utils2.getByTestId('members-page').textContent).toBe('0'); + wrapper.unmount(); + const wrapper2 = renderComponent(); + expect(wrapper2.getByTestId('members-page').textContent).toBe('0'); }); }); diff --git a/packages/frontend/editor-ui/src/views/ProjectSettings.vue b/packages/frontend/editor-ui/src/views/ProjectSettings.vue index 3020104d090..9a945cdf86d 100644 --- a/packages/frontend/editor-ui/src/views/ProjectSettings.vue +++ b/packages/frontend/editor-ui/src/views/ProjectSettings.vue @@ -6,7 +6,6 @@ import { deepCopy } from 'n8n-workflow'; import { N8nFormInput, N8nInput } from '@n8n/design-system'; import { useDebounceFn } from '@vueuse/core'; import { useUsersStore } from '@/stores/users.store'; -import type { IUser } from '@n8n/rest-api-client/api/users'; import { useI18n } from '@n8n/i18n'; import { type ResourceCounts, useProjectsStore } from '@/stores/projects.store'; import type { Project, ProjectRelation } from '@/types/projects.types'; @@ -44,6 +43,10 @@ const router = useRouter(); const telemetry = useTelemetry(); const documentTitle = useDocumentTitle(); +const showSaveError = (error: Error) => { + toast.showError(error, i18n.baseText('projects.settings.save.error.title')); +}; + const dialogVisible = ref(false); const upgradeDialogVisible = ref(false); @@ -86,10 +89,8 @@ const membersTableState = ref({ }); const usersList = computed(() => - usersStore.allUsers.filter((user: IUser) => { - const isAlreadySharedWithUser = (formData.value.relations || []).find( - (r: ProjectRelation) => r.id === user.id, - ); + usersStore.allUsers.filter((user) => { + const isAlreadySharedWithUser = (formData.value.relations || []).find((r) => r.id === user.id); return !isAlreadySharedWithUser; }), @@ -117,19 +118,36 @@ const projectMembersActions = computed>>(() }, ]); -const onAddMember = (userId: string) => { - isDirty.value = true; +const onAddMember = async (userId: string) => { + if (!projectsStore.currentProject) return; const user = usersStore.usersById[userId]; if (!user) return; - const { id, firstName, lastName, email } = user; - const relation = { id, firstName, lastName, email } as ProjectRelation; + const role = firstLicensedRole.value; + if (!role) return; - if (firstLicensedRole.value) { - relation.role = firstLicensedRole.value; + // Optimistically update UI + if (!formData.value.relations.find((r) => r.id === userId)) { + formData.value.relations.push({ id: userId, role }); } - formData.value.relations.push(relation); + try { + suppressNextSync.value = true; + await projectsStore.addMember(projectsStore.currentProject.id, { userId, role }); + toast.showMessage({ + type: 'success', + title: i18n.baseText('projects.settings.member.added.title'), + }); + telemetry.track('User added member to project', { + project_id: projectsStore.currentProject.id, + target_user_id: userId, + role, + }); + } catch (error) { + // Rollback optimistic change + formData.value.relations = formData.value.relations.filter((r) => r.id !== userId); + showSaveError(error); + } }; const onUpdateMemberRole = async ({ userId, role }: { userId: string; role: ProjectRole }) => { @@ -149,13 +167,8 @@ const onUpdateMemberRole = async ({ userId, role }: { userId: string; role: Proj formData.value.relations[memberIndex].role = role; try { - await projectsStore.updateProject(projectsStore.currentProject.id, { - relations: formData.value.relations.map((r: ProjectRelation) => ({ - userId: r.id, - role: r.role, - })), - }); - + suppressNextSync.value = true; + await projectsStore.updateMemberRole(projectsStore.currentProject.id, userId, role); toast.showMessage({ type: 'success', title: i18n.baseText('projects.settings.memberRole.updated.title'), @@ -193,11 +206,7 @@ async function onRemoveMember(userId: string) { try { // Prevent next sync from wiping unsaved edits suppressNextSync.value = true; - await projectsStore.updateProject(current.id, { - relations: current.relations - .filter((r) => r.id !== userId) - .map((r) => ({ userId: r.id, role: r.role })), - }); + await projectsStore.removeMember(current.id, userId); toast.showMessage({ type: 'success', title: i18n.baseText('projects.settings.member.removed.title'), @@ -208,7 +217,7 @@ async function onRemoveMember(userId: string) { }); } catch (error) { formData.value.relations.splice(idx, 0, removed); - toast.showError(error, i18n.baseText('projects.settings.save.error.title')); + showSaveError(error); } } @@ -223,12 +232,16 @@ const onMembersListAction = async ({ action, userId }: { action: string; userId: } }; -const onCancel = () => { +const resetFormData = () => { formData.value.relations = projectsStore.currentProject?.relations ? deepCopy(projectsStore.currentProject.relations) : []; formData.value.name = projectsStore.currentProject?.name ?? ''; formData.value.description = projectsStore.currentProject?.description ?? ''; +}; + +const onCancel = () => { + resetFormData(); isDirty.value = false; }; @@ -248,14 +261,14 @@ const makeFormDataDiff = (): FormDataDiff => { if (formData.value.relations.length !== projectsStore.currentProject.relations.length) { diff.memberAdded = formData.value.relations.filter( - (r: ProjectRelation) => !projectsStore.currentProject?.relations.find((cr) => cr.id === r.id), + (r) => !projectsStore.currentProject?.relations.find((cr) => cr.id === r.id), ); diff.memberRemoved = projectsStore.currentProject.relations.filter( - (cr: ProjectRelation) => !formData.value.relations.find((r) => r.id === cr.id), + (cr) => !formData.value.relations.find((r) => r.id === cr.id), ); } - diff.role = formData.value.relations.filter((r: ProjectRelation) => { + diff.role = formData.value.relations.filter((r) => { const currentRelation = projectsStore.currentProject?.relations.find((cr) => cr.id === r.id); return currentRelation?.role !== r.role && !diff.memberAdded?.find((ar) => ar.id === r.id); }); @@ -264,41 +277,34 @@ const makeFormDataDiff = (): FormDataDiff => { }; const sendTelemetry = (diff: FormDataDiff) => { + const projectId = projectsStore.currentProject?.id; + if (diff.name) { - telemetry.track('User changed project name', { - project_id: projectsStore.currentProject?.id, - name: diff.name, - }); + telemetry.track('User changed project name', { project_id: projectId, name: diff.name }); } - if (diff.memberAdded) { - diff.memberAdded.forEach((r) => { - telemetry.track('User added member to project', { - project_id: projectsStore.currentProject?.id, - target_user_id: r.id, - role: r.role, - }); + diff.memberAdded?.forEach((r) => { + telemetry.track('User added member to project', { + project_id: projectId, + target_user_id: r.id, + role: r.role, }); - } + }); - if (diff.memberRemoved) { - diff.memberRemoved.forEach((r) => { - telemetry.track('User removed member from project', { - project_id: projectsStore.currentProject?.id, - target_user_id: r.id, - }); + diff.memberRemoved?.forEach((r) => { + telemetry.track('User removed member from project', { + project_id: projectId, + target_user_id: r.id, }); - } + }); - if (diff.role) { - diff.role.forEach((r) => { - telemetry.track('User changed member role on project', { - project_id: projectsStore.currentProject?.id, - target_user_id: r.id, - role: r.role, - }); + diff.role?.forEach((r) => { + telemetry.track('User changed member role on project', { + project_id: projectId, + target_user_id: r.id, + role: r.role, }); - } + }); }; const updateProject = async () => { @@ -306,22 +312,13 @@ const updateProject = async () => { return; } try { - if (formData.value.relations.some((r) => r.role === 'project:personalOwner')) { - throw new Error('Invalid role selected for this project.'); - } - await projectsStore.updateProject(projectsStore.currentProject.id, { name: formData.value.name ?? '', - icon: projectIcon.value, description: formData.value.description ?? '', - relations: formData.value.relations.map((r: ProjectRelation) => ({ - userId: r.id, - role: r.role, - })), }); isDirty.value = false; } catch (error) { - toast.showError(error, i18n.baseText('projects.settings.save.error.title')); + showSaveError(error); throw error; } }; @@ -383,11 +380,18 @@ const selectProjectNameIfMatchesDefault = () => { }; const onIconUpdated = async () => { - await updateProject(); - toast.showMessage({ - title: i18n.baseText('projects.settings.icon.update.successful.title'), - type: 'success', - }); + if (!projectsStore.currentProject) return; + try { + await projectsStore.updateProject(projectsStore.currentProject.id, { + icon: projectIcon.value, + }); + toast.showMessage({ + title: i18n.baseText('projects.settings.icon.update.successful.title'), + type: 'success', + }); + } catch (error) { + showSaveError(error); + } }; // Skip one sync after targeted updates (e.g. removal) to preserve unsaved edits @@ -398,11 +402,7 @@ watch( suppressNextSync.value = false; return; } - formData.value.name = projectsStore.currentProject?.name ?? ''; - formData.value.description = projectsStore.currentProject?.description ?? ''; - formData.value.relations = projectsStore.currentProject?.relations - ? deepCopy(projectsStore.currentProject.relations) - : []; + resetFormData(); await nextTick(); selectProjectNameIfMatchesDefault(); if (projectsStore.currentProject?.icon && isIconOrEmoji(projectsStore.currentProject.icon)) { @@ -415,24 +415,17 @@ watch( // Add users property to the relation objects, // So that the table has access to the full user data const relationUsers = computed(() => - formData.value.relations.map((relation: ProjectRelation) => { + formData.value.relations.map((relation) => { const user = usersStore.usersById[relation.id]; - // Ensure type safety for UI display while preserving original role in formData - const safeRole: ProjectRole = isProjectRole(relation.role) ? relation.role : 'project:viewer'; + const safeRole = isProjectRole(relation.role) ? relation.role : 'project:viewer'; - if (!user) { - return { - ...relation, - role: safeRole, - firstName: null, - lastName: null, - email: null, - }; - } return { ...user, ...relation, role: safeRole, + firstName: user?.firstName ?? null, + lastName: user?.lastName ?? null, + email: user?.email ?? null, }; }), ); @@ -443,19 +436,16 @@ const membersTableData = computed(() => ({ })); const filteredMembersData = computed(() => { - if (!search.value.trim()) { - return membersTableData.value; - } + if (!search.value.trim()) return membersTableData.value; + const searchTerm = search.value.toLowerCase(); const filtered = relationUsers.value.filter((member) => { - const fullName = `${member.firstName || ''} ${member.lastName || ''}`.toLowerCase(); - const email = (member.email || '').toLowerCase(); + const fullName = `${member.firstName ?? ''} ${member.lastName ?? ''}`.toLowerCase(); + const email = (member.email ?? '').toLowerCase(); return fullName.includes(searchTerm) || email.includes(searchTerm); }); - return { - items: filtered, - count: filtered.length, - }; + + return { items: filtered, count: filtered.length }; }); const debouncedSearch = useDebounceFn(() => { @@ -485,11 +475,14 @@ onMounted(() => {
+ + {{ i18n.baseText('projects.settings.info') }} +
-
+
{ name="name" required data-test-id="project-settings-name-input" - :class="$style['project-name-input']" + :class="$style.projectNameInput" @enter="onSubmit" @input="onTextInput" @validate="isValid = $event" @@ -522,15 +515,42 @@ onMounted(() => { :maxlength="512" :autosize="true" data-test-id="project-settings-description-input" + :class="$style.projectDescriptionInput" @enter="onSubmit" @input="onTextInput" @validate="isValid = $event" />
+
+
+ {{ + i18n.baseText('projects.settings.message.unsavedChanges') + }} + {{ i18n.baseText('projects.settings.button.cancel') }} +
+ {{ i18n.baseText('projects.settings.button.save') }} +
- +

+ +

{ />
-
-
- {{ - i18n.baseText('projects.settings.message.unsavedChanges') - }} - {{ i18n.baseText('projects.settings.button.cancel') }} -
- {{ i18n.baseText('projects.settings.button.save') }} -
-

{{ i18n.baseText('projects.settings.danger.title') }}

{{ i18n.baseText('projects.settings.danger.message') }}
@@ -623,6 +620,8 @@ onMounted(() => { diff --git a/packages/testing/playwright/tests/ui/39-projects.spec.ts b/packages/testing/playwright/tests/ui/39-projects.spec.ts index 9ea3a7a304b..083512e2c0e 100644 --- a/packages/testing/playwright/tests/ui/39-projects.spec.ts +++ b/packages/testing/playwright/tests/ui/39-projects.spec.ts @@ -123,9 +123,11 @@ test.describe('Projects', () => { // Initially should have only the owner (current user) await n8n.projectSettings.expectTableHasMemberCount(1); - // Verify project settings action buttons are present - await expect(n8n.page.getByTestId('project-settings-save-button')).toBeVisible(); - await expect(n8n.page.getByTestId('project-settings-cancel-button')).toBeVisible(); + // Verify save/cancel buttons are not visible initially (no changes) + await expect(n8n.page.getByTestId('project-settings-save-button')).not.toBeVisible(); + await expect(n8n.page.getByTestId('project-settings-cancel-button')).not.toBeVisible(); + + // Delete button should always be visible await expect(n8n.page.getByTestId('project-settings-delete-button')).toBeVisible(); }); @@ -254,9 +256,9 @@ test.describe('Projects', () => { await n8n.page.goto(`/projects/${projectId}/settings`); await expect(n8n.projectSettings.getTitle()).toHaveText('Unsaved Changes Test'); - // Initially, save and cancel buttons should be disabled (no changes) - await expect(n8n.page.getByTestId('project-settings-save-button')).toBeDisabled(); - await expect(n8n.page.getByTestId('project-settings-cancel-button')).toBeDisabled(); + // Initially, save and cancel buttons should not be visible (no changes) + await expect(n8n.page.getByTestId('project-settings-save-button')).not.toBeVisible(); + await expect(n8n.page.getByTestId('project-settings-cancel-button')).not.toBeVisible(); // Make a change to the project name await n8n.projectSettings.fillProjectName('Modified Name'); @@ -271,9 +273,9 @@ test.describe('Projects', () => { // Cancel changes await n8n.projectSettings.clickCancelButton(); - // Buttons should be disabled again - await expect(n8n.page.getByTestId('project-settings-save-button')).toBeDisabled(); - await expect(n8n.page.getByTestId('project-settings-cancel-button')).toBeDisabled(); + // Buttons should not be visible again (no changes) + await expect(n8n.page.getByTestId('project-settings-save-button')).not.toBeVisible(); + await expect(n8n.page.getByTestId('project-settings-cancel-button')).not.toBeVisible(); }); test('should display delete project section with warning @auth:owner', async ({ n8n }) => {