From 6d4da176d1250219b1e18b7373cb0d81eeeb1505 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Wed, 22 Jun 2022 11:09:20 +0200 Subject: [PATCH] chore: Move provisionSubdomain from Team model to teamCreator command --- server/commands/teamCreator.test.ts | 41 +++++++++++++++++++++++++++++ server/commands/teamCreator.ts | 41 +++++++++++++++++++++-------- server/models/Team.test.ts | 41 ----------------------------- server/models/Team.ts | 29 -------------------- 4 files changed, 71 insertions(+), 81 deletions(-) diff --git a/server/commands/teamCreator.test.ts b/server/commands/teamCreator.test.ts index a76657dae..a7c6ef706 100644 --- a/server/commands/teamCreator.test.ts +++ b/server/commands/teamCreator.test.ts @@ -26,6 +26,47 @@ describe("teamCreator", () => { expect(isNewTeam).toEqual(true); }); + it("should set subdomain append if unavailable", async () => { + env.DEPLOYMENT = "hosted"; + + await buildTeam({ + subdomain: "myteam", + }); + const result = await teamCreator({ + name: "Test team", + subdomain: "myteam", + avatarUrl: "http://example.com/logo.png", + authenticationProvider: { + name: "google", + providerId: "example.com", + }, + }); + + expect(result.team.subdomain).toEqual("myteam1"); + }); + + it("should increment subdomain append if unavailable", async () => { + env.DEPLOYMENT = "hosted"; + + await buildTeam({ + subdomain: "myteam", + }); + await buildTeam({ + subdomain: "myteam1", + }); + const result = await teamCreator({ + name: "Test team", + subdomain: "myteam", + avatarUrl: "http://example.com/logo.png", + authenticationProvider: { + name: "google", + providerId: "example.com", + }, + }); + + expect(result.team.subdomain).toEqual("myteam2"); + }); + describe("self hosted", () => { it("should not allow creating multiple teams in installation", async () => { env.DEPLOYMENT = undefined; diff --git a/server/commands/teamCreator.ts b/server/commands/teamCreator.ts index 8b76ef6bb..06dfabbaf 100644 --- a/server/commands/teamCreator.ts +++ b/server/commands/teamCreator.ts @@ -1,3 +1,4 @@ +import { sequelize } from "@server/database/sequelize"; import env from "@server/env"; import { DomainNotAllowedError, MaximumTeamsError } from "@server/errors"; import Logger from "@server/logging/Logger"; @@ -88,11 +89,8 @@ async function teamCreator({ }); } - const transaction = await Team.sequelize!.transaction(); - let team; - - try { - team = await Team.create( + const team = await sequelize.transaction(async (transaction) => { + return Team.create( { name, avatarUrl, @@ -103,14 +101,13 @@ async function teamCreator({ transaction, } ); - await transaction.commit(); - } catch (err) { - await transaction.rollback(); - throw err; - } + }); + // Note provisioning the subdomain is done outside of the transaction as + // it is allowed to fail and the team can still be created, it also requires + // failed queries as part of iteration try { - await team.provisionSubdomain(subdomain); + await provisionSubdomain(team, subdomain); } catch (err) { Logger.error("Provisioning subdomain failed", err, { teamId: team.id, @@ -125,6 +122,28 @@ async function teamCreator({ }; } +async function provisionSubdomain(team: Team, requestedSubdomain: string) { + if (team.subdomain) { + return team.subdomain; + } + let subdomain = requestedSubdomain; + let append = 0; + + for (;;) { + try { + await team.update({ + subdomain, + }); + break; + } catch (err) { + // subdomain was invalid or already used, try again + subdomain = `${requestedSubdomain}${++append}`; + } + } + + return subdomain; +} + export default APM.traceFunction({ serviceName: "command", spanName: "teamCreator", diff --git a/server/models/Team.test.ts b/server/models/Team.test.ts index e64f3eb13..055bfa02b 100644 --- a/server/models/Team.test.ts +++ b/server/models/Team.test.ts @@ -21,44 +21,3 @@ describe("collectionIds", () => { expect(response[0]).toEqual(collection.id); }); }); - -describe("provisionSubdomain", () => { - it("should set subdomain if available", async () => { - const team = await buildTeam(); - const subdomain = await team.provisionSubdomain("testy"); - expect(subdomain).toEqual("testy"); - expect(team.subdomain).toEqual("testy"); - }); - - it("should set subdomain append if unavailable", async () => { - await buildTeam({ - subdomain: "myteam", - }); - const team = await buildTeam(); - const subdomain = await team.provisionSubdomain("myteam"); - expect(subdomain).toEqual("myteam1"); - expect(team.subdomain).toEqual("myteam1"); - }); - - it("should increment subdomain append if unavailable", async () => { - await buildTeam({ - subdomain: "myteam", - }); - await buildTeam({ - subdomain: "myteam1", - }); - const team = await buildTeam(); - const subdomain = await team.provisionSubdomain("myteam"); - expect(subdomain).toEqual("myteam2"); - expect(team.subdomain).toEqual("myteam2"); - }); - - it("should do nothing if subdomain already set", async () => { - const team = await buildTeam({ - subdomain: "example", - }); - const subdomain = await team.provisionSubdomain("myteam"); - expect(subdomain).toEqual("example"); - expect(team.subdomain).toEqual("example"); - }); -}); diff --git a/server/models/Team.ts b/server/models/Team.ts index bf94eefc3..5e86088fb 100644 --- a/server/models/Team.ts +++ b/server/models/Team.ts @@ -146,35 +146,6 @@ class Team extends ParanoidModel { ); } - // TODO: Move to command - provisionSubdomain = async function ( - requestedSubdomain: string, - options = {} - ) { - if (this.subdomain) { - return this.subdomain; - } - let subdomain = requestedSubdomain; - let append = 0; - - for (;;) { - try { - await this.update( - { - subdomain, - }, - options - ); - break; - } catch (err) { - // subdomain was invalid or already used, try again - subdomain = `${requestedSubdomain}${++append}`; - } - } - - return subdomain; - }; - provisionFirstCollection = async (userId: string) => { await this.sequelize!.transaction(async (transaction) => { const collection = await Collection.create(