amadan
feat(list): the verbs move to the chrome, and naming survives the move
The sheet list opened with two stacked forms: a title field with a
Create button, then a dashed drop zone holding a file input and four
lines of limits. The sheets themselves began below both. A list of
sheets should be a list of sheets, so both come off the page.
Create is a button in the top bar beside the account menu. It posts an
empty form, which handleSheetCreate already defaults to "Untitled
sheet", and lands you in the grid — one click, no field, no dialog.
Import moves to a page of its own at /sheets/import. It nearly went
into the File menu instead, and three things argued it out: a dropdown
is a poor container for a file picker plus three limits; a hidden input
behind a menu-shaped label never submits itself, so that item would
have needed script to import anything, and PRODUCT.md's second
principle makes the scriptless path the product rather than the
fallback; and the File menu exists only on a sheet page, so somebody
with no sheets yet — the person most likely to be importing one — would
have had no way in at all. The File menu links to the page instead, one
line changed. drop.js moved with the form it decorates, so drag and
drop is still the same submit of the same form.
Refusals follow the form. The reasoning for keeping one on the page
that caused it was right and is unchanged — no flash, no cookie, no
redirect — so renderSheetList's refusal parameter did not get deleted,
it became renderImportPage's.
Rename is the other half of this, not a feature beside it. Removing the
title field means every sheet is born "Untitled sheet", and there was
no rename route: layout.html said so in a comment explaining that a
title which looked editable would be a promise the app could not keep.
Left alone, this change would have produced a list of identically named
sheets distinguished only by their new thumbnails, with no way to fix
one. So the top bar's title becomes an inline form posting to
/sheets/{id}/rename, guarded by mayWrite, 404 not 403 for anyone else.
Inline rather than a popover, and that is not a style preference: a
<details> there would join rastrillo's rst-menus exclusive group, so
opening rename would close the File menu and be closed by it — the
nested-accordion trap TestNoNestedExclusiveDetails exists to catch.
The field reads as a title until it is focused, because a text input
sitting permanently in the top bar of a spreadsheet competes with the
grid for "the thing you type in" and the grid has to win by default.
The 22rem ellipsis moved off the control onto a wrapper on the way: on
an input, overflow: hidden clips the caret, so past 22rem you would be
editing text you could not see.
A renamed title is sealed under the same purpose createSheet uses. The
one rule has no exception for the second time a name is set, and
TestRenamedTitleIsSealedAtRest greps the column for it.
Three existing tests moved rather than changed meaning: import's
scriptless floor is now asked of /sheets/import, the affordance gate
asks the sheet page for name="title" since that is where naming lives,
and the refusal suite's comment about answering "on the list page" was
about to become a lie.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2eSeoc1hxEgMMH2AHGcCo
14 files changed,
+718
−82
internal/sheets/app.go+10 −0internal/sheets/handlers_import.go+2 −2internal/sheets/handlers_import_page.go+60 −0internal/sheets/handlers_import_refusals_test.go+13 −4internal/sheets/handlers_list_chrome_test.go+130 −0internal/sheets/handlers_rename.go+112 −0internal/sheets/handlers_rename_test.go+163 −0internal/sheets/handlers_sheets.go+16 −17internal/sheets/handlers_table_test.go+13 −2internal/sheets/static/sheets.css+61 −1internal/sheets/templates/import.html+53 −0internal/sheets/templates/layout.html+12 −0internal/sheets/templates/sheet.html+36 −2internal/sheets/templates/sheets.html+37 −54
diff --git a/internal/sheets/app.go b/internal/sheets/app.go| index e7a000d..614c2d6 100644 |
| --- a/internal/sheets/app.go |
| +++ b/internal/sheets/app.go |
| @@ -190,6 +190,13 @@ func (a *App) Router() http.Handler { |
| r.Use(a.requireSession) |
| r.Get("/sheets", a.handleSheetList) |
| r.Post("/sheets", a.handleSheetCreate) |
| + // Static before the {sheetID} pattern below. chi prefers an |
| + // exact segment over a wildcard whatever the order, so this is |
| + // belt and braces rather than load-bearing — but a route that |
| + // only works because of resolution order is a route one |
| + // refactor away from parsing "import" as a sheet id. |
| + // TestImportPathIsNotASheetID pins it. |
| + r.Get("/sheets/import", a.handleImportPage) |
| // Import. A whole workbook is too much work for a request, so |
| // it 303s to a job (spec §11) rather than holding the |
| // connection open — and the drop zone on the list posts to |
| @@ -197,6 +204,9 @@ func (a *App) Router() http.Handler { |
| // over the form, not a second way in. |
| r.Post("/sheets/import", a.handleSheetImport) |
| r.Get("/sheets/{sheetID}", a.handleSheetPage) |
| + // Rename. The other half of a Create button that no longer |
| + // asks for a name — see handlers_rename.go. |
| + r.Post("/sheets/{sheetID}/rename", a.handleSheetRename) |
| // The scriptless view. Spec §6 names the grid editor, drag-fill |
| // and live sync as the only script-required things; everything |
| // else, including editing any cell, works here. |
diff --git a/internal/sheets/handlers_import.go b/internal/sheets/handlers_import.go| index 09ba94f..5b7be53 100644 |
| --- a/internal/sheets/handlers_import.go |
| +++ b/internal/sheets/handlers_import.go |
| @@ -111,7 +111,7 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) { |
| data, filename, refusal := readUpload(w, r) |
| if refusal != "" { |
| - a.renderSheetList(w, r, http.StatusBadRequest, refusal) |
| + a.renderImportPage(w, r, http.StatusBadRequest, refusal) |
| return |
| } |
| @@ -131,7 +131,7 @@ func (a *App) handleSheetImport(w http.ResponseWriter, r *http.Request) { |
| // nothing and nobody asked for it. |
| a.discardSheet(s.ID) |
| if errors.Is(err, jobs.ErrOwnerBusy) { |
| - a.renderSheetList(w, r, http.StatusTooManyRequests, refusalBusy) |
| + a.renderImportPage(w, r, http.StatusTooManyRequests, refusalBusy) |
| return |
| } |
| a.fail(w, r, err) |
diff --git a/internal/sheets/handlers_import_page.go b/internal/sheets/handlers_import_page.go| new file mode 100644 |
| index 0000000..a5e78d9 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_import_page.go |
| @@ -0,0 +1,60 @@ |
| +package sheets |
| + |
| +import "net/http" |
| + |
| +// The import page. |
| +// |
| +// Separate from handlers_import.go, which holds the POST that does the |
| +// work, for the dullest possible reason: together they were 305 lines |
| +// and the file cap is 300 (AGENTS.md / Mechanical rules, enforced by |
| +// TestFileLineCap). The split falls in a sensible place anyway — this |
| +// file is the screen, that one is the upload. |
| + |
| +// importPageView is the import page's payload. |
| +type importPageView struct { |
| + // Error is a refusal to show above the form — one of the refusal |
| + // constants in handlers_import.go, never a wrapped error, because |
| + // everything shown here is read by somebody deciding what to do |
| + // differently. |
| + Error string |
| +} |
| + |
| +// handleImportPage draws the import form on a page of its own. |
| +// |
| +// Import used to be a second form on the sheet list, below the form |
| +// that made a blank sheet. Both came off that page: a list of sheets |
| +// should be a list of sheets, and the verbs moved into the chrome |
| +// around it. |
| +// |
| +// A page rather than an item inside the File menu, which is where this |
| +// nearly went. Three reasons, and the third is the one that decided it: |
| +// a dropdown is a poor container for a file picker plus three limits; a |
| +// hidden input behind a menu-shaped label never submits itself, so that |
| +// menu item would have needed script to import anything at all, and |
| +// import is meant to work with scripting off; and the File menu exists |
| +// only on a sheet page, so somebody with no sheets yet — exactly the |
| +// person most likely to be importing one — would have had no way in. |
| +func (a *App) handleImportPage(w http.ResponseWriter, r *http.Request) { |
| + // Behind requireSession already; this is the same belt-and-braces |
| + // actor check the sibling handlers make, and it is what makes the |
| + // handler safe to call from a test that does not mount the group. |
| + if _, ok := a.actor(r); !ok { |
| + http.Error(w, "Sign in first.", http.StatusForbidden) |
| + return |
| + } |
| + a.renderImportPage(w, r, http.StatusOK, "") |
| +} |
| + |
| +// renderImportPage draws that form, optionally carrying a refusal and |
| +// the status that goes with it. |
| +// |
| +// A refused import re-renders this page rather than redirecting to it, |
| +// so the refusal survives without a flash: the person lands back on the |
| +// form they posted from, with the reason above it. No session storage, |
| +// no cookie, no redirect, and it works with scripting off by |
| +// construction. That reasoning arrived here from renderSheetList along |
| +// with the form itself. |
| +func (a *App) renderImportPage(w http.ResponseWriter, r *http.Request, status int, refusal string) { |
| + a.render(w, r, "import", "Import a spreadsheet — Oficina Sheets", status, |
| + importPageView{Error: refusal}) |
| +} |
diff --git a/internal/sheets/handlers_import_refusals_test.go b/internal/sheets/handlers_import_refusals_test.go| index ac77d1b..1d448e6 100644 |
| --- a/internal/sheets/handlers_import_refusals_test.go |
| +++ b/internal/sheets/handlers_import_refusals_test.go |
| @@ -15,8 +15,12 @@ import ( |
| ) |
| // TestImportRefusesWhatItCannotRead covers every refusal a person can |
| -// hit before a job is ever started — each one answering on the list |
| -// page they posted from, with the reason beside the form. |
| +// hit before a job is ever started — each one answering on the import |
| +// page they posted from, with the reason above the form. |
| +// |
| +// That page used to be the sheet list, which was where the import form |
| +// lived. The form moved; the principle did not, and it is the reason a |
| +// refusal still needs no flash, no cookie and no redirect. |
| func TestImportRefusesWhatItCannotRead(t *testing.T) { |
| for _, tc := range []struct { |
| name string |
| @@ -175,11 +179,16 @@ func TestTitleFromFilename(t *testing.T) { |
| // product rather than the fallback, and drag-and-drop is the easiest |
| // place in this app to break it — a fetch-based uploader would look |
| // identical in a browser and leave nothing behind for anyone else. |
| +// Asked of /sheets/import, not of /sheets. The form moved to a page of |
| +// its own when the list page's two inline forms came off it; the |
| +// question this test asks is unchanged and is the reason the form |
| +// became a page rather than an item in the File menu, where a hidden |
| +// input behind a label would have needed script to submit at all. |
| func TestImportIsScriptless(t *testing.T) { |
| app, cookies := signIn(t) |
| - rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets", nil), cookies)) |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets/import", nil), cookies)) |
| if rec.Code != http.StatusOK { |
| - t.Fatalf("sheet list answered %d, want 200", rec.Code) |
| + t.Fatalf("import page answered %d, want 200", rec.Code) |
| } |
| body := rec.Body.String() |
diff --git a/internal/sheets/handlers_list_chrome_test.go b/internal/sheets/handlers_list_chrome_test.go| new file mode 100644 |
| index 0000000..c779f18 |
| --- /dev/null |
| +++ b/internal/sheets/handlers_list_chrome_test.go |
| @@ -0,0 +1,130 @@ |
| +package sheets |
| + |
| +import ( |
| + "bytes" |
| + "mime/multipart" |
| + "net/http" |
| + "net/http/httptest" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// The sheet list is a list of sheets. |
| +// |
| +// It used to open with two stacked forms — a title field with a Create |
| +// button, then a dashed drop zone with a file input and four lines of |
| +// limits — so the sheets themselves started below both. These tests pin |
| +// the removal, because the easiest way to undo it is for somebody to |
| +// add "just one small form" back to the top of the page. |
| + |
| +// signedInListBody renders the list for a fresh signed-in member. |
| +// preview_test.go's listBody takes an app and cookies because the |
| +// thumbnail tests need sheets in it; these tests want the chrome around |
| +// an empty one, so this wraps the sign-in rather than duplicating it. |
| +func signedInListBody(t *testing.T) string { |
| + t.Helper() |
| + app, cookies := signIn(t) |
| + return listBody(t, app, cookies) |
| +} |
| + |
| +func TestSheetListCarriesNoInlineForms(t *testing.T) { |
| + body := signedInListBody(t) |
| + |
| + // The title field and the file input are the two controls that were |
| + // on this page and must not be, named individually so a failure |
| + // says which one came back. |
| + for _, gone := range []string{ |
| + `name="title"`, |
| + `type="file"`, |
| + `enctype="multipart/form-data"`, |
| + "data-drop", |
| + } { |
| + if strings.Contains(body, gone) { |
| + t.Errorf("the sheet list is carrying %s again — that belongs in the top bar or on /sheets/import", gone) |
| + } |
| + } |
| +} |
| + |
| +// Create is a real form posting to /sheets, in the top bar. A link |
| +// would be wrong twice over: it makes a sheet, so it must not be a GET, |
| +// and it has to work with scripting off. |
| +func TestCreateLivesInTheTopBar(t *testing.T) { |
| + body := signedInListBody(t) |
| + |
| + bar := body[:strings.Index(body, `<main`)] |
| + if !strings.Contains(bar, `action="/sheets"`) || !strings.Contains(bar, `type="submit"`) { |
| + t.Error("the top bar has no Create form") |
| + } |
| + if !strings.Contains(bar, `href="/sheets/import"`) { |
| + t.Error("the top bar has no Import link") |
| + } |
| + // Beside the account menu, not adrift somewhere else in the shell. |
| + if strings.Index(bar, `action="/sheets"`) > strings.Index(bar, `action="/signout"`) { |
| + t.Error("Create renders after the account menu, not beside it") |
| + } |
| +} |
| + |
| +// The empty state is the one screen with nothing to do but say what to |
| +// do next, and it used to say "make one above" — which pointed at the |
| +// form this change removed. |
| +func TestEmptyStateDoesNotPointAtAFormThatIsGone(t *testing.T) { |
| + body := signedInListBody(t) |
| + if !strings.Contains(body, "No sheets yet") { |
| + t.Fatal("no empty state on a list with no sheets") |
| + } |
| + if strings.Contains(body, "above and it opens") || strings.Contains(body, "Make one above") { |
| + t.Error("the empty state still points at the removed form") |
| + } |
| + if !strings.Contains(body, `href="/sheets/import"`) { |
| + t.Error("the empty state offers no way to import, which is the one route a person with no sheets wants") |
| + } |
| +} |
| + |
| +// /sheets/import must reach the import page, not be parsed as a sheet |
| +// called "import". chi prefers a static segment over {sheetID} whatever |
| +// the registration order, so this passes today — it is pinned because a |
| +// route that only works by resolution order is one refactor from a 404 |
| +// that looks like a missing sheet. |
| +func TestImportPathIsNotASheetID(t *testing.T) { |
| + app, cookies := signIn(t) |
| + rec := do(t, app, as(httptest.NewRequest(http.MethodGet, "/sheets/import", nil), cookies)) |
| + if rec.Code != http.StatusOK { |
| + t.Fatalf("GET /sheets/import = %d, want 200", rec.Code) |
| + } |
| + if !strings.Contains(rec.Body.String(), `action="/sheets/import"`) { |
| + t.Error("/sheets/import did not render the import form") |
| + } |
| +} |
| + |
| +// A refused upload re-renders the import page, where the form is — |
| +// never the sheet list, which no longer has one to try again with. |
| +func TestRefusalRendersOnTheImportPage(t *testing.T) { |
| + app, cookies := signIn(t) |
| + |
| + // An empty multipart body: the shape a submit with nothing chosen |
| + // actually arrives in. A urlencoded post would refuse too, but with |
| + // "could not be read" rather than "choose a spreadsheet", which is |
| + // a different sentence about a different mistake. |
| + var buf bytes.Buffer |
| + w := multipart.NewWriter(&buf) |
| + if err := w.Close(); err != nil { |
| + t.Fatalf("close writer: %v", err) |
| + } |
| + req := httptest.NewRequest(http.MethodPost, "/sheets/import", &buf) |
| + req.Header.Set("Content-Type", w.FormDataContentType()) |
| + |
| + rec := do(t, app, as(req, cookies)) |
| + if rec.Code != http.StatusBadRequest { |
| + t.Fatalf("import with no file = %d, want 400", rec.Code) |
| + } |
| + body := rec.Body.String() |
| + if !strings.Contains(body, refusalNoFile) { |
| + t.Error("the refusal is not on the page") |
| + } |
| + if !strings.Contains(body, `action="/sheets/import"`) { |
| + t.Error("the refusal rendered somewhere without the form to retry from") |
| + } |
| + if strings.Contains(body, "Your sheets</h1>") { |
| + t.Error("the refusal rendered on the sheet list") |
| + } |
| +} |
diff --git a/internal/sheets/handlers_rename.go b/internal/sheets/handlers_rename.go| new file mode 100644 |
| index 0000000..38f19dc |
| --- /dev/null |
| +++ b/internal/sheets/handlers_rename.go |
| @@ -0,0 +1,112 @@ |
| +package sheets |
| + |
| +import ( |
| + "errors" |
| + "net/http" |
| + "strconv" |
| + "strings" |
| + |
| + "amadan.net/oficina/sheets/internal/sheets/store" |
| +) |
| + |
| +// Rename exists because Create stopped asking for a name. |
| +// |
| +// The sheet list used to carry a form with a title field, so a sheet |
| +// was named once, at the moment it was made, and never again — there |
| +// was no rename route and layout.html said so in a comment. That was |
| +// survivable only because the title field existed. Removing it in |
| +// favour of a one-click Create, which lands every new sheet as |
| +// "Untitled sheet", would have made the missing route the whole |
| +// product: a list of sheets all called the same thing, distinguished |
| +// only by their preview thumbnails, with no way to fix any of them. |
| +// |
| +// So this is not a feature that arrived beside the topbar Create |
| +// button. It is the other half of it. |
| + |
| +// errBlankTitle is returned rather than silently keeping the old title. |
| +// A person who cleared the field and submitted meant something by it, |
| +// and the honest answer is that a sheet needs a name — the same rule |
| +// createSheet applies. |
| +var errBlankTitle = errors.New("sheets: a sheet needs a title") |
| + |
| +// handleSheetRename renames a sheet from the title in the top bar. |
| +// |
| +// Editors, not just owners: renaming is an edit to the document, of a |
| +// piece with editing a cell, and mayWrite is what says who may make |
| +// one. A viewer or commenter gets the 404 that AGENTS.md requires for |
| +// a row that is not theirs to change — never a 403, which would |
| +// confirm the sheet exists to somebody who may not see it. |
| +func (a *App) handleSheetRename(w http.ResponseWriter, r *http.Request) { |
| + s, ok := a.sheetFromURL(w, r) |
| + if !ok { |
| + return |
| + } |
| + viewer, ok := a.actor(r) |
| + if !ok { |
| + a.notFound(w, r) |
| + return |
| + } |
| + if !a.mayWrite(s.ID, viewer) { |
| + a.notFound(w, r) |
| + return |
| + } |
| + |
| + // Read field by field into a local, never bound onto a GORM model |
| + // (AGENTS.md / Mechanical rules): a form that could reach the model |
| + // could set CreatedBy, or DeletedAt, or an id. |
| + if err := a.renameSheet(s.ID, r.FormValue("title")); err != nil { |
| + if errors.Is(err, errBlankTitle) { |
| + // Straight back to the sheet, which still shows the name it |
| + // had. Nothing was changed and there is nothing to explain |
| + // that the unchanged title does not already say. |
| + a.sheetRedirect(w, r, s.ID) |
| + return |
| + } |
| + a.fail(w, r, err) |
| + return |
| + } |
| + // 303 rather than a render: a POST that renders is a POST that |
| + // resubmits on reload. |
| + a.sheetRedirect(w, r, s.ID) |
| +} |
| + |
| +// sheetRedirect sends the browser back to a sheet. |
| +func (a *App) sheetRedirect(w http.ResponseWriter, r *http.Request, id uint64) { |
| + http.Redirect(w, r, "/sheets/"+strconv.FormatUint(id, 10), http.StatusSeeOther) |
| +} |
| + |
| +// renameSheet writes a new sealed title. |
| +// |
| +// The title is sealed under the same "sheet.title" purpose createSheet |
| +// uses. A rename that wrote plaintext would put a sheet's name in the |
| +// database file, and the one rule does not have an exception for the |
| +// second time a name is set. |
| +// |
| +// Update names its column explicitly rather than saving a struct, so |
| +// this statement cannot touch a field nobody asked it to. GORM bumps |
| +// UpdatedAt as part of it, which is what makes a renamed sheet rise to |
| +// the top of the list — correct, since renaming it is changing it. |
| +func (a *App) renameSheet(sheetID uint64, title string) error { |
| + if a.db == nil { |
| + return errNoDB |
| + } |
| + if a.sealer == nil { |
| + return errNoSealer |
| + } |
| + clean := strings.TrimSpace(title) |
| + if clean == "" { |
| + return errBlankTitle |
| + } |
| + // Truncated to the same limit createSheet applies, by runes rather |
| + // than bytes so a multi-byte title is cut where a character ends. |
| + if r := []rune(clean); len(r) > maxTitleRunes { |
| + clean = string(r[:maxTitleRunes]) |
| + } |
| + sealed, err := a.sealer.Seal("sheet.title", []byte(clean)) |
| + if err != nil { |
| + return err |
| + } |
| + return a.db.Model(&store.Sheet{}). |
| + Where("id = ?", sheetID). |
| + Update("title_sealed", sealed).Error |
| +} |
diff --git a/internal/sheets/handlers_rename_test.go b/internal/sheets/handlers_rename_test.go| new file mode 100644 |
| index 0000000..90a3e6f |
| --- /dev/null |
| +++ b/internal/sheets/handlers_rename_test.go |
| @@ -0,0 +1,163 @@ |
| +package sheets |
| + |
| +import ( |
| + "bytes" |
| + "net/http" |
| + "net/url" |
| + "strconv" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// Rename is the other half of a Create button that stopped asking for a |
| +// name. These tests are written against that pairing rather than |
| +// against the route on its own: the failure they exist to catch is a |
| +// list of sheets all called "Untitled sheet" with no way to fix any of |
| +// them. |
| + |
| +func renamePath(id uint64) string { |
| + return "/sheets/" + strconv.FormatUint(id, 10) + "/rename" |
| +} |
| + |
| +func TestRenameChangesTheTitle(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Untitled sheet") |
| + |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {"Q3 budget"}}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + if loc := rec.Header().Get("Location"); loc != "/sheets/"+strconv.FormatUint(s.ID, 10) { |
| + t.Errorf("rename redirected to %q, want the sheet", loc) |
| + } |
| + |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Q3 budget" { |
| + t.Errorf("title is %q after rename, want %q", got.Title, "Q3 budget") |
| + } |
| +} |
| + |
| +// The one rule reaches the second time a name is set, not just the |
| +// first. A rename that wrote the new title straight into the column |
| +// would put a sheet's name in the database file in plaintext, and |
| +// nothing about createSheet's sealing would have caught it. |
| +func TestRenamedTitleIsSealedAtRest(t *testing.T) { |
| + const secret = "Redundancies Q4" |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Untitled sheet") |
| + |
| + if rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {secret}}), cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d", rec.Code) |
| + } |
| + |
| + var raw []byte |
| + if err := app.db.Raw("SELECT title_sealed FROM sheets WHERE id = ?", s.ID). |
| + Row().Scan(&raw); err != nil { |
| + t.Fatalf("read back: %v", err) |
| + } |
| + if len(raw) == 0 { |
| + t.Fatal("no sealed title was stored") |
| + } |
| + if bytes.Contains(raw, []byte(secret)) { |
| + t.Fatal("the renamed title reached the database in plaintext") |
| + } |
| +} |
| + |
| +// A blank title keeps the name the sheet had. The alternative is a |
| +// sheet called nothing at all, which the list would render as a row you |
| +// cannot click by name. |
| +func TestRenameRefusesABlankTitle(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + for _, blank := range []string{"", " "} { |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {blank}}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename to %q = %d, want 303", blank, rec.Code) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("a blank rename changed the title to %q", got.Title) |
| + } |
| + } |
| +} |
| + |
| +// Truncation matches createSheet's, and by runes: cutting a multi-byte |
| +// title at a byte boundary stores half a character. |
| +func TestRenameTruncatesToTheSameCapAsCreate(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + long := strings.Repeat("é", maxTitleRunes+50) |
| + if rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {long}}), cookies)); rec.Code != http.StatusSeeOther { |
| + t.Fatalf("rename = %d", rec.Code) |
| + } |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if n := len([]rune(got.Title)); n != maxTitleRunes { |
| + t.Errorf("title is %d runes after rename, want %d", n, maxTitleRunes) |
| + } |
| +} |
| + |
| +// A viewer may not rename, and is told 404 rather than 403 — AGENTS.md |
| +// requires it, because a 403 confirms the sheet exists to somebody who |
| +// was never shown it. |
| +func TestRenameIsNotForViewers(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + guestOn(t, app, s.ID, "ada@elsewhere.example", SheetViewer) |
| + guest := signInAs(t, app, "ada@elsewhere.example") |
| + |
| + rec := do(t, app, as(postForm(renamePath(s.ID), |
| + url.Values{"title": {"Mine now"}}), guest)) |
| + if rec.Code != http.StatusNotFound { |
| + t.Errorf("a viewer renaming = %d, want 404", rec.Code) |
| + } |
| + |
| + got, err := app.sheetSummary(s.ID, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Budget" { |
| + t.Errorf("a viewer renamed the sheet to %q", got.Title) |
| + } |
| +} |
| + |
| +// TestCreateFromTheTopBarLandsAnUntitledSheet drives the button the |
| +// list page now carries: an empty POST, no title field anywhere in it. |
| +// |
| +// This is the whole reason rename exists, so it is pinned here rather |
| +// than left to the create handler's own tests: if an empty create ever |
| +// starts refusing instead of defaulting, the top bar's only action |
| +// breaks and the failure should name the button. |
| +func TestCreateFromTheTopBarLandsAnUntitledSheet(t *testing.T) { |
| + app, cookies := signIn(t) |
| + |
| + rec := do(t, app, as(postForm("/sheets", url.Values{}), cookies)) |
| + if rec.Code != http.StatusSeeOther { |
| + t.Fatalf("create with no title = %d, want 303: %s", rec.Code, rec.Body.String()) |
| + } |
| + id, err := strconv.ParseUint(strings.TrimPrefix(rec.Header().Get("Location"), "/sheets/"), 10, 64) |
| + if err != nil { |
| + t.Fatalf("Location %q is not a sheet URL: %v", rec.Header().Get("Location"), err) |
| + } |
| + got, err := app.sheetSummary(id, signedInMember(t, app)) |
| + if err != nil { |
| + t.Fatalf("sheetSummary: %v", err) |
| + } |
| + if got.Title != "Untitled sheet" { |
| + t.Errorf("a titleless create landed as %q, want %q", got.Title, "Untitled sheet") |
| + } |
| +} |
diff --git a/internal/sheets/handlers_sheets.go b/internal/sheets/handlers_sheets.go| index 88f122e..c4f6b3c 100644 |
| --- a/internal/sheets/handlers_sheets.go |
| +++ b/internal/sheets/handlers_sheets.go |
| @@ -19,15 +19,14 @@ type sheetRow struct { |
| } |
| // sheetListView is the sheet list's payload. |
| +// |
| +// It carried an ImportError until the import form moved off this page |
| +// to /sheets/import. The reasoning for keeping a refusal on the same |
| +// page as its form was right and still is — it needs no flash, no |
| +// cookie and no redirect — which is why the field moved with the form |
| +// rather than being dropped: it is importPageView.Error now. |
| type sheetListView struct { |
| Sheets []sheetRow |
| - // ImportError is a refused upload's explanation, empty on an |
| - // ordinary render. It lives on the list rather than in a flash |
| - // because the list is where the import form is: a refusal shown |
| - // beside the form that caused it needs no session storage, no |
| - // redirect and no cookie, and it works with scripting off by |
| - // construction. |
| - ImportError string |
| } |
| // sheetPageView is the grid page's payload. |
| @@ -79,18 +78,18 @@ type sheetPageView struct { |
| // handleSheetList renders the sheets the viewer may see. |
| func (a *App) handleSheetList(w http.ResponseWriter, r *http.Request) { |
| - a.renderSheetList(w, r, http.StatusOK, "") |
| + a.renderSheetList(w, r, http.StatusOK) |
| } |
| -// renderSheetList draws the list, optionally carrying a refused |
| -// upload's explanation and the status that goes with it. |
| +// renderSheetList draws the list. |
| // |
| -// A refused import re-renders this page rather than redirecting to it, |
| -// so the refusal survives without a flash: the person lands back on the |
| -// list they posted from, with the reason beside the form. The status is |
| -// the caller's because only the caller knows whether this is an |
| -// ordinary 200 or a refusal. |
| -func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int, importErr string) { |
| +// It used to take a refused upload's explanation and a status to go |
| +// with it, because the import form lived on this page and a refusal |
| +// belongs beside the form that caused it. The form moved to |
| +// /sheets/import and the refusal went with it — renderImportPage keeps |
| +// both the parameter and the reasoning. What is left here has one |
| +// caller and one status, so it no longer takes either. |
| +func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int) { |
| viewer, ok := a.actor(r) |
| if !ok { |
| http.Error(w, "Sign in first.", http.StatusForbidden) |
| @@ -106,7 +105,7 @@ func (a *App) renderSheetList(w http.ResponseWriter, r *http.Request, status int |
| rows = append(rows, sheetRow{SheetSummary: s, Preview: a.previewFor(s)}) |
| } |
| a.render(w, r, "sheets", "Your sheets — Oficina Sheets", status, |
| - sheetListView{Sheets: rows, ImportError: importErr}) |
| + sheetListView{Sheets: rows}) |
| } |
| // handleSheetCreate makes a sheet and sends you to it. |
diff --git a/internal/sheets/handlers_table_test.go b/internal/sheets/handlers_table_test.go| index c6d4dad..a9a28dc 100644 |
| --- a/internal/sheets/handlers_table_test.go |
| +++ b/internal/sheets/handlers_table_test.go |
| @@ -129,8 +129,19 @@ func TestEveryPageWorksWithoutScript(t *testing.T) { |
| // Sign-out is checked on /sheets rather than on "/": it is |
| // layout chrome, present on every signed-in page, and "/" |
| // stopped being one when it started redirecting onward. |
| - {"/sheets", []string{`action="/sheets"`, `name="title"`, `action="/signout"`}}, |
| - {fmt.Sprintf("/sheets/%d", s.ID), []string{fmt.Sprintf("/sheets/%d/table", s.ID)}}, |
| + // |
| + // The list no longer carries name="title": Create is a button |
| + // in the top bar that posts an empty form and lands the sheet |
| + // as "Untitled sheet". The naming moved to the rename field on |
| + // the sheet page, two rows down, which is where the title is |
| + // now asserted. |
| + {"/sheets", []string{`action="/sheets"`, `type="submit"`, `action="/signout"`}}, |
| + {"/sheets/import", []string{`action="/sheets/import"`, `type="file"`, `type="submit"`}}, |
| + {fmt.Sprintf("/sheets/%d", s.ID), []string{ |
| + fmt.Sprintf("/sheets/%d/table", s.ID), |
| + // Rename: the only place a sheet is named now, so it is |
| + // the one that has to survive with scripting off. |
| + fmt.Sprintf(`action="/sheets/%d/rename"`, s.ID), `name="title"`}}, |
| {fmt.Sprintf("/sheets/%d/table", s.ID), []string{"?page=1&edit=0,0", "Next rows"}}, |
| {fmt.Sprintf("/sheets/%d/table?page=1&edit=0,0", s.ID), |
| []string{`method="post"`, `name="raw"`, "Save"}}, |
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css| index 81a8c4c..702ef5d 100644 |
| --- a/internal/sheets/static/sheets.css |
| +++ b/internal/sheets/static/sheets.css |
| @@ -67,16 +67,76 @@ |
| /* The document title, in the top bar beside the brand. It truncates |
| * rather than wrapping: the bar is a fixed height, and a two-line title |
| - * would push the account menu out of it. */ |
| + * would push the account menu out of it. |
| + * |
| + * The clipping lives on the WRAPPER, not on the text or the input. It |
| + * used to sit directly on [data-doc-title] back when that was a span, |
| + * and left there it would clip a rename input's own caret: type past |
| + * 22rem and the cursor walks out of a box with overflow: hidden, so you |
| + * are editing text you cannot see. */ |
| [data-doc-title] { |
| color: var(--rst-text); |
| font-weight: 600; |
| max-inline-size: 22rem; |
| +} |
| + |
| +[data-doc-title-text] { |
| + display: block; |
| overflow: hidden; |
| text-overflow: ellipsis; |
| white-space: nowrap; |
| } |
| +/* The rename form. It reads as a title until somebody means to edit it: |
| + * no border, no field background, the same weight and colour as the |
| + * static version above. This is the one control on the page that has to |
| + * NOT look like a control at rest — a text input sitting permanently in |
| + * the top bar of a spreadsheet competes with the grid for "the thing |
| + * you type in", and the grid must win by default. */ |
| +form[data-doc-title] { |
| + display: flex; |
| + align-items: center; |
| + gap: var(--rst-sp-1); |
| +} |
| + |
| +[data-doc-title-input] { |
| + min-inline-size: 0; |
| + padding: var(--rst-sp-1); |
| + border: 1px solid transparent; |
| + border-radius: var(--rst-radius); |
| + background: none; |
| + color: inherit; |
| + font: inherit; |
| + text-overflow: ellipsis; |
| +} |
| + |
| +[data-doc-title-input]:hover { |
| + border-color: var(--rst-line); |
| +} |
| + |
| +/* Focus turns it into an obvious field. :focus-visible is not enough |
| + * here — a click into the box must show the same affordance a Tab into |
| + * it does, because at rest the two are indistinguishable. */ |
| +[data-doc-title-input]:focus { |
| + border-color: var(--rst-accent); |
| + background: var(--rst-surface); |
| + outline: none; |
| + text-overflow: clip; |
| +} |
| + |
| +/* The submit is hidden until the field is engaged, and hidden with |
| + * clip rather than display: none so it stays focusable — Tab from the |
| + * input must reach it. Scripting off, this button is the only way to |
| + * commit a rename that was not submitted with Enter. */ |
| +form[data-doc-title]:not(:focus-within) button { |
| + position: absolute; |
| + inline-size: 1px; |
| + block-size: 1px; |
| + overflow: hidden; |
| + clip-path: inset(50%); |
| + white-space: nowrap; |
| +} |
| + |
| [data-menubar] { |
| display: flex; |
| align-items: center; |
diff --git a/internal/sheets/templates/import.html b/internal/sheets/templates/import.html| new file mode 100644 |
| index 0000000..c3ec8da |
| --- /dev/null |
| +++ b/internal/sheets/templates/import.html |
| @@ -0,0 +1,53 @@ |
| +{{define "head"}} |
| +{{/* Drag and drop only. Everything the drop zone does, the form below |
| + it already does with scripting off — drop.js fills that same file |
| + input and submits that same form, so there is one route, one CSRF |
| + check and one code path either way. |
| + |
| + This moved here with the form it decorates. It used to sit on the |
| + sheet list, which is where the import form used to be. */}} |
| +<script defer src="{{asset "static/drop.js"}}"></script> |
| +{{end}} |
| + |
| +{{define "content"}} |
| +<header rst-page-header> |
| +<div rst-page-header-titles> |
| +<h1>Import a spreadsheet</h1> |
| +{{/* The limits are stated BEFORE the import, not discovered after it. |
| + Import reads one worksheet (xlsx.Import), and a workbook with |
| + three of them would otherwise arrive looking whole — the quiet |
| + kind of wrong PRODUCT.md's first principle forbids. |
| + |
| + They are a subhead here rather than a hint under the field |
| + because this page has the room the dropdown they nearly went in |
| + did not, and because drop.js overwrites the hint to speak about |
| + a drop — which would take the file limits down with it. */}} |
| +<p rst-page-header-sub>Excel .xlsx or .xlsm, up to 20 MB. The first |
| +worksheet is imported.</p> |
| +</div> |
| +</header> |
| + |
| +{{if .Data.Error}} |
| +{{/* A refusal renders HERE, on the page holding the form, rather than |
| + back on the sheet list. The list is not where the person is and |
| + no longer carries a form to try again with. */}} |
| +<div rst-callout rst-tone="negative"> |
| +<div rst-callout-body> |
| +<p>{{.Data.Error}}</p> |
| +</div> |
| +</div> |
| +{{end}} |
| + |
| +<form rst-form method="post" action="/sheets/import" enctype="multipart/form-data"> |
| +<div data-drop> |
| +<div rst-field-row> |
| +<div rst-field> |
| +<label rst-field-label for="file">Choose a file</label> |
| +<input rst-input id="file" name="file" type="file" accept=".xlsx,.xlsm"> |
| +<p rst-field-hint data-drop-hint>You can also drag a file onto this box.</p> |
| +</div> |
| +<button rst-btn="primary" type="submit">Import</button> |
| +</div> |
| +</div> |
| +</form> |
| +{{end}} |
diff --git a/internal/sheets/templates/layout.html b/internal/sheets/templates/layout.html| index 6920a26..77ed0ee 100644 |
| --- a/internal/sheets/templates/layout.html |
| +++ b/internal/sheets/templates/layout.html |
| @@ -78,6 +78,18 @@ |
| <nav rst-shell-nav> |
| {{if .SignedIn}}<a href="/sheets">Your sheets</a>{{end}} |
| </nav> |
| +{{/* A page's own top-bar actions, beside the account menu. |
| + |
| + Deliberately a block rather than a field on PageData, and |
| + deliberately empty by default: the sheet list fills it with Create |
| + and Import, and the grid page fills it with nothing, because the |
| + grid already carries a File menu holding both and a second Create |
| + in the bar above it would be the same verb twice on one screen. |
| + |
| + Inside the tail, so it collapses behind the narrow-viewport Menu |
| + disclosure along with the nav and the account rather than being |
| + the one control still floating in the bar at 360px. */}} |
| +{{block "topbaractions" .}}{{end}} |
| {{if .SignedIn}} |
| <details rst-dropdown rst-shell-account name="rst-menus"> |
| <summary>{{.Address}}<span rst-caret aria-hidden="true">{{icon "chevron-down"}}</span></summary> |
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html| index e29d0d0..88091f4 100644 |
| --- a/internal/sheets/templates/sheet.html |
| +++ b/internal/sheets/templates/sheet.html |
| @@ -23,7 +23,37 @@ |
| {{end}} |
| {{define "doctitle"}} |
| -<span data-doc-title title="{{.Data.Sheet.Title}}">{{.Data.Sheet.Title}}</span> |
| +{{/* The open document's title, and — for anyone who may edit it — the |
| + rename. |
| + |
| + This was static text, under a comment saying a rename route did |
| + not exist and that anything looking editable would be a promise |
| + the app could not keep. The route exists now, and it had to: the |
| + sheet list's Create button stopped asking for a title, so every |
| + sheet is born "Untitled sheet" and this is the only place the |
| + name is ever fixed. See handlers_rename.go. |
| + |
| + An inline form, not a popover. A <details> here would join |
| + rastrillo's "rst-menus" exclusive group along with the File menu |
| + and the account dropdown — opening one would close the others, |
| + and a rename that shuts the menu you opened it from is the |
| + nested-accordion trap TestNoNestedExclusiveDetails exists to |
| + catch. An input has no such problem. |
| + |
| + Scriptless by construction: it is a real form with a real submit. |
| + The button is visually hidden until the input has focus, so the |
| + bar stays a title bar until somebody means to edit it, and Enter |
| + submits from the input either way. */}} |
| +{{if .Data.MayWrite}} |
| +<form data-doc-title method="post" action="/sheets/{{.Data.Sheet.ID}}/rename"> |
| +<input data-doc-title-input name="title" value="{{.Data.Sheet.Title}}" |
| + aria-label="Sheet title" autocomplete="off" spellcheck="false"> |
| +<button rst-btn="ghost" type="submit">Rename</button> |
| +</form> |
| +{{else}} |
| +<span data-doc-title><span data-doc-title-text |
| + title="{{.Data.Sheet.Title}}">{{.Data.Sheet.Title}}</span></span> |
| +{{end}} |
| {{end}} |
| {{define "main"}} |
| @@ -92,7 +122,11 @@ |
| <summary>File</summary> |
| <div rst-dropdown-menu> |
| <form method="post" action="/sheets"><button type="submit">New sheet</button></form> |
| -<a href="/sheets">Import a spreadsheet…</a> |
| +{{/* A page, not a file input in this dropdown. A hidden input behind a |
| + menu-shaped label never submits itself, so the menu item would |
| + have needed script to import anything at all — see |
| + handleImportPage. */}} |
| +<a href="/sheets/import">Import a spreadsheet…</a> |
| <a href="/sheets/{{.Data.Sheet.ID}}/export.xlsx">Download as .xlsx</a> |
| <hr> |
| <a href="/sheets/{{.Data.Sheet.ID}}/table">Open as table</a> |
diff --git a/internal/sheets/templates/sheets.html b/internal/sheets/templates/sheets.html| index 2dbef11..ec7bc1e 100644 |
| --- a/internal/sheets/templates/sheets.html |
| +++ b/internal/sheets/templates/sheets.html |
| @@ -1,9 +1,31 @@ |
| -{{define "head"}} |
| -{{/* Drag and drop only. Everything the drop zone does, the form below |
| - it already does with scripting off — drop.js fills that same file |
| - input and submits that same form, so there is one route, one CSRF |
| - check and one code path either way. */}} |
| -<script defer src="{{asset "static/drop.js"}}"></script> |
| +{{/* The verbs live in the top bar; the page is the list. |
| + |
| + This screen used to open with two stacked forms — a title field |
| + with a Create button, then a dashed drop zone with a file input and |
| + four lines of limits — and the sheets themselves began below the |
| + fold of both. A list of sheets should be a list of sheets. |
| + |
| + Create is a button beside the account menu now, and makes a sheet |
| + called "Untitled sheet" in one click; the name is fixed afterwards |
| + by renaming it in the top bar of the sheet itself, which is what |
| + handlers_rename.go is for. Import moved to a page of its own at |
| + /sheets/import — see handleImportPage for why a page and not an |
| + item in the File menu. */}} |
| +{{define "topbaractions"}} |
| +{{if .SignedIn}} |
| +{{/* Import first, Create last: Create is the primary action and sits |
| + closest to the account menu, which is the corner the eye returns |
| + to. A link rather than a form, because it opens a page rather |
| + than changing anything. */}} |
| +<a rst-btn href="/sheets/import">Import</a> |
| +{{/* No title field, and so no dialog and no second step: the button |
| + IS the create. handleSheetCreate defaults an empty title to |
| + "Untitled sheet" and 303s into the new sheet, so a click lands |
| + you in a grid you can type into. */}} |
| +<form method="post" action="/sheets"> |
| +<button rst-btn="primary" type="submit">Create</button> |
| +</form> |
| +{{end}} |
| {{end}} |
| {{define "content"}} |
| @@ -14,51 +36,6 @@ |
| </div> |
| </header> |
| -{{if .Data.ImportError}} |
| -<div rst-callout rst-tone="negative"> |
| -<div rst-callout-body> |
| -<p>{{.Data.ImportError}}</p> |
| -</div> |
| -</div> |
| -{{end}} |
| - |
| -<form rst-form method="post" action="/sheets"> |
| -<div rst-field-row> |
| -<div rst-field> |
| -<label rst-field-label for="title">New sheet</label> |
| -<input rst-input id="title" name="title" type="text" placeholder="Untitled sheet" autocomplete="off"> |
| -</div> |
| -<button rst-btn="primary" type="submit">Create</button> |
| -</div> |
| -</form> |
| - |
| -{{/* Import. The file input and its button are the whole feature — they |
| - work with scripting off, and dropping a file onto the zone below |
| - does nothing this form does not already do. |
| - |
| - data-drop is drop.js's hook and is deliberately a data- attribute |
| - rather than an rst- one: the rst- namespace is rastrillo's, and an |
| - invented rst- attribute is a name tokens.css does not define, which |
| - is the mistake this app has already made once in CSS. */}} |
| -<form rst-form method="post" action="/sheets/import" enctype="multipart/form-data"> |
| -<div data-drop> |
| -<div rst-field-row> |
| -<div rst-field> |
| -<label rst-field-label for="file">Import a spreadsheet</label> |
| -<input rst-input id="file" name="file" type="file" accept=".xlsx,.xlsm"> |
| -{{/* The first-worksheet limit is stated BEFORE the import, not |
| - discovered after it. Import reads one worksheet (xlsx.Import), and |
| - a workbook with three of them would otherwise arrive looking whole |
| - — which is the quiet kind of wrong PRODUCT.md's first principle |
| - forbids. */}} |
| -<p rst-field-hint data-drop-hint>Excel .xlsx or .xlsm, up to 20 MB. The |
| -first worksheet is imported. You can also drag a file onto this box.</p> |
| -</div> |
| -<button rst-btn type="submit">Import</button> |
| -</div> |
| -</div> |
| -</form> |
| - |
| {{if .Data.Sheets}} |
| <ul rst-list data-sheet-list> |
| {{range .Data.Sheets}} |
| @@ -80,11 +57,17 @@ first worksheet is imported. You can also drag a file onto this box.</p> |
| {{end}} |
| </ul> |
| {{else}} |
| +{{/* The empty state points at the two controls in the top bar. It |
| + used to say "make one above", which was true when the form was |
| + directly above it and would now be a sentence pointing at a |
| + header. An empty screen is the one place in a product that has |
| + nothing to do but explain what to do next. */}} |
| <div rst-empty> |
| <p rst-empty-title>No sheets yet</p> |
| -<p rst-empty-body>Make one above and it opens with a blank grid you can type |
| -into straight away, or import an Excel file to start from what you already |
| -have.</p> |
| +<p rst-empty-body>Create one from the top bar and it opens with a blank |
| +grid you can type into straight away, or |
| +<a href="/sheets/import">import an Excel file</a> to start from what you |
| +already have.</p> |
| </div> |
| {{end}} |
| {{end}} |