diff --git a/server/auth-realtime/src/bootstrap.test.ts b/server/auth-realtime/src/bootstrap.test.ts index 3eaf2cca..cbcc50e6 100644 --- a/server/auth-realtime/src/bootstrap.test.ts +++ b/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, ) diff --git a/server/auth-realtime/src/bootstrap.ts b/server/auth-realtime/src/bootstrap.ts index c82edc30..7781791f 100644 --- a/server/auth-realtime/src/bootstrap.ts +++ b/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 @@ -45,7 +43,7 @@ export interface BootstrapDeps { name: string slug: string userId: string - }) => Promise<{ id: string }> + }) => Promise } // 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.`, ) diff --git a/server/auth-realtime/src/core.test.ts b/server/auth-realtime/src/core.test.ts index 862b3be5..20c41d23 100644 --- a/server/auth-realtime/src/core.test.ts +++ b/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, diff --git a/server/auth-realtime/src/core.ts b/server/auth-realtime/src/core.ts index 279f10a1..bf4ba102 100644 --- a/server/auth-realtime/src/core.ts +++ b/server/auth-realtime/src/core.ts @@ -93,7 +93,6 @@ export interface CoreClient { ): Promise initializeOrganization(organizationId: string): Promise teardownOrganization(organizationId: string): Promise - setDefaultOrganizationLogo(organizationId: string): Promise fetchBranches(documentId: string): Promise 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`, diff --git a/server/auth-realtime/src/index.ts b/server/auth-realtime/src/index.ts index e26af67f..667a339f 100644 --- a/server/auth-realtime/src/index.ts +++ b/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), diff --git a/server/auth-realtime/src/test-helpers.ts b/server/auth-realtime/src/test-helpers.ts index 8127d245..559f41f0 100644 --- a/server/auth-realtime/src/test-helpers.ts +++ b/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", diff --git a/server/core/internal/server/internal/org/http.go b/server/core/internal/server/internal/org/http.go index 8a6af1a3..1dbe4358 100644 --- a/server/core/internal/server/internal/org/http.go +++ b/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. diff --git a/server/core/internal/server/internal/org/http_test.go b/server/core/internal/server/internal/org/http_test.go index dd383941..69171734 100644 --- a/server/core/internal/server/internal/org/http_test.go +++ b/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) diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index 05123ac6..ecc25464 100644 --- a/server/core/internal/server/router.go +++ b/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) diff --git a/server/core/internal/server/router_test.go b/server/core/internal/server/router_test.go index 57157ddb..39ec2e24 100644 --- a/server/core/internal/server/router_test.go +++ b/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"])