Create clicks table and tracking (#36) #42

Merged

ldraney merged 2 commits from 36-clicks-table-tracking into main 2026-06-13 13:25:16 +00:00

Conversation 4 Commits 2 Files changed 12 +197 -3

ldraney commented 2026-06-10 02:01:17 +00:00

Owner

Copy link

Summary

Create a clicks table to record link usage and implement a redirect-through tracking pattern. Clicking a link card now POSTs to a tracking endpoint, records the click, then redirects to the actual URL.

Changes

• db/migrate/20260609195232_create_clicks.rb -- migration creating clicks table with link_id (FK), user_id (nullable, no FK constraint), session_id, clicked_at, plus indices
• app/models/click.rb -- belongs_to :link, validates at least one of user_id or session_id is present
• app/models/link.rb -- added has_many :clicks (dependent: :destroy) and #click_count method
• app/controllers/clicks_controller.rb -- POST create: finds link, records click with session_id, redirects to link.url
• config/routes.rb -- added post :click, on: :member, to: "clicks#create" nested under links
• app/views/links/_link.html.erb -- changed link title from direct link_to to button_to that POSTs through click tracking
• app/views/links/show.html.erb -- same tracking pattern, plus displays click count
• db/schema.rb -- updated with clicks table schema
• test/models/click_test.rb -- 7 tests covering all model validations
• test/controllers/clicks_controller_test.rb -- 3 tests covering recording, redirect, and 404 behavior
• test/fixtures/clicks.yml -- fixture data for tests

Test Plan

• bin/rails test -- 14 tests, 28 assertions, 0 failures, 0 errors
• bin/rails db:migrate -- migration runs cleanly on both test and development
• Click model validates at least one of user_id/session_id
• POST /links/:id/click records click and redirects to URL
• Link#click_count returns correct count
• Both index partial and show page use tracking links

Review Checklist

• Migration has no FK constraint on user_id (users table does not exist yet)
• No counter_cache column -- uses clicks.count via method
• Controller is minimal -- record and redirect only
• Ships ungated (no feature flag system exists yet)
• All existing tests continue to pass

Related Notes

None.

Related

Closes #36

## Summary Create a `clicks` table to record link usage and implement a redirect-through tracking pattern. Clicking a link card now POSTs to a tracking endpoint, records the click, then redirects to the actual URL. ## Changes - `db/migrate/20260609195232_create_clicks.rb` -- migration creating clicks table with link_id (FK), user_id (nullable, no FK constraint), session_id, clicked_at, plus indices - `app/models/click.rb` -- belongs_to :link, validates at least one of user_id or session_id is present - `app/models/link.rb` -- added has_many :clicks (dependent: :destroy) and `#click_count` method - `app/controllers/clicks_controller.rb` -- POST create: finds link, records click with session_id, redirects to link.url - `config/routes.rb` -- added `post :click, on: :member, to: "clicks#create"` nested under links - `app/views/links/_link.html.erb` -- changed link title from direct `link_to` to `button_to` that POSTs through click tracking - `app/views/links/show.html.erb` -- same tracking pattern, plus displays click count - `db/schema.rb` -- updated with clicks table schema - `test/models/click_test.rb` -- 7 tests covering all model validations - `test/controllers/clicks_controller_test.rb` -- 3 tests covering recording, redirect, and 404 behavior - `test/fixtures/clicks.yml` -- fixture data for tests ## Test Plan - [x] `bin/rails test` -- 14 tests, 28 assertions, 0 failures, 0 errors - [x] `bin/rails db:migrate` -- migration runs cleanly on both test and development - [x] Click model validates at least one of user_id/session_id - [x] POST /links/:id/click records click and redirects to URL - [x] Link#click_count returns correct count - [x] Both index partial and show page use tracking links ## Review Checklist - [x] Migration has no FK constraint on user_id (users table does not exist yet) - [x] No counter_cache column -- uses `clicks.count` via method - [x] Controller is minimal -- record and redirect only - [x] Ships ungated (no feature flag system exists yet) - [x] All existing tests continue to pass ## Related Notes None. ## Related Closes #36

ldraney added 1 commit 2026-06-10 02:01:17 +00:00

Add clicks table and redirect-through tracking (#36) ... 5eb9699749

Record link clicks via a POST endpoint that logs the click then
redirects to the target URL. Link cards and the show page now route
through this tracking endpoint instead of linking directly.

- Migration creates clicks table (link_id FK, user_id nullable with no
  FK, session_id, clicked_at) with indices
- Click model validates at least one of user_id/session_id
- ClicksController#create records click and redirects to link.url
- Link model gains has_many :clicks and #click_count
- Views use button_to for POST-based click tracking
- Full test coverage for model validations and controller behavior

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-10 02:02:07 +00:00

Author

Owner

Copy link

PR Review -- #42 Create clicks table and tracking

Migration

• create_clicks.rb -- Schema matches spec exactly: link_id with FK, user_id as nullable bigint with no FK constraint, session_id string, clicked_at NOT NULL. Indices on all lookup columns. Clean.

Model layer

• Click -- belongs_to :link, validates clicked_at presence, custom validation ensures at least one of user_id/session_id. Correct.
• Link -- has_many :clicks, dependent: :destroy and #click_count using clicks.count (no counter_cache per spec). Correct.

Controller

• Minimal: find link, record click, redirect. Uses allow_other_host: true for external URLs. Session ID fallback to SecureRandom.hex(16) when session.id is nil (handles edge cases in tests and fresh sessions). No unnecessary logic.

Routes

• post :click, on: :member, to: "clicks#create" nested under links -- generates POST /links/:id/click. Correct.

Views

• Both _link.html.erb and show.html.erb changed from link_to to button_to with POST method for redirect-through tracking. Show page displays click count. Correct.

Tests

• 7 model tests covering all validation paths (session_id only, user_id only, both, neither, missing clicked_at, missing link, association).
• 3 controller tests covering click recording + redirect, 404 for missing link, and click_count accuracy.
• All 14 tests pass (0 failures, 0 errors).

Note for future

• click_count issues a SELECT COUNT(*) per link. On the index page this is N+1. Acceptable now (spec says no counter_cache), but worth a .includes/counter_cache optimization when click volume grows.

VERDICT: APPROVE -- Implementation matches the spec in #36 across all file targets, constraints are respected, tests are comprehensive.

## PR Review -- #42 Create clicks table and tracking ### Migration - `create_clicks.rb` -- Schema matches spec exactly: `link_id` with FK, `user_id` as nullable bigint with no FK constraint, `session_id` string, `clicked_at` NOT NULL. Indices on all lookup columns. Clean. ### Model layer - `Click` -- `belongs_to :link`, validates `clicked_at` presence, custom validation ensures at least one of `user_id`/`session_id`. Correct. - `Link` -- `has_many :clicks, dependent: :destroy` and `#click_count` using `clicks.count` (no counter_cache per spec). Correct. ### Controller - Minimal: find link, record click, redirect. Uses `allow_other_host: true` for external URLs. Session ID fallback to `SecureRandom.hex(16)` when `session.id` is nil (handles edge cases in tests and fresh sessions). No unnecessary logic. ### Routes - `post :click, on: :member, to: "clicks#create"` nested under links -- generates `POST /links/:id/click`. Correct. ### Views - Both `_link.html.erb` and `show.html.erb` changed from `link_to` to `button_to` with POST method for redirect-through tracking. Show page displays click count. Correct. ### Tests - 7 model tests covering all validation paths (session_id only, user_id only, both, neither, missing clicked_at, missing link, association). - 3 controller tests covering click recording + redirect, 404 for missing link, and click_count accuracy. - All 14 tests pass (0 failures, 0 errors). ### Note for future - `click_count` issues a `SELECT COUNT(*)` per link. On the index page this is N+1. Acceptable now (spec says no counter_cache), but worth a `.includes`/counter_cache optimization when click volume grows. **VERDICT: APPROVE** -- Implementation matches the spec in #36 across all file targets, constraints are respected, tests are comprehensive.

ldraney commented 2026-06-10 02:04:14 +00:00

Author

Owner

Copy link

PR #42 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, PostgreSQL, Hotwire/Turbo, Minitest. This is a standard Rails MVC feature -- new model, controller, migration, views, and tests.

Migration (db/migrate/20260609195232_create_clicks.rb)

• Schema is sound. link_id has a FK constraint; user_id intentionally omits one (users table does not exist yet). Indices on user_id, session_id, clicked_at are appropriate for the anticipated queries.
• clicked_at is null: false at the DB level, matching the model validation. Good.
• Note: user_id is bigint but there is no users table yet. When the users table lands, a follow-up migration should add the FK constraint. This is documented in the PR body -- acceptable.

Model (app/models/click.rb)

• Custom validation must_have_attribution correctly enforces at least one identifier. Uses .blank? which covers both nil and empty string -- correct.
• belongs_to :link is non-optional by default in Rails 8.1, so the FK presence is validated automatically. Good.

Model (app/models/link.rb)

• has_many :clicks, dependent: :destroy -- correct. When a link is deleted, its clicks are cleaned up.
• click_count method uses clicks.count, which issues SELECT COUNT(*) each time. This is fine at current scale. When click volume grows, a counter_cache column would be more efficient, but the PR body explicitly acknowledges this trade-off. Acceptable.

Controller (app/controllers/clicks_controller.rb)

• Link.find(params[:id]) raises RecordNotFound on miss, which Rails maps to 404. Matches existing patterns in LinksController.
• Click.create! with bang -- if the create fails validation, this raises ActiveRecord::RecordInvalid which would 500. Since session_id is always populated by current_session_id and clicked_at is always set, the only failure path would be if the link somehow became nil between find and create!, which is a race condition not worth guarding against. Acceptable.
• redirect_to link.url, allow_other_host: true -- The allow_other_host: true is required since links are external URLs. This is NOT an open redirect vulnerability because the URL comes from a stored Link record whose url field is validated against http/https schemes at creation time. Users cannot control the redirect target through request parameters.
• current_session_id -- falls back to SecureRandom.hex(16) when session.id is nil. This means a user without a session cookie gets a random ID that is not persisted across requests. Each click from a cookieless client gets a different session_id. This is a known limitation, not a bug.

Views

• button_to generates a form with a POST, which is semantically correct for a state-changing action (recording a click). CSRF protection is handled automatically by Rails. Good.
• The link_to -> button_to change means the title is now a submit button inside a form. CSS classes link-card-track and link-card-track-form are added for styling. The styling for these classes is not included in this PR -- this may cause visual regression if the button renders with default browser button styling.

Routes

• post :click, on: :member, to: "clicks#create" nested under links produces POST /links/:id/click mapped to ClicksController#create. Clean and RESTful.

Tests

• Model tests (7 tests): cover valid with session_id only, user_id only, both, invalid without either, invalid without clicked_at, invalid without link, and association check. Good coverage.
• Controller tests (3 tests): cover happy path (records click + redirects), 404 for missing link, and click_count integration. Adequate.

BLOCKERS

None found.

• No secrets or credentials in code.
• No unvalidated user input -- params[:id] goes through Link.find (parameterized query). session_id is generated server-side.
• No DRY violations in auth/security paths.
• New functionality has test coverage (10 new tests total).

NITS

1. Missing button CSS -- The link-card-track and link-detail-track CSS classes are referenced but no stylesheet changes are included. The button_to helper renders an actual <button> element, which has default browser styling (border, background, padding). Without CSS to reset these, the link titles will look like clickable buttons rather than text links. Consider adding CSS in this PR or documenting it as a known follow-up.

2. N+1 potential on index -- app/views/links/index.html.erb renders each link via the _link.html.erb partial. The partial now uses click_link_path(link) which only needs the link ID (no extra query). However, if click_count is ever added to the index cards, an includes(:clicks) or counter_cache will be needed. No action required now, just a note.

3. current_session_id fallback -- When session.id is nil, the fallback generates a random hex that is not stored anywhere. This means the generated session_id cannot be correlated across requests for cookieless clients. If analytics accuracy matters, consider initializing the session to force a stable ID. Low priority.

4. Fixture naming -- clicks.yml uses generic names one, two, three. The links fixtures use descriptive names (alpha, bravo). Consider naming click fixtures descriptively (e.g., alpha_session_click, alpha_user_click, bravo_click) for readability. Minor.

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related
• Test plan documents passing test run (14 tests, 28 assertions, 0 failures)
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- all 11 changed files are directly related to click tracking
• db/schema.rb is updated (required when committing migrations)
• Commit message references parent issue (#36)

PROCESS OBSERVATIONS

• Scope: Tightly scoped to click tracking only. No feature flag gating, which the PR body acknowledges (no feature flag system exists yet). Clean.
• Forward compatibility: user_id column is pre-placed without FK for future users table. The PR body documents this explicitly. When issue #32 (Create users table) ships, a follow-up migration adding the FK constraint should be tracked.
• Change failure risk: Low. This is additive -- new table, new controller, new route. No existing behavior is changed except the link title click target (now POSTs through tracking instead of direct navigation). The redirect ensures the user experience is preserved.
• Deployment: Migration needs to run. Standard bin/rails db:migrate on deploy. No data migration needed.

VERDICT: APPROVED

## PR #42 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 8.1, PostgreSQL, Hotwire/Turbo, Minitest. This is a standard Rails MVC feature -- new model, controller, migration, views, and tests. **Migration (`db/migrate/20260609195232_create_clicks.rb`)** - Schema is sound. `link_id` has a FK constraint; `user_id` intentionally omits one (users table does not exist yet). Indices on `user_id`, `session_id`, `clicked_at` are appropriate for the anticipated queries. - `clicked_at` is `null: false` at the DB level, matching the model validation. Good. - Note: `user_id` is `bigint` but there is no users table yet. When the users table lands, a follow-up migration should add the FK constraint. This is documented in the PR body -- acceptable. **Model (`app/models/click.rb`)** - Custom validation `must_have_attribution` correctly enforces at least one identifier. Uses `.blank?` which covers both nil and empty string -- correct. - `belongs_to :link` is non-optional by default in Rails 8.1, so the FK presence is validated automatically. Good. **Model (`app/models/link.rb`)** - `has_many :clicks, dependent: :destroy` -- correct. When a link is deleted, its clicks are cleaned up. - `click_count` method uses `clicks.count`, which issues `SELECT COUNT(*)` each time. This is fine at current scale. When click volume grows, a `counter_cache` column would be more efficient, but the PR body explicitly acknowledges this trade-off. Acceptable. **Controller (`app/controllers/clicks_controller.rb`)** - `Link.find(params[:id])` raises `RecordNotFound` on miss, which Rails maps to 404. Matches existing patterns in `LinksController`. - `Click.create!` with bang -- if the create fails validation, this raises `ActiveRecord::RecordInvalid` which would 500. Since `session_id` is always populated by `current_session_id` and `clicked_at` is always set, the only failure path would be if the link somehow became nil between `find` and `create!`, which is a race condition not worth guarding against. Acceptable. - `redirect_to link.url, allow_other_host: true` -- The `allow_other_host: true` is required since links are external URLs. This is NOT an open redirect vulnerability because the URL comes from a stored `Link` record whose `url` field is validated against `http`/`https` schemes at creation time. Users cannot control the redirect target through request parameters. - `current_session_id` -- falls back to `SecureRandom.hex(16)` when `session.id` is nil. This means a user without a session cookie gets a random ID that is not persisted across requests. Each click from a cookieless client gets a different session_id. This is a known limitation, not a bug. **Views** - `button_to` generates a form with a POST, which is semantically correct for a state-changing action (recording a click). CSRF protection is handled automatically by Rails. Good. - The `link_to` -> `button_to` change means the title is now a submit button inside a form. CSS classes `link-card-track` and `link-card-track-form` are added for styling. The styling for these classes is not included in this PR -- this may cause visual regression if the button renders with default browser button styling. **Routes** - `post :click, on: :member, to: "clicks#create"` nested under links produces `POST /links/:id/click` mapped to `ClicksController#create`. Clean and RESTful. **Tests** - Model tests (7 tests): cover valid with session_id only, user_id only, both, invalid without either, invalid without clicked_at, invalid without link, and association check. Good coverage. - Controller tests (3 tests): cover happy path (records click + redirects), 404 for missing link, and click_count integration. Adequate. ### BLOCKERS None found. - No secrets or credentials in code. - No unvalidated user input -- `params[:id]` goes through `Link.find` (parameterized query). `session_id` is generated server-side. - No DRY violations in auth/security paths. - New functionality has test coverage (10 new tests total). ### NITS 1. **Missing button CSS** -- The `link-card-track` and `link-detail-track` CSS classes are referenced but no stylesheet changes are included. The `button_to` helper renders an actual `<button>` element, which has default browser styling (border, background, padding). Without CSS to reset these, the link titles will look like clickable buttons rather than text links. Consider adding CSS in this PR or documenting it as a known follow-up. 2. **N+1 potential on index** -- `app/views/links/index.html.erb` renders each link via the `_link.html.erb` partial. The partial now uses `click_link_path(link)` which only needs the link ID (no extra query). However, if click_count is ever added to the index cards, an `includes(:clicks)` or counter_cache will be needed. No action required now, just a note. 3. **`current_session_id` fallback** -- When `session.id` is nil, the fallback generates a random hex that is not stored anywhere. This means the generated session_id cannot be correlated across requests for cookieless clients. If analytics accuracy matters, consider initializing the session to force a stable ID. Low priority. 4. **Fixture naming** -- `clicks.yml` uses generic names `one`, `two`, `three`. The links fixtures use descriptive names (`alpha`, `bravo`). Consider naming click fixtures descriptively (e.g., `alpha_session_click`, `alpha_user_click`, `bravo_click`) for readability. Minor. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Test plan documents passing test run (14 tests, 28 assertions, 0 failures) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 11 changed files are directly related to click tracking - [x] `db/schema.rb` is updated (required when committing migrations) - [x] Commit message references parent issue (#36) ### PROCESS OBSERVATIONS - **Scope**: Tightly scoped to click tracking only. No feature flag gating, which the PR body acknowledges (no feature flag system exists yet). Clean. - **Forward compatibility**: `user_id` column is pre-placed without FK for future users table. The PR body documents this explicitly. When issue #32 (Create users table) ships, a follow-up migration adding the FK constraint should be tracked. - **Change failure risk**: Low. This is additive -- new table, new controller, new route. No existing behavior is changed except the link title click target (now POSTs through tracking instead of direct navigation). The redirect ensures the user experience is preserved. - **Deployment**: Migration needs to run. Standard `bin/rails db:migrate` on deploy. No data migration needed. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-11 03:14:56 +00:00

Fix QA review nits on click tracking PR ... 7be4d0acb8

- Add CSS resets for button_to elements (.link-card-track, .link-detail-track)
  so they render as plain links instead of browser-default buttons
- Fix current_session_id to persist generated IDs in the session via
  session[:click_tracking_id], enabling click correlation for cookieless clients
- Rename generic fixtures (one/two/three) to descriptive names
  (alpha_recent/alpha_old/bravo_recent) matching codebase convention

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-11 03:16:22 +00:00

Author

Owner

Copy link

PR #42 Re-Review

Re-review after nit fixes from commit 7be4d0a. Verifying 3 reported fixes from the previous review.

NIT FIX VERIFICATION

Nit 1 -- CSS resets for button_to elements: VERIFIED
The diff adds .link-card-track-form / .link-detail-track-form with display: inline and .link-card-track / .link-detail-track with a full button reset (background, border, padding, margin, font, color, cursor, text-align, text-decoration all zeroed out). Hover states use var(--color-accent) consistent with the existing .link-card-title a:hover pattern. The form: { class: "..." } parameter on button_to correctly targets the wrapping <form> element. Well done.

Nit 2 -- N+1 query potential: ACKNOWLEDGED SKIP
Original review noted this was a future concern, not applicable to current code. The click_count method on show.html.erb only fires for a single link, and the index partial does not display click counts. No action needed now. Correct to skip.

Nit 3 -- current_session_id persistence: VERIFIED
The controller now reads/writes session[:click_tracking_id] with the ||= pattern: session[:click_tracking_id] ||= session.id&.to_s || SecureRandom.hex(16). This correctly persists the generated ID so subsequent requests within the same session get the same tracking ID. The &.to_s safely handles nil session IDs (e.g., in test environments or before session initialization).

Nit 4 -- Fixture renaming: VERIFIED
Fixtures renamed from generic one/two/three to descriptive alpha_recent, alpha_old, bravo_recent. Names now communicate which link they belong to and their temporal relationship. Fixture references in tests (links(:alpha) etc.) align with the links fixture file.

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Minitest, PostgreSQL

Migration (db/migrate/20260609195232_create_clicks.rb)

• Correctly uses t.references :link, null: false, foreign_key: true for the link FK
• user_id is bigint with no FK constraint -- intentional and documented (users table does not exist yet)
• Individual indices on user_id, session_id, and clicked_at are appropriate for the query patterns (lookup by user, lookup by session, time-range queries)
• clicked_at is datetime, null: false -- good, prevents null timestamps

Model (app/models/click.rb)

• belongs_to :link with custom validation must_have_attribution ensuring at least one of user_id/session_id -- correct pattern for a pre-auth system where users table is pending
• validates :clicked_at, presence: true -- correct

Model (app/models/link.rb)

• has_many :clicks, dependent: :destroy -- correct cascade behavior for when links are deleted
• click_count delegates to clicks.count -- issues a SELECT COUNT(*) which is fine for single-record display

Controller (app/controllers/clicks_controller.rb)

• Link.find(params[:id]) raises ActiveRecord::RecordNotFound which Rails maps to 404 -- confirmed by the test
• Click.create! with bang -- raises on validation failure. Since session_id is always populated by current_session_id, and clicked_at is always Time.current, validation failure should not occur in normal flow. Acceptable.
• redirect_to link.url, allow_other_host: true -- necessary for external URLs. The URL is already validated at the model level via URI::DEFAULT_PARSER.make_regexp(%w[http https]) on Link, so this is safe.
• No CSRF concern: button_to generates a form with Rails' authenticity token by default.

Views

• button_to with method: :post correctly generates a form that POSTs rather than navigating directly -- this is the standard Rails pattern for tracking through redirect.
• pluralize(@link.click_count) handles singular/plural correctly.

Routes

• post :click, on: :member, to: "clicks#create" nested under links -- generates /links/:id/click which is clean and RESTful.

Tests (10 tests total: 7 model + 3 controller)

• Model tests cover: valid with session_id only, valid with user_id only, valid with both, invalid without either, invalid without clicked_at, invalid without link, belongs_to association. Thorough.
• Controller tests cover: recording + redirect, 404 for missing link, click_count increment. Good coverage.
• The assert_difference "Click.count", 1 pattern correctly verifies persistence.

BLOCKERS

None.

NITS

None remaining. All 3 actionable nits from the previous review have been addressed correctly.

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related -- all present and detailed
• Review Checklist included with explicit design decisions documented
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- all 12 files are directly related to click tracking
• Commit messages reference the parent issue (#36)
• Tests exist and cover model validations + controller behavior
• db/schema.rb updated to match migration

PROCESS OBSERVATIONS

Clean implementation of a tracking table with forward-compatible design (user_id column ready for auth integration). The decision to skip FK on user_id is documented in the PR body and makes sense given the users table does not exist yet. The button_to-with-redirect pattern is the standard Rails approach for click-through tracking without JavaScript dependencies.

VERDICT: APPROVED

## PR #42 Re-Review Re-review after nit fixes from commit 7be4d0a. Verifying 3 reported fixes from the previous review. ### NIT FIX VERIFICATION **Nit 1 -- CSS resets for button_to elements: VERIFIED** The diff adds `.link-card-track-form` / `.link-detail-track-form` with `display: inline` and `.link-card-track` / `.link-detail-track` with a full button reset (background, border, padding, margin, font, color, cursor, text-align, text-decoration all zeroed out). Hover states use `var(--color-accent)` consistent with the existing `.link-card-title a:hover` pattern. The `form: { class: "..." }` parameter on `button_to` correctly targets the wrapping `<form>` element. Well done. **Nit 2 -- N+1 query potential: ACKNOWLEDGED SKIP** Original review noted this was a future concern, not applicable to current code. The `click_count` method on show.html.erb only fires for a single link, and the index partial does not display click counts. No action needed now. Correct to skip. **Nit 3 -- current_session_id persistence: VERIFIED** The controller now reads/writes `session[:click_tracking_id]` with the `||=` pattern: `session[:click_tracking_id] ||= session.id&.to_s || SecureRandom.hex(16)`. This correctly persists the generated ID so subsequent requests within the same session get the same tracking ID. The `&.to_s` safely handles nil session IDs (e.g., in test environments or before session initialization). **Nit 4 -- Fixture renaming: VERIFIED** Fixtures renamed from generic `one`/`two`/`three` to descriptive `alpha_recent`, `alpha_old`, `bravo_recent`. Names now communicate which link they belong to and their temporal relationship. Fixture references in tests (`links(:alpha)` etc.) align with the links fixture file. ### DOMAIN REVIEW **Stack: Ruby on Rails 8.1, Minitest, PostgreSQL** **Migration (db/migrate/20260609195232_create_clicks.rb)** - Correctly uses `t.references :link, null: false, foreign_key: true` for the link FK - `user_id` is `bigint` with no FK constraint -- intentional and documented (users table does not exist yet) - Individual indices on `user_id`, `session_id`, and `clicked_at` are appropriate for the query patterns (lookup by user, lookup by session, time-range queries) - `clicked_at` is `datetime, null: false` -- good, prevents null timestamps **Model (app/models/click.rb)** - `belongs_to :link` with custom validation `must_have_attribution` ensuring at least one of user_id/session_id -- correct pattern for a pre-auth system where users table is pending - `validates :clicked_at, presence: true` -- correct **Model (app/models/link.rb)** - `has_many :clicks, dependent: :destroy` -- correct cascade behavior for when links are deleted - `click_count` delegates to `clicks.count` -- issues a `SELECT COUNT(*)` which is fine for single-record display **Controller (app/controllers/clicks_controller.rb)** - `Link.find(params[:id])` raises `ActiveRecord::RecordNotFound` which Rails maps to 404 -- confirmed by the test - `Click.create!` with bang -- raises on validation failure. Since session_id is always populated by `current_session_id`, and `clicked_at` is always `Time.current`, validation failure should not occur in normal flow. Acceptable. - `redirect_to link.url, allow_other_host: true` -- necessary for external URLs. The URL is already validated at the model level via `URI::DEFAULT_PARSER.make_regexp(%w[http https])` on Link, so this is safe. - No CSRF concern: `button_to` generates a form with Rails' authenticity token by default. **Views** - `button_to` with `method: :post` correctly generates a form that POSTs rather than navigating directly -- this is the standard Rails pattern for tracking through redirect. - `pluralize(@link.click_count)` handles singular/plural correctly. **Routes** - `post :click, on: :member, to: "clicks#create"` nested under links -- generates `/links/:id/click` which is clean and RESTful. **Tests (10 tests total: 7 model + 3 controller)** - Model tests cover: valid with session_id only, valid with user_id only, valid with both, invalid without either, invalid without clicked_at, invalid without link, belongs_to association. Thorough. - Controller tests cover: recording + redirect, 404 for missing link, click_count increment. Good coverage. - The `assert_difference "Click.count", 1` pattern correctly verifies persistence. ### BLOCKERS None. ### NITS None remaining. All 3 actionable nits from the previous review have been addressed correctly. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and detailed - [x] Review Checklist included with explicit design decisions documented - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 12 files are directly related to click tracking - [x] Commit messages reference the parent issue (#36) - [x] Tests exist and cover model validations + controller behavior - [x] `db/schema.rb` updated to match migration ### PROCESS OBSERVATIONS Clean implementation of a tracking table with forward-compatible design (user_id column ready for auth integration). The decision to skip FK on user_id is documented in the PR body and makes sense given the users table does not exist yet. The button_to-with-redirect pattern is the standard Rails approach for click-through tracking without JavaScript dependencies. ### VERDICT: APPROVED

ldraney commented 2026-06-12 04:12:52 +00:00

Author

Owner

Copy link

PR #42 Review

DOMAIN REVIEW

Stack: Ruby on Rails 8.1, Minitest, PostgreSQL, Turbo/Importmaps. Reviewing as a Rails domain expert.

Architecture: The redirect-through-tracking pattern (POST -> record -> redirect) is sound. Using button_to to generate a form that POSTs is the correct Rails approach -- it gets CSRF protection for free via the authenticity token that button_to embeds automatically.

Migration: Clean and correct. FK on link_id, no FK on user_id (documented as intentional since users table does not exist yet), appropriate indices on all lookup columns. The clicked_at column with null: false is good -- the model validates this too.

Model: The must_have_attribution custom validator is well-structured. belongs_to :link enforces presence by default in Rails 5+. dependent: :destroy on the has_many :clicks is correct for cleanup.

Session tracking: current_session_id has good fallback logic: session[:click_tracking_id] ||= session.id&.to_s || SecureRandom.hex(16). This ensures a value is always present, which means Click.create! will never fail the attribution validator for anonymous users.

BLOCKERS

None. No blockers identified.

NITS

1. N+1 risk in click_count (low priority now, high priority later): Link#click_count executes clicks.count which fires a SELECT COUNT(*) per call. Currently only used on show.html.erb (single link), so it is a single query. But if click count is ever added to the index partial (_link.html.erb), it will create an N+1. Consider adding a comment to the method noting this, or plan a counter_cache column when click counts appear on index pages.

   • File: app/models/link.rb, the click_count method

2. No rate limiting on click tracking endpoint: Any anonymous user can POST to /links/:id/click in a tight loop to inflate counts. This is not a security vulnerability but an analytics-integrity concern. Consider whether throttling (e.g., rack-attack or a uniqueness constraint on session_id+link_id per time window) is needed before click data drives any real decisions.

3. CSS DRY opportunity: The .link-card-track:hover and .link-detail-track:hover rules are identical. These could be combined:

   .link-card-track:hover,
   .link-detail-track:hover {
       color: var(--color-accent);
       text-decoration: none;
   }
   

   • File: app/assets/stylesheets/application.css

4. clicked_at vs created_at redundancy: The clicks table has both clicked_at (explicit) and created_at (Rails timestamps). In practice these will always be the same value since clicks are recorded at the moment they happen. The explicit clicked_at does communicate intent more clearly than created_at, so this is defensible, but worth noting the redundancy.

SOP COMPLIANCE

• Branch named after issue: 36-clicks-table-tracking references issue #36
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present
• Closes #36 present in Related section
• Tests exist and pass: 7 model tests + 3 controller tests, PR states 14 tests / 28 assertions / 0 failures
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- all 12 files are directly related to click tracking
• Commit messages are descriptive (PR title matches pattern)
• Related Notes section says "None" -- no plan slug referenced. Acceptable if no plan note exists for this ticket.

PROCESS OBSERVATIONS

• Change failure risk: LOW. This is additive-only (new table, new controller, new model). No existing functionality is modified except view templates swapping link_to for button_to. The existing link CRUD is untouched.
• Schema note: The user_id column without FK constraint is explicitly documented and matches the current state of the app (no users table yet). When #32 (Create users table) lands, a follow-up migration should add the FK constraint. Consider creating a ticket for this or adding a TODO comment in the migration.
• Test coverage is solid for the scope of the change. Model validations and controller happy/sad paths are all covered.

VERDICT: APPROVED

## PR #42 Review ### DOMAIN REVIEW **Stack:** Ruby on Rails 8.1, Minitest, PostgreSQL, Turbo/Importmaps. Reviewing as a Rails domain expert. **Architecture:** The redirect-through-tracking pattern (POST -> record -> redirect) is sound. Using `button_to` to generate a form that POSTs is the correct Rails approach -- it gets CSRF protection for free via the authenticity token that `button_to` embeds automatically. **Migration:** Clean and correct. FK on `link_id`, no FK on `user_id` (documented as intentional since users table does not exist yet), appropriate indices on all lookup columns. The `clicked_at` column with `null: false` is good -- the model validates this too. **Model:** The `must_have_attribution` custom validator is well-structured. `belongs_to :link` enforces presence by default in Rails 5+. `dependent: :destroy` on the `has_many :clicks` is correct for cleanup. **Session tracking:** `current_session_id` has good fallback logic: `session[:click_tracking_id] ||= session.id&.to_s || SecureRandom.hex(16)`. This ensures a value is always present, which means `Click.create!` will never fail the attribution validator for anonymous users. ### BLOCKERS **None.** No blockers identified. ### NITS 1. **N+1 risk in `click_count` (low priority now, high priority later):** `Link#click_count` executes `clicks.count` which fires a `SELECT COUNT(*)` per call. Currently only used on `show.html.erb` (single link), so it is a single query. But if click count is ever added to the index partial (`_link.html.erb`), it will create an N+1. Consider adding a comment to the method noting this, or plan a `counter_cache` column when click counts appear on index pages. - File: `app/models/link.rb`, the `click_count` method 2. **No rate limiting on click tracking endpoint:** Any anonymous user can POST to `/links/:id/click` in a tight loop to inflate counts. This is not a security vulnerability but an analytics-integrity concern. Consider whether throttling (e.g., `rack-attack` or a uniqueness constraint on session_id+link_id per time window) is needed before click data drives any real decisions. 3. **CSS DRY opportunity:** The `.link-card-track:hover` and `.link-detail-track:hover` rules are identical. These could be combined: ```css .link-card-track:hover, .link-detail-track:hover { color: var(--color-accent); text-decoration: none; } ``` - File: `app/assets/stylesheets/application.css` 4. **`clicked_at` vs `created_at` redundancy:** The `clicks` table has both `clicked_at` (explicit) and `created_at` (Rails timestamps). In practice these will always be the same value since clicks are recorded at the moment they happen. The explicit `clicked_at` does communicate intent more clearly than `created_at`, so this is defensible, but worth noting the redundancy. ### SOP COMPLIANCE - [x] Branch named after issue: `36-clicks-table-tracking` references issue #36 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes, Related sections all present - [x] `Closes #36` present in Related section - [x] Tests exist and pass: 7 model tests + 3 controller tests, PR states 14 tests / 28 assertions / 0 failures - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes -- all 12 files are directly related to click tracking - [x] Commit messages are descriptive (PR title matches pattern) - [ ] Related Notes section says "None" -- no plan slug referenced. Acceptable if no plan note exists for this ticket. ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** This is additive-only (new table, new controller, new model). No existing functionality is modified except view templates swapping `link_to` for `button_to`. The existing link CRUD is untouched. - **Schema note:** The `user_id` column without FK constraint is explicitly documented and matches the current state of the app (no users table yet). When `#32` (Create users table) lands, a follow-up migration should add the FK constraint. Consider creating a ticket for this or adding a TODO comment in the migration. - **Test coverage is solid** for the scope of the change. Model validations and controller happy/sad paths are all covered. ### VERDICT: APPROVED

ldraney merged commit e5352d7fb5 into main 2026-06-13 13:25:16 +00:00

ldraney deleted branch 36-clicks-table-tracking 2026-06-13 13:25:16 +00:00

ldraney referenced this pull request from a commit 2026-06-13 13:25:16 +00:00

Create clicks table and tracking (#36) (#42)

ldraney referenced this pull request 2026-06-14 11:35:01 +00:00

Add tab-based navigation with filter views (#40) #52

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/palinks!42

No description provided.