diff --git a/src/lib/components/Valoracion.svelte b/src/lib/components/Valoracion.svelte index 9f63910..a7f5afe 100644 --- a/src/lib/components/Valoracion.svelte +++ b/src/lib/components/Valoracion.svelte @@ -16,14 +16,13 @@ import { page } from '$app/state'; interface Props { - cancionSlug: string; /** Media de 1 a 5, o `null` si todavía no ha votado nadie. */ media: number | null; /** Lo que puso quien mira, si entró y votó. */ mia?: number; } - let { cancionSlug, media, mia }: Props = $props(); + let { media, mia }: Props = $props(); const ESTRELLAS = [1, 2, 3, 4, 5]; @@ -32,32 +31,25 @@ /** Se envía mientras vuela la respuesta, para que la elección no parpadee. */ let enviando = $state(undefined); - /** - * Último voto que la acción confirmó en esta visita. - * - * No se depende solo de que el `load` vuelva a traer `mia`: entre la - * respuesta de la acción y esa recarga el radio regresaba al valor anterior, - * y daba la impresión —además de dejar el control en un estado incoherente— - * de que un voto ya emitido no se podía modificar. - */ - let confirmada = $state<{ cancionSlug: string; puntuacion: number } | undefined>(undefined); - const elegida = $derived( - enviando ?? (confirmada?.cancionSlug === cancionSlug ? confirmada.puntuacion : undefined) ?? mia - ); + /** La acción confirmada, ligada a su ruta para no arrastrarla a otro tema. */ + let confirmada = $state<{ ruta: string; puntuacion: number } | undefined>(undefined); + const propia = $derived(confirmada?.ruta === volverA ? confirmada.puntuacion : mia); + const elegida = $derived(enviando ?? propia);
- {#if usuario} + {#if usuario && propia === undefined}
{ - const puntuacion = Number(formData.get('puntuacion')); - enviando = puntuacion; + enviando = Number(formData.get('puntuacion')); return async ({ result, update }) => { try { await update({ reset: false }); - if (result.type === 'success') confirmada = { cancionSlug, puntuacion }; + if (result.type === 'success') { + confirmada = { ruta: volverA, puntuacion: Number(formData.get('puntuacion')) }; + } } finally { enviando = undefined; } @@ -83,6 +75,12 @@
+ {:else if usuario && propia !== undefined} + {:else} Promise; }) => Promise; -/* - * El componente solo necesita aquí el contrato de mejora progresiva: capturar - * el formulario, ejecutar su callback y confirmar una respuesta correcta. La - * persistencia se comprueba en el módulo del servidor; esta prueba protege el - * estado que ve y puede volver a pulsar la persona. - */ vi.mock('$app/forms', () => ({ enhance(formulario: HTMLFormElement, alEnviar: AlEnviar) { const enviar = async (evento: SubmitEvent) => { @@ -46,23 +40,20 @@ vi.mock('$app/forms', () => ({ })); describe('Valoracion', () => { - it('permite reemplazar una valoración ya emitida', async () => { - const vista = render(Valoracion, { cancionSlug: 'prometiste', media: 2, mia: 2 }); + it('convierte el primer voto confirmado en una valoración de solo lectura', async () => { + render(Valoracion, { media: null }); - const dos = page.getByRole('radio', { name: '2 de 5' }); const cuatro = page.getByRole('radio', { name: '4 de 5' }); - await expect.element(dos).toBeChecked(); - await cuatro.click(); - await expect.element(cuatro).toBeChecked(); - await expect.element(dos).not.toBeChecked(); - await expect.element(cuatro).toBeEnabled(); + await expect.element(page.getByRole('radio')).not.toBeInTheDocument(); + await expect.element(page.getByRole('img', { name: 'Tu valoración: 4 de 5' })).toBeVisible(); + }); + + it('no ofrece controles para modificar una valoración existente', async () => { + render(Valoracion, { media: 3.5, mia: 2 }); - // La confirmación local pertenece a ese tema, no al componente reutilizado - // al navegar a otra ficha de canción. - await vista.rerender({ cancionSlug: 'otra-cancion', media: 1, mia: 1 }); - await expect.element(page.getByRole('radio', { name: '1 de 5' })).toBeChecked(); - await expect.element(cuatro).not.toBeChecked(); + await expect.element(page.getByRole('radio')).not.toBeInTheDocument(); + await expect.element(page.getByRole('img', { name: 'Tu valoración: 2 de 5' })).toBeVisible(); }); }); diff --git a/src/lib/server/db/schema/interaccion.ts b/src/lib/server/db/schema/interaccion.ts index f67b500..7881d48 100644 --- a/src/lib/server/db/schema/interaccion.ts +++ b/src/lib/server/db/schema/interaccion.ts @@ -120,8 +120,8 @@ export const accesoCancionUsuario = pgTable( * Lo que una cuenta le pone a un tema, de 1 a 5. * * La clave primaria es el par cuenta-tema, asi que nadie puede votar dos veces - * lo mismo: volver a votar reemplaza el voto anterior. Eso lo garantiza la base - * de datos, no el codigo de la accion, que es donde tiene que estar. + * lo mismo. La inserción ignora el conflicto en vez de actualizarlo: una + * valoración emitida es definitiva. * * El indice por `cancion_id` es para la media: se calcula por tema, y sin el * habria que recorrer la tabla entera en cada ficha. diff --git a/src/lib/server/valoraciones-persistencia.spec.ts b/src/lib/server/valoraciones-persistencia.spec.ts new file mode 100644 index 0000000..ac2046f --- /dev/null +++ b/src/lib/server/valoraciones-persistencia.spec.ts @@ -0,0 +1,50 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const dobles = vi.hoisted(() => { + const returning = vi.fn(); + const onConflictDoNothing = vi.fn(() => ({ returning })); + const values = vi.fn(() => ({ onConflictDoNothing })); + const insert = vi.fn(() => ({ values })); + const idDeCancion = vi.fn(); + return { returning, onConflictDoNothing, values, insert, idDeCancion }; +}); + +vi.mock('./db', () => ({ db: { insert: dobles.insert } })); +vi.mock('./canciones', () => ({ idDeCancion: dobles.idDeCancion })); +vi.mock('./db/schema', () => ({ + cancion: { id: 'cancion.id', slug: 'cancion.slug' }, + valoracion: { + usuarioId: 'valoracion.usuarioId', + cancionId: 'valoracion.cancionId', + puntuacion: 'valoracion.puntuacion' + } +})); + +import { valorar } from './valoraciones'; + +describe('persistencia de valoraciones', () => { + beforeEach(() => { + vi.clearAllMocks(); + dobles.idDeCancion.mockResolvedValue('cancion-1'); + }); + + it('inserta el primer voto pero no actualiza uno existente', async () => { + dobles.returning.mockResolvedValueOnce([{ puntuacion: 4 }]).mockResolvedValueOnce([]); + + await expect(valorar('usuario-1', 'prometiste', 4)).resolves.toBe('guardada'); + await expect(valorar('usuario-1', 'prometiste', 2)).resolves.toBe('ya-valorada'); + expect(dobles.onConflictDoNothing).toHaveBeenCalledTimes(2); + expect(dobles.values).toHaveBeenNthCalledWith(2, { + usuarioId: 'usuario-1', + cancionId: 'cancion-1', + puntuacion: 2 + }); + }); + + it('no intenta guardar el voto de una canción inexistente', async () => { + dobles.idDeCancion.mockResolvedValue(undefined); + + await expect(valorar('usuario-1', 'no-existe', 4)).resolves.toBe('cancion-inexistente'); + expect(dobles.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/server/valoraciones.ts b/src/lib/server/valoraciones.ts index 929b9ae..f381108 100644 --- a/src/lib/server/valoraciones.ts +++ b/src/lib/server/valoraciones.ts @@ -1,10 +1,9 @@ /** * Valoraciones de un tema. * - * Solo valora quien tiene cuenta, y una sola vez por tema: volver a votar - * reemplaza el voto anterior en lugar de sumar otro. Eso lo impone la clave - * primaria compuesta de la tabla, no este módulo; aquí solo se escribe de - * forma que la base de datos pueda hacer su trabajo. + * Solo valora quien tiene cuenta, y una sola vez por tema. El voto es + * definitivo: la clave primaria compuesta impide duplicarlo y la inserción no + * actualiza la fila cuando ya existe. * * No hace falta haber comprado el tema para valorarlo: la web permite * escucharlo completo a calidad reducida. Exigir la compra convertiría la @@ -85,40 +84,29 @@ export async function valoracionDe( } /** - * Guarda o reemplaza el voto de una cuenta. + * Guarda el voto definitivo de una cuenta. * - * Es un `upsert` sobre la clave primaria: si ya había voto, se pisa y se anota - * cuándo. La fecha de creación no se toca, para poder distinguir un voto viejo - * corregido de uno nuevo. + * El conflicto no actualiza: así esta regla no depende de que la interfaz + * esconda los radios y también se cumple ante dos peticiones simultáneas o un + * POST construido a mano. */ export async function valorar( usuarioId: string, cancionSlug: string, puntuacion: number -): Promise { +): Promise<'guardada' | 'ya-valorada' | 'cancion-inexistente'> { if (!esPuntuacionValida(puntuacion)) { throw new Error(`Puntuación fuera de rango: ${puntuacion}`); } const cancionId = await idDeCancion(cancionSlug); - if (!cancionId) return false; + if (!cancionId) return 'cancion-inexistente'; - await db + const [insertada] = await db .insert(valoracion) .values({ usuarioId, cancionId, puntuacion }) - .onConflictDoUpdate({ - target: [valoracion.usuarioId, valoracion.cancionId], - set: { puntuacion, actualizadaEn: new Date() } - }); - return true; -} - -/** Retira el voto de una cuenta. Sin voto, el tema vuelve a su media sin él. */ -export async function retirarValoracion(usuarioId: string, cancionSlug: string): Promise { - const cancionId = await idDeCancion(cancionSlug); - if (!cancionId) return; + .onConflictDoNothing({ target: [valoracion.usuarioId, valoracion.cancionId] }) + .returning({ puntuacion: valoracion.puntuacion }); - await db - .delete(valoracion) - .where(and(eq(valoracion.usuarioId, usuarioId), eq(valoracion.cancionId, cancionId))); + return insertada ? 'guardada' : 'ya-valorada'; } diff --git a/src/routes/(sitio)/canciones/[slug]/+page.server.ts b/src/routes/(sitio)/canciones/[slug]/+page.server.ts index 8afec65..ea3927c 100644 --- a/src/routes/(sitio)/canciones/[slug]/+page.server.ts +++ b/src/routes/(sitio)/canciones/[slug]/+page.server.ts @@ -8,13 +8,7 @@ import { portadaDeCancion } from '$lib/server/catalogo'; import { componer } from '$lib/server/markdown'; -import { - aPuntuacion, - resumenDeCancion, - retirarValoracion, - valoracionDe, - valorar -} from '$lib/server/valoraciones'; +import { aPuntuacion, resumenDeCancion, valoracionDe, valorar } from '$lib/server/valoraciones'; import { alternarCancionEnLista, crearListaConCancion, @@ -74,29 +68,19 @@ export const actions = { if (!locals.usuario) { return fail(401, { valoracion: { error: 'Entra con tu cuenta para valorar.' } }); } - // Un slug que no existe no se valora. Desde la fase 3 la tabla tiene - // clave foránea contra `cancion`, así que esto es lo que convierte un - // error de base de datos en un 404 con su mensaje. - if (!(await obtenerCancion(params.slug))) { - return fail(404, { valoracion: { error: 'Ese tema no existe.' } }); - } - const puntuacion = aPuntuacion((await request.formData()).get('puntuacion')); if (puntuacion === undefined) { return fail(400, { valoracion: { error: 'La valoración va de 1 a 5.' } }); } - await valorar(locals.usuario.id, params.slug, puntuacion); - return { valoracion: { puntuacion } }; - }, - - retirar: async ({ params, locals }) => { - if (!locals.usuario) { - return fail(401, { valoracion: { error: 'Entra con tu cuenta para valorar.' } }); + const resultado = await valorar(locals.usuario.id, params.slug, puntuacion); + if (resultado === 'cancion-inexistente') { + return fail(404, { valoracion: { error: 'Ese tema no existe.' } }); } - - await retirarValoracion(locals.usuario.id, params.slug); - return { valoracion: { puntuacion: undefined } }; + if (resultado === 'ya-valorada') { + return fail(409, { valoracion: { error: 'Ya has valorado este tema.' } }); + } + return { valoracion: { puntuacion } }; }, alternarLista: async ({ params, request, locals }) => { diff --git a/src/routes/(sitio)/canciones/[slug]/+page.svelte b/src/routes/(sitio)/canciones/[slug]/+page.svelte index 04fc1d1..744425f 100644 --- a/src/routes/(sitio)/canciones/[slug]/+page.svelte +++ b/src/routes/(sitio)/canciones/[slug]/+page.svelte @@ -159,11 +159,7 @@ {/if}
- +