jkuijperm/expenses_manager
1
0
Fork 0
Code Issues 13 Pull Requests Actions Packages Projects Releases Wiki Activity

TODO — Revisión de código #13

New Issue
Closed
opened 2026-07-08 08:30:44 +00:00 by jkuijperm · 0 comments
jkuijperm commented 2026-07-08 08:30:44 +00:00
Owner
Copy Link

TODO — Revisión de código (expenses_manager)

Revisión completa del repositorio expenses_manager (Django). Incluye bugs reales, riesgos de seguridad, huecos funcionales y deuda técnica encontrados al leer models.py, views.py, forms.py, urls.py, settings.py, el comando seed_demo, las plantillas, los tests, el Jenkinsfile y requirements.txt.

Los puntos marcados con 🔴 son bugs o riesgos reales (algo se rompe o es inseguro). Los marcados con 🟡 son mejoras de robustez/mantenibilidad, no bugs confirmados.

Progreso

Resueltos hasta ahora: 28 de 29 puntos.

  • Bug de fuel_create sin return en caso de POST inválido
  • goal_delete sin protección de POST
  • fuel_create sin filtrar categoría "gasolina" por owner
  • Cuentas inactivas sin distinción visual en account_list.html
  • Falta la plantilla settings/index.html (enlace roto)
  • Typo en ExpenseForm.Meta.widgets (checkboxes de tags no se aplicaban)
  • seed_demo no persiste is_staff/is_superuser
  • SECRET_KEY con fallback inseguro (ahora falla explícitamente en prod)
  • STATIC_ROOT (ya existía en prod, ahora unificado en el repo)
  • Hardening de producción (cookies secure, SSL redirect, proxy header)
  • LOGGING a stdout para docker logs
  • SQLite vs Postgres (unificado por DB_ENGINE; prod ya usaba Postgres)
  • Drivers de Postgres en requirements.txt (revisado: sí se usan)
  • ValueError en el filtro de tags de expense_list (500 con ?tag=abc)
  • except: genérico en el dashboard (ahora logueado con traceback)
  • Imports muertos y ruido en views.py
  • Registro duplicado de las auth urls en urls.py raíz
  • Colisión de prefijo /accounts/ (cuentas financieras → /finance-accounts/)
  • CRUD de categorías completo (category_edit + category_delete)
  • Ciclos en CategoryForm (una categoría ya no puede ser su propio padre)
  • fuel_delete + edición desde el listado de repostajes y vuelta al origen
  • Goal replanteado: tipos pago / presupuesto / ahorro, con periodo y fecha de inicio
  • monthly_balance() (ya estaba hecho) y monthly_net() optimizados
  • Cobertura de tests (de 9 a 64 tests, con regresiones de todo lo corregido)
  • FuelEntryForm refactorizado a ModelForm (deja de duplicar campos de Expense)
  • Decisión de soft-delete de cuentas documentada en el código
  • Eliminado el driver psycopg2-binary sobrante de requirements.txt
  • Recuperación de contraseña resuelta por diseño (reset por admin + pantalla informativa)

Hito importante: el settings.py de producción del NAS y el del repo estaban divergidos (Postgres, whitenoise, CSRF_TRUSTED_ORIGINS solo existían en el NAS). Ahora hay un único settings.py versionado controlado por variables de entorno (.env), desplegado y funcionando en producción.

Único punto sin cerrar: la mejora del pipeline de Jenkins (build de la imagen Docker + despliegue automático desde main), aplazada a propósito porque el despliegue se hace a mano de momento. La contraseña hardcodeada de seed_demo queda como riesgo asumido (uso local, mitigada con la guarda de DEBUG).

Bugs y riesgos con prioridad alta

  • 🔴 fuel_create no responde si el formulario POST es inválido. El return render(...) del caso GET está indentado dentro del else, así que si llega un POST con datos inválidos la función no llega a ningún return y Django lanza un error (ValueError: didn't return an HttpResponse). Hay que sacar ese render fuera del if/else, como en el resto de vistas. (Resuelto en rama dev.)

  • 🔴 goal_delete borra el objetivo con una simple petición GET. A diferencia de expense_delete, tag_delete, account_delete e income_delete, esta vista no comprueba request.method == "POST". Cualquier enlace, precarga del navegador o bot puede borrar un objetivo sin confirmación. Añadir la comprobación de POST (y su plantilla de confirmación, como en el resto de recursos). (Resuelto en rama dev: se añadió goals/confirm_delete.html.)

  • 🔴 Falta la plantilla settings/index.html. Confirmado revisando el repo: no existe ningún directorio templates/settings/. La ruta /settings/ (vista settings_index) lanza TemplateDoesNotExist en cuanto se visita. Es un enlace roto ahora mismo en producción. (Resuelto en rama dev: se creó settings/index.html como página hub con enlaces a Categorías/Etiquetas/Objetivos, ya que el menú de navegación no la usaba directamente.)

  • 🔴 fuel_create busca la categoría "gasolina" sin filtrar por usuario. Usa Category.objects.get(slug="gasolina") sin owner=request.user. Como el slug no es único entre usuarios (el unique_together de Category incluye owner), esto puede lanzar MultipleObjectsReturned si dos usuarios tienen esa categoría, o coger la categoría de otro usuario. (Resuelto en rama dev con get_or_create(slug="gasolina", owner=request.user, defaults={"name": "Gasolina"}).)

  • 🔴 Typo en ExpenseForm.Meta.widgets: la clave es "widget" en vez de "tags". Como "widget" no es un campo del formulario, el CheckboxSelectMultiple() pensado para las tags nunca se aplica. El campo tags se renderiza con el select múltiple por defecto, no con checkboxes. (Resuelto en rama dev: se corrigió la clave y, de paso, se rediseñó expense_form.html para renderizar las tags como "chips" en fila con scroll, en vez de la lista vertical por defecto de Django.)

  • 🔴 Comando seed_demo crea un superusuario con contraseña débil hardcodeada (demo1234). No hay ninguna guarda que impida ejecutar este comando en producción. Si se ejecuta ahí por error, queda un superusuario con credenciales conocidas. (Aceptado como riesgo asumido: se usa solo en local. Mitigado parcialmente por la guarda de DEBUG añadida en el punto anterior, que impide ejecutar el comando si DEBUG=False.)

  • 🔴 seed_demo no persiste is_staff/is_superuser si el usuario demo ya existe. En la rama else (usuario ya creado) se asignan esos atributos en el objeto en memoria pero nunca se llama a .save(), así que no se guardan en la base de datos. (Resuelto en rama dev: se movió la asignación de is_staff/is_superuser antes del .save() en ambas ramas — también afectaba a la rama created, que tampoco los persistía. Además se añadió una guarda if not settings.DEBUG: raise CommandError(...) para que el comando no pueda ejecutarse en producción.)

Seguridad y configuración de despliegue (settings.py)

  • 🔴 SECRET_KEY tiene un fallback inseguro hardcodeado ('fallback-secret-key-for-dev') que se usa silenciosamente si la variable de entorno no está definida. Si el .env falla al desplegar en el NAS, la app arranca igualmente con una clave insegura y conocida, sin avisar. Mejor que falle explícitamente si SECRET_KEY no está definido en producción. (Resuelto: el settings.py unificado lanza ImproperlyConfigured si falta SECRET_KEY con DEBUG=False; en dev usa un fallback explícitamente marcado como inseguro. Desplegado en producción.)

  • 🟡 No hay STATIC_ROOT definido. Necesario para collectstatic en un despliegue real vía Docker/gunicorn (sin runserver). Revisar cómo se están sirviendo los estáticos ahora mismo en el contenedor. (Resuelto: ya existía en el settings.py de producción del NAS —divergido del repo— junto con whitenoise; ahora está unificado y versionado en el repo.)

  • 🟡 Faltan ajustes de hardening para producción: CSRF_TRUSTED_ORIGINS, SESSION_COOKIE_SECURE, CSRF_COOKIE_SECURE, SECURE_SSL_REDIRECT. Relevante porque el dominio finanzas.kuijper.es probablemente está detrás de un reverse proxy HTTPS. (Resuelto: el settings.py unificado activa SESSION_COOKIE_SECURE, CSRF_COOKIE_SECURE, SECURE_SSL_REDIRECT y SECURE_PROXY_SSL_HEADER cuando DEBUG=False; CSRF_TRUSTED_ORIGINS y ALLOWED_HOSTS van por variable de entorno. Desplegado y verificado tras el reverse proxy nginx de Synology.)

  • 🟡 No hay configuración de LOGGING. Con DEBUG=False en producción, los errores no quedan registrados en ningún sitio visible, dificultando el diagnóstico de fallos reales (como el de settings/index.html de arriba). (Resuelto: LOGGING a consola/stdout, visible con docker-compose logs web.)

  • 🟡 SQLite como base de datos en un despliegue de uso continuo. Válido para uso personal, pero conviene documentar la estrategia de backups y tener en cuenta las limitaciones de concurrencia si el uso crece. (Punto revisado: producción usaba en realidad PostgreSQL en un settings.py divergido en el NAS. Ahora unificado: DB_ENGINE=postgresql por env en el NAS, SQLite por defecto en local. Queda como tarea aparte documentar la estrategia de backups de Postgres.)

  • 🟡 Falta el flujo de "olvidé mi contraseña". Solo está implementado el cambio de contraseña estando ya logueado (password_change_*.html). No existen las plantillas password_reset_*.html que requieren las URLs de django.contrib.auth, y tampoco hay EMAIL_BACKEND configurado para poder enviar el email de recuperación. (Decisión consciente de diseño: la app la usan 2 personas y no se monta recuperación por email para no mantener infraestructura de correo. El reset lo hace el admin bajo petición con docker-compose exec web python manage.py changepassword <usuario>. En vez de dejar el hueco, se añadió una pantalla informativa registration/password_help.html (ruta password-help/ vía TemplateView, enlazada desde el login) que indica al usuario que contacte con el administrador — evita cualquier TemplateDoesNotExist y da una salida clara.)

urls.py (proyecto raíz)

  • 🟡 Colisión de prefijo /accounts/. Se registra path('accounts/', include(django.contrib.auth.urls)) (login/logout/password cambio) y, a la vez, tu propia app usa /accounts/, /accounts/new/, etc. para el CRUD de cuentas financieras (montado vía path('', include('expenses.urls'))). Ahora mismo no colisionan literalmente porque las rutas de auth son más específicas, pero es un diseño frágil: dos conceptos distintos ("cuenta de usuario" y "cuenta financiera") comparten namespace de URL. (Resuelto en rama dev: las cuentas financieras pasan a /finance-accounts/.... Los name= de las rutas no cambiaron, así que ninguna plantilla necesitó tocarse.)

  • 🟡 Registro duplicado de las auth urls. Hay dos líneas que registran lo mismo: path('accounts/', include(urls)) (importando el módulo django.contrib.auth.urls directamente) y path('accounts/', include('django.contrib.auth.urls')) (por string). Una de las dos sobra. (Resuelto en rama dev: se conserva solo la forma por string, antes de expenses.urls para mantener la precedencia actual, y se eliminó el import from django.contrib.auth import urls que quedaba sin uso.)

views.py — robustez

  • 🟡 except: genérico en dashboard (bloque de accounts_charts al llamar a acc.monthly_balance(selected_year)). Silencia cualquier error real, dificultando el debug. Capturar la excepción concreta esperada, o al menos loguearla. (Resuelto: except Exception + logger.exception(...) con id de cuenta y año. El dashboard sigue sin romperse, pero el traceback aparece ahora en docker-compose logs web.)

  • 🟡 tag_ids = [int(t) for t in tag_ids] en expense_list no maneja ValueError. Un parámetro tag no numérico en la URL rompe la vista con un error 500, a diferencia del resto de filtros que usan el helper _get_int (que sí es seguro). (Resuelto: se filtran los valores no numéricos reutilizando _get_int — un valor inválido se ignora en vez de romper la vista, mismo comportamiento que el resto de filtros.)

  • 🟡 Inconsistencia en el borrado de recursos. account_delete hace soft-delete (active=False), mientras que expense_delete, tag_delete e income_delete borran de verdad. Puede ser intencional (para no romper históricos de saldo), pero merece una decisión explícita y, si es así, documentarla en el código. (Resuelto en rama dev: decisión confirmada como intencional y documentada con un docstring en account_delete que explica por qué las cuentas se desactivan en vez de borrarse —preservar los históricos de saldo de gastos/ingresos que la referencian—. La parte de UX (distinguir inactivas en el listado) ya se había resuelto por separado.)

  • 🟡 Las cuentas inactivas (active=False) no se distinguían visualmente en account_list.html. Al "eliminar" una cuenta (soft-delete), esta seguía apareciendo en el listado sin ningún cambio visible más allá de un True/False en crudo en una columna poco visible — daba la sensación de que el borrado "no funcionaba". (Resuelto en rama dev: columna "Estado" con badge Activa/Inactiva, fila atenuada con CSS .row-inactive, y el botón "Eliminar" ya no se muestra para cuentas ya inactivas.)

  • 🟡 Imports sin usar / ruido en views.py: truediv, django.template.context, is_valid_ipv6_address, y una línea comentada con un import mal escrito (dateutli en vez de dateutil). (Resuelto: eliminados esos cuatro, más django.contrib.auth.login (tampoco se usaba), y reordenado el bloque de imports al convenio stdlib → django → locales. En el mismo commit se limpiaron el dict MONTHS sin usar, la doble asignación by_category_qs = by_category, la variable chart_type que nunca llegaba al contexto, y se subió import calendar al principio del archivo.)

  • 🟡 category_list no tiene category_edit ni category_delete, a diferencia de tag/account/goal, que sí tienen el CRUD completo. Probablemente pendiente de implementar. (Resuelto en rama dev: añadidas ambas vistas con sus rutas y plantillas (categories/form.html y categories/confirm_delete.html), más la columna de acciones en categories/list.html. El borrado captura ProtectedError (categorías con gastos no se pueden borrar) y avisa en la confirmación de las subcategorías y objetivos que se eliminarían en cascada.)

  • 🟡 Falta fuel_delete. El recurso fuel tiene create/list/edit pero no borrado, a diferencia del resto de recursos del proyecto. (Resuelto en rama dev: añadida la vista fuel_delete (recibe el pk del Expense, como fuel_edit) con su ruta y fuel/confirm_delete.html; borrar el gasto elimina el FuelEntry en cascada. Además se añadieron los enlaces Editar/Eliminar en fuel/list.html, que antes no existían, y un helper _redirect_back con ?next= validado (url_has_allowed_host_and_scheme) para volver a la pantalla de origen — gastos o repostajes — tras editar o borrar. En expense_list.html los enlaces se eligen según expense.fuel_data (no por el slug de categoría, que puede quedar desfasado).)

models.py

  • 🟡 Account.monthly_balance() recorre mes a mes con queries repetidas. Funciona, pero es optimizable con una sola agregación en vez de iterar los 12 meses. (Punto obsoleto: ya lo resolviste en el commit de junio "Fixed function monthly_balance" — el método actual hace 4 consultas y recorre los meses en memoria. En esta revisión se detectó que monthly_net() sí conservaba el patrón viejo (24 consultas por cuenta y año) y se optimizó igual, quedando en 2.)

  • 🟡 Goal.progress() no filtra por cuenta ni por rango de fechas, solo por categoría. Si la intención es que la meta sea mensual o esté ligada a una cuenta concreta, el cálculo actual no lo refleja — habría que decidir el comportamiento esperado y ajustarlo. (Resuelto replanteando el modelo entero: Goal gana un campo kind con tres tipos —pago/deuda, presupuesto y ahorro—, más period (mensual/anual, para presupuestos), start_date, account (para ahorro) e include_subcategories. progress() despacha según el tipo y acota por fecha; se añadieron bar_width() (evita que la barra desborde al pasar del 100%) y progress_state() (colorea según el tipo: en un presupuesto acercarse al límite es alarma, no logro). GoalForm expone los campos nuevos y valida qué es obligatorio según el tipo. Migración 0010_... — solo AddField/AlterField, sin pérdida de datos. El ahorro se mide provisionalmente con el saldo de la cuenta asociada: es la única rama a tocar cuando llegue el módulo de inversiones.)

forms.py

  • 🟡 CategoryForm no excluye la propia categoría del queryset de parent al editar. Permite que una categoría se asigne a sí misma como padre, o crear ciclos (A → B → A), sin validación que lo impida. (Resuelto en rama dev: al editar, el desplegable excluye la propia categoría y toda su descendencia (a cualquier profundidad), más un clean_parent como validación de respaldo. Ojo: durante la implementación se produjo un fallo temporal en el que parent se guardaba siempre como None; el síntoma clásico es que clean_parent no devuelva el valor. Conviene un test que fije este comportamiento.)

  • 🟡 FuelEntryForm es un forms.Form plano que duplica a mano los campos de Expense (fecha, importe, cuenta). Funciona, pero cualquier cambio en esos modelos hay que replicarlo manualmente aquí. (Resuelto en rama dev: convertido en ModelForm de Expense (campos date/amount/account derivados del modelo) + los campos propios de FuelEntry (odometer/liters) añadidos aparte, con precarga al editar. fuel_create/fuel_edit simplificados con form.save(commit=False) en lugar de copiar campo por campo. Cubierto por test_fuel.py.)

Tests

  • 🟡 Cobertura de tests muy desigual. Existían tests para Expense básico, Income, dashboard y un sanity check, pero ninguno para Account, FuelEntry, Goal, Tag ni la lógica de slug de Category. (Resuelto: suite ampliada a 64 tests en verde. Nuevo conftest.py con fixtures (user, auth_client, account, category) y nuevos archivos test_accounts.py, test_fuel.py, test_goals.py, test_categories.py, test_expense_list.py y test_deletes.py. Incluye tests de regresión para todos los bugs corregidos en esta revisión: POST inválido en fuel_create, borrados que exigen POST, ?tag= no numérico, persistencia de parent en CategoryForm, PROTECT al borrar categorías con gastos y borrado en cascada de FuelEntry. Se arregló además test_dashboard_filters_by_year, que llevaba meses fallando: la clave de contexto by_month se renombró a chart_data en el commit de junio "Added the comparative table" y el test se quedó desactualizado.)

Dependencias y CI/CD

  • 🟡 requirements.txt incluye psycopg (v3) y psycopg2-binary (v2) — drivers de PostgreSQL instalados pero sin usar, ya que settings.py sigue en SQLite. O es preparación para migrar a Postgres (documentarlo), o son dependencias muertas que limpiar. (Resuelto en rama dev: Postgres sí se usa en producción, con psycopg v3 (+ psycopg-binary). Se eliminó psycopg2-binary==2.9.11, el driver v2 antiguo que no usaba nadie. De paso se había fijado antes whitenoise==6.12.0.)

  • 🟡 El pipeline de Jenkins solo ejecuta los tests, no construye ni valida la imagen Docker con la que realmente se despliega en el NAS. Un Dockerfile roto no lo detectaría el CI actual. (Aplazado a propósito: de momento el despliegue se hace a mano. Alcance deseado cuando se retome: (1) comprobar que el pipeline falla de verdad si fallan los tests — la suite llevaba meses en rojo sin que saltara ninguna alarma; (2) construir la imagen Docker en CI para detectar un Dockerfile roto antes de desplegar; (3) despliegue automático en el NAS cuando haya cambios en main y los tests pasen.)

Resumen rápido

Sección Bugs/riesgos reales (🔴) Mejoras (🟡) Total
Bugs y riesgos con prioridad alta 7 0 7
Seguridad y configuración (settings.py) 1 5 6
urls.py (proyecto raíz) 0 2 2
views.py — robustez 0 7 7
models.py 0 2 2
forms.py 0 2 2
Tests 0 1 1
Dependencias y CI/CD 0 2 2
Total 8 21 29

Resueltos: 28 de 29.

# TODO — Revisión de código (expenses_manager) Revisión completa del repositorio `expenses_manager` (Django). Incluye bugs reales, riesgos de seguridad, huecos funcionales y deuda técnica encontrados al leer `models.py`, `views.py`, `forms.py`, `urls.py`, `settings.py`, el comando `seed_demo`, las plantillas, los tests, el `Jenkinsfile` y `requirements.txt`. Los puntos marcados con 🔴 son bugs o riesgos reales (algo se rompe o es inseguro). Los marcados con 🟡 son mejoras de robustez/mantenibilidad, no bugs confirmados. ## Progreso Resueltos hasta ahora: **28 de 29** puntos. - [x] Bug de `fuel_create` sin `return` en caso de POST inválido - [x] `goal_delete` sin protección de `POST` - [x] `fuel_create` sin filtrar categoría "gasolina" por `owner` - [x] Cuentas inactivas sin distinción visual en `account_list.html` - [x] Falta la plantilla `settings/index.html` (enlace roto) - [x] Typo en `ExpenseForm.Meta.widgets` (checkboxes de tags no se aplicaban) - [x] `seed_demo` no persiste `is_staff`/`is_superuser` - [x] `SECRET_KEY` con fallback inseguro (ahora falla explícitamente en prod) - [x] `STATIC_ROOT` (ya existía en prod, ahora unificado en el repo) - [x] Hardening de producción (cookies secure, SSL redirect, proxy header) - [x] `LOGGING` a stdout para `docker logs` - [x] SQLite vs Postgres (unificado por `DB_ENGINE`; prod ya usaba Postgres) - [x] Drivers de Postgres en `requirements.txt` (revisado: sí se usan) - [x] `ValueError` en el filtro de tags de `expense_list` (500 con `?tag=abc`) - [x] `except:` genérico en el dashboard (ahora logueado con traceback) - [x] Imports muertos y ruido en `views.py` - [x] Registro duplicado de las auth urls en `urls.py` raíz - [x] Colisión de prefijo `/accounts/` (cuentas financieras → `/finance-accounts/`) - [x] CRUD de categorías completo (`category_edit` + `category_delete`) - [x] Ciclos en `CategoryForm` (una categoría ya no puede ser su propio padre) - [x] `fuel_delete` + edición desde el listado de repostajes y vuelta al origen - [x] `Goal` replanteado: tipos pago / presupuesto / ahorro, con periodo y fecha de inicio - [x] `monthly_balance()` (ya estaba hecho) y `monthly_net()` optimizados - [x] Cobertura de tests (de 9 a 64 tests, con regresiones de todo lo corregido) - [x] `FuelEntryForm` refactorizado a `ModelForm` (deja de duplicar campos de `Expense`) - [x] Decisión de soft-delete de cuentas documentada en el código - [x] Eliminado el driver `psycopg2-binary` sobrante de `requirements.txt` - [x] Recuperación de contraseña resuelta por diseño (reset por admin + pantalla informativa) **Hito importante:** el `settings.py` de producción del NAS y el del repo estaban divergidos (Postgres, whitenoise, CSRF_TRUSTED_ORIGINS solo existían en el NAS). Ahora hay un único `settings.py` versionado controlado por variables de entorno (`.env`), desplegado y funcionando en producción. **Único punto sin cerrar:** la mejora del pipeline de Jenkins (build de la imagen Docker + despliegue automático desde `main`), aplazada a propósito porque el despliegue se hace a mano de momento. La contraseña hardcodeada de `seed_demo` queda como riesgo asumido (uso local, mitigada con la guarda de `DEBUG`). --- ## Bugs y riesgos con prioridad alta - [x] 🔴 **`fuel_create` no responde si el formulario POST es inválido.** El `return render(...)` del caso GET está indentado dentro del `else`, así que si llega un POST con datos inválidos la función no llega a ningún `return` y Django lanza un error (`ValueError: didn't return an HttpResponse`). Hay que sacar ese `render` fuera del `if/else`, como en el resto de vistas. *(Resuelto en rama `dev`.)* - [x] 🔴 **`goal_delete` borra el objetivo con una simple petición GET.** A diferencia de `expense_delete`, `tag_delete`, `account_delete` e `income_delete`, esta vista no comprueba `request.method == "POST"`. Cualquier enlace, precarga del navegador o bot puede borrar un objetivo sin confirmación. Añadir la comprobación de POST (y su plantilla de confirmación, como en el resto de recursos). *(Resuelto en rama `dev`: se añadió `goals/confirm_delete.html`.)* - [x] 🔴 **Falta la plantilla `settings/index.html`.** Confirmado revisando el repo: no existe ningún directorio `templates/settings/`. La ruta `/settings/` (vista `settings_index`) lanza `TemplateDoesNotExist` en cuanto se visita. Es un enlace roto ahora mismo en producción. *(Resuelto en rama `dev`: se creó `settings/index.html` como página hub con enlaces a Categorías/Etiquetas/Objetivos, ya que el menú de navegación no la usaba directamente.)* - [x] 🔴 **`fuel_create` busca la categoría "gasolina" sin filtrar por usuario.** Usa `Category.objects.get(slug="gasolina")` sin `owner=request.user`. Como el slug no es único entre usuarios (el `unique_together` de `Category` incluye `owner`), esto puede lanzar `MultipleObjectsReturned` si dos usuarios tienen esa categoría, o coger la categoría de otro usuario. *(Resuelto en rama `dev` con `get_or_create(slug="gasolina", owner=request.user, defaults={"name": "Gasolina"})`.)* - [x] 🔴 **Typo en `ExpenseForm.Meta.widgets`: la clave es `"widget"` en vez de `"tags"`.** Como `"widget"` no es un campo del formulario, el `CheckboxSelectMultiple()` pensado para las tags nunca se aplica. El campo `tags` se renderiza con el select múltiple por defecto, no con checkboxes. *(Resuelto en rama `dev`: se corrigió la clave y, de paso, se rediseñó `expense_form.html` para renderizar las tags como "chips" en fila con scroll, en vez de la lista vertical por defecto de Django.)* - [ ] 🔴 **Comando `seed_demo` crea un superusuario con contraseña débil hardcodeada (`demo1234`).** No hay ninguna guarda que impida ejecutar este comando en producción. Si se ejecuta ahí por error, queda un superusuario con credenciales conocidas. *(Aceptado como riesgo asumido: se usa solo en local. Mitigado parcialmente por la guarda de `DEBUG` añadida en el punto anterior, que impide ejecutar el comando si `DEBUG=False`.)* - [x] 🔴 **`seed_demo` no persiste `is_staff`/`is_superuser` si el usuario demo ya existe.** En la rama `else` (usuario ya creado) se asignan esos atributos en el objeto en memoria pero nunca se llama a `.save()`, así que no se guardan en la base de datos. *(Resuelto en rama `dev`: se movió la asignación de `is_staff`/`is_superuser` antes del `.save()` en ambas ramas — también afectaba a la rama `created`, que tampoco los persistía. Además se añadió una guarda `if not settings.DEBUG: raise CommandError(...)` para que el comando no pueda ejecutarse en producción.)* --- ## Seguridad y configuración de despliegue (`settings.py`) - [x] 🔴 **`SECRET_KEY` tiene un fallback inseguro hardcodeado** (`'fallback-secret-key-for-dev'`) que se usa silenciosamente si la variable de entorno no está definida. Si el `.env` falla al desplegar en el NAS, la app arranca igualmente con una clave insegura y conocida, sin avisar. Mejor que falle explícitamente si `SECRET_KEY` no está definido en producción. *(Resuelto: el `settings.py` unificado lanza `ImproperlyConfigured` si falta `SECRET_KEY` con `DEBUG=False`; en dev usa un fallback explícitamente marcado como inseguro. Desplegado en producción.)* - [x] 🟡 **No hay `STATIC_ROOT` definido.** Necesario para `collectstatic` en un despliegue real vía Docker/gunicorn (sin `runserver`). Revisar cómo se están sirviendo los estáticos ahora mismo en el contenedor. *(Resuelto: ya existía en el `settings.py` de producción del NAS —divergido del repo— junto con whitenoise; ahora está unificado y versionado en el repo.)* - [x] 🟡 **Faltan ajustes de hardening para producción**: `CSRF_TRUSTED_ORIGINS`, `SESSION_COOKIE_SECURE`, `CSRF_COOKIE_SECURE`, `SECURE_SSL_REDIRECT`. Relevante porque el dominio `finanzas.kuijper.es` probablemente está detrás de un reverse proxy HTTPS. *(Resuelto: el `settings.py` unificado activa `SESSION_COOKIE_SECURE`, `CSRF_COOKIE_SECURE`, `SECURE_SSL_REDIRECT` y `SECURE_PROXY_SSL_HEADER` cuando `DEBUG=False`; `CSRF_TRUSTED_ORIGINS` y `ALLOWED_HOSTS` van por variable de entorno. Desplegado y verificado tras el reverse proxy nginx de Synology.)* - [x] 🟡 **No hay configuración de `LOGGING`.** Con `DEBUG=False` en producción, los errores no quedan registrados en ningún sitio visible, dificultando el diagnóstico de fallos reales (como el de `settings/index.html` de arriba). *(Resuelto: `LOGGING` a consola/stdout, visible con `docker-compose logs web`.)* - [x] 🟡 **SQLite como base de datos en un despliegue de uso continuo.** Válido para uso personal, pero conviene documentar la estrategia de backups y tener en cuenta las limitaciones de concurrencia si el uso crece. *(Punto revisado: producción usaba en realidad PostgreSQL en un `settings.py` divergido en el NAS. Ahora unificado: `DB_ENGINE=postgresql` por env en el NAS, SQLite por defecto en local. Queda como tarea aparte documentar la estrategia de backups de Postgres.)* - [x] 🟡 **Falta el flujo de "olvidé mi contraseña".** Solo está implementado el cambio de contraseña estando ya logueado (`password_change_*.html`). No existen las plantillas `password_reset_*.html` que requieren las URLs de `django.contrib.auth`, y tampoco hay `EMAIL_BACKEND` configurado para poder enviar el email de recuperación. *(Decisión consciente de diseño: la app la usan 2 personas y no se monta recuperación por email para no mantener infraestructura de correo. El reset lo hace el admin bajo petición con `docker-compose exec web python manage.py changepassword <usuario>`. En vez de dejar el hueco, se añadió una pantalla informativa `registration/password_help.html` (ruta `password-help/` vía `TemplateView`, enlazada desde el login) que indica al usuario que contacte con el administrador — evita cualquier `TemplateDoesNotExist` y da una salida clara.)* --- ## `urls.py` (proyecto raíz) - [x] 🟡 **Colisión de prefijo `/accounts/`.** Se registra `path('accounts/', include(django.contrib.auth.urls))` (login/logout/password cambio) y, a la vez, tu propia app usa `/accounts/`, `/accounts/new/`, etc. para el CRUD de **cuentas financieras** (montado vía `path('', include('expenses.urls'))`). Ahora mismo no colisionan literalmente porque las rutas de auth son más específicas, pero es un diseño frágil: dos conceptos distintos ("cuenta de usuario" y "cuenta financiera") comparten namespace de URL. *(Resuelto en rama `dev`: las cuentas financieras pasan a `/finance-accounts/...`. Los `name=` de las rutas no cambiaron, así que ninguna plantilla necesitó tocarse.)* - [x] 🟡 **Registro duplicado de las auth urls.** Hay dos líneas que registran lo mismo: `path('accounts/', include(urls))` (importando el módulo `django.contrib.auth.urls` directamente) y `path('accounts/', include('django.contrib.auth.urls'))` (por string). Una de las dos sobra. *(Resuelto en rama `dev`: se conserva solo la forma por string, antes de `expenses.urls` para mantener la precedencia actual, y se eliminó el import `from django.contrib.auth import urls` que quedaba sin uso.)* --- ## `views.py` — robustez - [x] 🟡 **`except:` genérico en `dashboard`** (bloque de `accounts_charts` al llamar a `acc.monthly_balance(selected_year)`). Silencia cualquier error real, dificultando el debug. Capturar la excepción concreta esperada, o al menos loguearla. *(Resuelto: `except Exception` + `logger.exception(...)` con id de cuenta y año. El dashboard sigue sin romperse, pero el traceback aparece ahora en `docker-compose logs web`.)* - [x] 🟡 **`tag_ids = [int(t) for t in tag_ids]` en `expense_list` no maneja `ValueError`.** Un parámetro `tag` no numérico en la URL rompe la vista con un error 500, a diferencia del resto de filtros que usan el helper `_get_int` (que sí es seguro). *(Resuelto: se filtran los valores no numéricos reutilizando `_get_int` — un valor inválido se ignora en vez de romper la vista, mismo comportamiento que el resto de filtros.)* - [x] 🟡 **Inconsistencia en el borrado de recursos.** `account_delete` hace soft-delete (`active=False`), mientras que `expense_delete`, `tag_delete` e `income_delete` borran de verdad. Puede ser intencional (para no romper históricos de saldo), pero merece una decisión explícita y, si es así, documentarla en el código. *(Resuelto en rama `dev`: decisión confirmada como intencional y documentada con un docstring en `account_delete` que explica por qué las cuentas se desactivan en vez de borrarse —preservar los históricos de saldo de gastos/ingresos que la referencian—. La parte de UX (distinguir inactivas en el listado) ya se había resuelto por separado.)* - [x] 🟡 **Las cuentas inactivas (`active=False`) no se distinguían visualmente en `account_list.html`.** Al "eliminar" una cuenta (soft-delete), esta seguía apareciendo en el listado sin ningún cambio visible más allá de un `True`/`False` en crudo en una columna poco visible — daba la sensación de que el borrado "no funcionaba". *(Resuelto en rama `dev`: columna "Estado" con badge Activa/Inactiva, fila atenuada con CSS `.row-inactive`, y el botón "Eliminar" ya no se muestra para cuentas ya inactivas.)* - [x] 🟡 **Imports sin usar / ruido en `views.py`**: `truediv`, `django.template.context`, `is_valid_ipv6_address`, y una línea comentada con un import mal escrito (`dateutli` en vez de `dateutil`). *(Resuelto: eliminados esos cuatro, más `django.contrib.auth.login` (tampoco se usaba), y reordenado el bloque de imports al convenio stdlib → django → locales. En el mismo commit se limpiaron el dict `MONTHS` sin usar, la doble asignación `by_category_qs = by_category`, la variable `chart_type` que nunca llegaba al contexto, y se subió `import calendar` al principio del archivo.)* - [x] 🟡 **`category_list` no tiene `category_edit` ni `category_delete`**, a diferencia de `tag`/`account`/`goal`, que sí tienen el CRUD completo. Probablemente pendiente de implementar. *(Resuelto en rama `dev`: añadidas ambas vistas con sus rutas y plantillas (`categories/form.html` y `categories/confirm_delete.html`), más la columna de acciones en `categories/list.html`. El borrado captura `ProtectedError` (categorías con gastos no se pueden borrar) y avisa en la confirmación de las subcategorías y objetivos que se eliminarían en cascada.)* - [x] 🟡 **Falta `fuel_delete`.** El recurso `fuel` tiene `create`/`list`/`edit` pero no borrado, a diferencia del resto de recursos del proyecto. *(Resuelto en rama `dev`: añadida la vista `fuel_delete` (recibe el pk del `Expense`, como `fuel_edit`) con su ruta y `fuel/confirm_delete.html`; borrar el gasto elimina el `FuelEntry` en cascada. Además se añadieron los enlaces Editar/Eliminar en `fuel/list.html`, que antes no existían, y un helper `_redirect_back` con `?next=` validado (`url_has_allowed_host_and_scheme`) para volver a la pantalla de origen — gastos o repostajes — tras editar o borrar. En `expense_list.html` los enlaces se eligen según `expense.fuel_data` (no por el slug de categoría, que puede quedar desfasado).)* --- ## `models.py` - [x] 🟡 **`Account.monthly_balance()` recorre mes a mes con queries repetidas.** Funciona, pero es optimizable con una sola agregación en vez de iterar los 12 meses. *(Punto obsoleto: ya lo resolviste en el commit de junio "Fixed function monthly_balance" — el método actual hace 4 consultas y recorre los meses en memoria. En esta revisión se detectó que `monthly_net()` sí conservaba el patrón viejo (24 consultas por cuenta y año) y se optimizó igual, quedando en 2.)* - [x] 🟡 **`Goal.progress()` no filtra por cuenta ni por rango de fechas, solo por categoría.** Si la intención es que la meta sea mensual o esté ligada a una cuenta concreta, el cálculo actual no lo refleja — habría que decidir el comportamiento esperado y ajustarlo. *(Resuelto replanteando el modelo entero: `Goal` gana un campo `kind` con tres tipos —pago/deuda, presupuesto y ahorro—, más `period` (mensual/anual, para presupuestos), `start_date`, `account` (para ahorro) e `include_subcategories`. `progress()` despacha según el tipo y acota por fecha; se añadieron `bar_width()` (evita que la barra desborde al pasar del 100%) y `progress_state()` (colorea según el tipo: en un presupuesto acercarse al límite es alarma, no logro). `GoalForm` expone los campos nuevos y valida qué es obligatorio según el tipo. Migración `0010_...` — solo `AddField`/`AlterField`, sin pérdida de datos. El ahorro se mide provisionalmente con el saldo de la cuenta asociada: es la única rama a tocar cuando llegue el módulo de inversiones.)* --- ## `forms.py` - [x] 🟡 **`CategoryForm` no excluye la propia categoría del queryset de `parent` al editar.** Permite que una categoría se asigne a sí misma como padre, o crear ciclos (A → B → A), sin validación que lo impida. *(Resuelto en rama `dev`: al editar, el desplegable excluye la propia categoría y toda su descendencia (a cualquier profundidad), más un `clean_parent` como validación de respaldo. **Ojo**: durante la implementación se produjo un fallo temporal en el que `parent` se guardaba siempre como `None`; el síntoma clásico es que `clean_parent` no devuelva el valor. Conviene un test que fije este comportamiento.)* - [x] 🟡 **`FuelEntryForm` es un `forms.Form` plano que duplica a mano los campos de `Expense`** (fecha, importe, cuenta). Funciona, pero cualquier cambio en esos modelos hay que replicarlo manualmente aquí. *(Resuelto en rama `dev`: convertido en `ModelForm` de `Expense` (campos `date`/`amount`/`account` derivados del modelo) + los campos propios de `FuelEntry` (`odometer`/`liters`) añadidos aparte, con precarga al editar. `fuel_create`/`fuel_edit` simplificados con `form.save(commit=False)` en lugar de copiar campo por campo. Cubierto por `test_fuel.py`.)* --- ## Tests - [x] 🟡 **Cobertura de tests muy desigual.** Existían tests para `Expense` básico, `Income`, `dashboard` y un sanity check, pero **ninguno** para `Account`, `FuelEntry`, `Goal`, `Tag` ni la lógica de slug de `Category`. *(Resuelto: suite ampliada a 64 tests en verde. Nuevo `conftest.py` con fixtures (`user`, `auth_client`, `account`, `category`) y nuevos archivos `test_accounts.py`, `test_fuel.py`, `test_goals.py`, `test_categories.py`, `test_expense_list.py` y `test_deletes.py`. Incluye tests de regresión para todos los bugs corregidos en esta revisión: POST inválido en `fuel_create`, borrados que exigen POST, `?tag=` no numérico, persistencia de `parent` en `CategoryForm`, `PROTECT` al borrar categorías con gastos y borrado en cascada de `FuelEntry`. Se arregló además `test_dashboard_filters_by_year`, que llevaba meses fallando: la clave de contexto `by_month` se renombró a `chart_data` en el commit de junio "Added the comparative table" y el test se quedó desactualizado.)* --- ## Dependencias y CI/CD - [x] 🟡 **`requirements.txt` incluye `psycopg` (v3) y `psycopg2-binary` (v2)** — drivers de PostgreSQL instalados pero sin usar, ya que `settings.py` sigue en SQLite. O es preparación para migrar a Postgres (documentarlo), o son dependencias muertas que limpiar. *(Resuelto en rama `dev`: Postgres sí se usa en producción, con `psycopg` v3 (+ `psycopg-binary`). Se eliminó `psycopg2-binary==2.9.11`, el driver v2 antiguo que no usaba nadie. De paso se había fijado antes `whitenoise==6.12.0`.)* - [ ] 🟡 **El pipeline de Jenkins solo ejecuta los tests**, no construye ni valida la imagen Docker con la que realmente se despliega en el NAS. Un `Dockerfile` roto no lo detectaría el CI actual. *(Aplazado a propósito: de momento el despliegue se hace a mano. Alcance deseado cuando se retome: (1) comprobar que el pipeline falla de verdad si fallan los tests — la suite llevaba meses en rojo sin que saltara ninguna alarma; (2) construir la imagen Docker en CI para detectar un `Dockerfile` roto antes de desplegar; (3) despliegue automático en el NAS cuando haya cambios en `main` y los tests pasen.)* --- ## Resumen rápido | Sección | Bugs/riesgos reales (🔴) | Mejoras (🟡) | Total | |---|---|---|---| | Bugs y riesgos con prioridad alta | 7 | 0 | 7 | | Seguridad y configuración (`settings.py`) | 1 | 5 | 6 | | `urls.py` (proyecto raíz) | 0 | 2 | 2 | | `views.py` — robustez | 0 | 7 | 7 | | `models.py` | 0 | 2 | 2 | | `forms.py` | 0 | 2 | 2 | | Tests | 0 | 1 | 1 | | Dependencias y CI/CD | 0 | 2 | 2 | | **Total** | **8** | **21** | **29** | **Resueltos: 28 de 29.**
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
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: jkuijperm/expenses_manager#13
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?