Commit cc3550cfbb
Verified · cmc
Layout: unified · split
docs/plans/2026-09-04-email-login.md +14 −19
| @@ -169,24 +169,22 @@ Expected: PASS — the new migration must not break existing store tests. | |||
| 169 | 169 | ||
| 170 | - [ ] **Step 6: Confirm the index is actually used** | 170 | - [ ] **Step 6: Confirm the index is actually used** |
| 171 | 171 | ||
| 172 | Run: | 172 | The count runs on every anonymous request, so a sequential scan here would |
| 173 | ```bash | 173 | make the throttle its own denial-of-service vector. Verify the plan names the |
| 174 | cd /Users/cmc/git/krz/gitbay && cat > /tmp/plan_explain_test.go <<'EOF' | 174 | index: |
| 175 | EOF | ||
| 176 | go test ./internal/store/ -run TestDashboardQueriesUseIndexes -v | ||
| 177 | ``` | ||
| 178 | Expected: PASS (unrelated, but proves the migration did not disturb existing | ||
| 179 | plans). Then verify by hand that the count uses the index: | ||
| 180 | 175 | ||
| 181 | ```bash | 176 | ```bash |
| 182 | sqlite3 "$(mktemp -d)/x.db" <<'EOF' | 177 | sqlite3 "$SCRATCH/plan.db" <<'EOF' |
| 183 | CREATE TABLE login_tokens (token_hash TEXT PRIMARY KEY, user_id INTEGER NOT NULL, | 178 | CREATE TABLE login_tokens (token_hash TEXT PRIMARY KEY, user_id INTEGER NOT NULL, |
| 184 | created_at TEXT NOT NULL, expires_at TEXT NOT NULL, used_at TEXT); | 179 | created_at TEXT NOT NULL, expires_at TEXT NOT NULL, used_at TEXT); |
| 185 | CREATE INDEX login_tokens_user_created ON login_tokens(user_id, created_at); | 180 | CREATE INDEX login_tokens_user_created ON login_tokens(user_id, created_at); |
| 186 | EXPLAIN QUERY PLAN SELECT count(*) FROM login_tokens WHERE user_id = 1 AND created_at > 'x'; | 181 | EXPLAIN QUERY PLAN SELECT count(*) FROM login_tokens WHERE user_id = 1 AND created_at > 'x'; |
| 187 | EOF | 182 | EOF |
| 188 | ``` | 183 | ``` |
| 189 | Expected: the plan names `login_tokens_user_created`, not `SCAN login_tokens`. | 184 | |
| 185 | Expected: the output names `login_tokens_user_created`. A line reading | ||
| 186 | `SCAN login_tokens` means the index is not being used and the migration is | ||
| 187 | wrong. | ||
| 190 | 188 | ||
| 191 | - [ ] **Step 7: Commit** | 189 | - [ ] **Step 7: Commit** |
| 192 | 190 | ||
| @@ -317,7 +315,7 @@ together because none of them is testable without the others. | |||
| 317 | - Create: `e2e/emaillogin_test.go` | 315 | - Create: `e2e/emaillogin_test.go` |
| 318 | 316 | ||
| 319 | **Interfaces:** | 317 | **Interfaces:** |
| 320 | - Consumes: `store.UserIDByVerifiedEmail(address string) (int64, bool)` (`internal/store/activity.go:11`); `store.PrimaryVerifiedEmail(userID int64) (string, error)` (`internal/store/mrs.go:440`); `store.UserByName`; `store.CountLoginTokensSince` (Task 1); `store.NewToken`; `store.CreateLoginToken`; `mail.Send(cfg config.Config, to, subject, body string) error`; `Server.apiLimit.allow(key string, write bool) (bool, time.Duration)`; `Server.clientIP(r)`. | 318 | - Consumes: `store.UserIDByVerifiedEmail(address string) (int64, bool)` (`internal/store/activity.go:11`); `store.PrimaryVerifiedEmail(userID int64) (string, error)` (`internal/store/mrs.go:440`); `store.UserByUsername`; `store.CountLoginTokensSince` (Task 1); `store.NewToken`; `store.CreateLoginToken`; `mail.Send(cfg config.Config, to, subject, body string) error`; `Server.apiLimit.allow(key string, write bool) (bool, time.Duration)`; `Server.clientIP(r)`. |
| 321 | - Produces: `func control.RequestLoginLink(cfg config.Config, st *store.Store, identifier string) error` | 319 | - Produces: `func control.RequestLoginLink(cfg config.Config, st *store.Store, identifier string) error` |
| 322 | 320 | ||
| 323 | - [ ] **Step 1: Write the failing e2e test** | 321 | - [ ] **Step 1: Write the failing e2e test** |
| @@ -511,7 +509,7 @@ func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) err | |||
| 511 | } | 509 | } |
| 512 | userID, address = id, identifier | 510 | userID, address = id, identifier |
| 513 | } else { | 511 | } else { |
| 514 | u, err := st.UserByName(identifier) | 512 | u, err := st.UserByUsername(identifier) |
| 515 | if err != nil { | 513 | if err != nil { |
| 516 | return nil | 514 | return nil |
| 517 | } | 515 | } |
| @@ -547,9 +545,7 @@ func RequestLoginLink(cfg config.Config, st *store.Store, identifier string) err | |||
| 547 | } | 545 | } |
| 548 | ``` | 546 | ``` |
| 549 | 547 | ||
| 550 | Check `st.UserByName`'s real name and signature before writing this — if the | 548 | `UserByUsername(name string) (User, error)` is at `internal/store/users.go:109`. |
| 551 | store spells it differently, use the store's spelling rather than adding a | ||
| 552 | wrapper. Run: `grep -n "func (s \*Store) UserByName" internal/store/users.go` | ||
| 553 | 549 | ||
| 554 | - [ ] **Step 4: Write the handler** | 550 | - [ ] **Step 4: Write the handler** |
| 555 | 551 | ||
| @@ -781,7 +777,6 @@ triple to a single `error`, recorded above under "Change from the spec". | |||
| 781 | Each is used with that signature everywhere it appears. `sessionSameSite` is | 777 | Each is used with that signature everywhere it appears. `sessionSameSite` is |
| 782 | declared in Task 2 and used in Task 2 only. | 778 | declared in Task 2 and used in Task 2 only. |
| 783 | 779 | ||
| 784 | **Unverified at plan time.** `store.UserByName` is used in Task 3 Step 3 but | 780 | **Unverified at plan time.** `Parity.org`'s table format is read at execution |
| 785 | its exact name and signature were not confirmed; Step 3 carries an explicit | 781 | rather than guessed, which is why Task 4 Step 2 says to read neighbouring rows |
| 786 | instruction to check before writing. `Parity.org`'s table format is likewise | 782 | first rather than giving the row verbatim. |
| 787 | read at execution rather than guessed. | ||