From 80458cb742227d9a9ed085e620f080658baf59fe Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Sat, 10 Oct 2026 16:08:43 +0200 Subject: [PATCH] feat(tags): edit tag names and colors Add a tag editing dialog, sidebar action, and API mutation. Expose the existing tag update operation through an authenticated HTTP route and refresh tag displays after edits. Add handler, router, database, API, and component regression coverage. Keep the existing palette, schema, and dependencies unchanged. WIP transfer snapshot v1. 149 focused Nuxt tests, tag-handler and domain race tests, formatting, and independent source review pass. Broader static, database, end-to-end, and aggregate checks remain incomplete; this is not release-ready. --- server/README.md | 16 + server/core/internal/db/tag_test.go | 49 ++- server/core/internal/server/0mock_db_test.go | 65 ++++ .../server/internal/tag/0mock_db_test.go | 65 ++++ .../core/internal/server/internal/tag/http.go | 44 +++ .../internal/server/internal/tag/http_test.go | 268 ++++++++++++++ server/core/internal/server/router.go | 1 + server/core/internal/server/router_test.go | 2 + web/app/components/AppSidebar.nuxt.test.ts | 72 +++- web/app/components/AppSidebar.vue | 17 + web/app/components/TagEditModal.nuxt.test.ts | 350 ++++++++++++++++++ web/app/components/TagEditModal.vue | 183 +++++++++ .../editor/DocumentTagList.nuxt.test.ts | 32 ++ .../composables/api/useTagAPI.nuxt.test.ts | 74 ++++ web/app/composables/api/useTagAPI.ts | 23 ++ .../[[organizationSlug]]/[[documentSlug]].vue | 6 + web/app/utils/api/tag.ts | 5 + web/i18n/locales/en/sidebar.json | 12 +- 18 files changed, 1273 insertions(+), 11 deletions(-) create mode 100644 web/app/components/TagEditModal.nuxt.test.ts create mode 100644 web/app/components/TagEditModal.vue 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" /> +