# Plan: 10 deudas técnicas por ROI (lote 3)

> Fecha: 2026-06-09
> Criterio: ROI = severidad / coste. MEDIA×BAJO (score 2) primero; ALTA×MEDIO (score 1.5) al final.
> Estado: PENDIENTE

---

## Selección y orden

| # | ID | Descripción | Severidad | Coste | Score |
|---|---|---|---|---|---|
| 1 | DT-035 | Métodos duplicados `UsuarioRepository` | MEDIA | BAJO | 2.0 |
| 2 | DT-036 | Validación en setter `GrupoMiembro` | MEDIA | BAJO | 2.0 |
| 3 | DT-038 | `htmlspecialchars` inline → `TelegramService` | MEDIA | BAJO | 2.0 |
| 4 | DT-018 | `ColeccionService` doble flush → uno solo | MEDIA | BAJO | 2.0 |
| 5 | DT-031 | Patrón try/catch duplicado en `EventoController` | MEDIA | BAJO | 2.0 |
| 6 | DT-032 | Verificación membresía duplicada en `EventoController` | MEDIA | BAJO | 2.0 |
| 7 | DT-037 | Transformación de datos en `ColeccionController` | MEDIA | BAJO | 2.0 |
| 8 | DT-020 | Queries sin índices explícitos en BD | MEDIA | BAJO | 2.0 |
| 9 | DT-007 | `TelegramBotRepository` inyectado en `GrupoController` | ALTA | MEDIO | 1.5 |
| 10 | DT-005 | Lógica de negocio en `EventoController` | ALTA | MEDIO | 1.5 |

---

## DT-035 — Métodos duplicados en `UsuarioRepository`

**Problema:** `usernameTagExists()` y `emailExists()` repiten el mismo patrón `findOneBy([...]) !== null`.

**Archivo:** `src/Repository/UsuarioRepository.php` (~L45-52)

### Cambio

Añadir método privado y simplificar los dos existentes:

```php
public function usernameTagExists(string $tag): bool
{
    return $this->existsBy('usernameTag', $tag);
}

public function emailExists(string $email): bool
{
    return $this->existsBy('email', $email);
}

private function existsBy(string $field, mixed $value): bool
{
    return $this->findOneBy([$field => $value]) !== null;
}
```

**Riesgo:** Ninguno. El comportamiento es idéntico.

---

## DT-036 — Validación en setter `GrupoMiembro::setPermisos()`

**Problema:** El setter valida silenciosamente permisos inválidos filtrándolos. La validación en setters de entidades es frágil y difícil de testear en aislamiento.

**Archivo:** `src/Entity/GrupoMiembro.php` (~L91-98)

### Cambio

Hacer el setter dumb; la validación es responsabilidad del llamador (`GrupoPermisoService`):

```php
/** @param string[] $permisos */
public function setPermisos(array $permisos): static
{
    $this->permisos = $permisos;
    return $this;
}
```

Verificar con `grep -rn "setPermisos"` que todos los llamadores pasan valores ya validados (valores de `PermisoGrupo::cases()`). Si alguno no lo hace, añadir la validación allí antes de aplicar este cambio.

**Riesgo:** Bajo. `grantPermission()` y `revokePermission()` no usan `setPermisos()` — operan sobre `$this->permisos` directamente y ya son seguros.

---

## DT-038 — `htmlspecialchars` inline en `GrupoController`

**Problema:** La sanitización para Telegram se hace manualmente en el controlador con `htmlspecialchars()` en lugar de estar encapsulada en `TelegramService`.

**Archivos:**
- `src/Controller/GrupoController.php` (~L568)
- `src/Service/TelegramService.php`

### Paso 1 — Añadir método en `TelegramService`

```php
public function sanitizeHtml(string $text): string
{
    return htmlspecialchars($text, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
}
```

### Paso 2 — Actualizar `GrupoController` (~L568)

Reemplazar:
```php
$mensajeSanitizado = htmlspecialchars($mensajeRaw, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
```
por:
```php
$mensajeSanitizado = $telegramService->sanitizeHtml($mensajeRaw);
```

**Riesgo:** Ninguno.

---

## DT-018 — `ColeccionService` doble flush

**Problema:** Dos `$this->em->flush()` dentro de `sincronizarDesdeBgg()`: uno tras el loop de importación (L55) y otro dentro del bloque try de enriquecimiento (L75). Viola el principio de un único flush al final de la operación.

**Archivo:** `src/Service/ColeccionService.php` (~L40-79)

### Cambio

Mover ambos flushes a un único `flush()` al final. El enriquecimiento va dentro de un try/catch que no necesita flush propio: si falla, el catch lo absorbe y el flush final persiste lo que haya podido enriquecer antes del error.

```php
$this->coleccionRepository->deleteByUsuario($usuario);

$importados = 0;
$bggIdsGuardados = [];

foreach ($juegosData as $data) {
    $juego = $this->upsertJuego($data);
    $this->agregarJuego($juego, $usuario);
    $importados++;
    $bggIdsGuardados[] = $juego->getBggId();
}

if (!empty($bggIdsGuardados)) {
    try {
        $detalles = $this->bggApi->obtenerDetallesJuegos($bggIdsGuardados);
        foreach ($detalles as $bggId => $detalle) {
            $juego = $this->juegoRepository->findOneBy(['bggId' => $bggId]);
            if ($juego === null) {
                continue;
            }
            if ($detalle['avg_weight'] !== null) {
                $juego->setAvgWeight((string) $detalle['avg_weight']);
            }
            $juego->setMinAge($detalle['min_age']);
            if ($detalle['designers'] !== null) {
                $juego->setDesigners(substr($detalle['designers'], 0, 500));
            }
            $juego->setIsExpansion($detalle['is_expansion']);
        }
    } catch (\Exception) {
        // Los detalles son opcionales; no revertir la importación
    }
}

$this->em->flush();

return $importados;
```

**Riesgo:** Bajo. Si `obtenerDetallesJuegos()` falla a mitad, el flush al final persiste la colección base más los detalles que se hayan podido actualizar antes del error. Mismo comportamiento neto que antes; solo cambia el punto exacto del commit.

---

## DT-031 — Patrón try/catch duplicado en `EventoController`

**Problema:** Los métodos `confirmar`, `revertir`, `cerrar` y `recordatorio` repiten cuatro veces el mismo bloque:
```php
try {
    $eventoService->algúnMétodo(...);
    $this->addFlash('success', '...');
} catch (\Exception $e) {
    $this->addFlash('error', $e->getMessage());
}
return $this->redirectToRoute('app_evento_show', ['id' => $evento->getId()]);
```

**Archivo:** `src/Controller/EventoController.php`

### Cambio

Añadir método privado al final de la clase:

```php
private function tryEventAction(callable $action, string $successMsg, int $eventoId): Response
{
    try {
        $action();
        $this->addFlash('success', $successMsg);
    } catch (\Exception $e) {
        $this->addFlash('error', $e->getMessage());
    }
    return $this->redirectToRoute('app_evento_show', ['id' => $eventoId]);
}
```

Reemplazar los cuatro bloques. Ejemplo para `confirmar()`:
```php
return $this->tryEventAction(
    fn() => $eventoService->confirmarFecha($evento, $fechaPropuesta, $usuario),
    'Fecha confirmada.',
    (int) $evento->getId(),
);
```

Ídem para `revertir()`, `cerrar()` y `recordatorio()`.

**Riesgo:** Ninguno.

---

## DT-032 — Verificación de membresía duplicada en `EventoController`

**Problema:** `show()` (L143), `votar()` (L171) y `coleccionMiembro()` (L312) repiten el mismo check:
```php
if (!$grupoMiembroRepository->isMember($evento->getGrupo(), $usuario)) {
    throw $this->createAccessDeniedException();
}
```

**Archivo:** `src/Controller/EventoController.php`

### Cambio

Añadir método privado:

```php
private function requireMembership(
    Evento $evento,
    \App\Entity\Usuario $usuario,
    GrupoMiembroRepository $repo,
): void {
    if (!$repo->isMember($evento->getGrupo(), $usuario)) {
        throw $this->createAccessDeniedException();
    }
}
```

Reemplazar los tres bloques por la llamada:
```php
$this->requireMembership($evento, $usuario, $grupoMiembroRepository);
```

**Riesgo:** Ninguno.

---

## DT-037 — Transformación de datos en `ColeccionController`

**Problema:** El loop que convierte `PuntuacionJuego[]` a `array<int, int>` (ID → puntuación) está inline en `ColeccionController::index()` (~L38-43) y se duplica en `puntuar()` (~L101-103). Lógica de query que pertenece al repositorio.

**Archivos:**
- `src/Repository/PuntuacionJuegoRepository.php`
- `src/Controller/ColeccionController.php`

### Paso 1 — Añadir método en `PuntuacionJuegoRepository`

```php
/** @return array<int, int> */
public function findMappedByJuegoId(Grupo $grupo, Usuario $usuario): array
{
    $mapped = [];
    foreach ($this->findByGrupoAndUsuario($grupo, $usuario) as $p) {
        $mapped[$p->getJuego()->getId()] = $p->getPuntuacion();
    }
    return $mapped;
}
```

### Paso 2 — Actualizar `ColeccionController`

En `index()`, reemplazar el bloque foreach (~L38-43) por:
```php
$misPuntuaciones = $grupo !== null
    ? $puntuacionRepo->findMappedByJuegoId($grupo, $usuario)
    : [];
```

En `puntuar()`, reemplazar el bloque foreach (~L101-103) por:
```php
'misPuntuaciones' => $puntuacionRepo->findMappedByJuegoId($grupo, $usuario),
```

**Riesgo:** Ninguno.

---

## DT-020 — Queries sin índices explícitos en BD

**Problema:** Queries con GROUP BY, HAVING y WHERE sobre campos que probablemente no tienen índices de BD explícitos. Impacta a `PuntuacionJuegoRepository::getRankingPonderadoByGrupo()` y `GrupoRepository::findGruposSinActividad()`.

**Acción:** Crear una migración con los índices faltantes.

```bash
bin/console doctrine:migrations:generate
```

En el archivo generado, añadir en `up()`:

```php
// puntuacion_juego: queries filtran por grupo_id y agrupan por juego_id
$this->addSql('CREATE INDEX idx_puntuacion_grupo ON puntuacion_juego (grupo_id)');
$this->addSql('CREATE INDEX idx_puntuacion_juego ON puntuacion_juego (juego_id)');

// eventos: HAVING sobre fecha_creacion en findGruposSinActividad
$this->addSql('CREATE INDEX idx_evento_fecha_creacion ON eventos (fecha_creacion)');

// grupo_miembros: JOIN/WHERE por usuario_id (la FK no garantiza índice en MySQL)
$this->addSql('CREATE INDEX idx_grupomiembro_usuario ON grupo_miembros (usuario_id)');
```

Y en `down()`:

```php
$this->addSql('DROP INDEX idx_puntuacion_grupo ON puntuacion_juego');
$this->addSql('DROP INDEX idx_puntuacion_juego ON puntuacion_juego');
$this->addSql('DROP INDEX idx_evento_fecha_creacion ON eventos');
$this->addSql('DROP INDEX idx_grupomiembro_usuario ON grupo_miembros');
```

> **Nota:** verificar con `SHOW INDEX FROM <tabla>` que los índices no existan ya antes de añadirlos.

Ejecutar: `bin/console doctrine:migrations:migrate`

**Riesgo:** Bajo. Los índices solo aceleran; no cambian el comportamiento.

---

## DT-007 — `TelegramBotRepository` inyectado directamente en `GrupoController`

**Problema:** `GrupoController::telegramConfig()` inyecta `TelegramBotRepository` para dos operaciones: encontrar un bot activo por ID y listar todos los bots activos. Los repositorios no deben inyectarse en controladores; deben pasar por un servicio.

**Archivos:**
- `src/Service/TelegramService.php`
- `src/Controller/GrupoController.php` (~L311-387)

### Paso 1 — Añadir repositorio e inyección en `TelegramService`

En el constructor de `TelegramService`, añadir:

```php
use App\Repository\TelegramBotRepository;

public function __construct(
    private readonly HttpClientInterface   $httpClient,
    private readonly TelegramBotTokenStore $tokenStore,
    private readonly TelegramBotRepository $botRepository,
    #[Autowire(service: 'monolog.logger.integration')] private readonly LoggerInterface $logger,
) {}
```

Añadir dos métodos públicos:

```php
public function findActiveBot(int $id): ?TelegramBot
{
    $bot = $this->botRepository->find($id);
    return ($bot !== null && $bot->isActivo()) ? $bot : null;
}

/** @return TelegramBot[] */
public function findActiveBots(): array
{
    return $this->botRepository->findActivos();
}
```

### Paso 2 — Actualizar `GrupoController::telegramConfig()`

Eliminar el parámetro `TelegramBotRepository $telegramBotRepository` de la firma del método.

Reemplazar en el cuerpo:
```php
// antes:
$bot = $telegramBotRepository->find($botId);
if ($bot === null || !$bot->isActivo()) {
    return new JsonResponse(['ok' => false, 'error' => 'Bot no encontrado o inactivo.']);
}
```
por:
```php
$bot = $telegramService->findActiveBot($botId);
if ($bot === null) {
    return new JsonResponse(['ok' => false, 'error' => 'Bot no encontrado o inactivo.']);
}
```

Y:
```php
// antes:
'bots' => $telegramBotRepository->findActivos(),
```
por:
```php
'bots' => $telegramService->findActiveBots(),
```

Eliminar el `use App\Repository\TelegramBotRepository;` del controlador si no se usa en otro método.

**Riesgo:** Bajo.

---

## DT-005 — Lógica de negocio en `EventoController::nuevo()`

**Problema:** `EventoController::nuevo()` contiene:
1. Validación de que el juego seleccionado pertenece al grupo (~L96-99).
2. Preparación del JSON de todos los juegos del grupo para la plantilla (~L120-123).
3. Extracción manual de datos del formulario con `$form->getData()['titulo']` (relacionado con DT-046, formulario sin `data_class`).

Los puntos 1 y 2 son responsabilidad de `EventoService`.

**Archivos:**
- `src/Service/EventoService.php`
- `src/Controller/EventoController.php` (~L65-131)
- `src/Repository/JuegoRepository.php`

### Paso 1 — Añadir `EventoService::resolveJuego()`

```php
public function resolveJuego(Grupo $grupo, int $juegoId): ?Juego
{
    if ($juegoId <= 0) {
        return null;
    }
    $juego = $this->juegoRepository->find($juegoId);
    if ($juego === null) {
        return null;
    }
    $idsGrupo = array_map(
        fn(Juego $j) => $j->getId(),
        $this->juegoRepository->findByGrupo($grupo->getId()),
    );
    return in_array($juego->getId(), $idsGrupo, true) ? $juego : null;
}
```

Requiere que `EventoService` tenga `JuegoRepository` inyectado. Comprobar si ya lo tiene; si no, añadirlo al constructor. Si supera 5 dependencias, registrar como deuda adicional (DT-006 ya cubre el SRP de este servicio).

### Paso 2 — Añadir `JuegoRepository::findForGrupoAsJson()`

Mueve la serialización para la plantilla al repositorio:

```php
/** @return string JSON con [{id, nombre, imagenUrl}] */
public function findForGrupoAsJson(int $grupoId): string
{
    $juegos = $this->findByGrupo($grupoId);
    return (string) json_encode(
        array_map(
            static fn(Juego $j) => [
                'id'       => $j->getId(),
                'nombre'   => $j->getNombre(),
                'imagenUrl' => $j->getImagenUrl(),
            ],
            $juegos,
        ),
        JSON_UNESCAPED_UNICODE,
    );
}
```

### Paso 3 — Actualizar `EventoController::nuevo()`

Reemplazar (~L96-99):
```php
$juegoId = (int) $request->request->get('juego_id', 0);
$juego   = $juegoId > 0 ? $juegoRepository->find($juegoId) : null;
if ($juego !== null && !in_array(...)) {
    $juego = null;
}
```
por:
```php
$juego = $eventoService->resolveJuego($grupo, (int) $request->request->get('juego_id', 0));
```

Reemplazar (~L120-123):
```php
$todosJuegosJson = json_encode(array_map(
    static fn(Juego $j) => [...],
    $juegoRepository->findByGrupo($grupo->getId()),
));
```
por:
```php
$todosJuegosJson = $juegoRepository->findForGrupoAsJson($grupo->getId());
```

Eliminar el import `use App\Entity\Juego;` del controlador si ya no se usa directamente.

**Riesgo:** Bajo. El comportamiento de `resolveJuego` es equivalente al inline actual.

---

## Orden de implementación

1. DT-035 — `UsuarioRepository::existsBy()` — 8 líneas, sin dependencias
2. DT-036 — `GrupoMiembro::setPermisos()` dumb — 3 líneas (grep llamadores primero)
3. DT-038 — `TelegramService::sanitizeHtml()` — 4 líneas en el servicio + 1 en el controlador
4. DT-031 — `EventoController::tryEventAction()` — método privado + 4 sustituciones
5. DT-032 — `EventoController::requireMembership()` — método privado + 3 sustituciones
6. DT-018 — `ColeccionService` flush único — reestructurar ~15 líneas
7. DT-037 — `PuntuacionJuegoRepository::findMappedByJuegoId()` — método nuevo + 2 sustituciones
8. DT-020 — Migración de índices — generar + ejecutar
9. DT-007 — Repo en `TelegramService`, eliminar del `GrupoController`
10. DT-005 — `EventoService::resolveJuego()` + `JuegoRepository::findForGrupoAsJson()`