amadan
Sharing is findable: shared documents on the list, and a way in
Slice 2 shipped grants that worked and could not be reached. A grantee
could open a shared document only by keeping its URL in a chat
scrollback, and the share page existed with nothing anywhere leading to
it. Both halves of that are the difference between "sharing works" and
"sharing is usable", and neither is a polish item: a person who has to
keep a link to reach a document does not have the document.
ListFor and SearchFor sit beside List and Search rather than widening
them, the split GetFor/Get already established. Widening in place would
have handed grantees to every caller that genuinely wants ownership —
silently, which is the failure mode worth paying a second name to avoid.
SearchFor exists because Search runs over List rather than over SQL, so
widening only the list page would have produced an app where a shared
document is on the list and cannot be searched for. That reads as a
broken search rather than as a scope decision, and it is the sort of
split a person meets on their second day. Both spellings run one
ranking (rankAndTrim), so the list, the palette and search cannot drift.
The share control goes in the shell's "nav" block, which topbar.html
renders inside [rst-shell-tail] immediately before the account
disclosure — the position Paul asked for, and a slot the design system
already owns rather than a bar this app draws for itself (spec §10).
It is a label, not an icon, and that is a finding rather than a
shortcut: rastrillo's icon set is a closed vocabulary of twelve with no
"share" in it, and Icon renders an unknown slug as nothing at all. So
{{icon "share"}} would have shipped an invisible control that no test
here would have caught. A thirteenth icon is a rastrillo change.
Owners only. A writer grantee is not offered it — no grant carries
re-sharing — because sharePage answers them 404, and a control that
visibly fails reads as a broken app rather than as a boundary.
Four mutations, each caught: CanShare pinned true, ListFor reverted to
owned, readable's subject test dropped, and SearchFor falling back to
owner scope. The third is the one that mattered — its first sed silently
did not match, so it was passing as caught while never having been
applied.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RVcEQ8NgrPCpRTb6JtBz1Q
6 files changed,
+285
−12
internal/docs/grants.go+26 −0internal/docs/handlers.go+36 −5internal/docs/search.go+39 −7internal/docs/share_nav_test.go+72 −0internal/docs/shared_list_test.go+99 −0internal/docs/views/show.html+13 −0
CI passed — run details
Intent
diff --git a/internal/docs/grants.go b/internal/docs/grants.go| index a8c9c3b..e663637 100644 |
| --- a/internal/docs/grants.go |
| +++ b/internal/docs/grants.go |
| @@ -223,3 +223,29 @@ func (s *Store) GetFor(ctx context.Context, a Actor, sqid string) (*Document, er |
| } |
| return &doc, nil |
| } |
| + |
| +// ListFor returns every non-deleted document the acting person may read — |
| +// the ones they own, plus the ones they hold a grant on — newest first, on |
| +// the same UpdatedAt clock List uses so this list and handlers.go's recency |
| +// buckets cannot disagree. |
| +// |
| +// A second method beside List rather than a widening of it, for the reason |
| +// GetFor gives: List is owner-scoped and callers exist that want exactly |
| +// that, and widening in place hands every one of them grantees silently. |
| +// |
| +// The order deliberately does not separate "mine" from "shared with me". |
| +// Recency is the sort the list page already teaches, and a shared document |
| +// somebody is working in today belongs above one of yours you have not |
| +// opened since spring. Which are yours is a question the row can answer |
| +// without reordering the list. |
| +func (s *Store) ListFor(ctx context.Context, a Actor) ([]*Document, error) { |
| + var docs []*Document |
| + err := readable(s.db.WithContext(ctx), a.PersonID, a.Subject). |
| + Where("documents.deleted_at IS NULL"). |
| + Order("documents.updated_at DESC, documents.id DESC"). |
| + Find(&docs).Error |
| + if err != nil { |
| + return nil, fmt.Errorf("docs: list readable documents: %w", err) |
| + } |
| + return docs, nil |
| +} |
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go| index 4260f0b..e920f1e 100644 |
| --- a/internal/docs/handlers.go |
| +++ b/internal/docs/handlers.go |
| @@ -194,7 +194,9 @@ func (h *handlers) actor(r *http.Request) (Actor, error) { |
| // the page — rendering it would be inventing display data, not |
| // showing real ownership. |
| func (h *handlers) list(w http.ResponseWriter, r *http.Request) { |
| - owner, err := h.owner(r) |
| + // actor, not owner: this page answers "what can I reach", which |
| + // since sharing landed is a wider question than "what is mine". |
| + who, err := h.actor(r) |
| if err != nil { |
| h.logger.Error("docs: resolve signed-in person", "err", err) |
| http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
| @@ -226,7 +228,7 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) { |
| } |
| if query != "" { |
| - hits, err := h.store.Search(r.Context(), owner, query) |
| + hits, err := h.store.SearchFor(r.Context(), who, query) |
| if err != nil { |
| h.logger.Error("docs: search documents", "err", err) |
| http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
| @@ -250,7 +252,7 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| - docs, err := h.store.List(r.Context(), owner) |
| + docs, err := h.store.ListFor(r.Context(), who) |
| if err != nil { |
| h.logger.Error("docs: list documents", "err", err) |
| http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
| @@ -326,8 +328,35 @@ type showPage struct { |
| // to happen in eleven locales nobody in this repo reads. One key, |
| // one string, and the link can never disagree with the page it |
| // leads to. |
| - BackLabel string |
| - Sqid string // views/show.html's [docs-editor] mount point's data-sqid, the editor bundle's own base for /d/{sqid}/updates |
| + BackLabel string |
| + Sqid string // views/show.html's [docs-editor] mount point's data-sqid, the editor bundle's own base for /d/{sqid}/updates |
| + |
| + // CanShare and ShareLabel drive the share control views/show.html |
| + // puts in the shell bar's "nav" block — which topbar.html renders |
| + // inside [rst-shell-tail], immediately before the account |
| + // disclosure. That is the position Paul asked for, and it is a slot |
| + // the shell already owns; drawing a bar of our own to hold it is the |
| + // chrome drift spec §10 says to watch for. |
| + // |
| + // CanShare is ownership, not readability. A grantee — a writer |
| + // included, since no grant carries re-sharing — is not offered it, |
| + // because sharePage is owner-only and answers them 404. Rendering a |
| + // control that visibly fails is worse than rendering none: it reads |
| + // as a broken app rather than as a boundary. |
| + // |
| + // ShareLabel reuses docs.share.title rather than adding a key of its |
| + // own, for BackLabel's reason directly above: one string, so a |
| + // control can never disagree with the page it leads to across |
| + // eleven locales. |
| + // |
| + // It is a label rather than a glyph because rastrillo's icon set has |
| + // no "share" in it. That set is a closed vocabulary of twelve, whose |
| + // own comments argue each addition, and Icon renders an unknown slug |
| + // as nothing at all — so {{icon "share"}} would have shipped an |
| + // invisible control that no test of ours would have caught. A |
| + // thirteenth icon is a rastrillo change, not a docs one. |
| + CanShare bool |
| + ShareLabel string |
| EditorScript string // views/show.html's <script src>, from editorScriptPath() — never a literal, so it can never drift from what staticHandler actually serves |
| // SaveRetrying/SaveFailed are the editor's own save-status copy |
| @@ -477,6 +506,8 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) { |
| SignOutLabel: rastrillo.T(r, "docs.signout.label"), |
| BackLabel: rastrillo.T(r, "docs.list.title"), |
| Sqid: doc.Sqid, |
| + CanShare: doc.OwnerID == actor.PersonID, |
| + ShareLabel: rastrillo.T(r, "docs.share.title"), |
| EditorScript: editorScriptPath(), |
| SaveRetrying: rastrillo.T(r, "docs.editor.status.retrying"), |
| SaveFailed: rastrillo.T(r, "docs.editor.status.failed"), |
diff --git a/internal/docs/search.go b/internal/docs/search.go| index 32fe3f5..e06ca09 100644 |
| --- a/internal/docs/search.go |
| +++ b/internal/docs/search.go |
| @@ -126,12 +126,44 @@ func (s *Store) Search(ctx context.Context, owner int64, query string) ([]*Docum |
| if q == "" { |
| return nil, nil |
| } |
| - |
| docs, err := s.List(ctx, owner) |
| if err != nil { |
| return nil, err |
| } |
| + return rankAndTrim(docs, q), nil |
| +} |
| + |
| +// SearchFor is Search over everything the acting person may read: theirs, |
| +// plus what has been shared with them. It sits beside Search for the same |
| +// reason ListFor sits beside List. |
| +// |
| +// It exists at all because Search runs over a list rather than over SQL, |
| +// so widening only the list page would have shipped an app where a shared |
| +// document is on the list and cannot be searched for. That reads as a |
| +// broken search, not as a scope decision, and it is the sort of split a |
| +// person hits on their second day rather than their first. |
| +// |
| +// Both spellings run the same ranking, so the list page, the palette and |
| +// this cannot drift — the plan's D3, kept while the scope widened. |
| +func (s *Store) SearchFor(ctx context.Context, a Actor, query string) ([]*Document, error) { |
| + q := normaliseForSearch(query) |
| + if q == "" { |
| + return nil, nil |
| + } |
| + docs, err := s.ListFor(ctx, a) |
| + if err != nil { |
| + return nil, err |
| + } |
| + return rankAndTrim(docs, q), nil |
| +} |
| +// rankAndTrim is the ranking both spellings share: keep the titles that |
| +// match, best rank first, capped at maxSearchResults. |
| +// |
| +// It takes documents rather than a scope so that the scope decision lives |
| +// in exactly one place per caller — the List or ListFor above it — and |
| +// cannot be half-applied here. |
| +func rankAndTrim(docs []*Document, q string) []*Document { |
| type scored struct { |
| doc *Document |
| rank searchRank |
| @@ -143,9 +175,9 @@ func (s *Store) Search(ctx context.Context, owner int64, query string) ([]*Docum |
| } |
| } |
| - // SliceStable, and List already returns newest-first: that makes |
| - // "most recently updated" the tie-break without re-reading a clock |
| - // or comparing timestamps here. |
| + // SliceStable, and the list arrives newest-first: that makes "most |
| + // recently updated" the tie-break without re-reading a clock or |
| + // comparing timestamps here. |
| sort.SliceStable(hits, func(i, j int) bool { return hits[i].rank < hits[j].rank }) |
| if len(hits) > maxSearchResults { |
| @@ -155,7 +187,7 @@ func (s *Store) Search(ctx context.Context, owner int64, query string) ([]*Docum |
| for _, h := range hits { |
| out = append(out, h.doc) |
| } |
| - return out, nil |
| + return out |
| } |
| // searchResult is one row of the JSON the palette reads. |
| @@ -177,14 +209,14 @@ type searchResult struct { |
| // here — the same property every other document route has, by the same |
| // mechanism rather than by a check of its own. |
| func (h *handlers) searchJSON(w http.ResponseWriter, r *http.Request) { |
| - owner, err := h.owner(r) |
| + who, err := h.actor(r) |
| if err != nil { |
| h.logger.Error("docs: resolve signed-in person", "err", err) |
| http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
| return |
| } |
| - docs, err := h.store.Search(r.Context(), owner, r.URL.Query().Get("q")) |
| + docs, err := h.store.SearchFor(r.Context(), who, r.URL.Query().Get("q")) |
| if err != nil { |
| h.logger.Error("docs: search documents", "err", err) |
| http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError) |
diff --git a/internal/docs/share_nav_test.go b/internal/docs/share_nav_test.go| new file mode 100644 |
| index 0000000..93ff9ce |
| --- /dev/null |
| +++ b/internal/docs/share_nav_test.go |
| @@ -0,0 +1,72 @@ |
| +package docs_test |
| + |
| +import ( |
| + "net/http" |
| + "strings" |
| + "testing" |
| + |
| + "github.com/carlosframework/docs/internal/docs" |
| +) |
| + |
| +// Paul's note: "there should be a 'share' icon probably beside the account |
| +// dropdown top right". Before this, the share page existed and nothing led |
| +// to it — sharing worked only for somebody who knew to type /share onto a |
| +// URL, which is not a feature, it is a rumour. |
| +// |
| +// The control goes in the shell's own "nav" block, which topbar.html puts |
| +// inside [rst-shell-tail] immediately before the account disclosure. That |
| +// is the requested position, and it is a slot the design system already |
| +// owns rather than a bar this app draws for itself (spec §10). |
| +func TestTheOwnerGetsAShareControlInTheBar(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + alice := signInCookies(t, app, "alice@example.com") |
| + sqid := createDoc(t, app, alice, "Minutes") |
| + |
| + rec := get(t, app, alice, "/d/"+sqid) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET /d/%s: status %d, want 200", sqid, rec.Code) |
| + } |
| + body := rec.Body.String() |
| + if !strings.Contains(body, "/d/"+sqid+"/share") { |
| + t.Error("the document page offers no route to its own share page") |
| + } |
| + |
| + // Position, not just presence: the link has to be in the shell's tail, |
| + // beside the account dropdown, rather than anywhere on the page. A |
| + // test that only asserted presence would pass on a link buried in the |
| + // footer. |
| + tail := body[strings.Index(body, "rst-shell-tail"):] |
| + acct := strings.Index(tail, "rst-shell-account") |
| + link := strings.Index(tail, "/d/"+sqid+"/share") |
| + if link < 0 || acct < 0 || link > acct { |
| + t.Error("the share control is not in the shell tail before the account dropdown") |
| + } |
| +} |
| + |
| +// A grantee must not be offered it. sharePage is owner-only and answers a |
| +// grantee with 404, so a share link on their page would be a control that |
| +// visibly fails — worse than no control, because it reads as a broken app |
| +// rather than as a boundary. |
| +func TestAGranteeIsNotOfferedTheShareControl(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + alice := signInCookies(t, app, "alice@example.com") |
| + sqid := createDoc(t, app, alice, "Minutes") |
| + bob := signInCookies(t, app, "bob@example.com") |
| + |
| + // A writer, deliberately: the strongest grant there is still does not |
| + // carry re-sharing (Store.Grant's own rule). |
| + post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer") |
| + |
| + rec := get(t, app, bob, "/d/"+sqid) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET /d/%s as Bob: status %d, want 200", sqid, rec.Code) |
| + } |
| + if strings.Contains(rec.Body.String(), "/d/"+sqid+"/share") { |
| + t.Error("a writer grantee was offered a share control they cannot use") |
| + } |
| + |
| + // And the boundary it stands for is real, not just hidden in markup. |
| + if rec := get(t, app, bob, "/d/"+sqid+"/share"); rec.Code != http.StatusNotFound { |
| + t.Errorf("GET share as a grantee: status %d, want 404", rec.Code) |
| + } |
| +} |
diff --git a/internal/docs/shared_list_test.go b/internal/docs/shared_list_test.go| new file mode 100644 |
| index 0000000..2813afa |
| --- /dev/null |
| +++ b/internal/docs/shared_list_test.go |
| @@ -0,0 +1,99 @@ |
| +package docs_test |
| + |
| +import ( |
| + "net/http" |
| + "strings" |
| + "testing" |
| + |
| + "github.com/carlosframework/docs/internal/docs" |
| +) |
| + |
| +// The difference between "sharing works" and "sharing is usable". Slice 2 |
| +// left a grantee able to open a shared document at its direct URL and with |
| +// no way to find it again: List was owner-only, so the document Alice |
| +// shared was reachable only by somebody who had kept the link. |
| +// |
| +// A person who has to keep a URL in a chat scrollback to reach a document |
| +// does not have the document. |
| +func TestASharedDocumentAppearsInTheGranteesList(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + alice := signInCookies(t, app, "alice@example.com") |
| + sqid := createDoc(t, app, alice, "Minutes") |
| + |
| + // Before the grant, Bob's list does not mention it. Without this the |
| + // test would pass on a list that shows everybody everything. |
| + bob := signInCookies(t, app, "bob@example.com") |
| + if rec := get(t, app, bob, "/"); strings.Contains(rec.Body.String(), "Minutes") { |
| + t.Fatal("Bob's list named a document nobody had shared with him") |
| + } |
| + |
| + if rec := post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader"); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("POST share: status %d, want 303", rec.Code) |
| + } |
| + |
| + rec := get(t, app, bob, "/") |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET / as Bob: status %d, want 200", rec.Code) |
| + } |
| + if !strings.Contains(rec.Body.String(), "Minutes") { |
| + t.Error("Bob's list does not name the document shared with him") |
| + } |
| + if !strings.Contains(rec.Body.String(), "/d/"+sqid) { |
| + t.Error("Bob's list names the document but does not link to it") |
| + } |
| + |
| + // Carol was granted nothing and sees nothing: the list widened by |
| + // exactly one grant, not into a list of everything. |
| + carol := signInCookies(t, app, "carol@example.com") |
| + if rec := get(t, app, carol, "/"); strings.Contains(rec.Body.String(), "Minutes") { |
| + t.Error("Carol's list named a document she holds no grant on") |
| + } |
| +} |
| + |
| +// Revoking takes it back out of the list, not just out of the document. A |
| +// revoke that left the row on the grantee's list would leave them clicking |
| +// into a 404 and reading it as a bug. |
| +func TestRevokingRemovesItFromTheGranteesList(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + alice := signInCookies(t, app, "alice@example.com") |
| + sqid := createDoc(t, app, alice, "Minutes") |
| + bob := signInCookies(t, app, "bob@example.com") |
| + |
| + post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader") |
| + if rec := get(t, app, bob, "/"); !strings.Contains(rec.Body.String(), "Minutes") { |
| + t.Fatal("precondition: the shared document is not on Bob's list") |
| + } |
| + |
| + if rec := post(t, app, alice, "/d/"+sqid+"/share/revoke", "address=bob@example.com"); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("POST revoke: status %d, want 303", rec.Code) |
| + } |
| + if rec := get(t, app, bob, "/"); strings.Contains(rec.Body.String(), "Minutes") { |
| + t.Error("a revoked document is still on Bob's list") |
| + } |
| +} |
| + |
| +// Search is the list's other door. Slice 2's Search ran over List, so |
| +// widening only the list handler would have produced an app where a shared |
| +// document is on the list but cannot be searched for — the kind of split |
| +// that reads as a broken search rather than as a scope decision. |
| +func TestAGranteeCanSearchForASharedDocument(t *testing.T) { |
| + app, _ := docs.NewForTest(t) |
| + alice := signInCookies(t, app, "alice@example.com") |
| + sqid := createDoc(t, app, alice, "Quarterly plan") |
| + bob := signInCookies(t, app, "bob@example.com") |
| + |
| + post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader") |
| + |
| + rec := get(t, app, bob, "/?q=quarterly") |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET /?q=quarterly as Bob: status %d, want 200", rec.Code) |
| + } |
| + if !strings.Contains(rec.Body.String(), "Quarterly plan") { |
| + t.Error("Bob cannot search for the document shared with him") |
| + } |
| + |
| + carol := signInCookies(t, app, "carol@example.com") |
| + if rec := get(t, app, carol, "/?q=quarterly"); strings.Contains(rec.Body.String(), "Quarterly plan") { |
| + t.Error("Carol's search found a document she holds no grant on") |
| + } |
| +} |
diff --git a/internal/docs/views/show.html b/internal/docs/views/show.html| index 3f4b6c0..ccc4b71 100644 |
| --- a/internal/docs/views/show.html |
| +++ b/internal/docs/views/show.html |
| @@ -66,6 +66,19 @@ |
| full-width menu row, so it needs no rst-btn of its own — which |
| is the point of putting it here instead of leaving it as a ghost |
| button in a page header. */}} |
| +{{/* nav is the shell's slot inside [rst-shell-tail], rendered |
| + immediately before the account disclosure — so this link lands |
| + beside the account dropdown at the top right, which is where a |
| + share control belongs and where Paul asked for one. |
| + |
| + A link, not a form: opening the share page is a read, and GET is |
| + what a prefetcher, a middle-click and a bookmark all expect. The |
| + mutations live on that page, each in its own POST. |
| + |
| + Its text rather than an icon because the design system has no |
| + share glyph — see showPage.CanShare's comment for why inventing |
| + one here would have shipped an invisible control. */}} |
| +{{define "nav"}}{{if .CanShare}}<a href="/d/{{.Sqid}}/share">{{.ShareLabel}}</a>{{end}}{{end}} |
| {{define "account"}}<form method="post" action="/signout"><button type="submit">{{.SignOutLabel}}</button></form>{{end}} |
| {{define "content"}}<style> |
| /* This screen is the one that is not a column of content. It is a |