diff --git a/app/components/ui/SportIconUploader.tsx b/app/components/ui/SportIconUploader.tsx index 0413117..9f46677 100644 --- a/app/components/ui/SportIconUploader.tsx +++ b/app/components/ui/SportIconUploader.tsx @@ -1,14 +1,21 @@ -import { useRef, useState } from "react"; +import { useId, useRef, useState } from "react"; import { ImageIcon, Trash2, Upload } from "lucide-react"; import { Button } from "~/components/ui/button"; import { SportIcon } from "~/components/SportIcon"; import { cn } from "~/lib/utils"; +interface SportIconOption { + id: string; + name: string; + iconUrl: string; +} + interface SportIconUploaderProps { name?: string; uploadUrl: string; initialIconUrl?: string | null; sportName?: string; + existingIconOptions?: SportIconOption[]; } const MAX_UPLOAD_BYTES = 1024 * 1024; @@ -19,7 +26,9 @@ export function SportIconUploader({ uploadUrl, initialIconUrl = null, sportName = "Sport icon", + existingIconOptions = [], }: SportIconUploaderProps) { + const selectId = useId(); const fileInputRef = useRef(null); const [iconUrl, setIconUrl] = useState(initialIconUrl ?? ""); const [selectedFile, setSelectedFile] = useState(null); @@ -60,6 +69,11 @@ export function SportIconUploader({ clearSelectedFile(); } + function chooseExistingIcon(nextIconUrl: string) { + setIconUrl(nextIconUrl); + clearSelectedFile(); + } + async function uploadSelectedFile() { if (!selectedFile) { setError("Choose an SVG first."); @@ -107,6 +121,30 @@ export function SportIconUploader({ className="sr-only" /> + {existingIconOptions.length > 0 && ( +
+ + +

+ Selecting a sport reuses its current icon URL; no upload required. +

+
+ )} + {iconUrl && !selectedFile && (
diff --git a/app/components/ui/__tests__/SportIconUploader.test.tsx b/app/components/ui/__tests__/SportIconUploader.test.tsx index 3dc7342..625f93a 100644 --- a/app/components/ui/__tests__/SportIconUploader.test.tsx +++ b/app/components/ui/__tests__/SportIconUploader.test.tsx @@ -28,6 +28,25 @@ describe("SportIconUploader", () => { expect(await screen.findByText("Choose an SVG file.")).toBeInTheDocument(); }); + it("chooses an icon from an existing sport", () => { + const { container } = render( + + ); + + fireEvent.change(screen.getByRole("combobox", { name: "Use an existing sport icon" }), { + target: { value: "nba.svg" }, + }); + + expect(container.querySelector('input[name="iconUrl"]')).toHaveValue("nba.svg"); + expect(screen.getByText("Current icon")).toBeInTheDocument(); + }); + it("uploads an SVG and updates the hidden input", async () => { vi.stubGlobal( "fetch", diff --git a/app/lib/__tests__/sport-icon-url.test.ts b/app/lib/__tests__/sport-icon-url.test.ts index 821f11d..d701293 100644 --- a/app/lib/__tests__/sport-icon-url.test.ts +++ b/app/lib/__tests__/sport-icon-url.test.ts @@ -41,5 +41,6 @@ describe("sport icon URL validation", () => { expect(parseSportIconUrlInput("https://res.cloudinary.com/demo/image/upload/v123/avatars/nfl.svg")).toBeUndefined(); expect(parseSportIconUrlInput("nfl.png")).toBeUndefined(); expect(parseSportIconUrlInput("../nfl.svg")).toBeUndefined(); + expect(parseSportIconUrlInput("/sports-icons/nfl.svg")).toBeUndefined(); }); }); diff --git a/app/lib/sport-icon-cleanup.server.ts b/app/lib/sport-icon-cleanup.server.ts new file mode 100644 index 0000000..9404891 --- /dev/null +++ b/app/lib/sport-icon-cleanup.server.ts @@ -0,0 +1,28 @@ +import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server"; +import { logger } from "~/lib/logger"; +import { isSportIconUrlUsed } from "~/models/sport"; + +interface DeleteSportIconIfUnusedArgs { + iconUrl: string; + excludeSportId?: string; + reason: "replaced" | "unused"; +} + +export async function deleteSportIconIfUnused({ + iconUrl, + excludeSportId, + reason, +}: DeleteSportIconIfUnusedArgs): Promise { + try { + if (await isSportIconUrlUsed(iconUrl, excludeSportId)) { + return; + } + } catch (error) { + logger.error("Failed to check whether sport icon is reused; skipping Cloudinary cleanup:", error); + return; + } + + deleteCloudinaryImageByUrl(iconUrl).catch((error) => { + logger.error(`Failed to delete ${reason} sport icon from Cloudinary:`, error); + }); +} diff --git a/app/models/sport.ts b/app/models/sport.ts index 959240f..9a47a98 100644 --- a/app/models/sport.ts +++ b/app/models/sport.ts @@ -1,4 +1,4 @@ -import { eq, sql } from "drizzle-orm"; +import { and, eq, ne, sql } from "drizzle-orm"; import { database } from "~/database/context"; import * as schema from "~/database/schema"; @@ -48,6 +48,17 @@ export async function findAllSports(): Promise { }); } +export async function isSportIconUrlUsed(iconUrl: string, excludeSportId?: string): Promise { + const db = database(); + const sport = await db.query.sports.findFirst({ + where: excludeSportId + ? and(eq(schema.sports.iconUrl, iconUrl), ne(schema.sports.id, excludeSportId)) + : eq(schema.sports.iconUrl, iconUrl), + columns: { id: true }, + }); + return Boolean(sport); +} + export async function findAllSportsWithActiveSeasons(): Promise { const db = database(); const sports = await db.query.sports.findMany({ diff --git a/app/routes/__tests__/admin.sports.$id.test.ts b/app/routes/__tests__/admin.sports.$id.test.ts index 757a582..df09b04 100644 --- a/app/routes/__tests__/admin.sports.$id.test.ts +++ b/app/routes/__tests__/admin.sports.$id.test.ts @@ -1,7 +1,9 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("~/models/sport", () => ({ + findAllSports: vi.fn(), findSportById: vi.fn(), + isSportIconUrlUsed: vi.fn(), updateSport: vi.fn(), })); vi.mock("~/lib/cloudinary.server", () => ({ @@ -17,10 +19,10 @@ vi.mock("~/services/simulations/registry", () => ({ import { action } from "../admin.sports.$id"; import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server"; -import { findSportById, updateSport } from "~/models/sport"; +import { findSportById, isSportIconUrlUsed, updateSport } from "~/models/sport"; function makeRequest(iconUrl: string) { - const formData = new FormData(); + const formData = new URLSearchParams(); formData.set("name", "NFL"); formData.set("type", "team"); formData.set("slug", "nfl"); @@ -30,12 +32,14 @@ function makeRequest(iconUrl: string) { return new Request("http://test/admin/sports/sport-1", { method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: formData, }); } beforeEach(() => { vi.clearAllMocks(); + vi.mocked(isSportIconUrlUsed).mockResolvedValue(false); vi.mocked(findSportById).mockResolvedValue({ id: "sport-1", name: "Old NFL", @@ -105,4 +109,33 @@ describe("admin sport edit action", () => { expect(deleteCloudinaryImageByUrl).toHaveBeenCalledWith(nextIconUrl); }); + + it("does not delete a reused existing icon when update fails", async () => { + const nextIconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/new.svg"; + vi.mocked(updateSport).mockRejectedValue(new Error("duplicate")); + vi.mocked(isSportIconUrlUsed).mockResolvedValue(true); + + await action({ + request: makeRequest(nextIconUrl), + params: { id: "sport-1" }, + context: {}, + } as never); + + expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled(); + }); + + it("returns the update error when icon reuse lookup fails during cleanup", async () => { + const nextIconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/new.svg"; + vi.mocked(updateSport).mockRejectedValue(new Error("database unavailable")); + vi.mocked(isSportIconUrlUsed).mockRejectedValue(new Error("database unavailable")); + + const response = await action({ + request: makeRequest(nextIconUrl), + params: { id: "sport-1" }, + context: {}, + } as never); + + expect(response).toEqual({ error: "Failed to update sport: database unavailable" }); + expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled(); + }); }); diff --git a/app/routes/__tests__/admin.sports.new.test.ts b/app/routes/__tests__/admin.sports.new.test.ts index bc461be..7ec18a7 100644 --- a/app/routes/__tests__/admin.sports.new.test.ts +++ b/app/routes/__tests__/admin.sports.new.test.ts @@ -2,6 +2,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("~/models/sport", () => ({ createSport: vi.fn(), + findAllSports: vi.fn(), + isSportIconUrlUsed: vi.fn(), })); vi.mock("~/lib/cloudinary.server", () => ({ deleteCloudinaryImageByUrl: vi.fn().mockResolvedValue(undefined), @@ -12,10 +14,10 @@ vi.mock("~/lib/logger", () => ({ import { action } from "../admin.sports.new"; import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server"; -import { createSport } from "~/models/sport"; +import { createSport, isSportIconUrlUsed } from "~/models/sport"; function makeRequest(iconUrl: string) { - const formData = new FormData(); + const formData = new URLSearchParams(); formData.set("name", "NFL"); formData.set("type", "team"); formData.set("slug", "nfl"); @@ -24,6 +26,7 @@ function makeRequest(iconUrl: string) { return new Request("http://test/admin/sports/new", { method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, body: formData, }); } @@ -31,6 +34,7 @@ function makeRequest(iconUrl: string) { beforeEach(() => { vi.clearAllMocks(); vi.mocked(createSport).mockResolvedValue({} as never); + vi.mocked(isSportIconUrlUsed).mockResolvedValue(false); }); describe("admin sport create action", () => { @@ -57,4 +61,33 @@ describe("admin sport create action", () => { expect(deleteCloudinaryImageByUrl).toHaveBeenCalledWith(iconUrl); }); + + it("does not delete a reused existing icon when create fails", async () => { + const iconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/nfl.svg"; + vi.mocked(createSport).mockRejectedValue(new Error("duplicate")); + vi.mocked(isSportIconUrlUsed).mockResolvedValue(true); + + await action({ + request: makeRequest(iconUrl), + params: {}, + context: {}, + } as never); + + expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled(); + }); + + it("returns the create error when icon reuse lookup fails during cleanup", async () => { + const iconUrl = "https://res.cloudinary.com/demo/image/upload/v1/sports-icons/nfl.svg"; + vi.mocked(createSport).mockRejectedValue(new Error("database unavailable")); + vi.mocked(isSportIconUrlUsed).mockRejectedValue(new Error("database unavailable")); + + const response = await action({ + request: makeRequest(iconUrl), + params: {}, + context: {}, + } as never); + + expect(response).toEqual({ error: "Failed to create sport. The slug might already exist." }); + expect(deleteCloudinaryImageByUrl).not.toHaveBeenCalled(); + }); }); diff --git a/app/routes/admin.sports.$id.tsx b/app/routes/admin.sports.$id.tsx index 2633837..e374a99 100644 --- a/app/routes/admin.sports.$id.tsx +++ b/app/routes/admin.sports.$id.tsx @@ -2,8 +2,8 @@ import { Form, Link, redirect } from "react-router"; import type { Route } from "./+types/admin.sports.$id"; import { logger } from "~/lib/logger"; -import { findSportById, updateSport } from "~/models/sport"; -import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server"; +import { findAllSports, findSportById, updateSport } from "~/models/sport"; +import { deleteSportIconIfUnused } from "~/lib/sport-icon-cleanup.server"; import { parseSportIconUrlInput } from "~/lib/sport-icon-url"; import { Button } from "~/components/ui/button"; import { Input } from "~/components/ui/input"; @@ -37,7 +37,14 @@ export async function loader({ params }: Route.LoaderArgs) { throw new Response("Sport not found", { status: 404 }); } - return { sport }; + const sports = await findAllSports(); + + return { + sport, + existingIconOptions: sports + .filter((option) => option.id !== sport.id && Boolean(option.iconUrl)) + .map((option) => ({ id: option.id, name: option.name, iconUrl: option.iconUrl as string })), + }; } export async function action({ request, params }: Route.ActionArgs) { @@ -90,8 +97,10 @@ export async function action({ request, params }: Route.ActionArgs) { }); if (existingSport?.iconUrl && existingSport.iconUrl !== updatedSport.iconUrl) { - deleteCloudinaryImageByUrl(existingSport.iconUrl).catch((error) => { - logger.error("Failed to delete replaced sport icon from Cloudinary:", error); + await deleteSportIconIfUnused({ + iconUrl: existingSport.iconUrl, + excludeSportId: params.id, + reason: "replaced", }); } @@ -99,9 +108,7 @@ export async function action({ request, params }: Route.ActionArgs) { } catch (error) { logger.error("Error updating sport:", error); if (iconUrl && iconUrl !== existingSport?.iconUrl) { - deleteCloudinaryImageByUrl(iconUrl).catch((deleteError) => { - logger.error("Failed to delete unused sport icon from Cloudinary:", deleteError); - }); + await deleteSportIconIfUnused({ iconUrl, excludeSportId: params.id, reason: "unused" }); } const message = error instanceof Error ? error.message : String(error); const isUniqueViolation = message.includes("unique") || message.includes("duplicate"); @@ -114,7 +121,7 @@ export async function action({ request, params }: Route.ActionArgs) { } export default function EditSport({ loaderData, actionData }: Route.ComponentProps) { - const { sport } = loaderData; + const { sport, existingIconOptions } = loaderData; return (
@@ -222,6 +229,7 @@ export default function EditSport({ loaderData, actionData }: Route.ComponentPro uploadUrl="/api/upload-sport-icon" initialIconUrl={sport.iconUrl} sportName={sport.name} + existingIconOptions={existingIconOptions} />
diff --git a/app/routes/admin.sports.new.tsx b/app/routes/admin.sports.new.tsx index 538bef9..09f5e5f 100644 --- a/app/routes/admin.sports.new.tsx +++ b/app/routes/admin.sports.new.tsx @@ -2,8 +2,8 @@ import { Form, Link, redirect } from "react-router"; import type { Route } from "./+types/admin.sports.new"; import { logger } from "~/lib/logger"; -import { createSport } from "~/models/sport"; -import { deleteCloudinaryImageByUrl } from "~/lib/cloudinary.server"; +import { createSport, findAllSports } from "~/models/sport"; +import { deleteSportIconIfUnused } from "~/lib/sport-icon-cleanup.server"; import { parseSportIconUrlInput } from "~/lib/sport-icon-url"; import { Button } from "~/components/ui/button"; import { Input } from "~/components/ui/input"; @@ -29,6 +29,15 @@ export function meta(): Route.MetaDescriptors { return [{ title: "New Sport - Brackt Admin" }]; } +export async function loader() { + const sports = await findAllSports(); + return { + existingIconOptions: sports + .filter((sport) => Boolean(sport.iconUrl)) + .map((sport) => ({ id: sport.id, name: sport.name, iconUrl: sport.iconUrl as string })), + }; +} + export async function action({ request }: Route.ActionArgs) { const formData = await request.formData(); const name = formData.get("name"); @@ -73,15 +82,13 @@ export async function action({ request }: Route.ActionArgs) { } catch (error) { logger.error("Error creating sport:", error); if (iconUrl) { - deleteCloudinaryImageByUrl(iconUrl).catch((deleteError) => { - logger.error("Failed to delete unused sport icon from Cloudinary:", deleteError); - }); + await deleteSportIconIfUnused({ iconUrl, reason: "unused" }); } return { error: "Failed to create sport. The slug might already exist." }; } } -export default function NewSport({ actionData }: Route.ComponentProps) { +export default function NewSport({ loaderData, actionData }: Route.ComponentProps) { return (
@@ -155,7 +162,11 @@ export default function NewSport({ actionData }: Route.ComponentProps) {
- +
{actionData?.error && (