diff --git a/server/auth-realtime/src/routes.test.ts b/server/auth-realtime/src/routes.test.ts index 58b1ee3e..95ac7b14 100644 --- a/server/auth-realtime/src/routes.test.ts +++ b/server/auth-realtime/src/routes.test.ts @@ -1461,7 +1461,7 @@ describe("createRoutes", () => { it("flushes the branch and answers once core has it", async ({ expect, }) => { - const { app, flushDocument } = build() + const { app, core, flushDocument } = build() const res = await app.request( "/internal/documents/doc1/branches/b1/flush", @@ -1469,6 +1469,8 @@ describe("createRoutes", () => { ) expect(res.status).toBe(204) + expect(core.fetchBranches).toHaveBeenCalledTimes(1) + expect(core.fetchBranches).toHaveBeenCalledWith("doc1") expect( flushDocument.mock.calls.map((call) => call[0]), ).toEqual(["doc1-b1"]) @@ -1480,7 +1482,7 @@ describe("createRoutes", () => { .mockRejectedValue( new Error("core unreachable"), ) - const { app } = build({ flushDocument }) + const { app, core } = build({ flushDocument }) const res = await app.request( "/internal/documents/doc1/branches/b1/flush", @@ -1488,6 +1490,13 @@ describe("createRoutes", () => { ) expect(res.status).toBe(500) + expect(await res.json()).toEqual({ + error: "pending changes could not be stored", + }) + expect(core.fetchBranches).toHaveBeenCalledTimes(1) + expect(core.fetchBranches).toHaveBeenCalledWith("doc1") + expect(flushDocument).toHaveBeenCalledTimes(1) + expect(flushDocument).toHaveBeenCalledWith("doc1-b1") }) }) diff --git a/server/core/AGENTS.md b/server/core/AGENTS.md index e191afb1..8f6879de 100644 --- a/server/core/AGENTS.md +++ b/server/core/AGENTS.md @@ -127,9 +127,9 @@ both**; a swallowed error is logged with a comment saying why. - Mutexes named `mu` (or `Mu`); lock-then-defer-unlock. For operations a caller may abandon use a context-aware mutex (`syncutil.Mutex`, to be ported into `pkg/` before first use). -- One consumer: a `Bind*` callback. Registries return an unsubscriber - func; dispatch reads under `RLock` and fans out via the supervisor, - never under the write lock. +- A `Bind*` method sets the sole consumer; registries return an unsubscribe + func. Dispatch holds `RLock` and fans out via the supervisor, never the + write lock. - Periodic work: `timeutil.NewPeriodicExec` or `timeutil.NewCron`, never hand-rolled tickers. Shutdown via a once-closed `chan struct{}` exposed as `ShutDownCh()`. Sleeps `select` on `ctx.Done()`, never bare `time.Sleep`. diff --git a/server/core/internal/assistant/edit/client.go b/server/core/internal/assistant/edit/client.go index 3ed8f7e1..100db81a 100644 --- a/server/core/internal/assistant/edit/client.go +++ b/server/core/internal/assistant/edit/client.go @@ -30,10 +30,9 @@ type Result struct { Errors []OpError `json:"errors"` } -// Client posts to the Node hocuspocus service's internal per-branch +// Client calls the Node hocuspocus service's internal per-branch // endpoints: batched operations and flushes. The base URL points at the -// Node service (e.g. http://auth-realtime:8081); endpoint replaces its -// path with the per-branch action path. +// Node service (e.g. http://auth-realtime:8081). type Client struct { // httpClient is the underlying HTTP client. Callers should // pass one with a reasonable timeout configured. diff --git a/server/core/internal/assistant/edit/client_test.go b/server/core/internal/assistant/edit/client_test.go index e66c0b3a..0e62714e 100644 --- a/server/core/internal/assistant/edit/client_test.go +++ b/server/core/internal/assistant/edit/client_test.go @@ -242,11 +242,12 @@ func Test_Client_Flush(t *testing.T) { path := "/api/internal/documents/" + docID.String() + "/branches/" + branchID.String() + "/flush" cc := map[string]struct { - BaseURL string - CloseEarly bool - StatusCode int - Body string - Err error + BaseURL string + CloseEarly bool + StatusCode int + Body string + TruncateBody bool + Err error }{ "Invalid base URL": { BaseURL: "://bad", @@ -261,6 +262,11 @@ func Test_Client_Flush(t *testing.T) { Body: `{"error":"pending changes could not be stored"}`, Err: assert.AnError, }, + "Unreadable error body": { + StatusCode: http.StatusInternalServerError, + TruncateBody: true, + Err: assert.AnError, + }, "Successful flush": { StatusCode: http.StatusNoContent, }, @@ -275,6 +281,12 @@ func Test_Client_Flush(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { gotPath, gotMethod = r.URL.Path, r.Method + // declare a longer body than is written, so the client's + // read of the error body fails. + if c.TruncateBody { + w.Header().Set("Content-Length", "100") + } + w.WriteHeader(c.StatusCode) _, err := w.Write([]byte(c.Body)) assert.NoError(t, err) diff --git a/server/core/internal/assistant/manager.go b/server/core/internal/assistant/manager.go index 19089a7d..b0a14a27 100644 --- a/server/core/internal/assistant/manager.go +++ b/server/core/internal/assistant/manager.go @@ -88,13 +88,14 @@ type Manager struct { // backs context summarisation and may be the same model. The editClient // is the edit pipe to the Node hocuspocus service; the search client // backs the search_documents tool and searchTrigger runs the search-job -// worker once a document write has committed; providerName labels token metrics so -// usage stays readable across a provider change; githubMan and -// webchangeClient are the integrations the hook tools check for; hookMan -// runs the hook writes; the tree notifier broadcasts sidebar refresh events after -// document tree mutations and is wired post-construction via -// SetTreeNotifier because the document handler that satisfies it is -// built later, inside server.NewServer. +// worker once a document write has committed; providerName labels token +// metrics so usage stays readable across a provider change. +// +// githubMan and webchangeClient tell the hook tools which integrations +// exist; hookMan runs hook writes. The tree notifier broadcasts sidebar +// refresh events after document tree mutations and is wired +// post-construction via SetTreeNotifier because the document handler +// that satisfies it is built later, inside server.NewServer. func NewManager( log *slog.Logger, db tools.DB, diff --git a/server/core/internal/assistant/manager_test.go b/server/core/internal/assistant/manager_test.go index a6524623..fb47fef7 100644 --- a/server/core/internal/assistant/manager_test.go +++ b/server/core/internal/assistant/manager_test.go @@ -17,6 +17,7 @@ import ( "github.com/oxynote/oxynote/server/core/internal/assistant/persist" persistMock "github.com/oxynote/oxynote/server/core/internal/assistant/persist/_mock" protocolMock "github.com/oxynote/oxynote/server/core/internal/assistant/protocol/_mock" + "github.com/oxynote/oxynote/server/core/internal/assistant/tools" toolsMock "github.com/oxynote/oxynote/server/core/internal/assistant/tools/_mock" "github.com/oxynote/oxynote/server/core/pkg/errutil" "github.com/oxynote/oxynote/server/core/pkg/memkit" @@ -43,6 +44,12 @@ type stubSearchTrigger struct{} func (*stubSearchTrigger) Trigger() {} +// stubHookManager satisfies the tools' HookManager. Its methods are never +// called. +type stubHookManager struct { + tools.HookManager +} + // stubChatModel builds a chat model that answers with the given // messages in order. The last one repeats once the script runs out, so // a test that under-specifies gets a terminating answer rather than a @@ -240,7 +247,8 @@ func Test_NewManager(t *testing.T) { fc := metricutil.NewFactory("test", prometheus.NewRegistry()) gh := &github.Manager{} wc := &webchange.Client{} - m := NewManager(discardLog(), nil, c.Pool, nil, nil, fc, nil, nil, &stubSearchTrigger{}, nil, gh, wc, nil, "claude") + hm := &stubHookManager{} + m := NewManager(discardLog(), nil, c.Pool, nil, nil, fc, nil, nil, &stubSearchTrigger{}, nil, gh, wc, hm, "claude") require.NotNil(t, m) assert.NotNil(t, m.log) @@ -252,6 +260,7 @@ func Test_NewManager(t *testing.T) { assert.NotNil(t, m.turns.m) assert.Same(t, gh, m.githubMan) assert.Same(t, wc, m.webchangeClient) + assert.Same(t, hm, m.hookMan) assert.Nil(t, m.tree) assert.Nil(t, m.tags) diff --git a/server/core/internal/assistant/tools/hook_test.go b/server/core/internal/assistant/tools/hook_test.go index a9456f3b..e5ceeed6 100644 --- a/server/core/internal/assistant/tools/hook_test.go +++ b/server/core/internal/assistant/tools/hook_test.go @@ -31,49 +31,34 @@ func hookDeps(db *DBMock, man *HookManagerMock) *Deps { return d } -// stubHookManager runs every hook write the way the hook manager does, -// leaving out the database: a write reads a copy of the stored hook, which -// is stored when given and stubHook otherwise. -func stubHookManager(stored ...*hook.Hook) *HookManagerMock { - inp := func(organizationID string) *hook.Input { - return hook.NewInput(organizationID, unconfiguredGithub(), webchange.NewClient("", "")) - } - - lookup := func() *hook.Hook { - if len(stored) > 0 { - hk := *stored[0] - - return &hk - } - - return stubHook() - } - +// stubHookManager accepts every hook write. A write answers with +// stubHook changed as the call asks, and an update or reset stamps +// UpdatedAt so a test can tell the answer from the hook it handed in. +func stubHookManager() *HookManagerMock { return &HookManagerMock{ - CreateHookFunc: func(ctx context.Context, ci hook.CreateInput, documentID xid.ID, organizationID, _ string) (*hook.Hook, error) { - // the manager refuses a block the branch does not hold. - if ci.BlockID.Valid && ci.BlockID.String != _stubContentUID { - return nil, hook.ErrBlockNotFound - } - - return hook.NewHook(ctx, ci, documentID, ci.BranchID, organizationID, inp(organizationID)) - }, - UpdateHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID string, ui hook.UpdateInput, _ string) (*hook.Hook, error) { - hk := lookup() - if err := hk.ApplyUpdate(ctx, ui, inp(organizationID)); err != nil { - return nil, err - } + CreateHookFunc: func(_ context.Context, ci hook.CreateInput, documentID xid.ID, organizationID, _ string) (*hook.Hook, error) { + hk := stubHook() + hk.Type = ci.Type + hk.DocumentID = null.ValueFrom(documentID) + hk.BranchID = null.ValueFrom(ci.BranchID) + hk.OrganizationID = null.StringFrom(organizationID) + hk.BlockID = ci.BlockID + hk.Settings = ci.Settings return hk, nil }, - DeleteHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID, _ string) error { - return lookup().Delete(ctx, inp(organizationID)) + UpdateHookFunc: func(_ context.Context, id, _ xid.ID, _ string, ui hook.UpdateInput, _ string) (*hook.Hook, error) { + hk := stubHook() + hk.ID = id + hk.Settings = ui.Settings + hk.UpdatedAt = null.TimeFrom(_stubScheduleTime) + + return hk, nil }, - ResetHookFunc: func(ctx context.Context, _, _ xid.ID, organizationID string) (*hook.Hook, error) { - hk := lookup() - if err := hk.Reset(ctx, inp(organizationID)); err != nil { - return nil, err - } + ResetHookFunc: func(_ context.Context, id, _ xid.ID, _ string) (*hook.Hook, error) { + hk := stubHook() + hk.ID = id + hk.UpdatedAt = null.TimeFrom(_stubScheduleTime) return hk, nil }, @@ -524,6 +509,11 @@ func Test_createHook_Execute(t *testing.T) { return nil, assert.AnError } + blockless := stubHookManager() + blockless.CreateHookFunc = func(context.Context, hook.CreateInput, xid.ID, string, string) (*hook.Hook, error) { + return nil, hook.ErrBlockNotFound + } + cc := map[string]struct { DB *DBMock Man *HookManagerMock @@ -550,6 +540,7 @@ func Test_createHook_Execute(t *testing.T) { }, "Unknown block": { DB: stubHookDB(), + Man: blockless, Args: `{` + targetArgs(_stubBranchID) + `,` + scheduled + `,"block_uid":"nope"}`, Err: fmt.Errorf("create_hook: %w", fmt.Errorf("block %s: %w", "nope", errUnknownBlock)), }, @@ -770,8 +761,6 @@ func Test_updateHook_Execute(t *testing.T) { return } - // the settings are replaced and the state reset: a fresh - // start rather than the old one carried over. ff := d.hookMan.(*HookManagerMock).UpdateHookCalls() require.Len(t, ff, 1) assert.Equal(t, _testHookID, ff[0].ID) @@ -780,8 +769,7 @@ func Test_updateHook_Execute(t *testing.T) { row := decodeHookRow(t, res) assert.Equal(t, _testHookID, row.ID) assert.JSONEq(t, string(ff[0].UI.Settings), string(row.Settings)) - assert.NotContains(t, string(row.State), "2026-01-01") - assert.Equal(t, "100", row.Score.String()) + assert.Equal(t, null.TimeFrom(_stubScheduleTime), row.UpdatedAt) assert.Equal(t, []Touched{{DocumentID: _testDocID, BranchID: _stubBranchID}}, inp.touched) }) @@ -906,7 +894,6 @@ func Test_resetHook_Execute(t *testing.T) { return } - // the settings stay as they were; only the state starts over. ff := d.hookMan.(*HookManagerMock).ResetHookCalls() require.Len(t, ff, 1) assert.Equal(t, _testHookID, ff[0].ID) @@ -914,8 +901,7 @@ func Test_resetHook_Execute(t *testing.T) { row := decodeHookRow(t, res) assert.Equal(t, _testHookID, row.ID) assert.JSONEq(t, string(stubHook().Settings), string(row.Settings)) - assert.NotContains(t, string(row.State), "2026-01-01") - assert.Equal(t, "100", row.Score.String()) + assert.Equal(t, null.TimeFrom(_stubScheduleTime), row.UpdatedAt) assert.Equal(t, []Touched{{DocumentID: _testDocID, BranchID: _stubBranchID}}, inp.touched) }) diff --git a/server/core/internal/assistant/tools/input.go b/server/core/internal/assistant/tools/input.go index f20eb5e5..5af6af35 100644 --- a/server/core/internal/assistant/tools/input.go +++ b/server/core/internal/assistant/tools/input.go @@ -66,13 +66,11 @@ type Deps struct { // runners hands out the runner a data-source tool reads through. runners DataSourceRunners - // githubMan is the GitHub App integration a github-tracking hook - // reads through; unconfigured on a deployment without the app. + // githubMan tells create_hook whether the GitHub App is configured. githubMan *github.Manager - // webchangeClient is the changedetection.io integration a - // url-watcher hook holds its watcher in; unconfigured on a - // deployment without it. + // webchangeClient tells create_hook whether changedetection.io is + // configured. webchangeClient *webchange.Client // hookMan runs the hook writes. @@ -1042,8 +1040,8 @@ func (i *input) CreateHook(documentID, branchID xid.ID, blockUID string, tp hook return nil, err } - // a hook whose integration is missing is refused anyway. Naming the - // integration's own error tells the model what is missing. + // the manager would refuse this too, but the integration's own error + // tells the model what is missing. switch tp { case hook.TypeGithubTracking: if !i.githubMan.Configured() { @@ -1119,14 +1117,12 @@ func (i *input) DeleteHook(hk *hook.Hook) error { } // docRef wraps the (documentID, branchID) pair the edit client needs to -// address a live Y.Doc. The branch is resolved to the document's -// default branch — multi-branch editing is out of scope for the -// assistant. +// address a live Y.Doc. type docRef struct { // DocumentID is the document's id. DocumentID xid.ID - // BranchID is the id of the document's default branch. + // BranchID is the id of the addressed branch. BranchID xid.ID } diff --git a/server/core/internal/assistant/tools/input_test.go b/server/core/internal/assistant/tools/input_test.go index ee3562d1..da4dae66 100644 --- a/server/core/internal/assistant/tools/input_test.go +++ b/server/core/internal/assistant/tools/input_test.go @@ -2409,6 +2409,11 @@ func Test_input_CreateHook(t *testing.T) { return nil, assert.AnError } + blockless := stubHookManager() + blockless.CreateHookFunc = func(context.Context, hook.CreateInput, xid.ID, string, string) (*hook.Hook, error) { + return nil, hook.ErrBlockNotFound + } + cc := map[string]struct { DB *DBMock Man *HookManagerMock @@ -2429,6 +2434,7 @@ func Test_input_CreateHook(t *testing.T) { }, "Unknown block": { DB: stubHookDB(), + Man: blockless, Branch: _stubBranchID, BlockUID: "nope", Type: hook.TypeScheduledReminder, @@ -2450,14 +2456,6 @@ func Test_input_CreateHook(t *testing.T) { Settings: processor.Settings(`{"url":"https://example.com"}`), Err: fmt.Errorf("%s: %w", hook.TypeURLWatcher, webchange.ErrNotConfigured), }, - "Settings the processor refuses": { - DB: stubHookDB(), - Branch: _stubBranchID, - Type: hook.TypeScheduledReminder, - Settings: processor.Settings(`{"scale":"bogus"}`), - Inserts: 1, - Err: processor.ErrInvalidScaleType, - }, "Error returned by hookMan.CreateHook": { DB: stubHookDB(), Man: failing, @@ -2520,8 +2518,6 @@ func Test_input_CreateHook(t *testing.T) { assert.Equal(t, null.StringFrom("org"), hk.OrganizationID) assert.Equal(t, null.NewString(c.BlockUID, c.BlockUID != ""), hk.BlockID) assert.Equal(t, c.Settings, hk.Settings) - assert.Equal(t, "100", hk.Score.String()) - assert.True(t, hk.State.Valid) }) } } @@ -2542,12 +2538,6 @@ func Test_input_UpdateHook(t *testing.T) { Touched []Touched Err error }{ - "Settings the processor refuses": { - DB: stubHookDB(), - Settings: processor.Settings(`{"scale":"bogus"}`), - Updates: 1, - Err: processor.ErrInvalidScaleType, - }, "Error returned by hookMan.UpdateHook": { DB: stubHookDB(), Man: failing, @@ -2587,12 +2577,11 @@ func Test_input_UpdateHook(t *testing.T) { return } - // the hook handed in takes the stored one, with the settings - // replaced and a fresh state. + // the hook handed in takes the stored one. assert.Equal(t, hk.ID, ff[0].ID) assert.Equal(t, "org", ff[0].OrganizationID) + assert.Equal(t, c.Settings, ff[0].UI.Settings) assert.Equal(t, c.Settings, hk.Settings) - assert.NotContains(t, string(hk.State.V), "2026-01-01") assert.True(t, hk.UpdatedAt.Valid) }) } @@ -2606,9 +2595,6 @@ func Test_input_ResetHook(t *testing.T) { return nil, assert.AnError } - invalid := stubHook() - invalid.Settings = processor.Settings(`{"scale":1}`) - cc := map[string]struct { DB *DBMock Man *HookManagerMock @@ -2617,13 +2603,6 @@ func Test_input_ResetHook(t *testing.T) { Touched []Touched Err error }{ - "Settings that do not decode": { - DB: stubHookDB(), - Man: stubHookManager(invalid), - Hook: invalid, - Updates: 1, - Err: assert.AnError, - }, "Error returned by hookMan.ResetHook": { DB: stubHookDB(), Man: failing, @@ -2657,10 +2636,10 @@ func Test_input_ResetHook(t *testing.T) { return } - // the settings stay; only the state starts over. + // the hook handed in takes the stored one. assert.Equal(t, c.Hook.ID, ff[0].ID) - assert.Equal(t, stubHook().Settings, c.Hook.Settings) - assert.NotContains(t, string(c.Hook.State.V), "2026-01-01") + assert.Equal(t, "org", ff[0].OrganizationID) + assert.True(t, c.Hook.UpdatedAt.Valid) }) } } @@ -2673,13 +2652,6 @@ func Test_input_DeleteHook(t *testing.T) { return assert.AnError } - // a url watcher tears its changedetection.io watcher down from its - // state, and a state it cannot read stops the delete before the row. - watcher := stubHook() - watcher.Type = hook.TypeURLWatcher - watcher.Settings = processor.Settings(`{"url":"https://example.com"}`) - watcher.State = null.ValueFrom(processor.State(`{`)) - cc := map[string]struct { DB *DBMock Man *HookManagerMock @@ -2688,13 +2660,6 @@ func Test_input_DeleteHook(t *testing.T) { Touched []Touched Err error }{ - "External teardown failed": { - DB: stubHookDB(), - Man: stubHookManager(watcher), - Hook: watcher, - Deletes: 1, - Err: assert.AnError, - }, "Error returned by hookMan.DeleteHook": { DB: stubHookDB(), Man: failing, @@ -2734,6 +2699,8 @@ func Test_input_DeleteHook(t *testing.T) { } assert.Equal(t, _testHookID, ff[0].ID) + assert.Equal(t, _testDocID, ff[0].DocumentID) + assert.Equal(t, "org", ff[0].OrganizationID) }) } } diff --git a/server/core/internal/db/document_branch.go b/server/core/internal/db/document_branch.go index 31e80eb3..d3bed19d 100644 --- a/server/core/internal/db/document_branch.go +++ b/server/core/internal/db/document_branch.go @@ -345,8 +345,8 @@ func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.T "updated_at": entry.UpdatedAt, } - // a system write has no author, and the entry still holds the - // edits of the one it has. + // a system write has no author. Keep the user whose edits the + // entry still holds. if entry.LastUpdatedBy.Valid { set["fk_last_updated_by"] = entry.LastUpdatedBy } @@ -385,10 +385,9 @@ func (a *agent) insertDocumentBranchHistoryEntry(ctx context.Context, tx *sqlx.T return a.trimDocumentBranchHistoryEntries(ctx, tx, entry.BranchID) } -// fetchDocumentBranchHistoryHooks fetches every hook of the branch a -// history entry may list, soft-deleted ones included, in a stable order so -// equal hook sets compare equal. history.NewHooks picks the ones the -// entry's content holds. +// fetchDocumentBranchHistoryHooks fetches every hook of the branch, +// soft-deleted ones included. The fixed order keeps the entry checksum +// stable. func (a *agent) fetchDocumentBranchHistoryHooks(ctx context.Context, q sqlx.QueryerContext, branchID xid.ID) ([]hook.Hook, error) { sqlq, args := a.selectDocumentHook(a.builder.Select()). Where(sq.Eq{"document_hooks.fk_branch_id": branchID}). diff --git a/server/core/internal/db/document_branch_test.go b/server/core/internal/db/document_branch_test.go index e66685cc..2263ad22 100644 --- a/server/core/internal/db/document_branch_test.go +++ b/server/core/internal/db/document_branch_test.go @@ -694,8 +694,8 @@ func insertHistoryEntry(t *testing.T, db *DB, entry history.Entry) { } // assertHistoryEntryEqual compares two entries, matching hook settings as -// JSON since jsonb normalises their spacing. The stored checksum must be -// the sum of what the expected entry records. +// JSON since jsonb normalises their spacing. The checksum is expected to +// be exp.Sum(). func assertHistoryEntryEqual(t *testing.T, exp, got history.Entry) { t.Helper() @@ -789,48 +789,75 @@ func Test_agent_trimDocumentBranchHistoryEntries(t *testing.T) { } func Test_agent_DeleteExpiredDocumentBranchHistoryEntries(t *testing.T) { - t.Parallel() + type tcase struct { + Before time.Time + // Rows maps each branch to how many entries it holds afterwards. + Rows map[xid.ID]int + // Newest maps each branch to its newest entry afterwards. + Newest map[xid.ID]xid.ID + } - db := prepTempDB(t) - docs := prepDocuments(t, db, 2, nil) - doc, other := docs[0], docs[1] + cc := map[string]func(*testing.T, *DB) tcase{ + "Nothing expired": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + entries := prepHistoryEntries(t, db, doc, 4) - entries := prepHistoryEntries(t, db, doc, 4) - require.Equal(t, 4, countHistoryEntries(t, db, doc.BranchID)) + return tcase{ + Before: entries[0].CreatedAt.Add(-time.Second), + Rows: map[xid.ID]int{doc.BranchID: 4}, + Newest: map[xid.ID]xid.ID{doc.BranchID: entries[3].ID}, + } + }, + "Expired entries go": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + entries := prepHistoryEntries(t, db, doc, 4) - otherEntries := prepHistoryEntries(t, db, other, 2) - require.Equal(t, 2, countHistoryEntries(t, db, other.BranchID)) + return tcase{ + Before: entries[2].CreatedAt, + Rows: map[xid.ID]int{doc.BranchID: 2}, + Newest: map[xid.ID]xid.ID{doc.BranchID: entries[3].ID}, + } + }, + "Newest entry of each branch stays": func(t *testing.T, db *DB) tcase { + docs := prepDocuments(t, db, 2, nil) + entries := prepHistoryEntries(t, db, docs[0], 4) + other := prepHistoryEntries(t, db, docs[1], 2) - // nothing is old enough yet. - require.NoError(t, db.DeleteExpiredDocumentBranchHistoryEntries( - context.Background(), - entries[0].CreatedAt.Add(-time.Second), - )) - assert.Equal(t, 4, countHistoryEntries(t, db, doc.BranchID)) - assert.Equal(t, 2, countHistoryEntries(t, db, other.BranchID)) + return tcase{ + Before: entries[3].CreatedAt.Add(time.Hour), + Rows: map[xid.ID]int{ + docs[0].BranchID: 1, + docs[1].BranchID: 1, + }, + Newest: map[xid.ID]xid.ID{ + docs[0].BranchID: entries[3].ID, + docs[1].BranchID: other[1].ID, + }, + } + }, + } - // everything created before the newest entry goes. - require.NoError(t, db.DeleteExpiredDocumentBranchHistoryEntries( - context.Background(), - entries[3].CreatedAt, - )) - assert.Equal(t, 1, countHistoryEntries(t, db, doc.BranchID)) + for cn, cfn := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - // each branch keeps its newest entry even once it has expired. - require.NoError(t, db.DeleteExpiredDocumentBranchHistoryEntries( - context.Background(), - entries[3].CreatedAt.Add(time.Hour), - )) + db := prepTempDB(t) + c := cfn(t, db) - var ids []xid.ID + require.NoError(t, db.DeleteExpiredDocumentBranchHistoryEntries(context.Background(), c.Before)) - q, args := db.builder.Select("id"). - From("document_branch_history_entries"). - Where(sq.Eq{"fk_branch_id": []xid.ID{doc.BranchID, other.BranchID}}). - MustSql() + for branchID, rows := range c.Rows { + assert.Equal(t, rows, countHistoryEntries(t, db, branchID)) + } - require.NoError(t, db.sql.Select(&ids, q, args...)) - assert.ElementsMatch(t, []xid.ID{entries[3].ID, otherEntries[1].ID}, ids) + for branchID, id := range c.Newest { + newest, err := db.fetchNewestDocumentBranchHistoryEntry(context.Background(), db.sql, branchID) + require.NoError(t, err) + require.NotNil(t, newest) + assert.Equal(t, id, newest.ID) + } + }) + } } func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { @@ -949,6 +976,21 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { Newest: first, } }, + "Equal entry in the same bucket keeps the newest untouched": func(t *testing.T, db *DB) tcase { + doc := prepDocuments(t, db, 1, nil)[0] + user := prepUsers(t, db, 1)[0] + + first := history.NewEntry(*doc, base.Add(5*time.Minute), null.StringFrom(user), hooks, false) + insertHistoryEntry(t, db, first) + + entry := history.NewEntry(*doc, base.Add(10*time.Minute), null.String{}, hooks, false) + + return tcase{ + Entry: entry, + Rows: 1, + Newest: first, + } + }, "Boundary entry equal to the newest one inserts": func(t *testing.T, db *DB) tcase { doc := prepDocuments(t, db, 1, nil)[0] @@ -1034,34 +1076,6 @@ func Test_agent_insertDocumentBranchHistoryEntry(t *testing.T) { } } -// prepHistoryBranch returns a branch whose content holds block "b1" and -// the hooks fn describes on it, fetched back so they match what a record -// reads. -func prepHistoryBranch(t *testing.T, db *DB, count int, fn func(int, *hook.Hook)) (*document.Document, []hook.Hook) { - t.Helper() - - branch := prepDocumentBranches(t, db, 1, func(_ int, doc *document.Document) { - doc.Content.Content[0].Attrs = document.Attributes{document.AttrUID: "b1"} - })[0] - - hooks := prepDocumentHooks(t, db, count, func(i int, hk *hook.Hook) { - hk.DocumentID = null.ValueFrom(branch.ID) - hk.OrganizationID = null.StringFrom(branch.OrganizationID) - hk.BranchID = null.ValueFrom(branch.BranchID) - hk.BlockID = null.StringFrom("b1") - hk.CreatedAt = hk.CreatedAt.Add(time.Duration(i) * time.Second) - - if fn != nil { - fn(i, hk) - } - }) - - doc, err := db.FetchDocumentUnsafeByBranchID(context.Background(), branch.BranchID) - require.NoError(t, err) - - return doc, hooks -} - func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { type tcase struct { // Tx, when set, is the transaction the record runs in. It is @@ -1079,9 +1093,37 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { Err error } + // prepBranch returns a branch whose content holds block "b1" and the + // hooks fn describes on it, fetched back so they match what a record + // reads. + prepBranch := func(t *testing.T, db *DB, count int, fn func(int, *hook.Hook)) (*document.Document, []hook.Hook) { + t.Helper() + + branch := prepDocumentBranches(t, db, 1, func(_ int, doc *document.Document) { + doc.Content.Content[0].Attrs = document.Attributes{document.AttrUID: "b1"} + })[0] + + hooks := prepDocumentHooks(t, db, count, func(i int, hk *hook.Hook) { + hk.DocumentID = null.ValueFrom(branch.ID) + hk.OrganizationID = null.StringFrom(branch.OrganizationID) + hk.BranchID = null.ValueFrom(branch.BranchID) + hk.BlockID = null.StringFrom("b1") + hk.CreatedAt = hk.CreatedAt.Add(time.Duration(i) * time.Second) + + if fn != nil { + fn(i, hk) + } + }) + + doc, err := db.FetchDocumentUnsafeByBranchID(context.Background(), branch.BranchID) + require.NoError(t, err) + + return doc, hooks + } + cc := map[string]func(*testing.T, *DB) tcase{ "Branch of another organization": func(t *testing.T, db *DB) tcase { - doc, _ := prepHistoryBranch(t, db, 1, nil) + doc, _ := prepBranch(t, db, 1, nil) return tcase{ BranchID: doc.BranchID, @@ -1090,13 +1132,14 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { } }, "Ordinary entry lists the hooks its content holds": func(t *testing.T, db *DB) tcase { - doc, hooks := prepHistoryBranch(t, db, 4, func(i int, hk *hook.Hook) { + doc, hooks := prepBranch(t, db, 4, func(i int, hk *hook.Hook) { switch i { case 1: - // the block is back; the sweep lifts the mark later. + // soft-deleted, but its block is in the content, so it + // is kept. hk.SoftDeletedAt = null.TimeFrom(timeutil.Now()) case 2: - // the block is gone; the sweep marks it later. + // its block is not in the content, so it is left out. hk.BlockID = null.StringFrom("removed") case 3: hk.BlockID = null.String{} @@ -1119,7 +1162,7 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { } }, "Boundary entry adds its own row": func(t *testing.T, db *DB) tcase { - doc, hooks := prepHistoryBranch(t, db, 1, nil) + doc, hooks := prepBranch(t, db, 1, nil) insertHistoryEntry(t, db, history.NewEntry(*doc, timeutil.Now(), null.String{}, history.Hooks{}, false)) @@ -1134,7 +1177,7 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { // a persist records inside the transaction that wrote the branch, // so the entry has to hold that write. "Inside a transaction the entry holds its write": func(t *testing.T, db *DB) tcase { - doc, hooks := prepHistoryBranch(t, db, 1, nil) + doc, hooks := prepBranch(t, db, 1, nil) var tx *Tx @@ -1198,6 +1241,98 @@ func Test_agent_RecordDocumentBranchHistoryEntry(t *testing.T) { } } +func Test_agent_fetchDocumentBranchHistoryHooks(t *testing.T) { + type tcase struct { + Context context.Context + BranchID xid.ID + Result []hook.Hook + Err error + } + + cc := map[string]func(*testing.T, *DB) tcase{ + "Cancelled context": func(_ *testing.T, _ *DB) tcase { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + return tcase{ + Context: ctx, + BranchID: xid.New(), + Err: assert.AnError, + } + }, + "Branch without hooks": func(_ *testing.T, _ *DB) tcase { + return tcase{ + Context: context.Background(), + BranchID: xid.New(), + Result: []hook.Hook{}, + } + }, + "Soft deleted hooks are included": func(t *testing.T, db *DB) tcase { + hooks := prepDocumentHooks(t, db, 2, func(i int, hk *hook.Hook) { + hk.CreatedAt = hk.CreatedAt.Add(time.Duration(i) * time.Second) + + if i == 1 { + hk.SoftDeletedAt = null.TimeFrom(timeutil.Now().Truncate(time.Second)) + } + }) + + return tcase{ + Context: context.Background(), + BranchID: hooks[0].BranchID.V, + Result: hooks, + } + }, + "Hooks of another branch are excluded": func(t *testing.T, db *DB) tcase { + hooks := prepDocumentHooks(t, db, 1, nil) + prepDocumentHooks(t, db, 1, nil) + + return tcase{ + Context: context.Background(), + BranchID: hooks[0].BranchID.V, + Result: hooks, + } + }, + "Ordered by creation time then id": func(t *testing.T, db *DB) tcase { + ids := []xid.ID{xid.New(), xid.New()} + + hooks := prepDocumentHooks(t, db, 3, func(i int, hk *hook.Hook) { + switch i { + case 0: + hk.CreatedAt = hk.CreatedAt.Add(time.Second) + case 1: + hk.ID = ids[1] + case 2: + hk.ID = ids[0] + } + }) + + return tcase{ + Context: context.Background(), + BranchID: hooks[0].BranchID.V, + Result: []hook.Hook{hooks[2], hooks[1], hooks[0]}, + } + }, + } + + for cn, cfn := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + db := prepTempDB(t) + c := cfn(t, db) + + res, err := db.fetchDocumentBranchHistoryHooks(c.Context, db.sql, c.BranchID) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + testutil.AssertFilterEqual(t, c.Result, res) + }) + } +} + func Test_agent_fetchNewestDocumentBranchHistoryEntry(t *testing.T) { t.Parallel() diff --git a/server/core/internal/db/document_file.go b/server/core/internal/db/document_file.go index 7d302bcd..fa7f0b81 100644 --- a/server/core/internal/db/document_file.go +++ b/server/core/internal/db/document_file.go @@ -139,10 +139,9 @@ func (a *agent) CheckDocumentFileReferenced(ctx context.Context, id string, docu // DeleteExpiredDocumentBranchHistoryEntries removes history entries created // before the given time, except the newest entry of each branch. Age -// trimming happens here rather than on insert because an idle branch never -// inserts again, and its entries would otherwise pin the files they -// reference forever. The newest entry pins nothing the live content does -// not. +// trimming runs here, not on insert, because an idle branch never inserts +// again. The newest entry is kept because it pins only files the live +// content holds. func (a *agent) DeleteExpiredDocumentBranchHistoryEntries(ctx context.Context, before time.Time) error { q, args := a.builder.Delete("document_branch_history_entries e"). Where(sq.Lt{"e.created_at": before}). diff --git a/server/core/internal/document/AGENTS.md b/server/core/internal/document/AGENTS.md index a2e3d2c6..ae15ba0d 100644 --- a/server/core/internal/document/AGENTS.md +++ b/server/core/internal/document/AGENTS.md @@ -28,8 +28,7 @@ reads it and its hooks, in the transaction that wrote the change. - Edits in one 30-minute bucket (by `created_at`) update the newest entry and its `updated_at`. An entry equal to the newest is skipped. A - null author (system write, no maintainers) never replaces a set one. -- An entry lists the hooks whose block its content holds. + null author (system write) never replaces a set one. - Hook create, update and delete flush the branch, then record an entry in their transaction. Reset does not. - Create, duplicate, fork and merge write a **boundary** entry that never @@ -60,9 +59,8 @@ request handler: later; NULL-FK rows skip the wait; files younger than a day are never touched. - `hook/manager` does the same for hooks, calling `Hook.Delete` before - dropping a row with a NULL FK. A merge detaches - the target's hooks (`DetachDocumentHooksByBranchID`) rather than - soft-deleting them. + dropping a row with a NULL FK. A merge detaches the target's hooks + (`DetachDocumentHooksByBranchID`) rather than soft-deleting them. - **Organization deletion is announced**: auth-realtime's `beforeDeleteOrganization` calls `POST /api/x/organizations/{id}/teardown` while rows still exist and throws if core fails. diff --git a/server/core/internal/document/history/history.go b/server/core/internal/document/history/history.go index 3369821a..613db05c 100644 --- a/server/core/internal/document/history/history.go +++ b/server/core/internal/document/history/history.go @@ -54,8 +54,7 @@ type Entry struct { // UpdatedAt is the timestamp of the last edit folded into the entry. UpdatedAt time.Time `json:"updatedAt" db:"updated_at"` - // Checksum is the stored Sum of the entry. Empty until the entry is - // stored. + // Checksum is the Sum stored with the entry. Unset on a new entry. Checksum string `json:"-" db:"checksum"` } @@ -83,17 +82,12 @@ func NewEntry(doc document.Document, at time.Time, by null.String, hooks Hooks, // Sum sums what the entry records: name, icon, content and hooks. Two // entries with the same sum restore the same branch. func (e Entry) Sum() (string, error) { - hooks := e.Hooks - if hooks == nil { - hooks = Hooks{} - } - raw, err := json.Marshal(struct { DocumentName string `json:"documentName"` Icon string `json:"icon"` Content document.RootBlock `json:"content"` Hooks Hooks `json:"hooks"` - }{e.DocumentName, e.Icon, e.Content, hooks}) + }{e.DocumentName, e.Icon, e.Content, e.Hooks}) if err != nil { return "", fmt.Errorf("marshaling history entry: %w", err) } @@ -120,8 +114,8 @@ type Hook struct { // Hooks is the list of hooks stored on a history entry. type Hooks []Hook -// NewHooks reduces hooks to what a history entry of the document records: -// those whose block its content holds, soft-deleted or not. +// NewHooks keeps what a history entry of doc records: document-level hooks +// and those whose block doc's content holds, soft-deleted or not. func NewHooks(doc document.Document, hooks []hook.Hook) Hooks { res := make(Hooks, 0, len(hooks)) diff --git a/server/core/internal/document/history/history_test.go b/server/core/internal/document/history/history_test.go index 1b607a16..77cc3682 100644 --- a/server/core/internal/document/history/history_test.go +++ b/server/core/internal/document/history/history_test.go @@ -1,7 +1,6 @@ package history import ( - "slices" "testing" "time" @@ -65,6 +64,8 @@ func Test_NewEntry(t *testing.T) { } func Test_Entry_Sum(t *testing.T) { + t.Parallel() + base := NewEntry( document.Document{ ID: xid.New(), @@ -90,6 +91,9 @@ func Test_Entry_Sum(t *testing.T) { false, ) + baseSum, err := base.Sum() + require.NoError(t, err) + cc := map[string]struct { Edit func(e *Entry) Same bool @@ -107,7 +111,7 @@ func Test_Entry_Sum(t *testing.T) { "Other hooks": { Edit: func(e *Entry) { e.Hooks = Hooks{} }, }, - "Other id, author, times, boundary and stored checksum": { + "Fields outside the snapshot": { Edit: func(e *Entry) { e.ID = xid.New() e.Checksum = "stale" @@ -141,36 +145,19 @@ func Test_Entry_Sum(t *testing.T) { t.Parallel() entry := base - entry.Content.Content = slices.Clone(base.Content.Content) c.Edit(&entry) sum, err := entry.Sum() - testutil.RequireEqualError(t, c.Err, err) + testutil.AssertEqualError(t, c.Err, err) if err != nil { return } - baseSum, err := base.Sum() - require.NoError(t, err) - assert.Len(t, sum, 64) assert.Equal(t, c.Same, sum == baseSum) }) } - - // nil hooks sum like no hooks, as both are stored as []. - empty := base - empty.Hooks = Hooks{} - - emptySum, err := empty.Sum() - require.NoError(t, err) - - empty.Hooks = nil - - nilSum, err := empty.Sum() - require.NoError(t, err) - assert.Equal(t, emptySum, nilSum) } func Test_NewHooks(t *testing.T) { @@ -185,40 +172,84 @@ func Test_NewHooks(t *testing.T) { }, } - assert.Equal(t, Hooks{}, NewHooks(doc, nil)) - assert.Equal(t, Hooks{ - { - Type: hook.TypeURLWatcher, - BlockID: null.StringFrom("b1"), - Settings: processor.Settings(`{"url":"https://example.com"}`), + cc := map[string]struct { + Hooks []hook.Hook + Result Hooks + }{ + "Nil hooks": { + Result: Hooks{}, }, - { - Type: hook.TypeScheduledReminder, - Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + "Document level hook is kept": { + Hooks: []hook.Hook{ + { + ID: xid.New(), + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + }, + }, + Result: Hooks{ + { + Type: hook.TypeScheduledReminder, + Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + }, + }, }, - }, NewHooks(doc, []hook.Hook{ - { - ID: xid.New(), - Type: hook.TypeURLWatcher, - BlockID: null.StringFrom("b1"), - Settings: processor.Settings(`{"url":"https://example.com"}`), - State: null.ValueFrom(processor.State(`{"watcher":"w1"}`)), - // the block is back, so the sweep lifts the mark on its next - // run. - SoftDeletedAt: null.TimeFrom(time.Now()), + "Hook whose block is present is kept": { + Hooks: []hook.Hook{ + { + ID: xid.New(), + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + State: null.ValueFrom(processor.State(`{"watcher":"w1"}`)), + }, + }, + Result: Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + }, }, - { - ID: xid.New(), - Type: hook.TypeURLWatcher, - BlockID: null.StringFrom("removed"), - Settings: processor.Settings(`{"url":"https://example.org"}`), + "Soft deleted hook whose block is present is kept": { + Hooks: []hook.Hook{ + { + ID: xid.New(), + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + SoftDeletedAt: null.TimeFrom(time.Now()), + }, + }, + Result: Hooks{ + { + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + }, }, - { - ID: xid.New(), - Type: hook.TypeScheduledReminder, - Settings: processor.Settings(`{"cron":"0 9 * * 1"}`), + "Hook whose block is absent is dropped": { + Hooks: []hook.Hook{ + { + ID: xid.New(), + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("removed"), + Settings: processor.Settings(`{"url":"https://example.org"}`), + }, + }, + Result: Hooks{}, }, - })) + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, c.Result, NewHooks(doc, c.Hooks)) + }) + } } func Test_Hooks_Value(t *testing.T) { diff --git a/server/core/internal/document/hook/hook.go b/server/core/internal/document/hook/hook.go index ec8c0137..761f3a04 100644 --- a/server/core/internal/document/hook/hook.go +++ b/server/core/internal/document/hook/hook.go @@ -177,12 +177,10 @@ func NewHook( } // NewCopy returns a copy of the hook for another branch, anchored to the -// given block. It is initializing, with a null state, until its first run -// sets it up, since the source's state can name the source's own watcher. -// The score is copied, so that run does not announce a change the source -// already had. -// A copy within the hook's document keeps its cross-branch ID; a copy into -// another document is a hook of its own. +// given block. The copy is initializing, with no state, since the source's +// state may name the source's own watcher. It keeps the score, so its +// first run does not announce a change the source already had. A copy +// within the document keeps the cross-branch ID. func (h *Hook) NewCopy(documentID, branchID xid.ID, blockID null.String) Hook { id := xid.New() diff --git a/server/core/internal/document/hook/hook_test.go b/server/core/internal/document/hook/hook_test.go index ffa42136..a95bcf7b 100644 --- a/server/core/internal/document/hook/hook_test.go +++ b/server/core/internal/document/hook/hook_test.go @@ -59,72 +59,75 @@ func Test_NewHook(t *testing.T) { documentID, branchID := xid.New(), xid.New() - t.Run("Valid settings create a reset hook", func(t *testing.T) { - t.Parallel() + cc := map[string]struct { + Inp CreateInput + Input *Input + Err error + }{ + "Malformed settings": { + Inp: CreateInput{ + Type: TypeScheduledReminder, + Settings: processor.Settings(`{not json`), + }, + Err: ErrInvalidSettings, + }, + "Invalid settings": { + Inp: CreateInput{ + Type: TypeScheduledReminder, + Settings: processor.Settings(`{"scale":"bogus","schedule":"2030-01-01T00:00:00Z"}`), + }, + Err: processor.ErrInvalidScaleType, + }, + "Unknown type": { + Inp: CreateInput{ + Type: Type("bogus"), + Settings: processor.Settings(`{}`), + }, + Err: ErrInvalidType, + }, + "Hook that cannot check its target is refused": { + Inp: CreateInput{ + Type: TypeURLWatcher, + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + Input: NewInput("org-1", nil, webchange.NewClient("", "")), + Err: errutil.New(http.StatusUnprocessableEntity, "document_hook.unconfigured", "the integration the hook needs is not configured"), + }, + "Successful creation": { + Inp: CreateInput{ + Type: TypeScheduledReminder, + BlockID: null.StringFrom("block-1"), + Settings: reminderSettings(t, time.Now().Add(time.Hour)), + }, + }, + } - h, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - BlockID: null.StringFrom("block-1"), - Settings: reminderSettings(t, time.Now().Add(time.Hour)), - }, documentID, branchID, "org-1", nil) - require.NoError(t, err) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - assert.False(t, h.ID.IsZero()) - assert.Equal(t, h.ID, h.CrossBranchID) - assert.Equal(t, TypeScheduledReminder, h.Type) - assert.Equal(t, null.ValueFrom(documentID), h.DocumentID) - assert.Equal(t, null.ValueFrom(branchID), h.BranchID) - assert.Equal(t, null.StringFrom("org-1"), h.OrganizationID) - assert.Equal(t, null.StringFrom("block-1"), h.BlockID) - assert.False(t, h.CreatedAt.IsZero()) + h, err := NewHook(context.Background(), c.Inp, documentID, branchID, "org-1", c.Input) + testutil.AssertEqualError(t, c.Err, err) - // the reset scored the fresh future schedule at full. - assert.True(t, h.Score.Equal(decimal.NewFromInt(100))) - assert.True(t, h.State.Valid) - assert.Equal(t, processor.StatusActive, h.Status) - }) + if err != nil { + return + } - t.Run("Malformed settings fail", func(t *testing.T) { - t.Parallel() + assert.False(t, h.ID.IsZero()) + assert.Equal(t, h.ID, h.CrossBranchID) + assert.Equal(t, c.Inp.Type, h.Type) + assert.Equal(t, null.ValueFrom(documentID), h.DocumentID) + assert.Equal(t, null.ValueFrom(branchID), h.BranchID) + assert.Equal(t, null.StringFrom("org-1"), h.OrganizationID) + assert.Equal(t, c.Inp.BlockID, h.BlockID) + assert.False(t, h.CreatedAt.IsZero()) - _, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - Settings: processor.Settings(`{not json`), - }, documentID, branchID, "org-1", nil) - assert.Equal(t, ErrInvalidSettings, err) - }) - - t.Run("Invalid settings fail", func(t *testing.T) { - t.Parallel() - - _, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - Settings: processor.Settings(`{"scale":"bogus","schedule":"2030-01-01T00:00:00Z"}`), - }, documentID, branchID, "org-1", nil) - assert.Equal(t, processor.ErrInvalidScaleType, err) - }) - - t.Run("Unknown type fails", func(t *testing.T) { - t.Parallel() - - _, err := NewHook(context.Background(), CreateInput{ - Type: Type("bogus"), - Settings: processor.Settings(`{}`), - }, documentID, branchID, "org-1", nil) - assert.Equal(t, ErrInvalidType, err) - }) - - t.Run("A hook that cannot check its target is refused", func(t *testing.T) { - t.Parallel() - - _, err := NewHook(context.Background(), CreateInput{ - Type: TypeURLWatcher, - Settings: processor.Settings(`{"url":"https://example.com"}`), - }, documentID, branchID, "org-1", NewInput("org-1", nil, webchange.NewClient("", ""))) - require.Error(t, err) - assert.Equal(t, http.StatusUnprocessableEntity, errutil.StatusCode(err, false)) - assert.EqualError(t, err, "the integration the hook needs is not configured") - }) + // the reset scored the fresh future schedule at full. + assert.True(t, h.Score.Equal(decimal.NewFromInt(100))) + assert.True(t, h.State.Valid) + assert.Equal(t, processor.StatusActive, h.Status) + }) + } } func Test_Hook_NewCopy(t *testing.T) { @@ -194,113 +197,166 @@ func Test_Hook_NewCopy(t *testing.T) { func Test_Hook_ApplyUpdate(t *testing.T) { t.Parallel() - h, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - Settings: reminderSettings(t, time.Now().Add(time.Hour)), - }, xid.New(), xid.New(), "org-1", nil) - require.NoError(t, err) + cc := map[string]struct { + Settings processor.Settings + Err error + }{ + "Malformed settings": { + Settings: processor.Settings(`{not json`), + Err: ErrInvalidSettings, + }, + // an already elapsed schedule resets the score straight to zero. + "Successful update": { + Settings: reminderSettings(t, time.Now().Add(time.Second)), + }, + } - // an already-elapsed schedule resets the score straight to zero. - newSettings := reminderSettings(t, time.Now().Add(time.Second)) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - require.NoError(t, h.ApplyUpdate(context.Background(), UpdateInput{Settings: newSettings}, nil)) + h, err := NewHook(context.Background(), CreateInput{ + Type: TypeScheduledReminder, + Settings: reminderSettings(t, time.Now().Add(time.Hour)), + }, xid.New(), xid.New(), "org-1", nil) + require.NoError(t, err) - assert.Equal(t, newSettings, h.Settings) - assert.True(t, h.UpdatedAt.Valid) - assert.True(t, h.Score.Equal(decimal.Zero)) + err = h.ApplyUpdate(context.Background(), UpdateInput{Settings: c.Settings}, nil) + testutil.AssertEqualError(t, c.Err, err) - assert.Equal(t, ErrInvalidSettings, h.ApplyUpdate(context.Background(), UpdateInput{ - Settings: processor.Settings(`{not json`), - }, nil)) + if err != nil { + return + } + + assert.Equal(t, c.Settings, h.Settings) + assert.True(t, h.UpdatedAt.Valid) + assert.True(t, h.Score.Equal(decimal.Zero)) + }) + } } func Test_Hook_Process(t *testing.T) { t.Parallel() - t.Run("Elapsed schedule scores zero", func(t *testing.T) { - t.Parallel() + // the state started two hours ago, so the schedule has elapsed. + elapsed, err := json.Marshal(processor.ScheduledReminderState{ + StartedAt: time.Now().Add(-2 * time.Hour), + }) + require.NoError(t, err) - h, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - Settings: reminderSettings(t, time.Now().Add(-time.Hour)), - }, xid.New(), xid.New(), "org-1", nil) - require.NoError(t, err) + cc := map[string]struct { + Hook Hook + Input *Input + Err error + Check func(*testing.T, Hook) + }{ + "Malformed settings": { + Hook: Hook{ + Type: TypeScheduledReminder, + Settings: processor.Settings(`{not json`), + State: null.ValueFrom(processor.State(`{}`)), + }, + Err: assert.AnError, + }, + "Elapsed schedule scores zero": { + Hook: Hook{ + Type: TypeScheduledReminder, + Settings: reminderSettings(t, time.Now().Add(-time.Hour)), + State: null.ValueFrom(processor.State(elapsed)), + Score: decimal.NewFromInt(100), + Status: processor.StatusActive, + }, + Check: func(t *testing.T, h Hook) { + assert.Equal(t, processor.StatusActive, h.Status) + assert.True(t, h.Score.Equal(decimal.Zero)) + }, + }, + "Hook never set up is reset": { + Hook: Hook{ + Type: TypeScheduledReminder, + Settings: reminderSettings(t, time.Now().Add(time.Hour)), + Status: processor.StatusActive, + }, + Check: func(t *testing.T, h Hook) { + assert.True(t, h.State.Valid) + assert.True(t, h.Score.Equal(decimal.NewFromInt(100))) + }, + }, + "Run that cannot check keeps score and state": { + Hook: Hook{ + Type: TypeURLWatcher, + Settings: processor.Settings(`{"url":"https://example.com"}`), + State: null.ValueFrom(processor.State(`{"watcherId":"w1"}`)), + Score: decimal.NewFromInt(40), + Status: processor.StatusActive, + }, + Input: NewInput("org-1", nil, webchange.NewClient("", "")), + Check: func(t *testing.T, h Hook) { + assert.Equal(t, processor.StatusUnconfigured, h.Status) + assert.True(t, h.Score.Equal(decimal.NewFromInt(40))) + assert.Equal(t, processor.State(`{"watcherId":"w1"}`), h.State.V) + }, + }, + } - // backdate the started-at state so the schedule has elapsed. - state, merr := json.Marshal(processor.ScheduledReminderState{ - StartedAt: time.Now().Add(-2 * time.Hour), + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + h := c.Hook + + err := h.Process(context.Background(), c.Input) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + c.Check(t, h) }) - require.NoError(t, merr) - - h.State = null.ValueFrom(processor.State(state)) - - require.NoError(t, h.Process(context.Background(), nil)) - assert.True(t, h.Score.Equal(decimal.Zero)) - }) - - t.Run("A hook never set up is reset", func(t *testing.T) { - t.Parallel() - - h := &Hook{ - Type: TypeScheduledReminder, - Settings: reminderSettings(t, time.Now().Add(time.Hour)), - Status: processor.StatusActive, - } - - require.NoError(t, h.Process(context.Background(), nil)) - assert.True(t, h.State.Valid) - assert.True(t, h.Score.Equal(decimal.NewFromInt(100))) - }) - - t.Run("A run that cannot check keeps score and state", func(t *testing.T) { - t.Parallel() - - h := &Hook{ - Type: TypeURLWatcher, - Settings: processor.Settings(`{"url":"https://example.com"}`), - State: null.ValueFrom(processor.State(`{"watcherId":"w1"}`)), - Score: decimal.NewFromInt(40), - Status: processor.StatusActive, - } - - require.NoError(t, h.Process(context.Background(), NewInput("org-1", nil, webchange.NewClient("", "")))) - assert.Equal(t, processor.StatusUnconfigured, h.Status) - assert.True(t, h.Score.Equal(decimal.NewFromInt(40))) - assert.Equal(t, processor.State(`{"watcherId":"w1"}`), h.State.V) - }) - - t.Run("Malformed settings fail", func(t *testing.T) { - t.Parallel() - - h := &Hook{ - Type: TypeScheduledReminder, - Settings: processor.Settings(`{not json`), - State: null.ValueFrom(processor.State(`{}`)), - } - - require.Error(t, h.Process(context.Background(), nil)) - }) + } } func Test_Hook_Delete(t *testing.T) { t.Parallel() - h, err := NewHook(context.Background(), CreateInput{ - Type: TypeScheduledReminder, - Settings: reminderSettings(t, time.Now().Add(time.Hour)), - }, xid.New(), xid.New(), "org-1", nil) - require.NoError(t, err) - - // scheduled reminders have no external resources; delete is a no-op. - assert.NoError(t, h.Delete(context.Background(), nil)) - - // a url watcher never set up has no watcher yet, so its teardown - // reaches nothing outside. - unset := Hook{ - Type: TypeURLWatcher, - Settings: processor.Settings(`{"url":"https://example.com"}`), + cc := map[string]struct { + Hook Hook + Err error + }{ + "Malformed settings": { + Hook: Hook{ + Type: TypeScheduledReminder, + Settings: processor.Settings(`{not json`), + State: null.ValueFrom(processor.State(`{}`)), + }, + Err: assert.AnError, + }, + // a url watcher never set up has no watcher yet, so its teardown + // reaches nothing outside. + "Hook never set up holds nothing": { + Hook: Hook{ + Type: TypeURLWatcher, + Settings: processor.Settings(`{"url":"https://example.com"}`), + }, + }, + // scheduled reminders have no external resources. + "Hook without external resources": { + Hook: Hook{ + Type: TypeScheduledReminder, + Settings: reminderSettings(t, time.Now().Add(time.Hour)), + State: null.ValueFrom(processor.State(`{}`)), + }, + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, c.Hook.Delete(context.Background(), nil)) + }) } - assert.NoError(t, unset.Delete(context.Background(), nil)) } func Test_Hook_ChangedFrom(t *testing.T) { @@ -358,9 +414,9 @@ func Test_Hook_ensurePrepared(t *testing.T) { t.Parallel() cc := map[string]struct { - Type Type - Settings processor.Settings - ExpectErr bool + Type Type + Settings processor.Settings + Err error }{ "Scheduled reminder settings prepare": { Type: TypeScheduledReminder, @@ -379,14 +435,14 @@ func Test_Hook_ensurePrepared(t *testing.T) { Settings: processor.Settings(`{"image": "nginx:latest"}`), }, "Malformed settings fail": { - Type: TypeScheduledReminder, - Settings: processor.Settings(`{not json`), - ExpectErr: true, + Type: TypeScheduledReminder, + Settings: processor.Settings(`{not json`), + Err: assert.AnError, }, "Unknown type fails": { - Type: Type("bogus"), - Settings: processor.Settings(`{}`), - ExpectErr: true, + Type: Type("bogus"), + Settings: processor.Settings(`{}`), + Err: assert.AnError, }, } @@ -397,16 +453,14 @@ func Test_Hook_ensurePrepared(t *testing.T) { h := &Hook{Type: c.Type, Settings: c.Settings} err := h.ensurePrepared() + testutil.AssertEqualError(t, c.Err, err) + assert.Equal(t, err == nil, h.prepared) - if c.ExpectErr { - require.Error(t, err) - assert.False(t, h.prepared) - + if err != nil { return } - require.NoError(t, err) - assert.True(t, h.prepared) + assert.NotNil(t, h.runner) assert.NotNil(t, h.runner) // preparation is memoized. diff --git a/server/core/internal/document/hook/manager/copy.go b/server/core/internal/document/hook/manager/copy.go index 3413b586..5ed0aaa5 100644 --- a/server/core/internal/document/hook/manager/copy.go +++ b/server/core/internal/document/hook/manager/copy.go @@ -2,15 +2,16 @@ package manager import ( "context" + "fmt" "github.com/guregu/null/v5" "github.com/rs/xid" ) -// CopyHooks copies the branch's hooks to another in the caller's -// transaction, not set up yet. newBlockIDs maps the source's block ids to -// the copy's, for a copy whose blocks were given new ids. When set, it -// re-anchors block hooks and drops those whose block it does not name. +// CopyHooks inserts copies of the branch's hooks into another branch, in +// the caller's transaction. The copies are not set up. When newBlockIDs is +// set, block hooks follow it to their new block and those it does not name +// are dropped. func CopyHooks( ctx context.Context, tx CopyTx, @@ -20,7 +21,7 @@ func CopyHooks( ) error { hooks, err := tx.FetchDocumentHooksByBranchID(ctx, fromBranchID, organizationID) if err != nil { - return err + return fmt.Errorf("fetching branch hooks: %w", err) } for _, hk := range hooks { @@ -36,7 +37,7 @@ func CopyHooks( } if err := tx.InsertDocumentHook(ctx, hk.NewCopy(documentID, toBranchID, blockID)); err != nil { - return err + return fmt.Errorf("inserting hook copy: %w", err) } } diff --git a/server/core/internal/document/hook/manager/copy_test.go b/server/core/internal/document/hook/manager/copy_test.go index a0c923cc..e158bf48 100644 --- a/server/core/internal/document/hook/manager/copy_test.go +++ b/server/core/internal/document/hook/manager/copy_test.go @@ -26,11 +26,11 @@ func Test_CopyHooks(t *testing.T) { Blocks []null.String Err error }{ - "Error returned by tx.FetchDocumentHooksByBranchID": { + "Error returned by CopyTx.FetchDocumentHooksByBranchID": { FetchErr: assert.AnError, Err: assert.AnError, }, - "Error returned by tx.InsertDocumentHook": { + "Error returned by CopyTx.InsertDocumentHook": { InsertErr: assert.AnError, Blocks: []null.String{null.StringFrom("old-uid")}, Err: assert.AnError, diff --git a/server/core/internal/document/hook/manager/manager.go b/server/core/internal/document/hook/manager/manager.go index 213f13b1..5ae51e74 100644 --- a/server/core/internal/document/hook/manager/manager.go +++ b/server/core/internal/document/hook/manager/manager.go @@ -110,7 +110,7 @@ func (m *Manager) SetUpBranch(ctx context.Context, branchID xid.ID, organization if err != nil { m.log.With("branch_id", branchID). With("error", err). - Error("fetching branch hooks") + Error("cannot fetch branch hooks") return } @@ -170,23 +170,23 @@ func (m *Manager) processHook(ctx context.Context, ps *ProcessingState, h hook.H if err != nil { m.log.With("hook_id", h.ID). With("error", err). - Error("waiting for the hook's lock") + Error("cannot lock hook") return } defer unlock() - // the row may have changed between the page read and the lock. A hook - // cut loose from its organization is out of any write's reach, so its - // row is as it was read. + // re-read the row, since a write may have changed it before the lock. + // A hook without an organization cannot be written, so the row read + // stands. if h.OrganizationID.Valid { stored, err := m.db.FetchDocumentHook(ctx, h.ID, h.OrganizationID.String) if err != nil { if !errutil.IsNotFound(err) { m.log.With("hook_id", h.ID). With("error", err). - Error("fetching document hook") + Error("cannot fetch document hook") } return @@ -358,9 +358,9 @@ func (m *Manager) notifyTransition(ctx context.Context, prev, h hook.Hook) { m.notifPub.PublishNotifications(h.OrganizationID.String, core, maintainers...) } -// undoSetup tears down the resource h got when it was set up, since its -// row was not stored. Nothing else points at the resource, so a failure -// is reported. +// undoSetup tears down the resource h got in this run, since the row +// naming it was not stored. Nothing else names it, so a failure is +// reported. func (m *Manager) undoSetup(ctx context.Context, prev, h hook.Hook) { if prev.State.Valid || !h.State.Valid { return diff --git a/server/core/internal/document/hook/manager/manager_test.go b/server/core/internal/document/hook/manager/manager_test.go index 80f6f012..ceec2a7b 100644 --- a/server/core/internal/document/hook/manager/manager_test.go +++ b/server/core/internal/document/hook/manager/manager_test.go @@ -317,9 +317,9 @@ func Test_Manager_processHooks(t *testing.T) { } } - wasNotified := func(count int) check { - return func(t *testing.T, _ *DBMock, notifier *changeRecorder, _ *fakePublisher, _ error) { - require.Len(t, notifier.hooks, count) + wasChangeCalled := func(count int) check { + return func(t *testing.T, _ *DBMock, changes *changeRecorder, _ *fakePublisher, _ error) { + require.Len(t, changes.hooks, count) } } @@ -384,7 +384,7 @@ func Test_Manager_processHooks(t *testing.T) { Write func(hook.Hook) hook.Hook Checks []check }{ - "Hook fetch failure is propagated": { + "Error returned by db.FetchPaginatedDocumentHooks": { Hooks: func(*testing.T) []hook.Hook { return nil }, FetchErr: assert.AnError, Checks: checks( @@ -442,7 +442,7 @@ func Test_Manager_processHooks(t *testing.T) { wasDeleteCalled(1), wasFetchDocumentCalled(0), wasUpdateCalled(0), - wasNotified(1), + wasChangeCalled(1), ), }, "Orphaned hook without a document is deleted": { @@ -640,7 +640,7 @@ func Test_Manager_processHooks(t *testing.T) { wasUpdateCalled(1), hasUpdatedScore(decimal.NewFromInt(100)), wasPublished(), - wasNotified(0), + wasChangeCalled(0), ), }, "Full-to-zero score drop notifies the maintainers": { @@ -653,7 +653,7 @@ func Test_Manager_processHooks(t *testing.T) { wasUpdateCalled(1), hasUpdatedScore(decimal.Zero), wasPublished(notification.NotificationDocumentHookTriggered), - wasNotified(1), + wasChangeCalled(1), ), }, // the score decays gradually, so by the time it reaches zero the @@ -717,7 +717,7 @@ func Test_Manager_processHooks(t *testing.T) { hasUpdatedStatus(processor.StatusActive), hasUpdatedScore(decimal.Zero), wasPublished(notification.NotificationDocumentHookTriggered), - wasNotified(1), + wasChangeCalled(1), ), }, "Maintainer fetch failure suppresses the notification": { @@ -852,10 +852,10 @@ func Test_Manager_processHooks(t *testing.T) { } pub := &fakePublisher{} - notifier := &changeRecorder{} + changes := &changeRecorder{} man := newTestManager(t, db, pub, wc) - man.BindHookChange(notifier.record) + man.BindHookChange(changes.record) var err error @@ -866,7 +866,7 @@ func Test_Manager_processHooks(t *testing.T) { } for _, ch := range c.Checks { - ch(t, db, notifier, pub, err) + ch(t, db, changes, pub, err) } if cd != nil { diff --git a/server/core/internal/document/hook/manager/write_test.go b/server/core/internal/document/hook/manager/write_test.go index 820a741f..6a1aeb11 100644 --- a/server/core/internal/document/hook/manager/write_test.go +++ b/server/core/internal/document/hook/manager/write_test.go @@ -70,6 +70,7 @@ func Test_Manager_CreateHook(t *testing.T) { Tx *TxMock Inserts int Commits int + Rollbacks int Watchers []string Err error }{ @@ -138,9 +139,10 @@ func Test_Manager_CreateHook(t *testing.T) { return assert.AnError }, }, - Inserts: 1, - Watchers: []string{"w-new"}, - Err: assert.AnError, + Inserts: 1, + Watchers: []string{"w-new"}, + Rollbacks: 1, + Err: assert.AnError, }, "Error returned by tx.Commit": { Type: hook.TypeURLWatcher, @@ -151,10 +153,11 @@ func Test_Manager_CreateHook(t *testing.T) { return assert.AnError }, }, - Inserts: 1, - Commits: 1, - Watchers: []string{"w-new"}, - Err: assert.AnError, + Inserts: 1, + Commits: 1, + Watchers: []string{"w-new"}, + Rollbacks: 1, + Err: assert.AnError, }, // the hook was not stored, so the watcher it created is torn down. "Error returned by tx.InsertDocumentHook": { @@ -166,18 +169,20 @@ func Test_Manager_CreateHook(t *testing.T) { return assert.AnError }, }, - Inserts: 1, - Watchers: []string{"w-new"}, - Err: assert.AnError, + Inserts: 1, + Watchers: []string{"w-new"}, + Rollbacks: 1, + Err: assert.AnError, }, "Successful creation": { - Type: hook.TypeURLWatcher, - BlockID: null.StringFrom("b1"), - Settings: processor.Settings(`{"url":"https://example.com"}`), - CD: true, - Tx: &TxMock{}, - Inserts: 1, - Commits: 1, + Type: hook.TypeURLWatcher, + BlockID: null.StringFrom("b1"), + Settings: processor.Settings(`{"url":"https://example.com"}`), + CD: true, + Tx: &TxMock{}, + Inserts: 1, + Commits: 1, + Rollbacks: 1, }, } @@ -233,6 +238,7 @@ func Test_Manager_CreateHook(t *testing.T) { require.Len(t, c.Tx.InsertDocumentHookCalls(), c.Inserts) assert.Len(t, c.Tx.CommitCalls(), c.Commits) + assert.Len(t, c.Tx.RollbackCalls(), c.Rollbacks) if cd != nil { assert.Equal(t, c.Watchers, cd.deletedWatchers()) @@ -302,6 +308,7 @@ func Test_Manager_UpdateHook(t *testing.T) { CommitErr error Updates int Records int + Rollbacks int Watchers []string Err error }{ @@ -345,6 +352,7 @@ func Test_Manager_UpdateHook(t *testing.T) { UpdateErr: assert.AnError, Updates: 1, Watchers: []string{"w-new"}, + Rollbacks: 1, Err: assert.AnError, }, "Error returned by tx.RecordDocumentBranchHistoryEntry tears down the setup": { @@ -353,6 +361,7 @@ func Test_Manager_UpdateHook(t *testing.T) { Updates: 1, Records: 1, Watchers: []string{"w-new"}, + Rollbacks: 1, Err: assert.AnError, }, "Error returned by tx.Commit tears down the setup": { @@ -361,17 +370,20 @@ func Test_Manager_UpdateHook(t *testing.T) { Updates: 1, Records: 1, Watchers: []string{"w-new"}, + Rollbacks: 1, Err: assert.AnError, }, "Hook without a branch has no history": { Settings: settings, Branchless: true, Updates: 1, + Rollbacks: 1, }, "Successful update": { - Settings: settings, - Updates: 1, - Records: 1, + Settings: settings, + Updates: 1, + Records: 1, + Rollbacks: 1, }, } @@ -438,6 +450,7 @@ func Test_Manager_UpdateHook(t *testing.T) { testutil.AssertEqualError(t, c.Err, err) assert.Len(t, tx.UpdateDocumentHookCalls(), c.Updates) + assert.Len(t, tx.RollbackCalls(), c.Rollbacks) assert.Equal(t, c.Watchers, cd.deletedWatchers()) rr := tx.RecordDocumentBranchHistoryEntryCalls() @@ -478,6 +491,9 @@ func Test_Manager_DeleteHook(t *testing.T) { branchID := xid.New() cc := map[string]struct { + // Held has another write hold the hook's lock until the delete + // gives up. + Held bool Hook func(*testing.T) hook.Hook OtherDocument bool // CDURL points changedetection at this address. @@ -490,8 +506,16 @@ func Test_Manager_DeleteHook(t *testing.T) { CommitErr error Deletes int Records int + Rollbacks int Err error }{ + "Hook another write holds": { + Held: true, + Hook: func(t *testing.T) hook.Hook { + return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + }, + Err: context.DeadlineExceeded, + }, "Error returned by db.FetchDocumentHook": { Hook: func(t *testing.T) hook.Hook { return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) @@ -543,9 +567,10 @@ func Test_Manager_DeleteHook(t *testing.T) { Hook: func(t *testing.T) hook.Hook { return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) }, - DelErr: assert.AnError, - Deletes: 1, - Err: assert.AnError, + DelErr: assert.AnError, + Deletes: 1, + Rollbacks: 1, + Err: assert.AnError, }, "Error returned by tx.RecordDocumentBranchHistoryEntry": { Hook: func(t *testing.T) hook.Hook { @@ -554,6 +579,7 @@ func Test_Manager_DeleteHook(t *testing.T) { RecordErr: assert.AnError, Deletes: 1, Records: 1, + Rollbacks: 1, Err: assert.AnError, }, "Error returned by tx.Commit": { @@ -563,6 +589,7 @@ func Test_Manager_DeleteHook(t *testing.T) { CommitErr: assert.AnError, Deletes: 1, Records: 1, + Rollbacks: 1, Err: assert.AnError, }, "Hook never set up is deleted": { @@ -572,8 +599,9 @@ func Test_Manager_DeleteHook(t *testing.T) { return hk }, - Deletes: 1, - Records: 1, + Deletes: 1, + Records: 1, + Rollbacks: 1, }, "Hook without a branch has no history": { Hook: func(t *testing.T) hook.Hook { @@ -582,14 +610,16 @@ func Test_Manager_DeleteHook(t *testing.T) { return hk }, - Deletes: 1, + Deletes: 1, + Rollbacks: 1, }, "Successful deletion": { Hook: func(t *testing.T) hook.Hook { return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) }, - Deletes: 1, - Records: 1, + Deletes: 1, + Records: 1, + Rollbacks: 1, }, } @@ -630,12 +660,26 @@ func Test_Manager_DeleteHook(t *testing.T) { changes := &changeRecorder{} man.BindHookChange(changes.record) + ctx := context.Background() + + if c.Held { + unlock, err := man.hookMu.Lock(ctx, hk.ID) + require.NoError(t, err) + + defer unlock() + + var cancel context.CancelFunc + + ctx, cancel = context.WithTimeout(ctx, 50*time.Millisecond) + defer cancel() + } + documentID := hk.DocumentID.V if c.OtherDocument { documentID = xid.New() } - err := man.DeleteHook(context.Background(), hk.ID, documentID, "org-1", "u1") + err := man.DeleteHook(ctx, hk.ID, documentID, "org-1", "u1") testutil.AssertEqualError(t, c.Err, err) // only a service the hook could not reach is reported as one. @@ -646,6 +690,7 @@ func Test_Manager_DeleteHook(t *testing.T) { ff := tx.DeleteDocumentHookCalls() require.Len(t, ff, c.Deletes) assert.Len(t, tx.RecordDocumentBranchHistoryEntryCalls(), c.Records) + assert.Len(t, tx.RollbackCalls(), c.Rollbacks) if err != nil { assert.Empty(t, changes.hooks) @@ -671,6 +716,9 @@ func Test_Manager_ResetHook(t *testing.T) { branchID := xid.New() cc := map[string]struct { + // Held has another write hold the hook's lock until the reset + // gives up. + Held bool Hook func(*testing.T) hook.Hook OtherDocument bool CD bool @@ -683,6 +731,13 @@ func Test_Manager_ResetHook(t *testing.T) { Watchers []string Err error }{ + "Hook another write holds": { + Held: true, + Hook: func(t *testing.T) hook.Hook { + return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) + }, + Err: context.DeadlineExceeded, + }, "Error returned by db.FetchDocumentHook": { Hook: func(t *testing.T) hook.Hook { return stubHook(t, branchID, time.Now().Add(time.Hour), time.Now()) @@ -774,12 +829,26 @@ func Test_Manager_ResetHook(t *testing.T) { changes := &changeRecorder{} man.BindHookChange(changes.record) + ctx := context.Background() + + if c.Held { + unlock, err := man.hookMu.Lock(ctx, stored.ID) + require.NoError(t, err) + + defer unlock() + + var cancel context.CancelFunc + + ctx, cancel = context.WithTimeout(ctx, 50*time.Millisecond) + defer cancel() + } + documentID := stored.DocumentID.V if c.OtherDocument { documentID = xid.New() } - hk, err := man.ResetHook(context.Background(), stored.ID, documentID, "org-1") + hk, err := man.ResetHook(ctx, stored.ID, documentID, "org-1") testutil.AssertEqualError(t, c.Err, err) ff := db.UpdateDocumentHookCalls() diff --git a/server/core/internal/document/hook/processor/container_image_watcher_test.go b/server/core/internal/document/hook/processor/container_image_watcher_test.go index eb4d6271..a6eaa3ea 100644 --- a/server/core/internal/document/hook/processor/container_image_watcher_test.go +++ b/server/core/internal/document/hook/processor/container_image_watcher_test.go @@ -74,134 +74,148 @@ func imageWatcherState(t *testing.T, digest string) State { func Test_ContainerImageWatcher_Validate(t *testing.T) { t.Parallel() - assert.NoError(t, (&ContainerImageWatcher{Image: "nginx:1.27"}).Validate()) - testutil.AssertEqualError(t, ErrInvalidImage, (&ContainerImageWatcher{Image: "INVALID image ref"}).Validate()) + cc := map[string]struct { + Image string + Err error + }{ + "Valid image reference": {Image: "nginx:1.27"}, + "Invalid image is rejected": {Image: "INVALID image ref", Err: ErrInvalidImage}, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + testutil.AssertEqualError(t, c.Err, (&ContainerImageWatcher{Image: c.Image}).Validate()) + }) + } } func Test_ContainerImageWatcher_Process(t *testing.T) { t.Parallel() - t.Run("Unchanged digest keeps the full score", func(t *testing.T) { - t.Parallel() + cc := map[string]struct { + // Image is watched instead of the fake registry's image. + Image string + Unauthorized bool + // Missing watches a tag the fake registry does not hold. + Missing bool + // Stale has the state hold an old digest instead of the current one. + Stale bool + // State is sent instead of a state built from the digest. + State State + Status Status + Score decimal.Decimal + Err error + }{ + "Malformed state": { + Image: "INVALID image ref", + State: State(`{not json`), + Err: assert.AnError, + }, + "Invalid image reference": { + Image: "INVALID image ref", + Err: assert.AnError, + }, + "Unauthorized registry is a status": { + Unauthorized: true, + Status: StatusUnauthorized, + }, + "Missing image is a status": { + Missing: true, + Status: StatusImageNotFound, + }, + "Changed digest drops the score to zero": { + Stale: true, + Status: StatusActive, + Score: decimal.Zero, + }, + "Unchanged digest keeps the full score": { + Status: StatusActive, + Score: decimal.NewFromInt(100), + }, + } - image, digest := newFakeRegistryImage(t, false) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - ciw := ContainerImageWatcher{Image: image} + image, digest := c.Image, "sha256:old" + if image == "" { + image, digest = newFakeRegistryImage(t, c.Unauthorized) + } - res, err := ciw.Process(context.Background(), stubInput{ - state: imageWatcherState(t, digest), + if c.Missing { + image = strings.Replace(image, ":v1", ":v2", 1) + } + + if c.Stale { + digest = "sha256:old" + } + + state := c.State + if state == nil { + state = imageWatcherState(t, digest) + } + + res, err := (&ContainerImageWatcher{Image: image}).Process(context.Background(), stubInput{state: state}) + testutil.AssertEqualError(t, c.Err, err) + + if err != nil { + return + } + + assert.Equal(t, c.Status, res.Status) + assert.True(t, res.Score.Equal(c.Score), "score %s", res.Score) + + if c.Status != StatusActive { + assert.Nil(t, res.State) + + return + } + + assert.Equal(t, state, res.State) }) - require.NoError(t, err) - - assert.Equal(t, StatusActive, res.Status) - assert.True(t, res.Score.Equal(decimal.NewFromInt(100))) - - var ciws ContainerImageWatcherState - - require.NoError(t, json.Unmarshal(res.State, &ciws)) - assert.Equal(t, digest, ciws.Digest) - }) - - t.Run("Changed digest drops the score to zero", func(t *testing.T) { - t.Parallel() - - image, _ := newFakeRegistryImage(t, false) - - ciw := ContainerImageWatcher{Image: image} - - res, err := ciw.Process(context.Background(), stubInput{ - state: imageWatcherState(t, "sha256:old"), - }) - require.NoError(t, err) - - assert.True(t, res.Score.Equal(decimal.Zero)) - }) - - t.Run("Unauthorized registry is a status", func(t *testing.T) { - t.Parallel() - - image, _ := newFakeRegistryImage(t, true) - - ciw := ContainerImageWatcher{Image: image} - - res, err := ciw.Process(context.Background(), stubInput{ - state: imageWatcherState(t, "sha256:old"), - }) - require.NoError(t, err) - - assert.Equal(t, StatusUnauthorized, res.Status) - assert.Nil(t, res.State) - }) - - t.Run("Missing image is a status", func(t *testing.T) { - t.Parallel() - - image, _ := newFakeRegistryImage(t, false) - - ciw := ContainerImageWatcher{Image: strings.Replace(image, ":v1", ":v2", 1)} - - res, err := ciw.Process(context.Background(), stubInput{ - state: imageWatcherState(t, "sha256:old"), - }) - require.NoError(t, err) - - assert.Equal(t, StatusImageNotFound, res.Status) - }) - - t.Run("Invalid image reference fails", func(t *testing.T) { - t.Parallel() - - ciw := ContainerImageWatcher{Image: "INVALID image ref"} - - _, err := ciw.Process(context.Background(), stubInput{ - state: imageWatcherState(t, ""), - }) - require.Error(t, err) - }) - - t.Run("Malformed state fails", func(t *testing.T) { - t.Parallel() - - ciw := ContainerImageWatcher{} - - _, err := ciw.Process(context.Background(), stubInput{state: State(`{not json`)}) - require.Error(t, err) - }) + } } func Test_ContainerImageWatcher_Reset(t *testing.T) { t.Parallel() - t.Run("Reset adopts the current digest at full score", func(t *testing.T) { - t.Parallel() + cc := map[string]struct { + Unauthorized bool + Status Status + Score decimal.Decimal + }{ + "Unauthorized registry is a status": { + Unauthorized: true, + Status: StatusUnauthorized, + }, + "Reset adopts the current digest at full score": { + Status: StatusActive, + Score: decimal.NewFromInt(100), + }, + } - image, digest := newFakeRegistryImage(t, false) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - ciw := ContainerImageWatcher{Image: image} + image, digest := newFakeRegistryImage(t, c.Unauthorized) - res, err := ciw.Reset(context.Background(), stubInput{}) - require.NoError(t, err) + res, err := (&ContainerImageWatcher{Image: image}).Reset(context.Background(), stubInput{}) + require.NoError(t, err) - assert.Equal(t, StatusActive, res.Status) - assert.True(t, res.Score.Equal(decimal.NewFromInt(100))) + assert.Equal(t, c.Status, res.Status) + assert.True(t, res.Score.Equal(c.Score), "score %s", res.Score) - var ciws ContainerImageWatcherState + if c.Status != StatusActive { + assert.Nil(t, res.State) - require.NoError(t, json.Unmarshal(res.State, &ciws)) - assert.Equal(t, digest, ciws.Digest) - }) + return + } - t.Run("Unauthorized registry is a status", func(t *testing.T) { - t.Parallel() - - image, _ := newFakeRegistryImage(t, true) - - ciw := ContainerImageWatcher{Image: image} - - res, err := ciw.Reset(context.Background(), stubInput{}) - require.NoError(t, err) - - assert.Equal(t, StatusUnauthorized, res.Status) - assert.Nil(t, res.State) - }) + assert.Equal(t, imageWatcherState(t, digest), res.State) + }) + } } diff --git a/server/core/internal/document/hook/processor/github_tracking_test.go b/server/core/internal/document/hook/processor/github_tracking_test.go index 28b71926..3a27e837 100644 --- a/server/core/internal/document/hook/processor/github_tracking_test.go +++ b/server/core/internal/document/hook/processor/github_tracking_test.go @@ -58,7 +58,7 @@ func Test_GithubTracking_Validate(t *testing.T) { t.Run(cn, func(t *testing.T) { t.Parallel() - assert.Equal(t, c.Err, c.Tracking.Validate()) + testutil.AssertEqualError(t, c.Err, c.Tracking.Validate()) }) } } diff --git a/server/core/internal/document/hook/processor/scheduled_reminder_test.go b/server/core/internal/document/hook/processor/scheduled_reminder_test.go index 898da3de..0902cb44 100644 --- a/server/core/internal/document/hook/processor/scheduled_reminder_test.go +++ b/server/core/internal/document/hook/processor/scheduled_reminder_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/shopspring/decimal" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -26,65 +27,63 @@ func Test_ScheduledReminder_Process(t *testing.T) { now := time.Now() - tests := map[string]struct { - Reminder ScheduledReminder - State State - ExpectErr bool - ExpectedScore decimal.Decimal + cc := map[string]struct { + Reminder ScheduledReminder + State State + Score decimal.Decimal + Err error }{ "Halfway through scores about half": { Reminder: ScheduledReminder{ Scale: ScaleTypeLinear, Schedule: now.Add(time.Hour), }, - State: reminderState(t, now.Add(-time.Hour)), - ExpectedScore: decimal.NewFromInt(50), + State: reminderState(t, now.Add(-time.Hour)), + Score: decimal.NewFromInt(50), }, "Elapsed schedule scores zero": { Reminder: ScheduledReminder{ Scale: ScaleTypeLinear, Schedule: now.Add(-time.Minute), }, - State: reminderState(t, now.Add(-time.Hour)), - ExpectedScore: decimal.Zero, + State: reminderState(t, now.Add(-time.Hour)), + Score: decimal.Zero, }, "Fresh schedule scores full": { Reminder: ScheduledReminder{ Scale: ScaleTypeLinear, Schedule: now.Add(1000 * time.Hour), }, - State: reminderState(t, now), - ExpectedScore: decimal.NewFromInt(100), + State: reminderState(t, now), + Score: decimal.NewFromInt(100), }, "Malformed state fails": { Reminder: ScheduledReminder{ Scale: ScaleTypeLinear, }, - State: State(`{not json`), - ExpectErr: true, + State: State(`{not json`), + Err: assert.AnError, }, } - for name, tc := range tests { - t.Run(name, func(t *testing.T) { + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { t.Parallel() - res, err := tc.Reminder.Process(context.Background(), stubInput{state: tc.State}) - - if tc.ExpectErr { - require.Error(t, err) + res, err := c.Reminder.Process(context.Background(), stubInput{state: c.State}) + testutil.AssertEqualError(t, c.Err, err) + if err != nil { return } - require.NoError(t, err) assert.Equal(t, StatusActive, res.Status) assert.True( t, - res.Score.Sub(tc.ExpectedScore).Abs().LessThanOrEqual(decimal.NewFromInt(1)), - "score %s should be within 1 of %s", res.Score, tc.ExpectedScore, + res.Score.Sub(c.Score).Abs().LessThanOrEqual(decimal.NewFromInt(1)), + "score %s should be within 1 of %s", res.Score, c.Score, ) - assert.JSONEq(t, string(tc.State), string(res.State)) + assert.JSONEq(t, string(c.State), string(res.State)) }) } } @@ -92,47 +91,49 @@ func Test_ScheduledReminder_Process(t *testing.T) { func Test_ScheduledReminder_Reset(t *testing.T) { t.Parallel() - t.Run("Future schedule starts at full score", func(t *testing.T) { - t.Parallel() - - sr := ScheduledReminder{ - Scale: ScaleTypeLinear, + cc := map[string]struct { + Schedule time.Time + Score decimal.Decimal + }{ + "Future schedule starts at full score": { Schedule: time.Now().Add(time.Hour), - } - - res, err := sr.Reset(context.Background(), stubInput{}) - require.NoError(t, err) - - assert.Equal(t, StatusActive, res.Status) - assert.True(t, res.Score.Equal(decimal.NewFromInt(100))) - - var srs ScheduledReminderState - - require.NoError(t, json.Unmarshal(res.State, &srs)) - assert.False(t, srs.StartedAt.IsZero()) - }) - - t.Run("Schedule inside the grace period starts at zero", func(t *testing.T) { - t.Parallel() - - sr := ScheduledReminder{ - Scale: ScaleTypeLinear, + Score: decimal.NewFromInt(100), + }, + "Schedule inside the grace period starts at zero": { Schedule: time.Now().Add(time.Second), - } + Score: decimal.Zero, + }, + } - res, err := sr.Reset(context.Background(), stubInput{}) - require.NoError(t, err) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - assert.True(t, res.Score.Equal(decimal.Zero)) - }) + sr := ScheduledReminder{ + Scale: ScaleTypeLinear, + Schedule: c.Schedule, + } + + res, err := sr.Reset(context.Background(), stubInput{}) + require.NoError(t, err) + + assert.Equal(t, StatusActive, res.Status) + assert.True(t, res.Score.Equal(c.Score), "score %s", res.Score) + + var srs ScheduledReminderState + + require.NoError(t, json.Unmarshal(res.State, &srs)) + assert.False(t, srs.StartedAt.IsZero()) + }) + } } func Test_ScheduledReminder_Validate(t *testing.T) { t.Parallel() - tests := map[string]struct { - Reminder ScheduledReminder - ExpectedErr error + cc := map[string]struct { + Reminder ScheduledReminder + Err error }{ "Linear scale with a schedule is valid": { Reminder: ScheduledReminder{ @@ -145,21 +146,21 @@ func Test_ScheduledReminder_Validate(t *testing.T) { Scale: ScaleType("exponential"), Schedule: time.Now().Add(time.Hour), }, - ExpectedErr: ErrInvalidScaleType, + Err: ErrInvalidScaleType, }, "Missing schedule is rejected": { Reminder: ScheduledReminder{ Scale: ScaleTypeLinear, }, - ExpectedErr: ErrMissingSchedule, + Err: ErrMissingSchedule, }, } - for name, tc := range tests { - t.Run(name, func(t *testing.T) { + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.ExpectedErr, tc.Reminder.Validate()) + testutil.AssertEqualError(t, c.Err, c.Reminder.Validate()) }) } } diff --git a/server/core/internal/document/hook/processor/url_watcher_test.go b/server/core/internal/document/hook/processor/url_watcher_test.go index 7515e32d..a67c4144 100644 --- a/server/core/internal/document/hook/processor/url_watcher_test.go +++ b/server/core/internal/document/hook/processor/url_watcher_test.go @@ -8,6 +8,7 @@ import ( "github.com/guregu/null/v5" "github.com/oxynote/oxynote/server/core/internal/apps/webchange" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/shopspring/decimal" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -50,12 +51,12 @@ func (f *fakeChangeDetection) DeleteWatcher(_ context.Context, watchID string) e return f.deleteErr } -// watcherState marshals a URL-watcher state for the watcher "w-1". -func watcherState(t *testing.T, lastChangedAt null.Time) State { +// watcherState marshals a URL-watcher state. +func watcherState(t *testing.T, watcherID string, lastChangedAt null.Time) State { t.Helper() raw, err := json.Marshal(URLWatcherState{ - WatcherID: "w-1", + WatcherID: watcherID, LastChangedAt: lastChangedAt, }) require.NoError(t, err) @@ -66,23 +67,23 @@ func watcherState(t *testing.T, lastChangedAt null.Time) State { func Test_URLWatcher_Validate(t *testing.T) { t.Parallel() - tests := map[string]struct { - URL string - ExpectedErr error + cc := map[string]struct { + URL string + Err error }{ "Https URL is valid": {URL: "https://example.com/page"}, "Http URL is valid": {URL: "http://example.com"}, - "Other scheme is rejected": {URL: "ftp://example.com", ExpectedErr: ErrInvalidURL}, - "Relative URL is rejected": {URL: "/page", ExpectedErr: ErrInvalidURL}, - "Missing host is rejected": {URL: "https://", ExpectedErr: ErrInvalidURL}, - "Unparsable URL is rejected": {URL: "https://exa mple.com/%zz", ExpectedErr: ErrInvalidURL}, + "Other scheme is rejected": {URL: "ftp://example.com", Err: ErrInvalidURL}, + "Relative URL is rejected": {URL: "/page", Err: ErrInvalidURL}, + "Missing host is rejected": {URL: "https://", Err: ErrInvalidURL}, + "Unparsable URL is rejected": {URL: "https://exa mple.com/%zz", Err: ErrInvalidURL}, } - for name, tc := range tests { - t.Run(name, func(t *testing.T) { + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.ExpectedErr, (&URLWatcher{URL: tc.URL}).Validate()) + testutil.AssertEqualError(t, c.Err, (&URLWatcher{URL: c.URL}).Validate()) }) } } @@ -92,103 +93,101 @@ func Test_URLWatcher_Process(t *testing.T) { changed := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - tests := map[string]struct { - CD *fakeChangeDetection - State State - ExpectErr bool - ExpectedScore decimal.Decimal - ExpectedStatus Status - Updated [][2]string + cc := map[string]struct { + CD *fakeChangeDetection + State State + Score decimal.Decimal + Status Status + Updated [][2]string + Err error }{ "Unchanged watch keeps the full score": { CD: &fakeChangeDetection{ watch: &webchange.Watch{URL: "https://example.com", LastChangedAt: changed}, }, - State: watcherState(t, null.TimeFrom(changed)), - ExpectedScore: decimal.NewFromInt(100), - ExpectedStatus: StatusActive, + State: watcherState(t, "w-1", null.TimeFrom(changed)), + Score: decimal.NewFromInt(100), + Status: StatusActive, }, "Newer change drops the score to zero": { CD: &fakeChangeDetection{ watch: &webchange.Watch{URL: "https://example.com", LastChangedAt: changed.Add(time.Hour)}, }, - State: watcherState(t, null.TimeFrom(changed)), - ExpectedScore: decimal.Zero, - ExpectedStatus: StatusActive, + State: watcherState(t, "w-1", null.TimeFrom(changed)), + Score: decimal.Zero, + Status: StatusActive, }, "Missing baseline adopts the watch timestamp": { CD: &fakeChangeDetection{ watch: &webchange.Watch{URL: "https://example.com", LastChangedAt: changed}, }, - State: watcherState(t, null.Time{}), - ExpectedScore: decimal.NewFromInt(100), - ExpectedStatus: StatusActive, + State: watcherState(t, "w-1", null.Time{}), + Score: decimal.NewFromInt(100), + Status: StatusActive, }, "Unreachable watch is a status": { CD: &fakeChangeDetection{ watch: &webchange.Watch{URL: "https://example.com", Unreachable: true}, }, - State: watcherState(t, null.Time{}), - ExpectedStatus: StatusUnreachableURL, + State: watcherState(t, "w-1", null.Time{}), + Status: StatusUnreachableURL, }, "Watcher on another URL is pointed back": { CD: &fakeChangeDetection{ watch: &webchange.Watch{URL: "https://old.example.com", LastChangedAt: changed}, }, - State: watcherState(t, null.TimeFrom(changed)), - ExpectedScore: decimal.NewFromInt(100), - ExpectedStatus: StatusActive, - Updated: [][2]string{{"w-1", "https://example.com"}}, + State: watcherState(t, "w-1", null.TimeFrom(changed)), + Score: decimal.NewFromInt(100), + Status: StatusActive, + Updated: [][2]string{{"w-1", "https://example.com"}}, }, "Removed watcher is created again": { CD: &fakeChangeDetection{ fetchErr: webchange.ErrWatcherNotFound, createdID: "w-new", }, - State: watcherState(t, null.Time{}), - ExpectedScore: decimal.NewFromInt(100), - ExpectedStatus: StatusActive, + State: watcherState(t, "w-1", null.Time{}), + Score: decimal.NewFromInt(100), + Status: StatusActive, }, "Unconfigured changedetection is a status": { - CD: &fakeChangeDetection{fetchErr: webchange.ErrNotConfigured}, - State: watcherState(t, null.Time{}), - ExpectedStatus: StatusUnconfigured, + CD: &fakeChangeDetection{fetchErr: webchange.ErrNotConfigured}, + State: watcherState(t, "w-1", null.Time{}), + Status: StatusUnconfigured, }, "Fetch failure is propagated": { - CD: &fakeChangeDetection{fetchErr: assert.AnError}, - State: watcherState(t, null.Time{}), - ExpectErr: true, + CD: &fakeChangeDetection{fetchErr: assert.AnError}, + State: watcherState(t, "w-1", null.Time{}), + Err: assert.AnError, }, "Malformed state fails": { - CD: &fakeChangeDetection{}, - State: State(`{not json`), - ExpectErr: true, + CD: &fakeChangeDetection{}, + State: State(`{not json`), + Err: assert.AnError, }, } - for name, tc := range tests { - t.Run(name, func(t *testing.T) { + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { t.Parallel() uw := URLWatcher{URL: "https://example.com"} - res, err := uw.Process(context.Background(), stubInput{state: tc.State, cd: tc.CD}) - - if tc.ExpectErr { - require.Error(t, err) + res, err := uw.Process(context.Background(), stubInput{state: c.State, cd: c.CD}) + testutil.AssertEqualError(t, c.Err, err) + if err != nil { return } - require.NoError(t, err) - assert.Equal(t, tc.ExpectedStatus, res.Status) - assert.True(t, res.Score.Equal(tc.ExpectedScore), "score %s", res.Score) + assert.Equal(t, c.Status, res.Status) + assert.True(t, res.Score.Equal(c.Score), "score %s", res.Score) - if tc.ExpectedStatus != StatusActive { + if c.Status != StatusActive { assert.Nil(t, res.State) } - assert.Equal(t, tc.Updated, tc.CD.updated) + assert.Equal(t, c.Updated, c.CD.updated) }) } } @@ -198,183 +197,150 @@ func Test_URLWatcher_Reset(t *testing.T) { changed := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) - t.Run("Missing watcher is created", func(t *testing.T) { - t.Parallel() + cc := map[string]struct { + CD *fakeChangeDetection + State State + Created []string + Updated [][2]string + Result Result + Err error + }{ + "Missing watcher is created": { + CD: &fakeChangeDetection{createdID: "w-new"}, + Created: []string{"https://example.com"}, + Result: Result{ + Status: StatusActive, + Score: decimal.NewFromInt(100), + State: watcherState(t, "w-new", null.Time{}), + }, + }, + "Existing watcher adopts the watch timestamp": { + CD: &fakeChangeDetection{ + watch: &webchange.Watch{URL: "https://example.com", LastChangedAt: changed}, + }, + State: watcherState(t, "w-1", null.Time{}), + Result: Result{ + Status: StatusActive, + Score: decimal.NewFromInt(100), + State: watcherState(t, "w-1", null.TimeFrom(changed)), + }, + }, + "Changed URL updates the watcher and clears the baseline": { + CD: &fakeChangeDetection{ + watch: &webchange.Watch{URL: "https://old.example.com", LastChangedAt: changed}, + }, + State: watcherState(t, "w-1", null.Time{}), + Updated: [][2]string{{"w-1", "https://example.com"}}, + Result: Result{ + Status: StatusActive, + Score: decimal.NewFromInt(100), + State: watcherState(t, "w-1", null.Time{}), + }, + }, + "Removed watcher is created again": { + CD: &fakeChangeDetection{ + fetchErr: webchange.ErrWatcherNotFound, + createdID: "w-new", + }, + State: watcherState(t, "w-1", null.Time{}), + Created: []string{"https://example.com"}, + Result: Result{ + Status: StatusActive, + Score: decimal.NewFromInt(100), + State: watcherState(t, "w-new", null.Time{}), + }, + }, + "Unconfigured changedetection is a status": { + CD: &fakeChangeDetection{createErr: webchange.ErrNotConfigured}, + Created: []string{"https://example.com"}, + Result: Result{Status: StatusUnconfigured}, + }, + "Error returned by ChangeDetection.UpdateWatcher": { + CD: &fakeChangeDetection{ + watch: &webchange.Watch{URL: "https://old.example.com"}, + updateErr: assert.AnError, + }, + State: watcherState(t, "w-1", null.Time{}), + Updated: [][2]string{{"w-1", "https://example.com"}}, + Err: assert.AnError, + }, + "Error returned by ChangeDetection.CreateWatcher": { + CD: &fakeChangeDetection{createErr: assert.AnError}, + Created: []string{"https://example.com"}, + Err: assert.AnError, + }, + "Malformed state": { + CD: &fakeChangeDetection{}, + State: State(`{not json`), + Err: assert.AnError, + }, + } - cd := &fakeChangeDetection{createdID: "w-new"} - uw := URLWatcher{URL: "https://example.com"} + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - res, err := uw.Reset(context.Background(), stubInput{cd: cd}) - require.NoError(t, err) + uw := URLWatcher{URL: "https://example.com"} - assert.Equal(t, StatusActive, res.Status) - assert.True(t, res.Score.Equal(decimal.NewFromInt(100))) - assert.Equal(t, []string{"https://example.com"}, cd.createdURLs) + res, err := uw.Reset(context.Background(), stubInput{state: c.State, cd: c.CD}) + testutil.AssertEqualError(t, c.Err, err) - var uws URLWatcherState + assert.Equal(t, c.Created, c.CD.createdURLs) + assert.Equal(t, c.Updated, c.CD.updated) - require.NoError(t, json.Unmarshal(res.State, &uws)) - assert.Equal(t, "w-new", uws.WatcherID) - assert.False(t, uws.LastChangedAt.Valid) - }) + if err != nil { + return + } - t.Run("Existing watcher adopts the watch timestamp", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{ - watch: &webchange.Watch{URL: "https://example.com", LastChangedAt: changed}, - } - uw := URLWatcher{URL: "https://example.com"} - - res, err := uw.Reset(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, + assert.Equal(t, c.Result.Status, res.Status) + assert.True(t, res.Score.Equal(c.Result.Score), "score %s", res.Score) + assert.Equal(t, c.Result.State, res.State) }) - require.NoError(t, err) - - var uws URLWatcherState - - require.NoError(t, json.Unmarshal(res.State, &uws)) - assert.Equal(t, null.TimeFrom(changed), uws.LastChangedAt) - assert.Empty(t, cd.updated) - }) - - t.Run("Changed URL updates the watcher and clears the baseline", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{ - watch: &webchange.Watch{URL: "https://old.example.com", LastChangedAt: changed}, - } - uw := URLWatcher{URL: "https://example.com"} - - res, err := uw.Reset(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, - }) - require.NoError(t, err) - - assert.Equal(t, [][2]string{{"w-1", "https://example.com"}}, cd.updated) - - var uws URLWatcherState - - require.NoError(t, json.Unmarshal(res.State, &uws)) - assert.Equal(t, "w-1", uws.WatcherID) - assert.False(t, uws.LastChangedAt.Valid) - }) - - t.Run("Removed watcher is created again", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{ - fetchErr: webchange.ErrWatcherNotFound, - createdID: "w-new", - } - - res, err := (&URLWatcher{URL: "https://example.com"}).Reset(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, - }) - require.NoError(t, err) - - var uws URLWatcherState - - require.NoError(t, json.Unmarshal(res.State, &uws)) - assert.Equal(t, "w-new", uws.WatcherID) - }) - - t.Run("Unconfigured changedetection is a status", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{createErr: webchange.ErrNotConfigured} - - res, err := (&URLWatcher{URL: "https://example.com"}).Reset(context.Background(), stubInput{cd: cd}) - require.NoError(t, err) - - assert.Equal(t, StatusUnconfigured, res.Status) - assert.Nil(t, res.State) - }) - - t.Run("Update failure is propagated", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{ - watch: &webchange.Watch{URL: "https://old.example.com"}, - updateErr: assert.AnError, - } - - _, err := (&URLWatcher{URL: "https://example.com"}).Reset(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, - }) - require.Error(t, err) - }) - - t.Run("Create failure is propagated", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{createErr: assert.AnError} - - _, err := (&URLWatcher{URL: "https://example.com"}).Reset(context.Background(), stubInput{cd: cd}) - require.Error(t, err) - }) - - t.Run("Malformed state fails", func(t *testing.T) { - t.Parallel() - - _, err := (&URLWatcher{}).Reset(context.Background(), stubInput{ - state: State(`{not json`), - cd: &fakeChangeDetection{}, - }) - require.Error(t, err) - }) + } } func Test_URLWatcher_Delete(t *testing.T) { t.Parallel() - t.Run("Existing watcher is deleted", func(t *testing.T) { - t.Parallel() + cc := map[string]struct { + CD *fakeChangeDetection + State State + Deleted []string + Err error + }{ + "Missing state is a no-op": { + CD: &fakeChangeDetection{}, + }, + "State without a watcher is a no-op": { + CD: &fakeChangeDetection{}, + State: State(`{}`), + }, + "Malformed state": { + CD: &fakeChangeDetection{}, + State: State(`{not json`), + Err: assert.AnError, + }, + "Error returned by ChangeDetection.DeleteWatcher": { + CD: &fakeChangeDetection{deleteErr: assert.AnError}, + State: watcherState(t, "w-1", null.Time{}), + Deleted: []string{"w-1"}, + Err: assert.AnError, + }, + "Existing watcher is deleted": { + CD: &fakeChangeDetection{}, + State: watcherState(t, "w-1", null.Time{}), + Deleted: []string{"w-1"}, + }, + } - cd := &fakeChangeDetection{} + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - err := (&URLWatcher{}).Delete(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, + err := (&URLWatcher{}).Delete(context.Background(), stubInput{state: c.State, cd: c.CD}) + testutil.AssertEqualError(t, c.Err, err) + + assert.Equal(t, c.Deleted, c.CD.deletedIDs) }) - require.NoError(t, err) - assert.Equal(t, []string{"w-1"}, cd.deletedIDs) - }) - - t.Run("Missing state is a no-op", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{} - - require.NoError(t, (&URLWatcher{}).Delete(context.Background(), stubInput{cd: cd})) - assert.Empty(t, cd.deletedIDs) - }) - - t.Run("State without a watcher is a no-op", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{} - - require.NoError(t, (&URLWatcher{}).Delete(context.Background(), stubInput{ - state: State(`{}`), - cd: cd, - })) - assert.Empty(t, cd.deletedIDs) - }) - - t.Run("Delete failure is propagated", func(t *testing.T) { - t.Parallel() - - cd := &fakeChangeDetection{deleteErr: assert.AnError} - - err := (&URLWatcher{}).Delete(context.Background(), stubInput{ - state: watcherState(t, null.Time{}), - cd: cd, - }) - require.Error(t, err) - }) + } } diff --git a/server/core/internal/notification/interpreter/interpreter_test.go b/server/core/internal/notification/interpreter/interpreter_test.go index a9579d1a..186017e0 100644 --- a/server/core/internal/notification/interpreter/interpreter_test.go +++ b/server/core/internal/notification/interpreter/interpreter_test.go @@ -250,8 +250,10 @@ func Test_Interpreter_interpretDocumentReviewRequestNotification(t *testing.T) { func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { cc := map[string]struct { - DB *DBMock - N notification.Notification + DB *DBMock + N notification.Notification + // Format defaults to _hookTriggeredFormat. + Format string Result *Message Err error }{ @@ -362,6 +364,20 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { Text: fmt.Sprintf("<%s#blk1|a block in My Doc> may be outdated — Scheduled Reminder", _testDocURL), }, }, + "Successful interpretation with the needs attention format": { + DB: stubDB(), + N: stubNotification(notification.NewDocumentHookNeedsAttentionNotification( + _testDocID, + hook.TypeURLWatcher, + null.StringFrom("blk1"), + _testBranchID, + processor.StatusUnreachableURL, + )), + Format: _hookNeedsAttentionFormat, + Result: &Message{ + Text: fmt.Sprintf("<%s#blk1|a block in My Doc> can no longer be checked — Website Changes", _testDocURL), + }, + }, } for cn, c := range cc { @@ -370,7 +386,12 @@ func Test_Interpreter_interpretDocumentHookNotification(t *testing.T) { i := NewInterpreter(c.DB, NewSlackFormatter(), "https://app.test") - res, err := i.interpretDocumentHookNotification(context.Background(), c.N, _hookTriggeredFormat) + format := c.Format + if format == "" { + format = _hookTriggeredFormat + } + + res, err := i.interpretDocumentHookNotification(context.Background(), c.N, format) testutil.AssertEqualError(t, c.Err, err) if err != nil { diff --git a/server/core/internal/server/internal/document/branch_http.go b/server/core/internal/server/internal/document/branch_http.go index a1cbe88a..b644e7b5 100644 --- a/server/core/internal/server/internal/document/branch_http.go +++ b/server/core/internal/server/internal/document/branch_http.go @@ -192,9 +192,9 @@ func (h *Handler) UpdateDocumentBranchByIDUnsafe(w http.ResponseWriter, r *http. } // a persist that changes nothing, such as the Yjs seed Hocuspocus - // sends on first load, leaves history alone. Only a persist that names - // its editors is attributed. A system write has no user, and without - // maintainers the branch's author is whoever edited before. + // sends on first load, leaves history alone. Only a user persist + // naming its maintainers is attributed. Otherwise LastUpdatedBy still + // names an earlier editor. if !doc.SnapshotEqual(ndoc) { var lastUpdatedBy null.String if !ui.System && len(ui.Maintainers) > 0 { diff --git a/server/core/internal/server/internal/document/branch_http_test.go b/server/core/internal/server/internal/document/branch_http_test.go index ed463fd7..740e4687 100644 --- a/server/core/internal/server/internal/document/branch_http_test.go +++ b/server/core/internal/server/internal/document/branch_http_test.go @@ -17,8 +17,7 @@ import ( "github.com/stretchr/testify/require" ) -// storedHook builds a stored hook whose processor needs no external -// dependencies. +// storedHook builds a stored hook on the document's main branch. func storedHook(typ hookCore.Type) hookCore.Hook { return hookCore.Hook{ ID: xid.New(), @@ -588,7 +587,7 @@ func Test_Handler_MergeBranches(t *testing.T) { RespCode: http.StatusInternalServerError, History: 1, }, - "Error returned by tx.FetchDocumentHooksByBranchID": { + "Error returned by Tx.FetchDocumentHooksByBranchID": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, Tx: &TxMock{ FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { @@ -598,6 +597,19 @@ func Test_Handler_MergeBranches(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, }, + "Error returned by Tx.InsertDocumentHook": { + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil + }, + InsertDocumentHookFunc: func(context.Context, hookCore.Hook) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, "Commit error": { DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, Tx: &TxMock{ @@ -611,8 +623,15 @@ func Test_Handler_MergeBranches(t *testing.T) { History: 1, }, "Successful merge": { - DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, - Tx: &TxMock{}, + DB: &DBMock{FetchDocumentByBranchIDFunc: fetchByBranch}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + hk := storedHook(hookCore.TypeScheduledReminder) + hk.BranchID = null.ValueFrom(_branchID2) + + return []hookCore.Hook{hk}, nil + }, + }, Body: validBody, RespCode: http.StatusOK, Committed: 1, @@ -666,6 +685,13 @@ func Test_Handler_MergeBranches(t *testing.T) { assert.Equal(t, _branchID2, hh[0].BranchID) assert.Equal(t, "org1", hh[0].OrganizationID) + ii := c.Tx.InsertDocumentHookCalls() + require.Len(t, ii, 1) + assert.Equal(t, null.ValueFrom(_branchID), ii[0].Hk.BranchID) + assert.Equal(t, null.ValueFrom(_documentID), ii[0].Hk.DocumentID) + assert.Equal(t, processor.StatusInitializing, ii[0].Hk.Status) + assert.False(t, ii[0].Hk.State.Valid) + pp := man.SetUpBranchCalls() require.Len(t, pp, 1) assert.Equal(t, _branchID, pp[0].BranchID) @@ -840,7 +866,7 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, }, - "Error returned by tx.FetchDocumentHooksByBranchID": { + "Error returned by Tx.FetchDocumentHooksByBranchID": { DB: &DBMock{ FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { return storedDoc(), nil @@ -854,6 +880,23 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { Body: validBody, RespCode: http.StatusInternalServerError, }, + "Error returned by Tx.InsertDocumentHook": { + DB: &DBMock{ + FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { + return storedDoc(), nil + }, + }, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil + }, + InsertDocumentHookFunc: func(context.Context, hookCore.Hook) error { + return errors.New("boom") + }, + }, + Body: validBody, + RespCode: http.StatusInternalServerError, + }, "Commit error": { DB: &DBMock{ FetchDocumentByBranchIDFunc: func(context.Context, xid.ID, string) (*documentCore.Document, error) { @@ -891,7 +934,11 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { return storedDoc(), nil }, }, - Tx: &TxMock{}, + Tx: &TxMock{ + FetchDocumentHooksByBranchIDFunc: func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{storedHook(hookCore.TypeScheduledReminder)}, nil + }, + }, Body: validBody, RespCode: http.StatusCreated, Committed: 1, @@ -951,6 +998,13 @@ func Test_Handler_CreateDocumentBranch(t *testing.T) { assert.Equal(t, _branchID, hh[0].BranchID) assert.Equal(t, "org1", hh[0].OrganizationID) + ii := c.Tx.InsertDocumentHookCalls() + require.Len(t, ii, 1) + assert.Equal(t, null.ValueFrom(forked.BranchID), ii[0].Hk.BranchID) + assert.Equal(t, null.ValueFrom(_documentID), ii[0].Hk.DocumentID) + assert.Equal(t, processor.StatusInitializing, ii[0].Hk.Status) + assert.False(t, ii[0].Hk.State.Valid) + pp := man.SetUpBranchCalls() require.Len(t, pp, 1) assert.Equal(t, forked.BranchID, pp[0].BranchID) diff --git a/server/core/internal/server/internal/document/document_http.go b/server/core/internal/server/internal/document/document_http.go index 29eb9860..3c3f4775 100644 --- a/server/core/internal/server/internal/document/document_http.go +++ b/server/core/internal/server/internal/document/document_http.go @@ -841,8 +841,9 @@ func (h *Handler) upsertBranchReviewer( // its search job and the boundary history entry of its first content, and // slots it at the top of its parent's tree, all in one transaction. A // document duplicated from a branch also takes that branch's tags and its -// hooks, re-anchored through newBlockIDs. The tree-change notification is -// left to the caller, since it must not fire before the commit. +// hooks, re-anchored through newBlockIDs. Copied hooks are set up after +// the commit. The tree-change notification is left to the caller, since +// it must not fire before the commit. func (h *Handler) insertDocumentTx( ctx context.Context, doc documentCore.Document, diff --git a/server/core/internal/server/internal/document/document_http_test.go b/server/core/internal/server/internal/document/document_http_test.go index 6ef056d8..888c5a6f 100644 --- a/server/core/internal/server/internal/document/document_http_test.go +++ b/server/core/internal/server/internal/document/document_http_test.go @@ -1402,6 +1402,10 @@ func Test_Handler_CreateDocument(t *testing.T) { assert.Equal(t, "org1", entry.OrganizationID) assert.True(t, entry.Boundary) assert.Equal(t, null.StringFrom("u1"), entry.By) + + // a new document has no source branch to copy hooks from. + assert.Empty(t, c.Tx.FetchDocumentHooksByBranchIDCalls()) + assert.Empty(t, hdl.hookMan.(*HookManagerMock).SetUpBranchCalls()) } }) } @@ -1606,8 +1610,8 @@ func Test_Handler_DuplicateDocument(t *testing.T) { Committed int TreeCbs int Copies int - // CopiedUID is a source block uid the hook copy is told to - // re-anchor. + // CopiedUID is the source block uid the case's hook is anchored + // to. CopiedUID string }{ "No session in context": { @@ -1703,8 +1707,15 @@ func Test_Handler_DuplicateDocument(t *testing.T) { Copies: 1, }, "Hooks follow the regenerated block uids": { - DB: imageDB(), - Tx: insertAwareTx(), + DB: imageDB(), + Tx: func() *TxMock { + tx := insertAwareTx() + tx.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { + return []hookCore.Hook{{BlockID: null.StringFrom("img-1-aaaaaaaaaaaaaaa")}}, nil + } + + return tx + }(), Storer: &StorerMock{}, RespCode: http.StatusCreated, Committed: 1, @@ -1712,7 +1723,7 @@ func Test_Handler_DuplicateDocument(t *testing.T) { Copies: 1, CopiedUID: "img-1-aaaaaaaaaaaaaaa", }, - "Error returned by tx.FetchDocumentHooksByBranchID": { + "Error returned by Tx.FetchDocumentHooksByBranchID": { DB: &DBMock{FetchDocumentFunc: fetchStored}, Tx: func() *TxMock { tx := insertAwareTx() @@ -1739,14 +1750,6 @@ func Test_Handler_DuplicateDocument(t *testing.T) { man := &HookManagerMock{} hdl.hookMan = man - // a hook anchored to the copied block is re-anchored to its new - // uid. - if c.CopiedUID != "" { - c.Tx.FetchDocumentHooksByBranchIDFunc = func(context.Context, xid.ID, string) ([]hookCore.Hook, error) { - return []hookCore.Hook{{BlockID: null.StringFrom(c.CopiedUID)}}, nil - } - } - rec := httptest.NewRecorder() hdl.DuplicateDocument(rec, newRequest(http.MethodPost, "", c.NoSession, c.OmitDoc, true)) diff --git a/server/core/internal/server/internal/document/hook/http.go b/server/core/internal/server/internal/document/hook/http.go index c7567e74..f4bea19c 100644 --- a/server/core/internal/server/internal/document/hook/http.go +++ b/server/core/internal/server/internal/document/hook/http.go @@ -24,8 +24,7 @@ type Handler struct { } } -// NewHandler creates a new handler instance with the provided logger, -// database and hook manager. +// NewHandler creates a hook handler. func NewHandler(log *slog.Logger, db DB, hookMan Manager) *Handler { return &Handler{ log: log, diff --git a/server/core/internal/server/internal/document/hook/http_test.go b/server/core/internal/server/internal/document/hook/http_test.go index 825df9d0..2d5dd810 100644 --- a/server/core/internal/server/internal/document/hook/http_test.go +++ b/server/core/internal/server/internal/document/hook/http_test.go @@ -40,8 +40,7 @@ func addSession(ctx context.Context) context.Context { }) } -// scheduledHook builds a stored hook whose processor needs no external -// dependencies. +// scheduledHook builds a stored hook of the request's document. func scheduledHook() *hookCore.Hook { return &hookCore.Hook{ ID: _hookID, @@ -65,33 +64,6 @@ func storedHookManager() *ManagerMock { } } -// notified is one hooks-change announcement a handler made. -type notified struct { - organizationID string - documentID xid.ID - branchID xid.ID -} - -// recordNotifications binds the handler's hooks-change callback to a -// list the test reads back. -func recordNotifications(hdl *Handler) *[]notified { - var out []notified - - hdl.hooks.changeCallback = func(organizationID string, documentID, branchID xid.ID) { - out = append(out, notified{organizationID, documentID, branchID}) - } - - return &out -} - -// assertNotified checks that a write announced nothing: the hook manager -// announces the hooks it changes. -func assertNotified(t *testing.T, got []notified) { - t.Helper() - - assert.Empty(t, got) -} - func Test_NewHandler(t *testing.T) { t.Parallel() @@ -238,7 +210,6 @@ func Test_Handler_FetchDocumentHooks(t *testing.T) { log: slog.New(slog.DiscardHandler), db: c.DB, } - got := recordNotifications(&hdl) req := httptest.NewRequest(http.MethodGet, "http://test.com/"+c.Query, http.NoBody) @@ -256,9 +227,6 @@ func Test_Handler_FetchDocumentHooks(t *testing.T) { hdl.FetchDocumentHooks(rec, req.WithContext(ctx)) - // a read announces nothing. - assert.Empty(t, *got) - for _, ch := range c.Checks { ch(t, c.DB, rec) } @@ -358,14 +326,12 @@ func Test_Handler_CreateDocumentHook(t *testing.T) { t.Parallel() hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) - got := recordNotifications(hdl) rec := httptest.NewRecorder() hdl.CreateDocumentHook(rec, hookRequest(http.MethodPost, c.Body, c.NoSession, c.OmitDoc, true)) assert.Equal(t, c.RespCode, rec.Code) - assertNotified(t, *got) if c.RespBody != "" { assert.JSONEq(t, c.RespBody, rec.Body.String()) @@ -456,14 +422,12 @@ func Test_Handler_UpdateDocumentHook(t *testing.T) { t.Parallel() hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) - got := recordNotifications(hdl) rec := httptest.NewRecorder() hdl.UpdateDocumentHook(rec, hookRequest(http.MethodPut, c.Body, c.NoSession, c.OmitDoc, c.OmitHook)) assert.Equal(t, c.RespCode, rec.Code) - assertNotified(t, *got) if c.RespBody != "" { assert.JSONEq(t, c.RespBody, rec.Body.String()) @@ -537,14 +501,12 @@ func Test_Handler_ResetDocumentHook(t *testing.T) { t.Parallel() hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) - got := recordNotifications(hdl) rec := httptest.NewRecorder() hdl.ResetDocumentHook(rec, hookRequest(http.MethodPut, "", c.NoSession, c.OmitDoc, c.OmitHook)) assert.Equal(t, c.RespCode, rec.Code) - assertNotified(t, *got) ff := c.Man.ResetHookCalls() require.Len(t, ff, c.Resets) @@ -612,14 +574,12 @@ func Test_Handler_DeleteDocumentHook(t *testing.T) { t.Parallel() hdl := NewHandler(slog.New(slog.DiscardHandler), &DBMock{}, c.Man) - got := recordNotifications(hdl) rec := httptest.NewRecorder() hdl.DeleteDocumentHook(rec, hookRequest(http.MethodDelete, "", c.NoSession, c.OmitDoc, c.OmitHook)) assert.Equal(t, c.RespCode, rec.Code) - assertNotified(t, *got) ff := c.Man.DeleteHookCalls() require.Len(t, ff, c.Deletes) diff --git a/server/core/internal/server/server_test.go b/server/core/internal/server/server_test.go index 219de8b6..2503ffce 100644 --- a/server/core/internal/server/server_test.go +++ b/server/core/internal/server/server_test.go @@ -153,6 +153,8 @@ func Test_NewServer(t *testing.T) { t.Run("Successful construction", func(t *testing.T) { t.Parallel() + hookMan := &HookManagerMock{} + srv, err := NewServer( log, Options{ @@ -174,7 +176,7 @@ func Test_NewServer(t *testing.T) { githubMan, slackMan, webchange.NewClient("", ""), - &HookManagerMock{}, + hookMan, stubSearcher{}, stubSearchTrigger{}, notifier, @@ -209,6 +211,7 @@ func Test_NewServer(t *testing.T) { assert.NotNil(t, srv.handlers.email) assert.NotNil(t, srv.handlers.ai) assert.NotNil(t, srv.handlers.mcp) + assert.Len(t, hookMan.BindHookChangeCalls(), 1) // nothing above is configured, so every capability reports false; // the search gateway mock is the one switched on. diff --git a/server/core/pkg/syncutil/keyed_mutex_test.go b/server/core/pkg/syncutil/keyed_mutex_test.go index 1c14c719..44557aa8 100644 --- a/server/core/pkg/syncutil/keyed_mutex_test.go +++ b/server/core/pkg/syncutil/keyed_mutex_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/oxynote/oxynote/server/core/pkg/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -24,30 +25,54 @@ func Test_NewKeyedMutex(t *testing.T) { func Test_KeyedMutex_Lock(t *testing.T) { t.Parallel() - km := NewKeyedMutex[string]() - - unlock, err := km.Lock(context.Background(), "a") - require.NoError(t, err) - - // a second caller gives up once its context ends. - ctx, cancel := context.WithCancel(context.Background()) + ended, cancel := context.WithCancel(context.Background()) cancel() - _, err = km.Lock(ctx, "a") - assert.Equal(t, context.Canceled, err) + cc := map[string]struct { + Context context.Context + // Held is the key another caller holds during the lock. + Held string + Err error + }{ + "Free key is locked": { + Context: context.Background(), + }, + "Held key gives up when the context ends": { + Context: ended, + Held: "a", + Err: context.Canceled, + }, + "Other key is independent": { + Context: context.Background(), + Held: "b", + }, + } - // another key is not held by the first one's mutex. - other, err := km.Lock(context.Background(), "b") - require.NoError(t, err) + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() - other() - unlock() + km := NewKeyedMutex[string]() - // every mutex is dropped once no one uses it. - assert.Empty(t, km.locks) + held := func() {} - unlock, err = km.Lock(context.Background(), "a") - require.NoError(t, err) + if c.Held != "" { + var err error - unlock() + held, err = km.Lock(context.Background(), c.Held) + require.NoError(t, err) + } + + unlock, err := km.Lock(c.Context, "a") + testutil.AssertEqualError(t, c.Err, err) + + if err == nil { + unlock() + } + + held() + + assert.Empty(t, km.locks) + }) + } } diff --git a/web/app/components/NotificationBox.nuxt.test.ts b/web/app/components/NotificationBox.nuxt.test.ts index 5c20b86d..086be48b 100644 --- a/web/app/components/NotificationBox.nuxt.test.ts +++ b/web/app/components/NotificationBox.nuxt.test.ts @@ -203,35 +203,33 @@ describe("", { concurrent: false }, () => { ) }) - it("names the hook that fired", async ({ expect }) => { - seedNotifications([ - makeNotification({ + it.for([ + { + name: "names the hook that fired", + input: { code: NotificationCode.DocumentHookTrigerred, - metadata: { - documentId: DOC_ID, - branchId: "b", - blockId: null, - type: DocumentHookType.URLWatcher, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(wrapper.text()).toContain("hook was triggered") - }) - - it("names the hook that can no longer check its target", async ({ - expect, - }) => { + type: DocumentHookType.URLWatcher, + }, + expected: "notification.messages.document-hook-triggered-description", + }, + { + name: "names the hook that needs attention", + input: { + code: NotificationCode.DocumentHookNeedsAttention, + type: DocumentHookType.GitHubTracking, + }, + expected: + "notification.messages.document-hook-needs-attention-description", + }, + ])("$name", async ({ input, expected }, { expect }) => { seedNotifications([ makeNotification({ - code: NotificationCode.DocumentHookNeedsAttention, + code: input.code, metadata: { documentId: DOC_ID, branchId: "b", blockId: null, - type: DocumentHookType.GitHubTracking, + type: input.type, }, }), ]) @@ -239,9 +237,7 @@ describe("", { concurrent: false }, () => { const wrapper = await mountBox() expect(wrapper.text()).toContain( - t("notification.messages.document-hook-needs-attention-description", { - hook: t("editor.hooks.github-tracking.title"), - }), + t(expected, { hook: t(`editor.hooks.${input.type}.title`) }), ) }) @@ -333,36 +329,63 @@ describe("", { concurrent: false }, () => { }) it.for([ - { type: DocumentHookType.URLWatcher, expected: "mingcute:earth-2-line" }, { - type: DocumentHookType.GitHubTracking, + name: "marks a url watcher trigger with a globe icon", + input: { + code: NotificationCode.DocumentHookTrigerred, + type: DocumentHookType.URLWatcher, + }, + expected: "mingcute:earth-2-line", + }, + { + name: "marks a github tracking trigger with a github icon", + input: { + code: NotificationCode.DocumentHookTrigerred, + type: DocumentHookType.GitHubTracking, + }, expected: "simple-icons:github", }, - { type: DocumentHookType.ScheduledReminder, expected: "lucide:timer" }, { - type: DocumentHookType.ContainerImageWatcher, + name: "marks a scheduled reminder trigger with a timer icon", + input: { + code: NotificationCode.DocumentHookTrigerred, + type: DocumentHookType.ScheduledReminder, + }, + expected: "lucide:timer", + }, + { + name: "marks a container image watcher trigger with a container icon", + input: { + code: NotificationCode.DocumentHookTrigerred, + type: DocumentHookType.ContainerImageWatcher, + }, expected: "lucide:container", }, - ])( - "marks a $type hook trigger with its own icon", - async ({ type, expected }, { expect }) => { - seedNotifications([ - makeNotification({ - code: NotificationCode.DocumentHookTrigerred, - metadata: { - documentId: DOC_ID, - branchId: "b", - blockId: null, - type: type, - }, - }), - ]) - - const wrapper = await mountBox() - - expect(renderedIconNames(wrapper)).toContain(expected) + { + name: "marks a hook that needs attention with its own icon", + input: { + code: NotificationCode.DocumentHookNeedsAttention, + type: DocumentHookType.ContainerImageWatcher, + }, + expected: "lucide:container", }, - ) + ])("$name", async ({ input, expected }, { expect }) => { + seedNotifications([ + makeNotification({ + code: input.code, + metadata: { + documentId: DOC_ID, + branchId: "b", + blockId: null, + type: input.type, + }, + }), + ]) + + const wrapper = await mountBox() + + expect(renderedIconNames(wrapper)).toContain(expected) + }) it("marks a hook type this build does not know with a warning icon", async ({ expect, @@ -553,6 +576,19 @@ describe("", { concurrent: false }, () => { }, expected: `/acme/Runbook-${DOC_ID}`, }, + { + name: "opens a hook that needs attention at the block it watches", + notification: { + code: NotificationCode.DocumentHookNeedsAttention, + metadata: { + documentId: DOC_ID, + branchId: "b", + blockId: "block-7", + type: DocumentHookType.URLWatcher, + }, + }, + expected: `/acme/Runbook-${DOC_ID}#block-7`, + }, { name: "opens a new comment at the block it is anchored to", notification: { diff --git a/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts b/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts index 8e02bb90..4f3b158f 100644 --- a/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts +++ b/web/app/components/editor/hooks/HookConfigPanel.nuxt.test.ts @@ -179,7 +179,7 @@ describe("", { concurrent: false }, () => { expected: false, }, { - name: "says nothing of setup for a copy that cannot check its target", + name: "says nothing of setup for a copy whose integration is missing", input: { state: null, status: "unconfigured" }, expected: false, }, diff --git a/web/app/components/editor/hooks/container-image-watcher/ConfigMenu.nuxt.test.ts b/web/app/components/editor/hooks/container-image-watcher/ConfigMenu.nuxt.test.ts index 294515b0..8d878ac6 100644 --- a/web/app/components/editor/hooks/container-image-watcher/ConfigMenu.nuxt.test.ts +++ b/web/app/components/editor/hooks/container-image-watcher/ConfigMenu.nuxt.test.ts @@ -89,30 +89,30 @@ describe("", { concurrent: false }, () => { expect(menuText()).toContain(IMAGE) }) - it("says the image of a failing hook is unreachable", async ({ expect }) => { - await mountMenu({ - hook: imageHook({ - state: { digest: "sha256:old" }, - status: "unauthorized", - }), - }) + it.for([ + { + name: "an unreachable image", + input: "unauthorized", + expected: "editor.hooks.container-image-watcher.detail-unreachable", + }, + { + name: "an image gone from its registry", + input: "image_not_found", + expected: "editor.hooks.container-image-watcher.detail-missing", + }, + ] as const)( + "names $name on the hook's row", + async ({ input, expected }, { expect }) => { + await mountMenu({ hook: imageHook({ status: input }) }) - const detail = t("editor.hooks.container-image-watcher.detail-unreachable") - expect(menuText()).toContain( - t("editor.hooks.subtext-detail", { subtext: IMAGE, detail: detail }), - ) - }) - - it("says the image of a hook is gone from its registry", async ({ - expect, - }) => { - await mountMenu({ hook: imageHook({ status: "image_not_found" }) }) - - const detail = t("editor.hooks.container-image-watcher.detail-missing") - expect(menuText()).toContain( - t("editor.hooks.subtext-detail", { subtext: IMAGE, detail: detail }), - ) - }) + expect(menuText()).toContain( + t("editor.hooks.subtext-detail", { + subtext: IMAGE, + detail: t(expected), + }), + ) + }, + ) it("prefills the image reference of the hook it edits", async ({ expect, @@ -177,20 +177,24 @@ describe("", { concurrent: false }, () => { expect(hookNotice().querySelector(".font-semibold")).toBeNull() }) - it("warns about an image it cannot reach", async ({ expect }) => { - await mountMenu({ - hook: imageHook({ - state: { digest: "sha256:old" }, - status: "unauthorized", - }), - }) + it.for([ + { + name: "an image it cannot reach", + input: "unauthorized", + expected: "editor.hooks.container-image-watcher.unreachable-image", + }, + { + name: "an image gone from its registry", + input: "image_not_found", + expected: "editor.hooks.container-image-watcher.missing-image", + }, + ] as const)("warns about $name", async ({ input, expected }, { expect }) => { + await mountMenu({ hook: imageHook({ status: input }) }) await openHookSubMenu(t(TITLE)) expect(hookNotice().dataset.hookStatus).toBe("needs-attention") - expect(hookNotice().textContent.trim()).toBe( - t("editor.hooks.container-image-watcher.unreachable-image"), - ) + expect(hookNotice().textContent.trim()).toBe(t(expected)) }) it("keeps the create button out of reach until an image is typed", async ({ diff --git a/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts b/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts index 06a1b04e..ab2110c3 100644 --- a/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts +++ b/web/app/components/editor/hooks/github-tracking/ConfigMenu.nuxt.test.ts @@ -276,8 +276,8 @@ describe("", { concurrent: false }, () => { }, ) - // a server without github cannot be fixed from the settings, so the - // notice does not send the reader there + // github is also disconnected here. The notice still names the server, + // because no setting can fix it. it("warns that github is not set up on this server", async ({ expect }) => { mockGitHub({ connected: false }) await mountMenu({ hook: githubHook({ status: "unconfigured" }) }) diff --git a/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue b/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue index d8a5c763..b0fd60a1 100644 --- a/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue +++ b/web/app/components/editor/hooks/github-tracking/ConfigMenu.vue @@ -98,9 +98,8 @@ const invalidData = computed(() => { const failure = computed(() => checkStatus.value ? checkFailure(checkStatus.value) : null, ) -// a server without GitHub comes first, since no setup an editor does can -// fix it. Then the setup an editor still has to do, then what the last -// check found. +// a server without GitHub comes first, because no editor can fix it. Then +// the setup an editor still has to do, then what the last check found. const notice = computed(() => { if (failure.value?.notice === "unconfigured") { return "unconfigured" diff --git a/web/app/components/editor/hooks/hook-actions.nuxt.test.ts b/web/app/components/editor/hooks/hook-actions.nuxt.test.ts index 026f19cd..a4179b0b 100644 --- a/web/app/components/editor/hooks/hook-actions.nuxt.test.ts +++ b/web/app/components/editor/hooks/hook-actions.nuxt.test.ts @@ -87,34 +87,33 @@ describe("useHookActions", { concurrent: false }, () => { ]) }) - it.for([ - { name: "creating", update: false }, - { name: "updating", update: true }, - ])( - "names the reason core refused $name the hook for", - async ({ update }, { expect }) => { - const refused = (_call: RecordedCall, event: H3Event) => { - setResponseStatus(event, 422) + it("shows core's reason when it refuses to create the hook", async ({ + expect, + }) => { + mockEndpoint("POST", `/api/documents/${DOCUMENT_ID}/hooks`, refused) + const { actions } = await mountActions(null) - return { - code: "document_hook.unconfigured", - message: "the hook cannot check its target: unconfigured", - } - } - mockEndpoint("POST", `/api/documents/${DOCUMENT_ID}/hooks`, refused) - mockEndpoint("PUT", HOOK_PATH, refused) - const { actions } = await mountActions(update ? undefined : null) + const done = await actions.create(SETTINGS) - const done = update - ? await actions.update(SETTINGS) - : await actions.create(SETTINGS) + expect(done).toBe(false) + expect(raisedToasts()).toMatchObject([ + { type: "error", title: t("editor.hooks.errors.codes.unconfigured") }, + ]) + }) - expect(done).toBe(false) - expect(raisedToasts()).toMatchObject([ - { type: "error", title: t("editor.hooks.errors.codes.unconfigured") }, - ]) - }, - ) + it("shows core's reason when it refuses to update the hook", async ({ + expect, + }) => { + mockEndpoint("PUT", HOOK_PATH, refused) + const { actions } = await mountActions() + + const done = await actions.update(SETTINGS) + + expect(done).toBe(false) + expect(raisedToasts()).toMatchObject([ + { type: "error", title: t("editor.hooks.errors.codes.unconfigured") }, + ]) + }) it("updates the hook's settings after closing the menu", async ({ expect, @@ -276,3 +275,12 @@ function failing(_call: RecordedCall, event: H3Event) { return { message: "boom" } } + +function refused(_call: RecordedCall, event: H3Event) { + setResponseStatus(event, 422) + + return { + code: "document_hook.unconfigured", + message: "the hook cannot check its target: unconfigured", + } +} diff --git a/web/app/components/editor/hooks/hook-actions.ts b/web/app/components/editor/hooks/hook-actions.ts index d8d108c6..fa090c53 100644 --- a/web/app/components/editor/hooks/hook-actions.ts +++ b/web/app/components/editor/hooks/hook-actions.ts @@ -3,8 +3,8 @@ import { hookErrorMessage } from "./hook-errors" // useHookActions sends the requests a hook's menu makes. Each one closes // the menu before it goes out, raises a toast when it fails, and resolves -// to whether it went through. A refusal core gives a reason for shows that -// reason. +// to whether it went through. When core names why it refused, the toast +// shows that reason. export function useHookActions(options: { type: DocumentHookType nodeId: () => string | null diff --git a/web/app/components/editor/hooks/hook-errors.test.ts b/web/app/components/editor/hooks/hook-errors.test.ts index f9c59c0d..7a10290a 100644 --- a/web/app/components/editor/hooks/hook-errors.test.ts +++ b/web/app/components/editor/hooks/hook-errors.test.ts @@ -4,33 +4,37 @@ import { hookErrorMessage } from "./hook-errors" describe("hookErrorMessage", () => { it.for([ { - name: "a check that failed", - input: "document_hook.missing_repository", - expected: "editor.hooks.errors.codes.missing-repository", + name: "names the message of a check that failed", + input: { data: { code: "document_hook.missing_repository" } }, + expected: "message:editor.hooks.errors.codes.missing-repository", }, { - name: "settings the processor refused", - input: "document_hook.invalid_image", - expected: "editor.hooks.errors.codes.invalid-image", + name: "names the message of settings the processor refused", + input: { data: { code: "document_hook.invalid_image" } }, + expected: "message:editor.hooks.errors.codes.invalid-image", }, { - name: "a service the hook could not reach", - input: "document_hook.upstream_unavailable", - expected: "editor.hooks.errors.codes.upstream-unavailable", + name: "names the message of a service the hook could not reach", + input: { data: { code: "document_hook.upstream_unavailable" } }, + expected: "message:editor.hooks.errors.codes.upstream-unavailable", }, - ])("names the message of $name", ({ input, expected }, { expect }) => { - expect(hookErrorMessage({ data: { code: input } }, t)).toBe( - `message:${expected}`, - ) - }) - - it("names nothing for an unknown code", ({ expect }) => { - expect(hookErrorMessage({ data: { code: "general" } }, t)).toBeUndefined() - }) - - it("names nothing for an error without a body", ({ expect }) => { - expect(hookErrorMessage(new Error("boom"), t)).toBeUndefined() - expect(hookErrorMessage(undefined, t)).toBeUndefined() + { + name: "names nothing for an unknown code", + input: { data: { code: "general" } }, + expected: undefined, + }, + { + name: "names nothing for an error without a body", + input: new Error("boom"), + expected: undefined, + }, + { + name: "names nothing for a missing error", + input: undefined, + expected: undefined, + }, + ])("$name", ({ input, expected }, { expect }) => { + expect(hookErrorMessage(input, t)).toBe(expected) }) }) diff --git a/web/app/components/editor/hooks/hook-errors.ts b/web/app/components/editor/hooks/hook-errors.ts index 6338adb0..f78de86c 100644 --- a/web/app/components/editor/hooks/hook-errors.ts +++ b/web/app/components/editor/hooks/hook-errors.ts @@ -1,4 +1,4 @@ -// the refusals core answers a hook write with, keyed by error code. +// the message key for each code core refuses a hook write with. const HOOK_ERROR_KEYS: Record = { "document_hook.unconfigured": "editor.hooks.errors.codes.unconfigured", "document_hook.missing_installation": diff --git a/web/app/components/editor/hooks/hook-status.test.ts b/web/app/components/editor/hooks/hook-status.test.ts index ed879156..7148b7f0 100644 --- a/web/app/components/editor/hooks/hook-status.test.ts +++ b/web/app/components/editor/hooks/hook-status.test.ts @@ -1,5 +1,10 @@ import { describe, it } from "vitest" -import { hookGroupStatus, hookStatus } from "./hook-status" +import { + hookGroupStatus, + hookStatus, + type HookGroupStatus, + type HookStatus, +} from "./hook-status" import { DocumentHookType, type DocumentHook, @@ -14,7 +19,7 @@ describe("hookStatus", () => { it.for<{ name: string input: { score: string; status: DocumentHookStatus } - expected: string + expected: HookStatus }>([ { name: "shows a waiting hook as fresh", @@ -75,7 +80,7 @@ describe("hookGroupStatus", () => { it.for<{ name: string input: { score: string; status: DocumentHookStatus }[] - expected: string | null + expected: HookGroupStatus | null }>([ { name: "has no status without hooks", diff --git a/web/app/components/editor/hooks/hook-status.ts b/web/app/components/editor/hooks/hook-status.ts index 32f2499a..abb86d58 100644 --- a/web/app/components/editor/hooks/hook-status.ts +++ b/web/app/components/editor/hooks/hook-status.ts @@ -40,7 +40,7 @@ export const HOOK_BAR_STATUS_CLASS: Record< // hookStatus puts a failed check first. A hook that cannot check what it // watches needs fixing before its trigger means anything. A copy still -// being set up keeps the score its source had. +// being set up has not failed. It shows the score its source had. export function hookStatus(hook: DocumentHook): HookStatus { if (hook.status !== "active" && hook.status !== "initializing") { return "needs-attention" diff --git a/web/app/components/editor/hooks/url-watcher/ConfigMenu.nuxt.test.ts b/web/app/components/editor/hooks/url-watcher/ConfigMenu.nuxt.test.ts index 9488f2fe..888c147e 100644 --- a/web/app/components/editor/hooks/url-watcher/ConfigMenu.nuxt.test.ts +++ b/web/app/components/editor/hooks/url-watcher/ConfigMenu.nuxt.test.ts @@ -94,27 +94,30 @@ describe("", { concurrent: false }, () => { ) }) - it("says the page of a failing hook is unreachable", async ({ expect }) => { - await mountMenu({ - hook: urlHook({ state: {}, status: "unreachable_url" }), - }) + it.for([ + { + name: "an unreachable page", + input: "unreachable_url", + expected: "editor.hooks.url-watcher.detail-unreachable", + }, + { + name: "missing change detection", + input: "unconfigured", + expected: "editor.hooks.url-watcher.detail-unconfigured", + }, + ] as const)( + "names $name on the hook's row", + async ({ input, expected }, { expect }) => { + await mountMenu({ hook: urlHook({ state: {}, status: input }) }) - const detail = t("editor.hooks.url-watcher.detail-unreachable") - expect(menuText()).toContain( - t("editor.hooks.subtext-detail", { subtext: SHOWN_URL, detail: detail }), - ) - }) - - it("says change detection is not set up for a hook that needs it", async ({ - expect, - }) => { - await mountMenu({ hook: urlHook({ status: "unconfigured" }) }) - - const detail = t("editor.hooks.url-watcher.detail-unconfigured") - expect(menuText()).toContain( - t("editor.hooks.subtext-detail", { subtext: SHOWN_URL, detail: detail }), - ) - }) + expect(menuText()).toContain( + t("editor.hooks.subtext-detail", { + subtext: SHOWN_URL, + detail: t(expected), + }), + ) + }, + ) it("prefills the address of the hook it edits", async ({ expect }) => { await mountMenu({ hook: urlHook() }) @@ -180,17 +183,24 @@ describe("", { concurrent: false }, () => { expect(hookNotice().querySelector(".font-semibold")).toBeNull() }) - it("warns about a site it cannot reach", async ({ expect }) => { - await mountMenu({ - hook: urlHook({ state: {}, status: "unreachable_url" }), - }) + it.for([ + { + name: "a site it cannot reach", + input: "unreachable_url", + expected: "editor.hooks.url-watcher.unreachable-url", + }, + { + name: "a server without change detection", + input: "unconfigured", + expected: "editor.hooks.url-watcher.unconfigured", + }, + ] as const)("warns about $name", async ({ input, expected }, { expect }) => { + await mountMenu({ hook: urlHook({ state: {}, status: input }) }) await openHookSubMenu(t(TITLE)) expect(hookNotice().dataset.hookStatus).toBe("needs-attention") - expect(hookNotice().textContent.trim()).toBe( - t("editor.hooks.url-watcher.unreachable-url"), - ) + expect(hookNotice().textContent.trim()).toBe(t(expected)) }) it("keeps the create button out of reach until an address is typed", async ({ diff --git a/web/app/utils/api/document.ts b/web/app/utils/api/document.ts index 508ccef7..da921dfb 100644 --- a/web/app/utils/api/document.ts +++ b/web/app/utils/api/document.ts @@ -67,8 +67,8 @@ export interface DocumentHook { settings: DocumentHookSettings // null until the hook is set up state: DocumentHookState | null - // whether the last check of what the hook watches worked. Score and - // state keep their last values while it is not active. + // active, initializing, or why the last check failed. Score and state + // keep their last values while a check fails. status: DocumentHookStatus score: string // decimal; between 0 and 100 (default: 100) createdAt: Date | string @@ -120,8 +120,7 @@ export type DocumentHookStatus = | DocumentHookStatusURLWatcher | DocumentHookStatusContainerImageWatcher -// initializing marks a copy its first run has not set up yet. Any other -// status than active names why the last check failed. +// initializing marks a copied hook before its first run. export type DocumentHookStatusScheduledReminder = "active" | "initializing" export type DocumentHookStatusGitHubTracking = | "active" diff --git a/web/i18n/locales/en/editor.json b/web/i18n/locales/en/editor.json index 45c0aa02..7c07ed3f 100644 --- a/web/i18n/locales/en/editor.json +++ b/web/i18n/locales/en/editor.json @@ -398,7 +398,7 @@ "reset-failed": "Failed to reset the hook. Please try again.", "renew-failed": "Failed to renew the hook. Please try again." }, - "initializing": "Initializing this hook. It will be ready in a moment.", + "initializing": "Setting up this hook. It will be ready in a moment.", "scheduled-reminder": { "title": "Scheduled Reminder", "description": "Nudge maintainers on a date", @@ -508,7 +508,7 @@ "triggered-notice": "{url} changed, so maintainers were notified.", "url-fallback": "the webpage", "unreachable-url": "The selected webpage cannot be reached. They may be temporarily down, no longer exist, or have anti-scraping measures in place.", - "unconfigured": "Website change detection is not set up on this server, so changes cannot be detected.", + "unconfigured": "Website change detection is not set up on this server, so this hook cannot check the page.", "url-input-label": "Link to Website", "url-input-placeholder": "https://example.com" }, diff --git a/web/i18n/locales/en/notification.json b/web/i18n/locales/en/notification.json index 258b5a5a..6bb57f96 100644 --- a/web/i18n/locales/en/notification.json +++ b/web/i18n/locales/en/notification.json @@ -12,7 +12,7 @@ "document-new-comment-reply-description": "{user} has posted a reply to a comment", "document-review-request-description": "Review requested for the draft version", "document-hook-triggered-description": "{hook} hook was triggered", - "document-hook-needs-attention-description": "{hook} hook can no longer check its target", + "document-hook-needs-attention-description": "{hook} hook needs attention", "default-description": "You have a new notification" }, "actions": {