collapse default logo insertion to init organization handler

server/auth-realtime/src/bootstrap.test.ts

@@ -3,7 +3,7 @@ import {
 	bootstrapSingleOrganization,
 	createDefaultAdminLookup,
 } from "./bootstrap.js"
-import { stubCore, stubLog, stubStore, testEnv } from "./test-helpers.js"
+import { stubLog, stubStore, testEnv } from "./test-helpers.js"
 
 // a single-organization deployment with no organization and no admin yet,
 // unless a test says otherwise.
@@ -38,11 +38,10 @@ function deps(
 			maxOrganizations: overrides.maxOrganizations ?? 1,
 		}),
 		store,
-		core: stubCore(),
 		log: stubLog(),
 		accounts,
 		hashPassword: vi.fn().mockResolvedValue("hashed"),
-		createOrganization: vi.fn().mockResolvedValue({ id: "org-1" }),
+		createOrganization: vi.fn().mockResolvedValue({}),
 	}
 }
 
@@ -76,9 +75,6 @@ describe("bootstrapSingleOrganization", () => {
 			slug: "oxynote",
 			userId: "admin-1",
 		})
-		expect(d.core.setDefaultOrganizationLogo).toHaveBeenCalledWith(
-			"org-1",
-		)
 		expect.soft(d.log.warn).toHaveBeenCalledTimes(1)
 	})
 
@@ -115,33 +111,14 @@ describe("bootstrapSingleOrganization", () => {
 
 		expect.soft(d.accounts.findUserByEmail).not.toHaveBeenCalled()
 		expect.soft(d.createOrganization).not.toHaveBeenCalled()
-		expect.soft(
-			d.core.setDefaultOrganizationLogo,
-		).not.toHaveBeenCalled()
 		expect.soft(d.log.warn).not.toHaveBeenCalled()
 	})
 
-	it("propagates a failed organization creation before setting a logo", async ({
-		expect,
-	}) => {
+	it("propagates a failed organization creation", async ({ expect }) => {
 		const failure = new Error("core unreachable")
 		const d = deps()
 		d.createOrganization.mockRejectedValue(failure)
 
-		await expect(bootstrapSingleOrganization(d)).rejects.toBe(
-			failure,
-		)
-		expect.soft(
-			d.core.setDefaultOrganizationLogo,
-		).not.toHaveBeenCalled()
-		expect.soft(d.log.warn).not.toHaveBeenCalled()
-	})
-
-	it("propagates a failed logo upload", async ({ expect }) => {
-		const failure = new Error("core unreachable")
-		const d = deps()
-		d.core.setDefaultOrganizationLogo.mockRejectedValue(failure)
-
 		await expect(bootstrapSingleOrganization(d)).rejects.toBe(
 			failure,
 		)

server/auth-realtime/src/bootstrap.ts

@@ -1,4 +1,3 @@
-import type { CoreClient } from "./core.js"
 import type { Store } from "./db.js"
 import type { Env } from "./env.js"
 import type { Logger } from "./logging.js"
@@ -35,7 +34,6 @@ export interface AccountAdapter {
 export interface BootstrapDeps {
 	env: Env
 	store: Store
-	core: CoreClient
 	log: Logger
 	accounts: AccountAdapter
 	hashPassword: (password: string) => Promise<string>
@@ -45,7 +43,7 @@ export interface BootstrapDeps {
 		name: string
 		slug: string
 		userId: string
-	}) => Promise<{ id: string }>
+	}) => Promise<unknown>
 }
 
 // bootstrapSingleOrganization creates the organization and its admin when a
@@ -55,7 +53,6 @@ export interface BootstrapDeps {
 export async function bootstrapSingleOrganization({
 	env,
 	store,
-	core,
 	log,
 	accounts,
 	hashPassword,
@@ -94,13 +91,11 @@ export async function bootstrapSingleOrganization({
 		userId = user.id
 	}
 
-	const organization = await createOrganization({
+	await createOrganization({
 		...bootstrapOrganization,
 		userId,
 	})
 
-	await core.setDefaultOrganizationLogo(organization.id)
-
 	log.warn(
 		`created the ${bootstrapOrganization.name} organization. Sign in as ${bootstrapAdmin.email} with the default password and change both.`,
 	)

server/auth-realtime/src/core.test.ts

@@ -108,24 +108,6 @@ describe("createCoreClient", () => {
 		})
 	})
 
-	describe("setDefaultOrganizationLogo", () => {
-		it("puts to the organization's default logo endpoint", async ({
-			expect,
-		}) => {
-			const http = stubHttp()
-
-			await createCoreClient(
-				BASE_URL,
-				http,
-			).setDefaultOrganizationLogo("org-1")
-
-			expect(http.put).toHaveBeenCalledTimes(1)
-			expect(http.put).toHaveBeenCalledWith(
-				"http://core:8080/api/x/organizations/org-1/logo/default",
-			)
-		})
-	})
-
 	describe("fetchBranches", () => {
 		it("returns the branch list core responded with", async ({
 			expect,

server/auth-realtime/src/core.ts

@@ -93,7 +93,6 @@ export interface CoreClient {
 	): Promise<void>
 	initializeOrganization(organizationId: string): Promise<void>
 	teardownOrganization(organizationId: string): Promise<void>
-	setDefaultOrganizationLogo(organizationId: string): Promise<void>
 	fetchBranches(documentId: string): Promise<BranchSummary[]>
 	fetchBranchContent(
 		documentId: string,
@@ -158,12 +157,6 @@ export function createCoreClient(
 			)
 		},
 
-		async setDefaultOrganizationLogo(organizationId) {
-			await http.put(
-				`${internal}/organizations/${organizationId}/logo/default`,
-			)
-		},
-
 		async fetchBranches(documentId) {
 			const response = await http.get(
 				`${internal}/documents/${documentId}/branches`,

server/auth-realtime/src/index.ts

@@ -48,7 +48,6 @@ const authContext = await auth.$context
 await bootstrapSingleOrganization({
 	env,
 	store,
-	core,
 	log,
 	accounts: authContext.internalAdapter,
 	hashPassword: (password) => authContext.password.hash(password),

server/auth-realtime/src/test-helpers.ts

@@ -42,9 +42,6 @@ export function stubCore(): StubCore {
 		sendEmail: vi.fn().mockResolvedValue(undefined),
 		initializeOrganization: vi.fn().mockResolvedValue(undefined),
 		teardownOrganization: vi.fn().mockResolvedValue(undefined),
-		setDefaultOrganizationLogo: vi
-			.fn()
-			.mockResolvedValue(undefined),
 		fetchBranches: vi.fn().mockResolvedValue([]),
 		fetchBranchContent: vi.fn().mockResolvedValue({
 			documentName: "Doc",

server/core/internal/server/internal/org/http.go

@@ -25,8 +25,7 @@ import (
 	"github.com/rs/xid"
 )
 
-// _defaultLogo is the Oxynote logo an organization can be given before it
-// uploads its own.
+// _defaultLogo is the Oxynote logo every organization starts with.
 //
 //go:embed default_logo.png
 var _defaultLogo []byte
@@ -86,6 +85,12 @@ func (h *Handler) InitializeOrganization(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
+	// the default logo is a nicety like the demo content below, so a
+	// failure is logged rather than returned.
+	if _, err = h.storeLogo(r.Context(), id, _defaultLogo, "image/png"); err != nil {
+		h.log.Error("cannot store default logo", slog.String("error", err.Error()))
+	}
+
 	// the demo data source is inserted before the transaction opens: a
 	// failed statement aborts a Postgres transaction, so logging and
 	// carrying on inside one would only move the failure to the commit.
@@ -205,27 +210,6 @@ func (h *Handler) UploadOrganizationLogo(w http.ResponseWriter, r *http.Request)
 	)
 }
 
-// SetDefaultOrganizationLogo gives the organization the Oxynote logo.
-func (h *Handler) SetDefaultOrganizationLogo(w http.ResponseWriter, r *http.Request) {
-	id, err := h.extractOrganizationParameter(r)
-	if err != nil {
-		httpserver.RespondError(h.log, w, err)
-		return
-	}
-
-	if _, err = h.storeLogo(r.Context(), id, _defaultLogo, "image/png"); err != nil {
-		httpserver.RespondError(h.log, w, err)
-		return
-	}
-
-	httpserver.Respond(
-		h.log,
-		w,
-		nil,
-		http.StatusNoContent,
-	)
-}
-
 // storeLogo stores the organization's logo and points the organization at
 // it, returning the logo's location. The object is removed again when the
 // organization cannot be updated.

server/core/internal/server/internal/org/http_test.go

@@ -157,12 +157,12 @@ func assertWelcomeDocumentTagged(t *testing.T, tx *TxMock, count int) {
 }
 
 func Test_Handler_InitializeOrganization(t *testing.T) {
-	type check func(*testing.T, *DBMock, *TxMock, *httptest.ResponseRecorder)
+	type check func(*testing.T, *DBMock, *TxMock, *StorerMock, *httptest.ResponseRecorder)
 
 	checks := func(cc ...check) []check { return cc }
 
 	hasResp := func(code int, body string) check {
-		return func(t *testing.T, _ *DBMock, _ *TxMock, rec *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, _ *TxMock, _ *StorerMock, rec *httptest.ResponseRecorder) {
 			assert.Equal(t, code, rec.Code)
 
 			if body == "" {
@@ -175,7 +175,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 	}
 
 	wasInsertDataSourceCalled := func(count int) check {
-		return func(t *testing.T, db *DBMock, _ *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, db *DBMock, _ *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			ff := db.InsertDataSourceCalls()
 			require.Len(t, ff, count)
 
@@ -190,8 +190,38 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 		}
 	}
 
+	wasUploadCalled := func(count int) check {
+		return func(t *testing.T, _ *DBMock, _ *TxMock, storer *StorerMock, _ *httptest.ResponseRecorder) {
+			ff := storer.UploadCalls()
+			require.Len(t, ff, count)
+
+			if count == 0 {
+				return
+			}
+
+			assert.Equal(t, "organizations/org2/logo", ff[0].Folder)
+			assert.Equal(t, "org2", ff[0].ID)
+			assert.Equal(t, _defaultLogo, ff[0].Data)
+			assert.Equal(t, "image/png", ff[0].ContentType)
+		}
+	}
+
+	wasUpdateLogoCalled := func(count int) check {
+		return func(t *testing.T, db *DBMock, _ *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
+			ff := db.UpdateOrganizationLogoCalls()
+			require.Len(t, ff, count)
+
+			if count == 0 {
+				return
+			}
+
+			assert.Equal(t, "org2", ff[0].OrganizationID)
+			assert.Regexp(t, `^loc/api/organizations/logo\?v=\d{14}$`, ff[0].Logo)
+		}
+	}
+
 	wasInsertDocumentCalled := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			ff := tx.InsertDocumentCalls()
 			require.Len(t, ff, count)
 
@@ -206,7 +236,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 	}
 
 	wasUpsertMaintainersCalled := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			ff := tx.UpsertDocumentMaintainersCalls()
 			require.Len(t, ff, count)
 
@@ -220,25 +250,25 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 	}
 
 	wasInsertTagCalled := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			assertSeededTags(t, tx, count)
 		}
 	}
 
 	wasWelcomeDocumentTagged := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			assertWelcomeDocumentTagged(t, tx, count)
 		}
 	}
 
 	wasSearchJobInserted := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			assert.Len(t, tx.InsertSearchJobCalls(), count)
 		}
 	}
 
 	wasCommitCalled := func(count int) check {
-		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+		return func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 			assert.Len(t, tx.CommitCalls(), count)
 			assert.NotEmpty(t, tx.RollbackCalls())
 		}
@@ -258,6 +288,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 			OmitID: true,
 			Checks: checks(
 				hasResp(http.StatusNotFound, `{"code":"general","message":"not found"}`),
+				wasUploadCalled(0),
 				wasInsertDataSourceCalled(0),
 				wasInsertDocumentCalled(0),
 			),
@@ -286,10 +317,29 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 			Tx: &TxMock{},
 			Checks: checks(
 				hasResp(http.StatusBadRequest, `{"code":"organization.no_members","message":"organization has no members"}`),
+				wasUploadCalled(0),
 				wasInsertDataSourceCalled(0),
 				wasInsertDocumentCalled(0),
 			),
 		},
+		"Error returned by DB.UpdateOrganizationLogo is tolerated": {
+			Members: []string{"member1"},
+			DB: &DBMock{
+				UpdateOrganizationLogoFunc: func(context.Context, string, string) error {
+					return assert.AnError
+				},
+			},
+			Tx: &TxMock{},
+			Checks: checks(
+				func(t *testing.T, _ *DBMock, _ *TxMock, _ *StorerMock, rec *httptest.ResponseRecorder) {
+					assert.Equal(t, http.StatusCreated, rec.Code)
+				},
+				wasUploadCalled(1),
+				wasUpdateLogoCalled(1),
+				wasInsertDocumentCalled(1),
+				wasCommitCalled(1),
+			),
+		},
 		"Data source insertion error is tolerated": {
 			Members: []string{"member1"},
 			DB: &DBMock{
@@ -299,7 +349,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 			},
 			Tx: &TxMock{},
 			Checks: checks(
-				func(t *testing.T, _ *DBMock, _ *TxMock, rec *httptest.ResponseRecorder) {
+				func(t *testing.T, _ *DBMock, _ *TxMock, _ *StorerMock, rec *httptest.ResponseRecorder) {
 					assert.Equal(t, http.StatusCreated, rec.Code)
 				},
 				wasInsertDataSourceCalled(1),
@@ -308,7 +358,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 				// the charts have no data source to read, so the welcome
 				// document ships without them rather than with a dangling
 				// reference.
-				func(t *testing.T, _ *DBMock, tx *TxMock, _ *httptest.ResponseRecorder) {
+				func(t *testing.T, _ *DBMock, tx *TxMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
 					ff := tx.InsertDocumentCalls()
 					require.NotEmpty(t, ff)
 
@@ -404,10 +454,12 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 				},
 			},
 			Checks: checks(
-				func(t *testing.T, _ *DBMock, _ *TxMock, rec *httptest.ResponseRecorder) {
+				func(t *testing.T, _ *DBMock, _ *TxMock, _ *StorerMock, rec *httptest.ResponseRecorder) {
 					assert.Equal(t, http.StatusCreated, rec.Code)
 					assert.Contains(t, rec.Body.String(), "Welcome to Oxynote!")
 				},
+				wasUploadCalled(1),
+				wasUpdateLogoCalled(1),
 				wasInsertDataSourceCalled(1),
 				wasInsertDocumentCalled(1),
 				wasUpsertMaintainersCalled(1),
@@ -432,10 +484,14 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 				return c.Members, c.MembersErr
 			}
 
+			storer := &StorerMock{}
+
 			hdl := Handler{
 				log:           slog.New(slog.DiscardHandler),
 				db:            withTx(db, c.Tx, c.BeginErr),
+				storer:        storer,
 				searchTrigger: &SearchTriggerMock{},
+				publicURL:     "loc",
 			}
 
 			req := httptest.NewRequest(http.MethodPost, "http://test.com/", http.NoBody)
@@ -451,7 +507,7 @@ func Test_Handler_InitializeOrganization(t *testing.T) {
 			hdl.InitializeOrganization(rec, req.WithContext(ctx))
 
 			for _, ch := range c.Checks {
-				ch(t, db, c.Tx, rec)
+				ch(t, db, c.Tx, storer, rec)
 			}
 		})
 	}
@@ -653,124 +709,6 @@ func Test_Handler_UploadOrganizationLogo(t *testing.T) {
 	}
 }
 
-func Test_Handler_SetDefaultOrganizationLogo(t *testing.T) {
-	type check func(*testing.T, *DBMock, *StorerMock, *httptest.ResponseRecorder)
-
-	checks := func(cc ...check) []check { return cc }
-
-	hasResp := func(code int, body string) check {
-		return func(t *testing.T, _ *DBMock, _ *StorerMock, rec *httptest.ResponseRecorder) {
-			assert.Equal(t, code, rec.Code)
-
-			if body == "" {
-				assert.Zero(t, rec.Body.Len(), rec.Body.String())
-				return
-			}
-
-			assert.JSONEq(t, body, rec.Body.String())
-		}
-	}
-
-	wasUploadCalled := func(count int) check {
-		return func(t *testing.T, _ *DBMock, storer *StorerMock, _ *httptest.ResponseRecorder) {
-			ff := storer.UploadCalls()
-			require.Len(t, ff, count)
-
-			if count == 0 {
-				return
-			}
-
-			assert.Equal(t, "organizations/org2/logo", ff[0].Folder)
-			assert.Equal(t, "org2", ff[0].ID)
-			assert.Equal(t, _defaultLogo, ff[0].Data)
-			assert.Equal(t, "image/png", ff[0].ContentType)
-		}
-	}
-
-	wasUpdateLogoCalled := func(count int) check {
-		return func(t *testing.T, db *DBMock, _ *StorerMock, _ *httptest.ResponseRecorder) {
-			ff := db.UpdateOrganizationLogoCalls()
-			require.Len(t, ff, count)
-
-			if count == 0 {
-				return
-			}
-
-			assert.Equal(t, "org2", ff[0].OrganizationID)
-			assert.Regexp(t, `^loc/api/organizations/logo\?v=\d{14}$`, ff[0].Logo)
-		}
-	}
-
-	cc := map[string]struct {
-		DB     *DBMock
-		Storer *StorerMock
-		OmitID bool
-		Checks []check
-	}{
-		"Missing organization ID parameter": {
-			DB:     &DBMock{},
-			Storer: &StorerMock{},
-			OmitID: true,
-			Checks: checks(
-				hasResp(http.StatusNotFound, `{"code":"general","message":"not found"}`),
-				wasUploadCalled(0),
-				wasUpdateLogoCalled(0),
-			),
-		},
-		"Error returned by Storer.Upload": {
-			DB: &DBMock{},
-			Storer: &StorerMock{
-				UploadFunc: func(context.Context, string, string, []byte, string) error {
-					return assert.AnError
-				},
-			},
-			Checks: checks(
-				hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`),
-				wasUploadCalled(1),
-				wasUpdateLogoCalled(0),
-			),
-		},
-		"Successful update": {
-			DB:     &DBMock{},
-			Storer: &StorerMock{},
-			Checks: checks(
-				hasResp(http.StatusNoContent, ""),
-				wasUploadCalled(1),
-				wasUpdateLogoCalled(1),
-			),
-		},
-	}
-
-	for cn, c := range cc {
-		t.Run(cn, func(t *testing.T) {
-			t.Parallel()
-
-			hdl := Handler{
-				log:       slog.New(slog.DiscardHandler),
-				db:        c.DB,
-				storer:    c.Storer,
-				publicURL: "loc",
-			}
-
-			req := httptest.NewRequest(http.MethodPut, "http://test.com/", http.NoBody)
-
-			ctx := req.Context()
-
-			if !c.OmitID {
-				ctx = testutil.AddChiCtx(ctx, "organizationId", "org2")
-			}
-
-			rec := httptest.NewRecorder()
-
-			hdl.SetDefaultOrganizationLogo(rec, req.WithContext(ctx))
-
-			for _, ch := range c.Checks {
-				ch(t, c.DB, c.Storer, rec)
-			}
-		})
-	}
-}
-
 func Test_Handler_storeLogo(t *testing.T) {
 	type check func(*testing.T, *DBMock, *StorerMock, string, *bytes.Buffer)
 

server/core/internal/server/router.go

@@ -143,7 +143,6 @@ func (s *Server) internalRouter() chi.Router {
 
 	r.Post("/organizations/{organizationId}/initialize", s.handlers.organization.InitializeOrganization)
 	r.Post("/organizations/{organizationId}/teardown", s.handlers.organization.TeardownOrganization)
-	r.Put("/organizations/{organizationId}/logo/default", s.handlers.organization.SetDefaultOrganizationLogo)
 
 	r.Route("/documents/{documentId}", func(sr chi.Router) {
 		sr.Get("/branches", s.handlers.document.FetchDocumentBranchesUnsafe)

server/core/internal/server/router_test.go

@@ -80,7 +80,6 @@ func Test_Server_httpRouter(t *testing.T) {
 
 	// the service-to-service surface stays where it is.
 	assert.True(t, routes["POST /api/x/organizations/{organizationId}/teardown"])
-	assert.True(t, routes["PUT /api/x/organizations/{organizationId}/logo/default"])
 	assert.True(t, routes["PUT /api/x/documents/{documentId}/branch/{branchId}/"])
 	assert.True(t, routes["POST /api/x/email"])