add URL redirects with ETS-cached plug, broken URL tracking, and admin UI
All checks were successful
deploy / deploy (push) Successful in 3m30s
All checks were successful
deploy / deploy (push) Successful in 3m30s
Redirects context with redirect/broken_url schemas, chain flattening, ETS cache for fast lookups in the request pipeline. BrokenUrlTracker plug logs 404s. Auto-redirect on product slug change via upsert_product hook. Admin redirects page with active/broken tabs, manual create form. RedirectPrunerWorker cleans up old broken URLs. 1227 tests passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
91
test/berrypod/redirects_integration_test.exs
Normal file
91
test/berrypod/redirects_integration_test.exs
Normal file
@@ -0,0 +1,91 @@
|
||||
defmodule Berrypod.RedirectsIntegrationTest do
|
||||
use Berrypod.DataCase, async: true
|
||||
|
||||
alias Berrypod.{Products, Redirects}
|
||||
import Berrypod.ProductsFixtures
|
||||
|
||||
setup do
|
||||
Redirects.create_table()
|
||||
:ok
|
||||
end
|
||||
|
||||
describe "upsert_product/2 auto-redirect on slug change" do
|
||||
test "creates redirect when title changes" do
|
||||
conn = provider_connection_fixture(%{provider_type: "printify"})
|
||||
|
||||
# Create product
|
||||
{:ok, product, :created} =
|
||||
Products.upsert_product(conn, %{
|
||||
title: "Classic Tee",
|
||||
provider_product_id: "slug-change-test",
|
||||
status: "active",
|
||||
visible: true,
|
||||
provider_data: %{"version" => 1}
|
||||
})
|
||||
|
||||
assert product.slug == "classic-tee"
|
||||
|
||||
# Update with different title (different checksum via provider_data)
|
||||
{:ok, updated, :updated} =
|
||||
Products.upsert_product(conn, %{
|
||||
title: "Classic Tee V2",
|
||||
provider_product_id: "slug-change-test",
|
||||
status: "active",
|
||||
visible: true,
|
||||
provider_data: %{"version" => 2}
|
||||
})
|
||||
|
||||
assert updated.slug == "classic-tee-v2"
|
||||
|
||||
# Redirect should exist
|
||||
assert {:ok, %{to_path: "/products/classic-tee-v2"}} =
|
||||
Redirects.lookup("/products/classic-tee")
|
||||
end
|
||||
|
||||
test "does not create redirect when title is unchanged" do
|
||||
conn = provider_connection_fixture(%{provider_type: "printify"})
|
||||
|
||||
{:ok, _product, :created} =
|
||||
Products.upsert_product(conn, %{
|
||||
title: "Unchanged Product",
|
||||
provider_product_id: "no-slug-change",
|
||||
status: "active",
|
||||
visible: true,
|
||||
provider_data: %{"version" => 1}
|
||||
})
|
||||
|
||||
# Update with same title but different data
|
||||
{:ok, _updated, :updated} =
|
||||
Products.upsert_product(conn, %{
|
||||
title: "Unchanged Product",
|
||||
provider_product_id: "no-slug-change",
|
||||
status: "active",
|
||||
visible: true,
|
||||
description: "now with a description",
|
||||
provider_data: %{"version" => 2}
|
||||
})
|
||||
|
||||
assert :not_found = Redirects.lookup("/products/unchanged-product")
|
||||
end
|
||||
end
|
||||
|
||||
describe "delete_product/1 auto-redirect" do
|
||||
test "creates redirect to category collection page" do
|
||||
product = product_fixture(%{title: "Doomed Shirt", category: "T-Shirts"})
|
||||
|
||||
{:ok, _} = Products.delete_product(product)
|
||||
|
||||
assert {:ok, %{to_path: "/collections/t-shirts"}} =
|
||||
Redirects.lookup("/products/#{product.slug}")
|
||||
end
|
||||
|
||||
test "creates redirect to / when no category" do
|
||||
product = product_fixture(%{title: "No Category Item", category: nil})
|
||||
|
||||
{:ok, _} = Products.delete_product(product)
|
||||
|
||||
assert {:ok, %{to_path: "/"}} =
|
||||
Redirects.lookup("/products/#{product.slug}")
|
||||
end
|
||||
end
|
||||
end
|
||||
225
test/berrypod/redirects_test.exs
Normal file
225
test/berrypod/redirects_test.exs
Normal file
@@ -0,0 +1,225 @@
|
||||
defmodule Berrypod.RedirectsTest do
|
||||
use Berrypod.DataCase, async: true
|
||||
|
||||
alias Berrypod.Redirects
|
||||
alias Berrypod.Redirects.Redirect
|
||||
|
||||
setup do
|
||||
Redirects.create_table()
|
||||
:ok
|
||||
end
|
||||
|
||||
describe "create_auto/1" do
|
||||
test "creates a redirect" do
|
||||
assert {:ok, redirect} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/old-slug",
|
||||
to_path: "/products/new-slug",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
assert redirect.from_path == "/products/old-slug"
|
||||
assert redirect.to_path == "/products/new-slug"
|
||||
assert redirect.status_code == 301
|
||||
assert redirect.source == "auto_slug_change"
|
||||
assert redirect.hit_count == 0
|
||||
end
|
||||
|
||||
test "is idempotent on conflict" do
|
||||
attrs = %{
|
||||
from_path: "/products/idempotent",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
}
|
||||
|
||||
assert {:ok, _} = Redirects.create_auto(attrs)
|
||||
assert {:ok, _} = Redirects.create_auto(attrs)
|
||||
|
||||
# Only one redirect exists
|
||||
assert [_] = Repo.all(from r in Redirect, where: r.from_path == "/products/idempotent")
|
||||
end
|
||||
|
||||
test "flattens redirect chains on creation" do
|
||||
# A -> B
|
||||
{:ok, _} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/a",
|
||||
to_path: "/products/b",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
# B -> C (should flatten A -> C as well)
|
||||
{:ok, _} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/b",
|
||||
to_path: "/products/c",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
# A should now point directly to C
|
||||
assert {:ok, %{to_path: "/products/c"}} = Redirects.lookup("/products/a")
|
||||
# B -> C still works
|
||||
assert {:ok, %{to_path: "/products/c"}} = Redirects.lookup("/products/b")
|
||||
end
|
||||
end
|
||||
|
||||
describe "create_manual/1" do
|
||||
test "creates a manual redirect" do
|
||||
assert {:ok, redirect} =
|
||||
Redirects.create_manual(%{
|
||||
from_path: "/old-page",
|
||||
to_path: "/new-page"
|
||||
})
|
||||
|
||||
assert redirect.source == "admin"
|
||||
assert redirect.status_code == 301
|
||||
end
|
||||
end
|
||||
|
||||
describe "lookup/1" do
|
||||
test "returns redirect when found" do
|
||||
{:ok, _} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/lookup-test",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
assert {:ok, %{to_path: "/products/new", status_code: 301}} =
|
||||
Redirects.lookup("/products/lookup-test")
|
||||
end
|
||||
|
||||
test "returns :not_found when no redirect exists" do
|
||||
assert :not_found = Redirects.lookup("/products/nonexistent")
|
||||
end
|
||||
end
|
||||
|
||||
describe "increment_hit_count/1" do
|
||||
test "increments the hit count" do
|
||||
{:ok, redirect} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/hits-test",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
assert redirect.hit_count == 0
|
||||
|
||||
Redirects.increment_hit_count(%{id: redirect.id})
|
||||
|
||||
updated = Redirects.get_redirect!(redirect.id)
|
||||
assert updated.hit_count == 1
|
||||
end
|
||||
end
|
||||
|
||||
describe "delete_redirect/1" do
|
||||
test "deletes a redirect and clears cache" do
|
||||
{:ok, redirect} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/delete-test",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
assert {:ok, %{to_path: "/products/new"}} = Redirects.lookup("/products/delete-test")
|
||||
|
||||
{:ok, _} = Redirects.delete_redirect(redirect)
|
||||
|
||||
assert :not_found = Redirects.lookup("/products/delete-test")
|
||||
end
|
||||
end
|
||||
|
||||
describe "broken URLs" do
|
||||
test "record_broken_url creates a new entry" do
|
||||
{:ok, broken_url} = Redirects.record_broken_url("/products/broken", 42)
|
||||
|
||||
assert broken_url.path == "/products/broken"
|
||||
assert broken_url.prior_analytics_hits == 42
|
||||
assert broken_url.recent_404_count == 1
|
||||
end
|
||||
|
||||
test "record_broken_url increments count on existing entry" do
|
||||
{:ok, _} = Redirects.record_broken_url("/products/repeat-404", 10)
|
||||
{:ok, updated} = Redirects.record_broken_url("/products/repeat-404", 10)
|
||||
|
||||
assert updated.recent_404_count == 2
|
||||
end
|
||||
|
||||
test "list_broken_urls returns pending sorted by impact" do
|
||||
{:ok, _} = Redirects.record_broken_url("/products/low-traffic", 5)
|
||||
{:ok, _} = Redirects.record_broken_url("/products/high-traffic", 100)
|
||||
|
||||
[first, second] = Redirects.list_broken_urls()
|
||||
assert first.path == "/products/high-traffic"
|
||||
assert second.path == "/products/low-traffic"
|
||||
end
|
||||
|
||||
test "ignore_broken_url marks as ignored" do
|
||||
{:ok, broken_url} = Redirects.record_broken_url("/products/ignore-me", 1)
|
||||
{:ok, ignored} = Redirects.ignore_broken_url(broken_url)
|
||||
|
||||
assert ignored.status == "ignored"
|
||||
assert Redirects.list_broken_urls("pending") == []
|
||||
end
|
||||
end
|
||||
|
||||
describe "prune_stale_redirects/1" do
|
||||
test "prunes auto redirects with 0 hits older than threshold" do
|
||||
# Insert a stale redirect directly
|
||||
{:ok, _} =
|
||||
%Redirect{}
|
||||
|> Redirect.changeset(%{
|
||||
from_path: "/products/stale",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|> Repo.insert()
|
||||
|
||||
# Backdate it
|
||||
Repo.update_all(
|
||||
from(r in Redirect, where: r.from_path == "/products/stale"),
|
||||
set: [inserted_at: DateTime.add(DateTime.utc_now(), -100, :day)]
|
||||
)
|
||||
|
||||
{:ok, count} = Redirects.prune_stale_redirects(90)
|
||||
assert count == 1
|
||||
assert :not_found = Redirects.lookup("/products/stale")
|
||||
end
|
||||
|
||||
test "preserves redirects with hits" do
|
||||
{:ok, redirect} =
|
||||
%Redirect{}
|
||||
|> Redirect.changeset(%{
|
||||
from_path: "/products/has-hits",
|
||||
to_path: "/products/new",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|> Repo.insert()
|
||||
|
||||
# Add a hit and backdate
|
||||
Repo.update_all(
|
||||
from(r in Redirect, where: r.id == ^redirect.id),
|
||||
set: [hit_count: 5, inserted_at: DateTime.add(DateTime.utc_now(), -100, :day)]
|
||||
)
|
||||
|
||||
{:ok, count} = Redirects.prune_stale_redirects(90)
|
||||
assert count == 0
|
||||
end
|
||||
|
||||
test "preserves manual redirects regardless of age" do
|
||||
{:ok, _} =
|
||||
Redirects.create_manual(%{
|
||||
from_path: "/products/manual-old",
|
||||
to_path: "/products/new"
|
||||
})
|
||||
|
||||
Repo.update_all(
|
||||
from(r in Redirect, where: r.from_path == "/products/manual-old"),
|
||||
set: [inserted_at: DateTime.add(DateTime.utc_now(), -200, :day)]
|
||||
)
|
||||
|
||||
{:ok, count} = Redirects.prune_stale_redirects(90)
|
||||
assert count == 0
|
||||
end
|
||||
end
|
||||
end
|
||||
26
test/berrypod_web/plugs/broken_url_tracker_test.exs
Normal file
26
test/berrypod_web/plugs/broken_url_tracker_test.exs
Normal file
@@ -0,0 +1,26 @@
|
||||
defmodule BerrypodWeb.Plugs.BrokenUrlTrackerTest do
|
||||
use BerrypodWeb.ConnCase, async: true
|
||||
|
||||
alias Berrypod.Redirects
|
||||
|
||||
setup do
|
||||
Redirects.create_table()
|
||||
:ok
|
||||
end
|
||||
|
||||
test "records broken URL on 404", %{conn: conn} do
|
||||
conn = get(conn, "/zz-nonexistent-path")
|
||||
|
||||
assert conn.status in [404, 500]
|
||||
|
||||
[broken_url] = Redirects.list_broken_urls()
|
||||
assert broken_url.path == "/zz-nonexistent-path"
|
||||
assert broken_url.recent_404_count == 1
|
||||
end
|
||||
|
||||
test "skips static asset paths", %{conn: conn} do
|
||||
get(conn, "/assets/missing-file.js")
|
||||
|
||||
assert Redirects.list_broken_urls() == []
|
||||
end
|
||||
end
|
||||
92
test/berrypod_web/plugs/redirects_test.exs
Normal file
92
test/berrypod_web/plugs/redirects_test.exs
Normal file
@@ -0,0 +1,92 @@
|
||||
defmodule BerrypodWeb.Plugs.RedirectsTest do
|
||||
use BerrypodWeb.ConnCase, async: true
|
||||
|
||||
alias Berrypod.Redirects
|
||||
|
||||
setup do
|
||||
Redirects.create_table()
|
||||
:ok
|
||||
end
|
||||
|
||||
describe "trailing slash normalisation" do
|
||||
test "strips trailing slash and redirects", %{conn: conn} do
|
||||
conn = get(conn, "/products/foo/")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/foo"]
|
||||
end
|
||||
|
||||
test "preserves query params on trailing slash redirect", %{conn: conn} do
|
||||
conn = get(conn, "/products/foo/?Color=Sand&Size=S")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/foo?Color=Sand&Size=S"]
|
||||
end
|
||||
|
||||
test "does not strip trailing slash from root path", %{conn: conn} do
|
||||
conn = get(conn, "/")
|
||||
|
||||
# Should not redirect — just pass through to the app
|
||||
refute conn.status == 301
|
||||
end
|
||||
end
|
||||
|
||||
describe "case normalisation" do
|
||||
test "lowercases path and redirects", %{conn: conn} do
|
||||
conn = get(conn, "/Products/Foo")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/foo"]
|
||||
end
|
||||
|
||||
test "preserves query param casing", %{conn: conn} do
|
||||
conn = get(conn, "/Products/Foo?Color=Sand")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/foo?Color=Sand"]
|
||||
end
|
||||
|
||||
test "does not redirect already-lowercase paths", %{conn: conn} do
|
||||
# This will get a 404 or 200 from the app, not a 301
|
||||
conn = get(conn, "/products/nonexistent-lowercase-path")
|
||||
|
||||
refute conn.status == 301
|
||||
end
|
||||
end
|
||||
|
||||
describe "custom redirects" do
|
||||
test "redirects matching path with 301", %{conn: conn} do
|
||||
{:ok, _} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/old-tee",
|
||||
to_path: "/products/new-tee",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
conn = get(conn, "/products/old-tee")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/new-tee"]
|
||||
end
|
||||
|
||||
test "preserves query string on custom redirect", %{conn: conn} do
|
||||
{:ok, _} =
|
||||
Redirects.create_auto(%{
|
||||
from_path: "/products/old-hoodie",
|
||||
to_path: "/products/new-hoodie",
|
||||
source: "auto_slug_change"
|
||||
})
|
||||
|
||||
conn = get(conn, "/products/old-hoodie?Color=Sand&Size=S")
|
||||
|
||||
assert conn.status == 301
|
||||
assert get_resp_header(conn, "location") == ["/products/new-hoodie?Color=Sand&Size=S"]
|
||||
end
|
||||
|
||||
test "passes through when no redirect exists", %{conn: conn} do
|
||||
conn = get(conn, "/products/no-redirect-here")
|
||||
|
||||
refute conn.status == 301
|
||||
end
|
||||
end
|
||||
end
|
||||
@@ -1,2 +1,3 @@
|
||||
Berrypod.Redirects.create_table()
|
||||
ExUnit.start(exclude: [:benchmark])
|
||||
Ecto.Adapters.SQL.Sandbox.mode(Berrypod.Repo, :manual)
|
||||
|
||||
Reference in New Issue
Block a user