diff --git a/server/README.md b/server/README.md index 027e592d..0d48a979 100644 --- a/server/README.md +++ b/server/README.md @@ -10,6 +10,22 @@ By default the following ports are opened: - `8080` - core application's HTTP and WebSocket server. - `8081` - auth-realtime's Hocuspocus HTTP and WebSocket server. +# Tag API + +`PUT /api/tags/{tagId}` renames or recolours an existing tag in the active +organization. The route requires a session and is available through the +front door at `/core/api/tags/{tagId}`. + +Send a JSON object with `tagName`, `color`, or both. Omitted or `null` fields +keep their current values; at least one field must be set. A name cannot be +empty, and a colour must be a hex value from the existing tag palette. +Successful updates return `204 No Content` and notify the organization's +`change@tag-tree` subscribers. The tag's ID, display order, and document +branch assignments stay unchanged. + +Invalid input returns `400`, duplicate names return `409`, and missing tags +or tags outside the active organization return `404`. + # Better Auth ## Using Migrations diff --git a/server/core/internal/db/tag_test.go b/server/core/internal/db/tag_test.go index 0fa76bc3..c833a4bb 100644 --- a/server/core/internal/db/tag_test.go +++ b/server/core/internal/db/tag_test.go @@ -730,6 +730,7 @@ func Test_agent_UpdateTag(t *testing.T) { OrganizationID string Tag tag.Tag Input tag.UpdateInput + BranchIDs []xid.ID Name string Color string Err error @@ -817,6 +818,32 @@ func Test_agent_UpdateTag(t *testing.T) { Color: "#3b82f6", } }, + "Preserves assignments across documents and branches": func(t *testing.T, db *DB) tcase { + tg := prepTags(t, db, 1, func(_ int, tg *tag.Tag) { + tg.SortIndex = 7 + tg.CreatedBy = null.StringFrom(prepUsers(t, db, 1)[0]) + })[0] + first := prepTaggedDocument(t, db, tg) + second := prepTaggedDocument(t, db, tg) + branch := prepDocumentBranches(t, db, 1, func(_ int, br *document.Document) { + br.ID = first.ID + br.OrganizationID = tg.OrganizationID + })[0] + + prepBranchTags(t, db, tg.OrganizationID, branch.BranchID, tg.ID) + + return tcase{ + OrganizationID: tg.OrganizationID, + Tag: tg, + Input: tag.UpdateInput{ + TagName: null.StringFrom("Release"), + Color: null.StringFrom("#00a63e"), + }, + BranchIDs: []xid.ID{first.BranchID, second.BranchID, branch.BranchID}, + Name: "Release", + Color: "#00a63e", + } + }, } for cn, cfn := range cc { @@ -836,19 +863,23 @@ func Test_agent_UpdateTag(t *testing.T) { err := db.UpdateTag(ctx, c.OrganizationID, c.Tag.ID, c.Input) testutil.RequireEqualError(t, c.Err, err) - q, args := db.builder.Select("tag_name", "color"). - From("tags"). - Where(sq.Eq{"id": c.Tag.ID}). + q, args := db.selectTag(c.Tag.OrganizationID). + Where(sq.Eq{"tags.id": c.Tag.ID}). MustSql() - var stored struct { - TagName string `db:"tag_name"` - Color string `db:"color"` - } + var stored tag.Tag require.NoError(t, sqlx.Get(db.sql, &stored, q, args...)) - assert.Equal(t, c.Name, stored.TagName) - assert.Equal(t, c.Color, stored.Color) + + exp := c.Tag + exp.TagName = c.Name + exp.Color = c.Color + testutil.AssertFilterEqual(t, exp, stored, time.Time{}) + assert.Equal(t, exp.CreatedAt, stored.CreatedAt.UTC()) + + for _, branchID := range c.BranchIDs { + assert.Equal(t, []xid.ID{c.Tag.ID}, branchTagIDs(t, db, branchID)) + } }) } } diff --git a/server/core/internal/server/0mock_db_test.go b/server/core/internal/server/0mock_db_test.go index 6f8e632c..820d6781 100644 --- a/server/core/internal/server/0mock_db_test.go +++ b/server/core/internal/server/0mock_db_test.go @@ -299,6 +299,9 @@ var _ DB = &DBMock{} // UpdateSlackUserLinkFunc: func(ctx context.Context, link slackCore.UserLink) error { // panic("mock out the UpdateSlackUserLink method") // }, +// UpdateTagFunc: func(ctx context.Context, organizationID string, id xid.ID, inp tag.UpdateInput) error { +// panic("mock out the UpdateTag method") +// }, // UpdateTagTreeFunc: func(ctx context.Context, tree tag.Summaries, organizationID string) error { // panic("mock out the UpdateTagTree method") // }, @@ -585,6 +588,9 @@ type DBMock struct { // UpdateSlackUserLinkFunc mocks the UpdateSlackUserLink method. UpdateSlackUserLinkFunc func(ctx context.Context, link slackCore.UserLink) error + // UpdateTagFunc mocks the UpdateTag method. + UpdateTagFunc func(ctx context.Context, organizationID string, id xid.ID, inp tag.UpdateInput) error + // UpdateTagTreeFunc mocks the UpdateTagTree method. UpdateTagTreeFunc func(ctx context.Context, tree tag.Summaries, organizationID string) error @@ -1372,6 +1378,17 @@ type DBMock struct { // Link is the link argument value. Link slackCore.UserLink } + // UpdateTag holds details about calls to the UpdateTag method. + UpdateTag []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // OrganizationID is the organizationID argument value. + OrganizationID string + // ID is the id argument value. + ID xid.ID + // Inp is the inp argument value. + Inp tag.UpdateInput + } // UpdateTagTree holds details about calls to the UpdateTagTree method. UpdateTagTree []struct { // Ctx is the ctx argument value. @@ -1506,6 +1523,7 @@ type DBMock struct { lockUpdateOrganizationLogo sync.RWMutex lockUpdateSlackAppOrganizationID sync.RWMutex lockUpdateSlackUserLink sync.RWMutex + lockUpdateTag sync.RWMutex lockUpdateTagTree sync.RWMutex lockUpdateUserImage sync.RWMutex lockUpsertDocumentBranchView sync.RWMutex @@ -5320,6 +5338,53 @@ func (mock *DBMock) UpdateSlackUserLinkCalls() []struct { return calls } +// UpdateTag calls UpdateTagFunc. +func (mock *DBMock) UpdateTag(ctx context.Context, organizationID string, id xid.ID, inp tag.UpdateInput) error { + callInfo := struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tag.UpdateInput + }{ + Ctx: ctx, + OrganizationID: organizationID, + ID: id, + Inp: inp, + } + mock.lockUpdateTag.Lock() + mock.calls.UpdateTag = append(mock.calls.UpdateTag, callInfo) + mock.lockUpdateTag.Unlock() + if mock.UpdateTagFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.UpdateTagFunc(ctx, organizationID, id, inp) +} + +// UpdateTagCalls gets all the calls that were made to UpdateTag. +// Check the length with: +// +// len(mockedDB.UpdateTagCalls()) +func (mock *DBMock) UpdateTagCalls() []struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tag.UpdateInput +} { + var calls []struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tag.UpdateInput + } + mock.lockUpdateTag.RLock() + calls = mock.calls.UpdateTag + mock.lockUpdateTag.RUnlock() + return calls +} + // UpdateTagTree calls UpdateTagTreeFunc. func (mock *DBMock) UpdateTagTree(ctx context.Context, tree tag.Summaries, organizationID string) error { callInfo := struct { diff --git a/server/core/internal/server/internal/tag/0mock_db_test.go b/server/core/internal/server/internal/tag/0mock_db_test.go index 2f9afb58..fecd5da3 100644 --- a/server/core/internal/server/internal/tag/0mock_db_test.go +++ b/server/core/internal/server/internal/tag/0mock_db_test.go @@ -42,6 +42,9 @@ var _ DB = &DBMock{} // UnassignBranchTagFunc: func(ctx context.Context, organizationID string, documentID xid.ID, branchID xid.ID, tagID xid.ID) error { // panic("mock out the UnassignBranchTag method") // }, +// UpdateTagFunc: func(ctx context.Context, organizationID string, id xid.ID, inp tagCore.UpdateInput) error { +// panic("mock out the UpdateTag method") +// }, // UpdateTagTreeFunc: func(ctx context.Context, tree tagCore.Summaries, organizationID string) error { // panic("mock out the UpdateTagTree method") // }, @@ -73,6 +76,9 @@ type DBMock struct { // UnassignBranchTagFunc mocks the UnassignBranchTag method. UnassignBranchTagFunc func(ctx context.Context, organizationID string, documentID xid.ID, branchID xid.ID, tagID xid.ID) error + // UpdateTagFunc mocks the UpdateTag method. + UpdateTagFunc func(ctx context.Context, organizationID string, id xid.ID, inp tagCore.UpdateInput) error + // UpdateTagTreeFunc mocks the UpdateTagTree method. UpdateTagTreeFunc func(ctx context.Context, tree tagCore.Summaries, organizationID string) error @@ -153,6 +159,17 @@ type DBMock struct { // TagID is the tagID argument value. TagID xid.ID } + // UpdateTag holds details about calls to the UpdateTag method. + UpdateTag []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // OrganizationID is the organizationID argument value. + OrganizationID string + // ID is the id argument value. + ID xid.ID + // Inp is the inp argument value. + Inp tagCore.UpdateInput + } // UpdateTagTree holds details about calls to the UpdateTagTree method. UpdateTagTree []struct { // Ctx is the ctx argument value. @@ -170,6 +187,7 @@ type DBMock struct { lockInsertTag sync.RWMutex lockSetTagVisibility sync.RWMutex lockUnassignBranchTag sync.RWMutex + lockUpdateTag sync.RWMutex lockUpdateTagTree sync.RWMutex } @@ -500,6 +518,53 @@ func (mock *DBMock) UnassignBranchTagCalls() []struct { return calls } +// UpdateTag calls UpdateTagFunc. +func (mock *DBMock) UpdateTag(ctx context.Context, organizationID string, id xid.ID, inp tagCore.UpdateInput) error { + callInfo := struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tagCore.UpdateInput + }{ + Ctx: ctx, + OrganizationID: organizationID, + ID: id, + Inp: inp, + } + mock.lockUpdateTag.Lock() + mock.calls.UpdateTag = append(mock.calls.UpdateTag, callInfo) + mock.lockUpdateTag.Unlock() + if mock.UpdateTagFunc == nil { + var ( + errOut error + ) + return errOut + } + return mock.UpdateTagFunc(ctx, organizationID, id, inp) +} + +// UpdateTagCalls gets all the calls that were made to UpdateTag. +// Check the length with: +// +// len(mockedDB.UpdateTagCalls()) +func (mock *DBMock) UpdateTagCalls() []struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tagCore.UpdateInput +} { + var calls []struct { + Ctx context.Context + OrganizationID string + ID xid.ID + Inp tagCore.UpdateInput + } + mock.lockUpdateTag.RLock() + calls = mock.calls.UpdateTag + mock.lockUpdateTag.RUnlock() + return calls +} + // UpdateTagTree calls UpdateTagTreeFunc. func (mock *DBMock) UpdateTagTree(ctx context.Context, tree tagCore.Summaries, organizationID string) error { callInfo := struct { diff --git a/server/core/internal/server/internal/tag/http.go b/server/core/internal/server/internal/tag/http.go index e94a27dd..0ea88bf5 100644 --- a/server/core/internal/server/internal/tag/http.go +++ b/server/core/internal/server/internal/tag/http.go @@ -118,6 +118,46 @@ func (h *Handler) CreateTag(w http.ResponseWriter, r *http.Request) { httpserver.Respond(h.log, w, t, http.StatusCreated) } +// UpdateTag handles renaming or recolouring an existing tag. +func (h *Handler) UpdateTag(w http.ResponseWriter, r *http.Request) { + session, ok := auth.RequireSession(h.log, w, r) + if !ok { + return + } + + tagID, err := httpserver.ExtractNamedID(r, "tagId") + if err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + var inp tagCore.UpdateInput + + if err := httpserver.DecodeJSON(r, &inp); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := inp.Validate(); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + if err := h.db.UpdateTag( + r.Context(), + session.ActiveOrganizationID, + tagID, + inp, + ); err != nil { + httpserver.RespondError(h.log, w, err) + return + } + + h.NotifyTreeChange(session.ActiveOrganizationID) + + httpserver.Respond(h.log, w, nil, http.StatusNoContent) +} + // SetTagVisibility handles whether the caller keeps a tag out of their own // sidebar. Nobody else's tree changes, so only the caller is told. func (h *Handler) SetTagVisibility(w http.ResponseWriter, r *http.Request) { @@ -298,6 +338,10 @@ type DB interface { // InsertTag should store a new tag at the end of its organization's tags. InsertTag(ctx context.Context, t tagCore.Tag) error + // UpdateTag should rename or recolour a tag in the organization, + // preserving its assignments and any field left unset. + UpdateTag(ctx context.Context, organizationID string, id xid.ID, inp tagCore.UpdateInput) error + // SetTagVisibility should record whether one user keeps a tag out of // their sidebar, leaving every other user's view untouched. SetTagVisibility( diff --git a/server/core/internal/server/internal/tag/http_test.go b/server/core/internal/server/internal/tag/http_test.go index c598f9f5..76ca2a0d 100644 --- a/server/core/internal/server/internal/tag/http_test.go +++ b/server/core/internal/server/internal/tag/http_test.go @@ -10,9 +10,11 @@ import ( "testing" "github.com/go-chi/chi/v5" + "github.com/guregu/null/v5" documentCore "github.com/oxynote/oxynote/server/core/internal/document" "github.com/oxynote/oxynote/server/core/internal/server/internal/auth" tagCore "github.com/oxynote/oxynote/server/core/internal/tag" + "github.com/oxynote/oxynote/server/core/pkg/errutil" wsMock "github.com/oxynote/wetsocks/wsserver/_mock" "github.com/rs/xid" "github.com/stretchr/testify/assert" @@ -383,6 +385,272 @@ func Test_Handler_CreateTag(t *testing.T) { } } +func Test_Handler_UpdateTag(t *testing.T) { + t.Parallel() + + type check func(*testing.T, *DBMock, *wsMock.Topic, *httptest.ResponseRecorder) + + checks := func(cc ...check) []check { return cc } + + hasResp := func(code int, body string) check { + return func(t *testing.T, _ *DBMock, _ *wsMock.Topic, rec *httptest.ResponseRecorder) { + assert.Equal(t, code, rec.Code) + + if body == "" { + assert.Zero(t, rec.Body.Len(), rec.Body.String()) + return + } + + assert.JSONEq(t, body, rec.Body.String()) + } + } + + wasUpdateCalled := func(count int, inp tagCore.UpdateInput) check { + return func(t *testing.T, db *DBMock, _ *wsMock.Topic, _ *httptest.ResponseRecorder) { + calls := db.UpdateTagCalls() + require.Len(t, calls, count) + + if count == 0 { + return + } + + assert.Equal(t, "org1", calls[0].OrganizationID) + assert.Equal(t, _tagID, calls[0].ID) + assert.Equal(t, inp, calls[0].Inp) + } + } + + wasPublishCalled := func(count int) check { + return func(t *testing.T, _ *DBMock, tpc *wsMock.Topic, _ *httptest.ResponseRecorder) { + calls := tpc.PublishManyCalls() + require.Len(t, calls, count) + + if count == 0 { + return + } + + assert.Equal(t, TreeChangeMessage{}, calls[0].Payload) + assert.True(t, calls[0].Filter(sessionCtx(), "topic")) + assert.True(t, calls[0].Filter(otherUserCtx(), "topic")) + assert.False(t, calls[0].Filter(context.Background(), "topic")) + assert.False(t, calls[0].Filter(auth.AddSessionToContext(context.Background(), auth.Session{ + UserID: "u1", + ActiveOrganizationID: "org2", + }), "topic")) + } + } + + stubDB := func(err error) *DBMock { + return &DBMock{ + UpdateTagFunc: func(context.Context, string, xid.ID, tagCore.UpdateInput) error { + return err + }, + } + } + + cc := map[string]struct { + DB *DBMock + NoSession bool + TagID string + JSON string + Checks []check + }{ + "No session in context": { + DB: &DBMock{}, + NoSession: true, + TagID: _tagID.String(), + JSON: `{`, + Checks: checks( + hasResp(http.StatusUnauthorized, `{"code":"account.not_authenticated","message":"not authenticated"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Malformed tag id": { + DB: &DBMock{}, + TagID: "not-an-xid", + JSON: `{"tagName":"Release"}`, + Checks: checks( + hasResp(http.StatusNotFound, `{"code":"general","message":"not found"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Malformed payload": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"request.invalid_json","message":"invalid JSON body"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Non-string name": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"tagName":123}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"request.invalid_json","message":"invalid JSON body"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Empty update": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.empty_update","message":"tag update needs a name or a colour"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Null fields": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"tagName":null,"color":null}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.empty_update","message":"tag update needs a name or a colour"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Empty name": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"tagName":"","color":"#00a63e"}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.invalid_name","message":"tag name cannot be empty"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Empty colour": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"color":""}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.invalid_color","message":"tag colour must be one of the palette"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Off-palette colour": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"color":"#22c55e"}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.invalid_color","message":"tag colour must be one of the palette"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Colour name instead of hex": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"color":"green"}`, + Checks: checks( + hasResp(http.StatusBadRequest, `{"code":"tag.invalid_color","message":"tag colour must be one of the palette"}`), + wasUpdateCalled(0, tagCore.UpdateInput{}), + wasPublishCalled(0), + ), + }, + "Error returned by DB.UpdateTag": { + DB: stubDB(assert.AnError), + TagID: _tagID.String(), + JSON: `{"tagName":"Release"}`, + Checks: checks( + hasResp(http.StatusInternalServerError, `{"code":"general","message":"internal server error"}`), + wasUpdateCalled(1, tagCore.UpdateInput{TagName: null.StringFrom("Release")}), + wasPublishCalled(0), + ), + }, + "Duplicate name returned by DB.UpdateTag": { + DB: stubDB(tagCore.ErrDuplicateTagName), + TagID: _tagID.String(), + JSON: `{"tagName":"Release"}`, + Checks: checks( + hasResp(http.StatusConflict, `{"code":"tag.duplicate_name","message":"tag name is already in use"}`), + wasUpdateCalled(1, tagCore.UpdateInput{TagName: null.StringFrom("Release")}), + wasPublishCalled(0), + ), + }, + "Missing or foreign tag returned by DB.UpdateTag": { + DB: stubDB(errutil.ErrNotFound), + TagID: _tagID.String(), + JSON: `{"tagName":"Release"}`, + Checks: checks( + hasResp(http.StatusNotFound, `{"code":"general","message":"not found"}`), + wasUpdateCalled(1, tagCore.UpdateInput{TagName: null.StringFrom("Release")}), + wasPublishCalled(0), + ), + }, + "Successful rename": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"tagName":"Release"}`, + Checks: checks( + hasResp(http.StatusNoContent, ""), + wasUpdateCalled(1, tagCore.UpdateInput{TagName: null.StringFrom("Release")}), + wasPublishCalled(1), + ), + }, + "Successful recolour": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"color":"#00a63e"}`, + Checks: checks( + hasResp(http.StatusNoContent, ""), + wasUpdateCalled(1, tagCore.UpdateInput{Color: null.StringFrom("#00a63e")}), + wasPublishCalled(1), + ), + }, + "Successful rename and recolour": { + DB: &DBMock{}, + TagID: _tagID.String(), + JSON: `{"tagName":"Release","color":"#00a63e"}`, + Checks: checks( + hasResp(http.StatusNoContent, ""), + wasUpdateCalled(1, tagCore.UpdateInput{ + TagName: null.StringFrom("Release"), + Color: null.StringFrom("#00a63e"), + }), + wasPublishCalled(1), + ), + }, + } + + for cn, c := range cc { + t.Run(cn, func(t *testing.T) { + t.Parallel() + + tpc := &wsMock.Topic{} + hdl := Handler{ + log: slog.New(slog.DiscardHandler), + db: c.DB, + } + hdl.BindTreeChange(tpc) + + req := withParams( + httptest.NewRequest(http.MethodPut, "http://test.com/", strings.NewReader(c.JSON)), + map[string]string{"tagId": c.TagID}, + ) + + if !c.NoSession { + req = req.WithContext(addSession(req.Context())) + } + + rec := httptest.NewRecorder() + hdl.UpdateTag(rec, req) + + for _, check := range c.Checks { + check(t, c.DB, tpc, rec) + } + }) + } +} + func Test_Handler_SetTagVisibility(t *testing.T) { t.Parallel() diff --git a/server/core/internal/server/router.go b/server/core/internal/server/router.go index e593bf2b..eb9585a9 100644 --- a/server/core/internal/server/router.go +++ b/server/core/internal/server/router.go @@ -371,6 +371,7 @@ func (s *Server) router() chi.Router { r.Route("/tags", func(sr chi.Router) { sr.Post("/", s.handlers.tag.CreateTag) + sr.Put("/{tagId}", s.handlers.tag.UpdateTag) sr.Put("/{tagId}/visibility", s.handlers.tag.SetTagVisibility) sr.Delete("/{tagId}", s.handlers.tag.DeleteTag) sr.Route("/tree", func(ssr chi.Router) { diff --git a/server/core/internal/server/router_test.go b/server/core/internal/server/router_test.go index f305a9f3..2e9acc31 100644 --- a/server/core/internal/server/router_test.go +++ b/server/core/internal/server/router_test.go @@ -77,6 +77,8 @@ func Test_Server_httpRouter(t *testing.T) { // the capability signal lives on the session-authed surface (the "/" // mount walks as "*"). assert.True(t, routes["GET /api/*/capabilities"]) + assert.True(t, routes["PUT /api/*/tags/{tagId}"]) + assert.False(t, routes["PUT /api/x/tags/{tagId}"]) // MCP is off while its handler is nil, so nothing is mounted for it. for route := range routes { diff --git a/web/app/components/AppSidebar.nuxt.test.ts b/web/app/components/AppSidebar.nuxt.test.ts index b1f2bd2f..c0577b04 100644 --- a/web/app/components/AppSidebar.nuxt.test.ts +++ b/web/app/components/AppSidebar.nuxt.test.ts @@ -4,10 +4,12 @@ import { clearQueryCache, disposeMockEndpoints, mockEndpoint, + runInApp, seedQueryData, } from "~/composables/api/test-helpers" import { mockNuxtImport } from "@nuxt/test-utils/runtime" import AppSidebar from "./AppSidebar.vue" +import useTagAPI from "~/composables/api/useTagAPI" import { at, clearTeleportedOverlays, @@ -739,6 +741,73 @@ describe("", { concurrent: false }, () => { expect(calls[0]?.body).toEqual({ hidden: true }) }) + it("refreshes the sidebar name and colour after editing a tag", async ({ + expect, + }) => { + const tags = seededTags() + stubQueries({ tags }) + const calls = mockEndpoint("PUT", `/api/tags/${TAG_A}`, () => { + tags[0] = tagElement(TAG_A, "Live", "#f97316") + + return {} + }) + const wrapper = await mountSidebar() + await settleMutations() + const api = runInApp(() => useTagAPI()) + + await api.updateTag.mutateAsync({ + id: TAG_A, + req: { tagName: "Live", color: "#f97316" }, + }) + await settleMutations() + + expect(calls).toHaveLength(1) + expect(tagIds(wrapper)).toEqual([TAG_A, TAG_B]) + expect(tagNames(wrapper)[0]).toContain("Live") + expect( + at(tagRows(wrapper), 0).get("span.rounded-full").attributes("style"), + ).toBe("background-color: #f97316;") + }) + + it("does not offer to edit a tag before it is saved", async ({ + expect, + }) => { + stubQueries({ + tags: [ + { + ...tagElement("local-tag", "Saving", "#22c55e"), + localOptimisticInsert: true, + }, + ], + }) + const wrapper = await mountSidebar() + await settleMutations() + + await openRowMenu(wrapper, "Saving") + + expect(openMenuRows()).not.toContain( + t("sidebar.item-dropdown-menu-buttons.edit-tag"), + ) + }) + + it("asks the page to edit a tag from its options menu", async ({ + expect, + }) => { + stubQueries({ tags: seededTags() }) + const calls = mockEndpoint("PUT", `/api/tags/${TAG_A}`, () => ({})) + const wrapper = await mountSidebar() + await settleMutations() + + await openRowMenu(wrapper, "Production") + menuItem(t("sidebar.item-dropdown-menu-buttons.edit-tag")).click() + await settleMutations() + + expect(sidebar(wrapper).emitted("edit-tag")).toEqual([ + [{ id: TAG_A, tagName: "Production", color: "#22c55e" }], + ]) + expect(calls).toHaveLength(0) + }) + it("asks the page to delete a tag from its options menu", async ({ expect, }) => { @@ -796,7 +865,7 @@ describe("", { concurrent: false }, () => { ]) }) - it("offers a tag its own two actions", async ({ expect }) => { + it("offers a tag its own actions", async ({ expect }) => { stubQueries({ tags: seededTags() }) const wrapper = await mountSidebar() await settleMutations() @@ -804,6 +873,7 @@ describe("", { concurrent: false }, () => { await openRowMenu(wrapper, "Production") expect(openMenuRows()).toEqual([ + t("sidebar.item-dropdown-menu-buttons.edit-tag"), t("sidebar.item-dropdown-menu-buttons.hide-tag"), t("sidebar.item-dropdown-menu-buttons.delete-tag"), ]) diff --git a/web/app/components/AppSidebar.vue b/web/app/components/AppSidebar.vue index 673ab407..80561cde 100644 --- a/web/app/components/AppSidebar.vue +++ b/web/app/components/AppSidebar.vue @@ -27,6 +27,7 @@ const emit = defineEmits<{ (e: "open-settings", target: "org-members"): void (e: "delete-document" | "duplicate-document", id: string): void (e: "delete-tag", tag: { id: string; name: string }): void + (e: "edit-tag", tag: Pick): void (e: "create-document", parentId: string | null): void }>() @@ -348,6 +349,22 @@ function handleTagVisibilityToggle(tag: TagTreeElement) { function tagActions(tag: TagTreeElement): SidebarItemAction[] { return [ + ...(isXid(tag.id) + ? [ + { + id: "edit-tag", + name: t("sidebar.item-dropdown-menu-buttons.edit-tag"), + icon: "mingcute:edit-2-line", + fn: () => { + emit("edit-tag", { + id: tag.id, + tagName: tag.tagName, + color: tag.color, + }) + }, + }, + ] + : []), { id: "hide-tag", name: t("sidebar.item-dropdown-menu-buttons.hide-tag"), diff --git a/web/app/components/TagEditModal.nuxt.test.ts b/web/app/components/TagEditModal.nuxt.test.ts new file mode 100644 index 00000000..dffb0e15 --- /dev/null +++ b/web/app/components/TagEditModal.nuxt.test.ts @@ -0,0 +1,350 @@ +import { mountSuspended } from "@nuxt/test-utils/runtime" +import { DOMWrapper, enableAutoUnmount } from "@vue/test-utils" +import { afterEach, beforeEach, describe, it, vi } from "vitest" +import { setResponseStatus } from "h3" +import TagEditModal from "./TagEditModal.vue" +import { + clearQueryCache, + disposeMockEndpoints, + makeXid, + mockDeferredEndpoint, + mockEndpoint, + seedQueryData, +} from "~/composables/api/test-helpers" +import { + clearTeleportedOverlays, + settleMutations, + t, + teleportedButton, +} from "./test-helpers" +import { stubSelectableColors } from "./editor/test-helpers/theme" + +const TAG_ID = makeXid("tag") +const TARGET = { id: TAG_ID, tagName: "Production", color: "#010000" } +const UPDATE_URL = `/api/tags/${TAG_ID}` + +enableAutoUnmount(afterEach) + +// the modal, query cache and palette share the app's document +describe("", { concurrent: false }, () => { + beforeEach(() => { + clearTeleportedOverlays() + clearQueryCache() + stubSelectableColors() + const context = { + clearRect: () => undefined, + fillRect: () => undefined, + fillStyle: "", + getImageData: () => ({ + data: [ + ...(context.fillStyle.startsWith("rgb(") + ? (context.fillStyle.match(/\d+/g) ?? []).map(Number) + : [1, 3, 5].map((offset) => + Number.parseInt( + context.fillStyle.slice(offset, offset + 2), + 16, + ), + )), + 255, + ], + }), + } + vi.spyOn(HTMLCanvasElement.prototype, "getContext").mockReturnValue( + context as unknown as CanvasRenderingContext2D, + ) + seedQueryData(["tags", "tree"], [{ ...TARGET, hidden: false }]) + mockEndpoint("GET", "/api/tags/tree", () => [{ ...TARGET, hidden: false }]) + }) + afterEach(disposeMockEndpoints) + + it("stays closed without a target", async ({ expect }) => { + await mountModal(null) + + expect(dialog()).toBeNull() + }) + + it("starts with the existing name and colour and no unsaved changes", async ({ + expect, + }) => { + await mountModal() + + expect(nameInput().element.value).toBe(TARGET.tagName) + expect( + teleportedButton(t("sidebar.tag-edit-modal.color-label")).querySelector( + "span", + )?.style.backgroundColor, + ).toBe("#010000") + expect(saveButton().disabled).toBe(true) + }) + + it("opens the colour picker above the dialog", async ({ expect }) => { + await mountModal() + + teleportedButton(t("sidebar.tag-edit-modal.color-label")).click() + await nextTick() + + expect( + document.body + .querySelector("[data-slot='popover-content']") + ?.classList.contains("z-[calc(theme(zIndex.modal)+5)]!"), + ).toBe(true) + }) + + it("keeps Save disabled when the selected CSS colour matches the saved hex", async ({ + expect, + }) => { + document.documentElement.style.setProperty( + "--selectable-color-1", + "rgb(1, 0, 0)", + ) + await mountModal() + + await chooseColour(0) + + expect(saveButton().disabled).toBe(true) + }) + + it("discards changes when the backdrop is clicked", async ({ expect }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + const wrapper = await mountModal() + await nameInput().setValue("Discard me") + await settleMutations() + + clickBackdrop() + await nextTick() + + expect(calls).toHaveLength(0) + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }) + + it("discards changes on Escape", async ({ expect }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + const wrapper = await mountModal() + await nameInput().setValue("Discard me") + + nameInput().element.dispatchEvent( + new KeyboardEvent("keydown", { key: "Escape", bubbles: true }), + ) + await nextTick() + + expect(calls).toHaveLength(0) + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }) + + it.for([ + { + name: "renames", + rename: true, + recolour: false, + expected: { tagName: "Live" }, + }, + { + name: "recolours", + rename: false, + recolour: true, + expected: { color: "#040000" }, + }, + { + name: "renames and recolours", + rename: true, + recolour: true, + expected: { tagName: "Live", color: "#040000" }, + }, + ])( + "$name the target and closes after saving", + async ({ rename, recolour, expected }, { expect }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + const wrapper = await mountModal() + + if (rename) { + await nameInput().setValue(" Live ") + } + + if (recolour) { + await chooseColour() + } + + saveButton().click() + await settleMutations() + + expect(calls).toHaveLength(1) + expect(calls[0]?.body).toEqual(expected) + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }, + ) + + it.for(["", " "])( + "does not save an empty name '%s'", + async (name, { expect }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + await mountModal() + + await nameInput().setValue(name) + await form().trigger("submit") + await settleMutations() + + expect(saveButton().disabled).toBe(true) + expect(calls).toHaveLength(0) + }, + ) + + it.for([ + "sidebar.tag-edit-modal.cancel-button", + "general.modal-close-screen-reader-hint", + ])("discards changes through %s", async (key, { expect }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + const wrapper = await mountModal() + await nameInput().setValue("Live") + + teleportedButton(t(key)).click() + await nextTick() + + expect(calls).toHaveLength(0) + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }) + + it("restores saved values when reopened after cancellation", async ({ + expect, + }) => { + const calls = mockEndpoint("PUT", UPDATE_URL, () => ({})) + const wrapper = await mountModal() + await nameInput().setValue("Discard me") + await wrapper.setProps({ modelValue: null }) + + await wrapper.setProps({ modelValue: { ...TARGET } }) + + expect(nameInput().element.value).toBe(TARGET.tagName) + expect(saveButton().disabled).toBe(true) + expect(calls).toHaveLength(0) + }) + + it.for([ + { + name: "duplicate", + code: "tag.duplicate_name", + key: "sidebar.tag-edit-modal.duplicate-name", + }, + { + name: "failed", + code: "unknown", + key: "sidebar.errors.update-tag-failed", + }, + ])( + "keeps a $name save open so it can be corrected and retried", + async ({ code, key }, { expect }) => { + let fails = true + const calls = mockEndpoint("PUT", UPDATE_URL, (_call, event) => { + if (fails) { + setResponseStatus(event, 409) + return { code } + } + + return {} + }) + const wrapper = await mountModal() + await nameInput().setValue("Live") + + saveButton().click() + await settleMutations() + + expect(dialog()?.querySelector("[role='alert']")?.textContent).toBe( + t(key), + ) + expect(wrapper.emitted("update:modelValue")).toBeUndefined() + expect(nameInput().element.value).toBe("Live") + expect(saveButton().disabled).toBe(false) + fails = false + saveButton().click() + await settleMutations() + + expect(calls).toHaveLength(2) + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }, + ) + + it("prevents repeated submissions and dismissal while saving", async ({ + expect, + }) => { + const request = mockDeferredEndpoint("PUT", UPDATE_URL) + const wrapper = await mountModal() + await nameInput().setValue("Live") + saveButton().click() + await request.reached + + clickBackdrop() + await form().trigger("submit") + teleportedButton(t("general.modal-close-screen-reader-hint")).click() + nameInput().element.dispatchEvent( + new KeyboardEvent("keydown", { key: "Escape", bubbles: true }), + ) + await nextTick() + + expect(request.calls).toHaveLength(1) + expect(saveButton().disabled).toBe(true) + expect(nameInput().element.disabled).toBe(true) + expect( + teleportedButton(t("sidebar.tag-edit-modal.cancel-button")).disabled, + ).toBe(true) + expect(wrapper.emitted("update:modelValue")).toBeUndefined() + request.resolve({}) + await settleMutations() + + expect(wrapper.emitted("update:modelValue")?.at(-1)).toEqual([null]) + }) +}) + +function mountModal(target: typeof TARGET | null = { ...TARGET }) { + return mountSuspended(TagEditModal, { props: { modelValue: target } }) +} + +function dialog() { + return document.body.querySelector( + "[data-slot='dialog-content']", + ) +} + +function nameInput() { + const input = dialog()?.querySelector("input") + if (!input) { + throw new Error("missing tag name input") + } + + return new DOMWrapper(input) +} + +function saveButton() { + return teleportedButton(t("sidebar.tag-edit-modal.save-button")) +} + +async function chooseColour(index = 3) { + teleportedButton(t("sidebar.tag-edit-modal.color-label")).click() + await nextTick() + const button = document.body.querySelectorAll( + "[data-slot='popover-content'] button", + )[index] + if (!button) { + throw new Error("missing tag colour swatch") + } + + button.click() + await nextTick() +} + +function form() { + const element = dialog()?.querySelector("form") + if (!element) { + throw new Error("missing tag form") + } + + return new DOMWrapper(element) +} + +function clickBackdrop() { + document.body.querySelector("[data-slot='dialog-overlay']")?.dispatchEvent( + new PointerEvent("pointerdown", { + bubbles: true, + pointerType: "mouse", + button: 0, + }), + ) +} diff --git a/web/app/components/TagEditModal.vue b/web/app/components/TagEditModal.vue new file mode 100644 index 00000000..9d4c172f --- /dev/null +++ b/web/app/components/TagEditModal.vue @@ -0,0 +1,183 @@ + + + diff --git a/web/app/components/editor/DocumentTagList.nuxt.test.ts b/web/app/components/editor/DocumentTagList.nuxt.test.ts index da73b8f9..d69361a5 100644 --- a/web/app/components/editor/DocumentTagList.nuxt.test.ts +++ b/web/app/components/editor/DocumentTagList.nuxt.test.ts @@ -9,6 +9,7 @@ import { runInApp, } from "~/composables/api/test-helpers" import useDocumentAPI from "~/composables/api/useDocumentAPI" +import useTagAPI from "~/composables/api/useTagAPI" import DocumentTagList from "./DocumentTagList.vue" import TagPill from "./TagPill.vue" import ColorSelect from "./ColorSelect.vue" @@ -187,6 +188,37 @@ describe("", { concurrent: false }, () => { expect(wrapper.text()).toContain(t("editor.tags.label")) }) + it("refreshes an assigned pill after editing its tag without reassigning it", async ({ + expect, + }) => { + let tag = makeTag(TAG_A, "Production", "#1a9e4a") + const treeCalls = mockEndpoint("GET", "/api/tags/tree", () => [tag]) + const branchCalls = mockEndpoint("GET", BRANCH_TAGS_URL, () => [TAG_A]) + const updateCalls = mockEndpoint("PUT", `/api/tags/${TAG_A}`, () => { + tag = { ...tag, tagName: "Live", color: "#f97316" } + + return {} + }) + const wrapper = await mountTags() + const api = runInApp(() => useTagAPI()) + + await api.updateTag.mutateAsync({ + id: TAG_A, + req: { tagName: "Live", color: "#f97316" }, + }) + await settleMutations() + + expect(updateCalls).toHaveLength(1) + expect(treeCalls).toHaveLength(2) + expect(branchCalls).toHaveLength(1) + expect( + wrapper.findAllComponents(TagPill).map((pill) => ({ + name: pill.props("name"), + color: pill.props("color"), + })), + ).toEqual([{ name: "Live", color: "#f97316" }]) + }) + it("refetches the branch's tags when the server says they changed", async ({ expect, }) => { diff --git a/web/app/composables/api/useTagAPI.nuxt.test.ts b/web/app/composables/api/useTagAPI.nuxt.test.ts index 491c0058..2ad83e43 100644 --- a/web/app/composables/api/useTagAPI.nuxt.test.ts +++ b/web/app/composables/api/useTagAPI.nuxt.test.ts @@ -346,6 +346,80 @@ describe("useTagAPI", { concurrent: false }, () => { }) }) + describe("updateTag", () => { + it.for([ + { name: "renames", req: { tagName: "Live" } }, + { name: "recolours", req: { color: "#f97316" } }, + { + name: "renames and recolours", + req: { tagName: "Live", color: "#f97316" }, + }, + ])( + "$name a tag without changing its assignments", + async ({ req }, { expect }) => { + const original = makeTag(TAG_A, "Production", [ + makeDoc(DOC_A, "Runbook"), + ]) + const updated = { ...original, ...req } + const treeCalls = mockEndpoint("GET", TREE_URL, () => [updated]) + const putCalls = mockEndpoint("PUT", `/api/tags/${TAG_A}`, () => ({})) + seedQueryData(TREE_KEY, [original]) + seedQueryData(BRANCH_A_TAGS_KEY, [TAG_A]) + const api = makeTagAPI() + + await api.updateTag.mutateAsync({ id: TAG_A, req }) + await flushPromises() + + expect(putCalls).toHaveLength(1) + expect(putCalls[0]?.body).toEqual(req) + expect(treeCalls).toHaveLength(1) + expect(readTree()).toEqual([updated]) + expect(branchTagIds()).toEqual([TAG_A]) + }, + ) + + it("keeps the current tag while saving and after a failed save", async ({ + expect, + }) => { + const original = makeTag(TAG_A, "Production", [makeDoc(DOC_A, "Runbook")]) + const treeCalls = mockEndpoint("GET", TREE_URL, () => []) + const put = mockDeferredEndpoint("PUT", `/api/tags/${TAG_A}`) + seedQueryData(TREE_KEY, [original]) + seedQueryData(BRANCH_A_TAGS_KEY, [TAG_A]) + const api = makeTagAPI() + const pending = api.updateTag.mutateAsync({ + id: TAG_A, + req: { tagName: "Live" }, + }) + const rejected = expect(pending).rejects.toThrow() + await put.reached + + expect(readTree()).toEqual([original]) + put.reject(createError({ statusCode: 409 })) + await rejected + + expect(put.calls).toHaveLength(1) + expect(treeCalls).toHaveLength(0) + expect(readTree()).toEqual([original]) + expect(branchTagIds()).toEqual([TAG_A]) + }) + + it("bails out for a non-xid tag without a request", async ({ expect }) => { + const treeCalls = mockEndpoint("GET", TREE_URL, () => []) + const putCalls = mockEndpoint("PUT", `/api/tags/${SHORT_ID}`, () => ({})) + seedQueryData(TREE_KEY, [makeTag(SHORT_ID, "Draft")]) + const api = makeTagAPI() + + await api.updateTag.mutateAsync({ + id: SHORT_ID, + req: { tagName: "Live" }, + }) + + expect(putCalls).toHaveLength(0) + expect(treeCalls).toHaveLength(0) + }) + }) + describe("updateTagVisibility", () => { it("bails out for a non-xid tag id without a request", async ({ expect, diff --git a/web/app/composables/api/useTagAPI.ts b/web/app/composables/api/useTagAPI.ts index dc34c0d2..208afe4e 100644 --- a/web/app/composables/api/useTagAPI.ts +++ b/web/app/composables/api/useTagAPI.ts @@ -7,6 +7,7 @@ import type { TagTreeElement, TagTreeResponse, TagTreeUpdateRequest, + TagUpdateRequest, TagVisibilityRequest, UnprocessedTagTreeUpdateRequest, BranchTagRequest, @@ -170,6 +171,27 @@ export default function () { }, }) + const updateTag = useMutation({ + mutation: async ({ id, req }: { id: string; req: TagUpdateRequest }) => { + if (!isXid(id)) { + // optimistic inserts have not reached the server yet + return + } + + await $coreAPIClient(`/api/tags/${id}`, { + method: "PUT", + body: req, + }) + }, + async onSuccess(_data, { id }) { + if (!isXid(id)) { + return + } + + await queryCache.invalidateQueries({ key: TAG_QUERY_KEYS.tree }) + }, + }) + const updateTagVisibility = useMutation({ onMutate({ id, req }: { id: string; req: TagVisibilityRequest }) { if (!isXid(id)) { @@ -543,6 +565,7 @@ export default function () { updateTagTreeDocumentCache, updateTagTree, createTag, + updateTag, updateTagVisibility, deleteTag, useFetchBranchTags, diff --git a/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue b/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue index 232bd059..a01893b6 100644 --- a/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue +++ b/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue @@ -145,6 +145,10 @@ const pendingDocDeletion = ref<{ deleteDocument: () => Promise name: string } | null>(null) +const pendingTagEdit = ref | null>(null) const pendingTagDeletion = ref<{ deleteTag: () => Promise name: string @@ -742,6 +746,7 @@ function clearLinkedBlock() { @duplicate-document="handleDocumentDuplication" @delete-document="handleDocumentDeletion" @delete-tag="handleTagDeletion" + @edit-tag="pendingTagEdit = $event" @open-settings="(target) => (settingModalOpen = target)" @toggle-settings="settingModalOpen = !settingModalOpen" @toggle-shortcuts="shortcutsModalOpen = !shortcutsModalOpen" @@ -762,6 +767,7 @@ function clearLinkedBlock() { v-model="pendingDocDeletion" /> +