diff --git a/api.planx.uk/docs/index.ts b/api.planx.uk/docs/index.ts index 7ecc2744c1..a334ba41ad 100644 --- a/api.planx.uk/docs/index.ts +++ b/api.planx.uk/docs/index.ts @@ -96,7 +96,7 @@ const responses = { schema: { type: "object", properties: { - message: { + error: { type: "string", example: "Error!", }, diff --git a/api.planx.uk/modules/team/index.test.ts b/api.planx.uk/modules/team/controller.test.ts similarity index 68% rename from api.planx.uk/modules/team/index.test.ts rename to api.planx.uk/modules/team/controller.test.ts index 6edd6da877..13a50c208b 100644 --- a/api.planx.uk/modules/team/index.test.ts +++ b/api.planx.uk/modules/team/controller.test.ts @@ -6,26 +6,20 @@ const mockAddMember = jest.fn(); const mockRemoveMember = jest.fn(); const mockChangeMemberRole = jest.fn(); -jest.mock("@opensystemslab/planx-core", () => { - return { - CoreDomainClient: jest.fn().mockImplementation(() => ({ - team: { - addMember: () => mockAddMember(), - removeMember: () => mockRemoveMember(), - changeMemberRole: () => mockChangeMemberRole(), - }, - })), - }; -}); +jest.mock("./service", () => ({ + addMember: () => mockAddMember(), + changeMemberRole: () => mockChangeMemberRole(), + removeMember: () => mockRemoveMember(), +})); const auth = authHeader({ role: "platformAdmin" }); describe("Adding a user to a team", () => { it("requires authentication", async () => { await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamViewer", }) .expect(401); @@ -33,10 +27,10 @@ describe("Adding a user to a team", () => { it("requires the 'platformAdmin' role", async () => { await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(authHeader({ role: "teamEditor" })) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamViewer", }) .expect(403); @@ -44,7 +38,7 @@ describe("Adding a user to a team", () => { it("validates that userId is required", async () => { await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(auth) .send({ role: "teamViewer", @@ -58,10 +52,10 @@ describe("Adding a user to a team", () => { it("validates that role is required", async () => { await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", }) .expect(400) .then((res) => { @@ -72,10 +66,10 @@ describe("Adding a user to a team", () => { it("validates that role must be an accepted value", async () => { await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "pirate", }) .expect(400) @@ -85,32 +79,30 @@ describe("Adding a user to a team", () => { }); }); - it("handles Hasura / DB errors", async () => { - mockAddMember.mockResolvedValue(false); + it("handles an error thrown in the service", async () => { + mockAddMember.mockRejectedValueOnce("Something went wrong in the service"); await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(500) .then((res) => { expect(mockAddMember).toHaveBeenCalled(); - expect(res.body).toHaveProperty("message"); - expect(res.body.message).toMatch(/Failed to add member to team/); + expect(res.body).toHaveProperty("error"); + expect(res.body.error).toMatch(/Failed to add member to team/); }); }); it("can successfully add a team member", async () => { - mockAddMember.mockResolvedValue(true); - await supertest(app) - .put("/team/123/add-member") + .put("/team/council/add-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(200) @@ -121,20 +113,19 @@ describe("Adding a user to a team", () => { }); }); }); - describe("Removing a user from a team", () => { it("requires authentication", async () => { await supertest(app) - .delete("/team/123/remove-member") + .delete("/team/council/remove-member") .send({ - userId: 123, + userEmail: "newbie@council.gov", }) .expect(401); }); it("validates that userId is required", async () => { await supertest(app) - .delete("/team/123/remove-member") + .delete("/team/council/remove-member") .set(auth) .send({}) .expect(400) @@ -144,31 +135,31 @@ describe("Removing a user from a team", () => { }); }); - it("handles Hasura / DB errors", async () => { - mockRemoveMember.mockResolvedValue(false); + it("handles an error thrown in the service", async () => { + mockRemoveMember.mockRejectedValueOnce( + "Something went wrong in the service", + ); await supertest(app) - .delete("/team/123/remove-member") + .delete("/team/council/remove-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", }) .expect(500) .then((res) => { expect(mockRemoveMember).toHaveBeenCalled(); - expect(res.body).toHaveProperty("message"); - expect(res.body.message).toMatch(/Failed to remove member from team/); + expect(res.body).toHaveProperty("error"); + expect(res.body.error).toMatch(/Failed to remove member from team/); }); }); it("can successfully remove a team member", async () => { - mockRemoveMember.mockResolvedValue(true); - await supertest(app) - .delete("/team/123/remove-member") + .delete("/team/council/remove-member") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(200) @@ -183,9 +174,9 @@ describe("Removing a user from a team", () => { describe("Changing a user's role", () => { it("requires authentication", async () => { await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(401); @@ -193,7 +184,7 @@ describe("Changing a user's role", () => { it("validates that userId is required", async () => { await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .set(auth) .send({ role: "teamEditor", @@ -207,10 +198,10 @@ describe("Changing a user's role", () => { it("validates that role is required", async () => { await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", }) .expect(400) .then((res) => { @@ -221,10 +212,10 @@ describe("Changing a user's role", () => { it("validates that role is an accepted value", async () => { await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "professor", }) .expect(400) @@ -234,32 +225,32 @@ describe("Changing a user's role", () => { }); }); - it("handles Hasura / DB errors", async () => { - mockChangeMemberRole.mockResolvedValue(false); + it("handles an error thrown in the service", async () => { + mockChangeMemberRole.mockRejectedValueOnce( + "Something went wrong in the service", + ); await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(500) .then((res) => { expect(mockChangeMemberRole).toHaveBeenCalled(); - expect(res.body).toHaveProperty("message"); - expect(res.body.message).toMatch(/Failed to change role/); + expect(res.body).toHaveProperty("error"); + expect(res.body.error).toMatch(/Failed to change role/); }); }); it("can successfully change a user's role", async () => { - mockChangeMemberRole.mockResolvedValue(true); - await supertest(app) - .patch("/team/123/change-member-role") + .patch("/team/council/change-member-role") .set(auth) .send({ - userId: 123, + userEmail: "newbie@council.gov", role: "teamEditor", }) .expect(200) diff --git a/api.planx.uk/modules/team/controller.ts b/api.planx.uk/modules/team/controller.ts index 03d12aa738..4c12cf6e07 100644 --- a/api.planx.uk/modules/team/controller.ts +++ b/api.planx.uk/modules/team/controller.ts @@ -1,6 +1,7 @@ +import * as Service from "./service"; import { z } from "zod"; -import { getClient } from "../../client"; import { ValidatedRequestHandler } from "../../shared/middleware/validate"; +import { ServerError } from "../../errors"; interface TeamMemberResponse { message: string; @@ -8,10 +9,10 @@ interface TeamMemberResponse { export const upsertMemberSchema = z.object({ params: z.object({ - teamId: z.string().transform((val) => parseInt(val)), + teamSlug: z.string().toLowerCase(), }), body: z.object({ - userId: z.number(), + userEmail: z.string().email(), role: z.enum(["teamEditor", "teamViewer"]), }), }); @@ -23,10 +24,10 @@ export type UpsertMember = ValidatedRequestHandler< export const removeMemberSchema = z.object({ params: z.object({ - teamId: z.string().transform((val) => parseInt(val)), + teamSlug: z.string().toLowerCase(), }), body: z.object({ - userId: z.number(), + userEmail: z.string().email(), }), }); @@ -35,47 +36,50 @@ export type RemoveMember = ValidatedRequestHandler< TeamMemberResponse >; -export const addMember: UpsertMember = async (req, res) => { - const { teamId } = req.params; - const { userId, role } = req.body; - - const $client = getClient(); - const isSuccess = await $client.team.addMember({ teamId, userId, role }); - - if (!isSuccess) - return res.status(500).json({ message: "Failed to add member to team" }); - - res.send({ message: "Successfully added user to team" }); +export const addMember: UpsertMember = async (req, res, next) => { + const { teamSlug } = req.params; + const { userEmail, role } = req.body; + + try { + await Service.addMember({ userEmail, teamSlug, role }); + return res.send({ message: "Successfully added user to team" }); + } catch (error) { + return next( + new ServerError({ + message: "Failed to add member to team", + cause: error, + }), + ); + } }; -export const changeMemberRole: UpsertMember = async (req, res) => { - const { teamId } = req.params; - const { userId, role } = req.body; - - const $client = getClient(); - const isSuccess = await $client.team.changeMemberRole({ - teamId, - userId, - role, - }); - - if (!isSuccess) - return res.status(500).json({ message: "Failed to change role" }); - - res.send({ message: "Successfully changed role" }); +export const changeMemberRole: UpsertMember = async (req, res, next) => { + const { teamSlug } = req.params; + const { userEmail, role } = req.body; + + try { + await Service.changeMemberRole({ userEmail, teamSlug, role }); + return res.send({ message: "Successfully changed role" }); + } catch (error) { + return next( + new ServerError({ message: "Failed to change role", cause: error }), + ); + } }; -export const removeMember: RemoveMember = async (req, res) => { - const { teamId } = req.params; - const { userId } = req.body; - - const $client = getClient(); - const isSuccess = await $client.team.removeMember({ teamId, userId }); - - if (!isSuccess) - return res - .status(500) - .json({ message: "Failed to remove member from team" }); - - res.send({ message: "Successfully removed user from team" }); +export const removeMember: RemoveMember = async (req, res, next) => { + const { teamSlug } = req.params; + const { userEmail } = req.body; + + try { + await Service.removeMember({ userEmail, teamSlug }); + return res.send({ message: "Successfully removed user from team" }); + } catch (error) { + return next( + new ServerError({ + message: "Failed to remove member from team", + cause: error, + }), + ); + } }; diff --git a/api.planx.uk/modules/team/docs.yaml b/api.planx.uk/modules/team/docs.yaml index 50fd6e99cd..4007fdb016 100644 --- a/api.planx.uk/modules/team/docs.yaml +++ b/api.planx.uk/modules/team/docs.yaml @@ -7,23 +7,23 @@ tags: description: Team and Team Member related requests components: parameters: - teamId: + teamSlug: in: path - name: teamId - type: integer + name: teamSlug + type: string required: true schemas: UpsertMember: type: object properties: - userId: - type: number - example: 123 + userEmail: + type: email + example: einstein@princeton.edu role: type: string enum: ["teamViewer", "teamEditor"] paths: - /team/{teamId}/add-member: + /team/{teamSlug}/add-member: put: summary: Add user to team, and assign them a role description: "Requires authentication via a Cloudflare WARP client @@ -31,7 +31,7 @@ paths: Please login at [https://api.editor.planx.uk/team](https://api.editor.planx.uk/team){:target='_blank'}" tags: ["team"] parameters: - - $ref: "#/components/parameters/teamId" + - $ref: "#/components/parameters/teamSlug" requestBody: required: true content: @@ -43,7 +43,7 @@ paths: $ref: "#/components/responses/SuccessMessage" "500": $ref: "#/components/responses/ErrorMessage" - /team/{teamId}/change-member-role: + /team/{teamSlug}/change-member-role: patch: summary: Change role of an existing team member description: "Requires authentication via a Cloudflare WARP client @@ -51,7 +51,7 @@ paths: Please login at [https://api.editor.planx.uk/team](https://api.editor.planx.uk/team){:target='_blank'}" tags: ["team"] parameters: - - $ref: "#/components/parameters/teamId" + - $ref: "#/components/parameters/teamSlug" requestBody: required: true content: @@ -63,7 +63,7 @@ paths: $ref: "#/components/responses/SuccessMessage" "500": $ref: "#/components/responses/ErrorMessage" - /team/{teamId}/remove-member: + /team/{teamSlug}/remove-member: delete: summary: Remover user from team description: "Requires authentication via a Cloudflare WARP client @@ -71,7 +71,7 @@ paths: Please login at [https://api.editor.planx.uk/team](https://api.editor.planx.uk/team){:target='_blank'}" tags: ["team"] parameters: - - $ref: "#/components/parameters/teamId" + - $ref: "#/components/parameters/teamSlug" requestBody: required: true content: @@ -79,9 +79,9 @@ paths: schema: type: object properties: - userId: - type: integer - example: 123 + userEmail: + type: email + example: einstein@princeton.edu responses: "200": $ref: "#/components/responses/SuccessMessage" diff --git a/api.planx.uk/modules/team/routes.ts b/api.planx.uk/modules/team/routes.ts index 8d2c54c872..08ff00ea5b 100644 --- a/api.planx.uk/modules/team/routes.ts +++ b/api.planx.uk/modules/team/routes.ts @@ -7,17 +7,17 @@ const router = Router(); router.use(AuthMiddleware.usePlatformAdminAuth); router.put( - "/:teamId/add-member", + "/:teamSlug/add-member", validate(Controller.upsertMemberSchema), Controller.addMember, ); router.patch( - "/:teamId/change-member-role", + "/:teamSlug/change-member-role", validate(Controller.upsertMemberSchema), Controller.changeMemberRole, ); router.delete( - "/:teamId/remove-member", + "/:teamSlug/remove-member", validate(Controller.removeMemberSchema), Controller.removeMember, ); diff --git a/api.planx.uk/modules/team/service.test.ts b/api.planx.uk/modules/team/service.test.ts new file mode 100644 index 0000000000..34d0b4cca3 --- /dev/null +++ b/api.planx.uk/modules/team/service.test.ts @@ -0,0 +1,108 @@ +import { getJWT } from "../../tests/mockJWT"; +import { userContext } from "../auth/middleware"; + +import { + getUserAndTeam, + addMember, + removeMember, + changeMemberRole, +} from "./service"; + +const getStoreMock = jest.spyOn(userContext, "getStore"); + +const mockTeam = { id: 123 }; +const mockUser = { id: 456 }; + +const mockAddMember = jest.fn(); +const mockRemoveMember = jest.fn(); +const mockChangeMemberRole = jest.fn(); +const mockGetTeamBySlug = jest.fn().mockResolvedValue(mockTeam); +const mockGetUserByEmail = jest.fn().mockResolvedValue(mockUser); + +jest.mock("@opensystemslab/planx-core", () => { + return { + CoreDomainClient: jest.fn().mockImplementation(() => ({ + team: { + getBySlug: () => mockGetTeamBySlug(), + addMember: () => mockAddMember(), + removeMember: () => mockRemoveMember(), + changeMemberRole: () => mockChangeMemberRole(), + }, + user: { + getByEmail: () => mockGetUserByEmail(), + }, + })), + }; +}); + +describe("TeamService", () => { + beforeEach(() => { + getStoreMock.mockReturnValue({ + user: { + sub: "123", + email: "test@opensystemslab.io", + jwt: getJWT({ role: "teamEditor" }), + }, + }); + }); + + describe("getUserAndTeam helper", () => { + it("handles invalid teamSlugs", async () => { + mockGetTeamBySlug.mockResolvedValueOnce(null); + await expect(() => + getUserAndTeam({ + userEmail: "newbie@council.gov", + teamSlug: "not-a-team", + }), + ).rejects.toThrow(); + }); + + it("handles invalid emails", async () => { + mockGetUserByEmail.mockResolvedValueOnce(null); + await expect(() => + getUserAndTeam({ + userEmail: "invalid-email@council.gov", + teamSlug: "a-real-team", + }), + ).rejects.toThrow(); + }); + }); + + describe("addMember", () => { + it("handles Hasura failures", async () => { + mockAddMember.mockResolvedValueOnce(false); + await expect(() => + addMember({ + userEmail: "newbie@council.gov", + role: "teamEditor", + teamSlug: "a-real-team", + }), + ).rejects.toThrow(); + }); + }); + + describe("removeMember", () => { + it("handles Hasura failures", async () => { + mockRemoveMember.mockResolvedValueOnce(false); + await expect(() => + removeMember({ + userEmail: "newbie@council.gov", + teamSlug: "a-real-team", + }), + ).rejects.toThrow(); + }); + }); + + describe("changeMemberRole", () => { + it("handles Hasura failures", async () => { + mockChangeMemberRole.mockResolvedValueOnce(false); + await expect(() => + changeMemberRole({ + userEmail: "newbie@council.gov", + role: "teamEditor", + teamSlug: "a-real-team", + }), + ).rejects.toThrow(); + }); + }); +}); diff --git a/api.planx.uk/modules/team/service.ts b/api.planx.uk/modules/team/service.ts new file mode 100644 index 0000000000..2e814cee49 --- /dev/null +++ b/api.planx.uk/modules/team/service.ts @@ -0,0 +1,74 @@ +import { getClient } from "../../client"; +import { TeamRole } from "@opensystemslab/planx-core/types"; + +interface UpsertMember { + userEmail: string; + teamSlug: string; + role: TeamRole; +} + +interface RemoveUser { + userEmail: string; + teamSlug: string; +} + +export const addMember = async ({ + userEmail, + teamSlug, + role, +}: UpsertMember) => { + const $client = getClient(); + const { user, team } = await getUserAndTeam({ userEmail, teamSlug }); + + const isSuccess = await $client.team.addMember({ + teamId: team.id, + userId: user.id, + role, + }); + if (!isSuccess) throw Error(`Failed to assign ${userEmail} to ${teamSlug}`); +}; + +export const changeMemberRole = async ({ + userEmail, + teamSlug, + role, +}: UpsertMember) => { + const $client = getClient(); + const { user, team } = await getUserAndTeam({ userEmail, teamSlug }); + + const isSuccess = await $client.team.changeMemberRole({ + teamId: team.id, + userId: user.id, + role, + }); + if (!isSuccess) + throw Error(`Failed to assign ${role} (${teamSlug}) to ${userEmail}`); +}; + +export const removeMember = async ({ userEmail, teamSlug }: RemoveUser) => { + const $client = getClient(); + const { user, team } = await getUserAndTeam({ userEmail, teamSlug }); + const isSuccess = await $client.team.removeMember({ + teamId: team.id, + userId: user.id, + }); + if (!isSuccess) throw Error(`Failed to remove ${userEmail} from ${teamSlug}`); +}; + +export const getUserAndTeam = async ({ + userEmail, + teamSlug, +}: { + userEmail: string; + teamSlug: string; +}) => { + const $client = getClient(); + + const team = await $client.team.getBySlug(teamSlug); + if (!team) throw Error(`Unable to find team matching slug ${teamSlug}`); + + const user = await $client.user.getByEmail(userEmail); + if (!user) throw Error(`Unable to find team matching email ${userEmail}`); + + return { team, user }; +}; diff --git a/api.planx.uk/modules/user/routes.ts b/api.planx.uk/modules/user/routes.ts index 7ddd39ee7d..c569a70308 100644 --- a/api.planx.uk/modules/user/routes.ts +++ b/api.planx.uk/modules/user/routes.ts @@ -6,6 +6,6 @@ import { createUserSchema, createUser } from "./controller"; const router = Router(); router.use(usePlatformAdminAuth); -router.put("/user", validate(createUserSchema), createUser); +router.put("/", validate(createUserSchema), createUser); export default router; diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 771e48312d..17ff987d3a 100644 --- a/api.planx.uk/server.ts +++ b/api.planx.uk/server.ts @@ -195,7 +195,7 @@ app.use(urlencoded({ extended: true })); app.use(authRoutes); app.use(miscRoutes); -app.use(userRoutes); +app.use("/user", userRoutes); app.use("/team", teamRoutes); app.use("/gis", router);