Proyecto Final - Entrega 1 - CRM #101
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdded extensive project documentation: README overhaul, TRACEABILITY, glossary, prompts, user stories (MH1–MH5, SH1–SH2), and a full tickets set (T1–T15). All additions are Markdown files documenting requirements, flows, acceptance criteria, and ticket-to-story mappings; no executable code or public API implementations were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@historias-usuario/MH1.md`:
- Line 12: The user story currently mixes "API key" and "email/password"
authentication causing ambiguity; decide and state the MVP scope: either remove
the "email/password" phrase so the story only describes API key login flow
(store API key, include in headers, backend filters by company, handle 401 by
clearing storage and redirecting to login), or expand the story to explicitly
support both methods by adding acceptance criteria for "email/password" (fields,
validation rules, optional enablement flag, storage/session behavior, and how
401 is handled for credentials), and update references to "login", "dashboard",
"API key", and "401" accordingly.
In `@tickets/T3.md`:
- Line 16: AC1 is ambiguous about supported authentication methods; decide
whether POST /api/auth/login will accept only api_key or both api_key and
email/password. If only API key is in scope, remove all "email/password"
mentions in T3 and MH1 and any alternate request examples/sequence paths; if
both are supported, add a second request example JSON for email/password (as
suggested) and update the sequence diagram to show both authentication flows and
label the POST /api/auth/login behavior accordingly (reference AC1, the existing
request example in T3, and MH1 for places to edit).
In `@tickets/T9.md`:
- Around line 12-20: No almacenes la API key en localStorage (clave
flotiko_api_key); cambia la implementación de login (POST /api/auth/login) y del
cliente HTTP (axios interceptor) para usar cookies httpOnly o almacenamiento en
memoria con refresh: en el endpoint de login hace que el backend devuelva/setee
una cookie httpOnly (o un short‑lived token más refresh token) en lugar de
retornar la key para guardarla en localStorage; en el cliente, modifica la
llamada al login para incluir credentials (fetch/axios withCredentials) y
elimina cualquier lectura/escritura a localStorage (buscar y retirar referencias
a flotiko_api_key, get/setApiKey helpers); actualiza el interceptor que añadía
Authorization: Bearer {api_key} para enviar cookies automáticamente (o leer el
token desde un in‑memory store) y en la respuesta 401 limpiar la sesión en
memoria y redirigir a /login; mantiene la lógica de redirección en /login para
usuarios ya autenticados pero basada en la presencia/validez del cookie o token
en memoria en lugar de localStorage.
🧹 Nitpick comments (24)
historias-usuario/MH4.md (1)
46-46: Clarify business rule for future maintenance dates.The QA test case mentions future dates with "si la regla de negocio lo prohíbe" (if the business rule prohibits it), but the rule isn't defined. This is acceptable for initial documentation, but the business rule should be clarified in T7 (API ticket) before implementation to ensure consistent validation.
Consider documenting the decision in T7.md or the data model: Should the system allow scheduling future maintenance (e.g., booking an appointment), or should maintenance dates always be historical (recording completed work)?
tickets/T7.md (2)
17-17: Clarify required/optional fields for maintenance-types and workshops CRUD.AC2 mentions "CRUD completo" for maintenance-types and workshops but doesn't specify which fields are required or optional for creating/updating these entities. This information would help ensure consistent validation across all CRUD operations.
Consider adding a brief field list for each entity, for example:
MaintenanceType: name (required), description (optional), company_id (auto-assigned)Workshop: name (required), address, phone, company_id (auto-assigned)
20-21: Define the business rules for "completado" and "in_workshop" status.AC6 references business rules (
completado,in_workshop) but these rules are not defined in this ticket. Consider either defining them here or explicitly referencing where they are documented to avoid implementation ambiguity.Add a brief explanation of these status transitions, such as:
- When does a maintenance move to "completado"?
- What triggers "in_workshop" status?
- Can a maintenance be edited after completion?
historias-usuario/MH1.md (2)
33-33: Specify the exact fields returned by the login endpoint.AC3 mentions "información necesaria (usuario, empresa)" which is vague. For API contract clarity, specify the exact response fields expected.
Reference the response structure from T3.md (lines 38-50) or explicitly list the fields:
- user: id, name, email, company_id
- company: id, name
This ensures consistency between the user story and the technical ticket implementation.
16-27: Consider adding email/password authentication path to the flowchart.The flowchart shows the API key validation flow, but if email/password login is supported (as mentioned in the description), the flowchart should illustrate both authentication paths for completeness.
If email/password authentication is in scope, consider adding a decision node after "Pantalla Login" to show the two authentication methods branching into their respective validation flows.
tickets.md (1)
18-18: Clarify T1's relationship to user stories in the traceability matrix.Line 18 states T1 is "Base MH1–MH5", indicating it provides foundational infrastructure for all must-have stories. However, line 57 in the traceability matrix shows T1 with dashes ("—") for all story columns, which could be interpreted as no relationship.
Consider either:
- Adding a footnote explaining that "—" means "foundational/indirect support" vs "X" meaning "direct implementation"
- Using a different symbol for T1 (e.g., "○" or "Base") to distinguish infrastructure tickets from feature-implementing tickets
- Adding a legend above the matrix explaining the symbols
This would make the traceability clearer for readers unfamiliar with the project structure.
Also applies to: 57-57
tickets/T6.md (2)
20-20: Clarify the specific placement of calculation logic.AC5 mentions logic should be "en un servicio o en eventos del modelo (FuelRefillService / model events)" which creates ambiguity. The technical notes (line 97) mention both, but don't specify which component is responsible for which calculation.
Consider specifying:
FuelRefillService: Business logic for creating refills, updating vehicle.kms, calculating total_cost- Model events (
creating/created): Lightweight hooks for data consistency (e.g., ensuring total_cost is set)This clarity helps prevent logic duplication and ensures consistent implementation.
21-21: Specify the consumption calculation formula and edge cases.AC6 mentions calculating consumption (L/100km) and cost per km "comparando con el repostaje anterior", but doesn't specify:
- The exact formula for consumption: (liters / (current_kms - previous_kms)) * 100?
- How to handle the first refill (no previous refill exists)
- How to handle refills created out of chronological order
- Whether partial tank refills affect the calculation
Consider adding a subsection under "Notas técnicas" documenting these calculation details, or reference T8 if those details are specified there.
tickets/T11.md (4)
18-18: Clarify the scope and source of vehicle alerts.AC3 states alerts should be shown "si la API las expone o se pueden derivar de los datos", which creates ambiguity about whether alerts are in scope for this ticket.
Consider specifying:
- Are alerts calculated in the backend (T5/T8) or frontend?
- What specific alerts should be displayed (próximo mantenimiento, ITV vencida, seguro próximo a vencer)?
- What are the trigger conditions for each alert?
If alerts are out of scope for this ticket, remove this acceptance criterion or mark it as optional/future enhancement.
21-21: Specify the redirect destination after successful refill creation.AC6 mentions "redirección a detalle o historial" which is ambiguous. The user experience would be more consistent with a clear redirect destination.
Consider specifying:
- Default redirect to vehicle detail page (showing updated last refill)
- Or redirect to history page (showing new refill in chronological list)
- Or provide user choice (e.g., "Save and view detail" vs "Save and add another")
Aligning this with the UX flow described in MH3.md would ensure consistency.
22-22: Detail the frontend validation rules for better implementation consistency.AC7 mentions "validación en frontend básica (campos obligatorios, números positivos)" but doesn't specify the exact rules for each field.
Consider adding specific validation rules:
fecha: required, not in futurekm: required, positive integer, >= vehicle's current kms (warning if significantly higher)liters: required, positive number, reasonable range (e.g., 10-100L)price_per_liter: required, positive number with 2-3 decimalsfuel_type: required, matches vehicle's fuel type or show warning if differentThis ensures consistent validation across frontend and backend (T6).
26-26: Consider documenting mobile/responsive design requirements.Given that fleet management is often used on-site during refueling, the vehicle detail and refill form screens would benefit from explicit mobile-responsive requirements.
Consider adding a note about:
- Mobile-first design approach
- Touch-friendly form inputs (large buttons, appropriate input types)
- Offline capability considerations (for areas with poor connectivity at gas stations)
tickets/T3.md (1)
16-20: Consider documenting security measures for the login endpoint.The acceptance criteria cover functional requirements but don't mention security best practices for authentication endpoints.
Consider adding acceptance criteria for:
- Rate limiting: Limit login attempts per IP/API key to prevent brute force attacks (e.g., 5 attempts per minute)
- API key expiration: How are expired API keys handled? Should the response indicate remaining validity?
- Audit logging: Should login attempts (successful and failed) be logged for security monitoring?
These are particularly important for a multi-tenant system handling company data.
tickets/T15.md (3)
12-12: Specify the integration point for the gas station finder in the refill flow.The description mentions "en el formulario de Añadir repostaje (o en un paso previo)" which is ambiguous about where this feature fits in the user flow.
Consider specifying:
- Option A: Add a "Buscar gasolineras cercanas" button in the AddFuelRefill.jsx form above the station name field
- Option B: Create a preliminary step before the refill form where users can optionally select a gas station
- Option C: Show nearby stations automatically when the form loads (if user grants location permission)
Clarifying this will help frontend implementation and ensure consistency with the UX described in SH2.md.
16-16: Document API parameters for the nearby gas stations search.AC1 shows the API call pattern
GET /api/gas-stations/nearby?latitude=...&longitude=...but doesn't specify other potentially useful parameters.Consider documenting whether the API supports:
radius(km): Search radius from the location (default value?)fuel_type_id: Filter stations by fuel type available (to match vehicle's fuel type)limit: Maximum number of stations to returnIf these parameters aren't currently supported, consider whether they would improve the feature's usefulness.
16-20: Consider UX enhancements for loading and caching.The acceptance criteria cover core functionality but could benefit from mentioning user experience details.
Consider adding:
- AC6: Show loading indicator while fetching geolocation and API data
- AC7: Cache nearby stations for a short period (e.g., 5 minutes) to avoid repeated API calls if user adds multiple refills at the same location
- AC8: Display distance to each station (calculated from user's current location)
These enhancements would significantly improve the feature's usability without major development effort.
tickets/T5.md (1)
16-20: Update endpoint verb may be inconsistent with “CRUD REST”.
If this is intended as a RESTful update, considerPUT/PATCH /api/vehicles/{id}instead ofPOSTto avoid ambiguity in client implementations and docs.tickets/T13.md (1)
16-16: Clarify the maintenances filter query to avoid ambiguity.AC1 cites filtering by
vehicle_idbut doesn’t show the query shape forGET /api/maintenances. Consider making it explicit like the fuel-refills example to prevent divergent implementations.♻️ Suggested doc tweak
-- **AC1**: Ruta `/vehicle/:id/history` (o pestaña Historial dentro del detalle) que muestre repostajes y mantenimientos del vehículo; datos desde `GET /api/fuel-refills?vehicle_id=...` y `GET /api/maintenances` filtrado por vehicle_id (o endpoint equivalente). +- **AC1**: Ruta `/vehicle/:id/history` (o pestaña Historial dentro del detalle) que muestre repostajes y mantenimientos del vehículo; datos desde `GET /api/fuel-refills?vehicle_id=...` y `GET /api/maintenances?vehicle_id=...` (o endpoint equivalente).tickets/T2.md (1)
16-19: Specify header precedence and a minimal 401 error shape.If both headers are sent, clients need to know which one wins. Also, defining the 401 JSON keys avoids inconsistent frontend handling.
♻️ Suggested doc tweak
-- **AC1**: Middleware (ej. `ApiAuth`) que lea el header Authorization o X-API-Key y valide la key contra la tabla `api_keys` (o equivalente). +- **AC1**: Middleware (ej. `ApiAuth`) que lea el header Authorization o X-API-Key (si ambos existen, priorizar Authorization) y valide la key contra la tabla `api_keys` (o equivalente). ... -- **AC3**: Si la key falta o es inválida, el middleware responde 401 Unauthorized con cuerpo JSON. +- **AC3**: Si la key falta o es inválida, el middleware responde 401 Unauthorized con cuerpo JSON (p. ej. `{ "message": "Invalid API key" }`).historias-usuario/MH5.md (1)
24-25: Make the diagram endpoint consistent with the API path.Use the same
/api/vehicles/{id}/reportnotation used elsewhere to avoid confusion.♻️ Suggested doc tweak
- ReporteVista --> ApiReport[GET vehicles id report] - ApiReport --> Metricas[Consumo L 100km coste por km] + ReporteVista --> ApiReport[GET /api/vehicles/{id}/report] + ApiReport --> Metricas[Consumo L/100 km, coste por km]readme.md (2)
200-228: Normalize the vehicle plate field naming across sections.The data model uses
plate_numberwhile the API example useslicense_plate. Pick one and use it consistently to avoid integration mistakes.♻️ Suggested doc tweak (example)
-- **Vehicle (vehicles):** `id` PK, `company_id` FK, `brand_id` FK, `plate_number`, `model`, `year`, `kms`, `default_fuel_type`. +- **Vehicle (vehicles):** `id` PK, `company_id` FK, `brand_id` FK, `license_plate`, `model`, `year`, `kms`, `default_fuel_type`.
52-53: Add a brief security note about localStorage.Since the API key is stored in localStorage, it’s worth calling out the XSS risk and suggesting mitigations (CSP, sanitization) or an alternative storage strategy if feasible.
tickets/T4.md (1)
17-21: Clarify 403 vs 404 policy for cross-company access.You mention 404 for “other company” in the example; it would help to explicitly state whether the API should return 403 or 404 for unauthorized cross-tenant access so backend and frontend align.
Also applies to: 66-66
tickets/T8.md (1)
55-63: Align the “insufficient data” message language.Consider making the
messagefield Spanish (or explicitly label it as a fixed English string) to keep the API examples consistent.
|
|
||
| ## Descripción | ||
|
|
||
| El usuario debe poder autenticarse en la aplicación móvil o web introduciendo su API key (o credenciales si se habilita login por email/contraseña). Tras un login correcto, la app guarda la API key y redirige al dashboard. Todas las peticiones posteriores incluyen la API key en el header; el backend filtra los datos por la empresa asociada a esa clave. Si la API key es inválida o expira, el backend responde 401 y el frontend debe redirigir a la pantalla de login y limpiar el almacenamiento local. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify the scope of email/password authentication.
The description mentions both API key and email/password authentication with conditional phrasing ("o credenciales si se habilita login por email/contraseña"). This creates ambiguity about whether email/password login is in scope for the MVP or a future enhancement.
Consider either:
- If API key is the only authentication method for MVP, remove references to email/password to avoid confusion
- If both methods are supported, add explicit acceptance criteria covering email/password validation and clarify configuration requirements
🤖 Prompt for AI Agents
In `@historias-usuario/MH1.md` at line 12, The user story currently mixes "API
key" and "email/password" authentication causing ambiguity; decide and state the
MVP scope: either remove the "email/password" phrase so the story only describes
API key login flow (store API key, include in headers, backend filters by
company, handle 401 by clearing storage and redirecting to login), or expand the
story to explicitly support both methods by adding acceptance criteria for
"email/password" (fields, validation rules, optional enablement flag,
storage/session behavior, and how 401 is handled for credentials), and update
references to "login", "dashboard", "API key", and "401" accordingly.
|
|
||
| ## Criterios de aceptación | ||
|
|
||
| - **AC1**: Existe `POST /api/auth/login` que acepta API key (o email/password) y devuelve respuesta JSON con datos de usuario/empresa necesarios para el frontend (sin exponer datos sensibles). |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify whether email/password authentication is in scope.
AC1 mentions the endpoint "acepta API key (o email/password)" but the request example (lines 30-34) only shows api_key. This creates the same ambiguity found in MH1.md.
Consider either:
- If only API key authentication is in the MVP scope, remove references to email/password throughout T3 and MH1
- If both methods are supported, add a second request example showing email/password authentication:
{
"email": "juan@empresa.com",
"password": "securepassword"
}And update the sequence diagram to show both authentication paths.
🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Considere alternativas para este anglicismo (recomendable sobre todo en contextos más formales).
Context: ...terios de aceptación - AC1: Existe POST /api/auth/login que acepta API key (o ...
(POST)
🤖 Prompt for AI Agents
In `@tickets/T3.md` at line 16, AC1 is ambiguous about supported authentication
methods; decide whether POST /api/auth/login will accept only api_key or both
api_key and email/password. If only API key is in scope, remove all
"email/password" mentions in T3 and MH1 and any alternate request
examples/sequence paths; if both are supported, add a second request example
JSON for email/password (as suggested) and update the sequence diagram to show
both authentication flows and label the POST /api/auth/login behavior
accordingly (reference AC1, the existing request example in T3, and MH1 for
places to edit).
| Implementar la pantalla de login donde el usuario introduce su API key (o credenciales), llamar al endpoint de login, guardar la API key en localStorage y redirigir al dashboard. Implementar la lógica para que las peticiones API incluyan el header Authorization y que en 401 se limpie la sesión y se redirija a login. | ||
|
|
||
| ## Criterios de aceptación | ||
|
|
||
| - **AC1**: Existe la ruta `/login` y un componente/página Login con formulario (campo API key o email/password según backend). | ||
| - **AC2**: Al enviar el formulario se llama a `POST /api/auth/login`; en éxito se guarda la API key en `localStorage` (ej. clave `flotiko_api_key`) y se redirige a `/` (dashboard). | ||
| - **AC3**: Cliente HTTP (axios) configurado con interceptor que añade `Authorization: Bearer {api_key}` leyendo de localStorage; si no hay API key y la ruta no es login, redirigir a `/login`. | ||
| - **AC4**: Interceptor de respuesta: si la respuesta es 401, eliminar API key de localStorage, opcionalmente mostrar mensaje "Sesión expirada" y redirigir a `/login`. | ||
| - **AC5**: Si el usuario ya tiene API key válida y accede a `/login`, se puede redirigir directamente al dashboard para evitar pantalla de login innecesaria. |
There was a problem hiding this comment.
Reconsider storing API key in localStorage (XSS exposure).
If possible, prefer short‑lived tokens with httpOnly cookies or in‑memory storage + refresh flow to reduce risk.
🧰 Tools
🪛 LanguageTool
[grammar] ~17-~17: Agrega un signo de puntuación.
Context: ...según backend). - AC2: Al enviar el formulario se llama a POST /api/auth/login; en é...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[style] ~17-~17: Considere alternativas para este anglicismo (recomendable sobre todo en contextos más formales).
Context: ...2**: Al enviar el formulario se llama a POST /api/auth/login; en éxito se guarda la...
(POST)
[grammar] ~17-~17: Agrega un signo de puntuación.
Context: ...o se llama a POST /api/auth/login; en éxito se guarda la API key en localStorage ...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
🤖 Prompt for AI Agents
In `@tickets/T9.md` around lines 12 - 20, No almacenes la API key en localStorage
(clave flotiko_api_key); cambia la implementación de login (POST
/api/auth/login) y del cliente HTTP (axios interceptor) para usar cookies
httpOnly o almacenamiento en memoria con refresh: en el endpoint de login hace
que el backend devuelva/setee una cookie httpOnly (o un short‑lived token más
refresh token) en lugar de retornar la key para guardarla en localStorage; en el
cliente, modifica la llamada al login para incluir credentials (fetch/axios
withCredentials) y elimina cualquier lectura/escritura a localStorage (buscar y
retirar referencias a flotiko_api_key, get/setApiKey helpers); actualiza el
interceptor que añadía Authorization: Bearer {api_key} para enviar cookies
automáticamente (o leer el token desde un in‑memory store) y en la respuesta 401
limpiar la sesión en memoria y redirigir a /login; mantiene la lógica de
redirección en /login para usuarios ya autenticados pero basada en la
presencia/validez del cookie o token en memoria en lugar de localStorage.
…ama ER, sin refs a flotiko_backend/frontend
…a a carpeta documentacion
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@glosario.md`:
- Line 20: Update the "API key" glossary entry to standardize the authentication
header to the project-wide choice: replace the dual-header phrasing with
"Authorization: Bearer {api_key}" (as referenced in tickets T3,T5,T6,T7,T9) and,
if both headers must be supported, explicitly document that "X-API-Key" is
legacy/optional and describe when to use it and how the backend validates the
key; edit the line containing the API key glossary entry to reflect this single
standard and add a short sentence about fallback/compatibility only if both are
required.
In `@readme.md`:
- Line 32: Replace the placeholder lines in readme.md: update "Indicar URL de la
app desplegada si existe." with the actual deployed app URL (or remove if none),
replace "Indicar URL del repo AI4Devs-finalproject" with the real repository URL
or GitHub link, and add the missing screenshots/video tutorial section by
embedding image files or a hosted video link with captions and alt text; search
for those exact placeholder strings to locate the spots and ensure links use
full URLs or relative paths and that images are included in the repo (e.g.,
assets/screenshots) and referenced in the README.
In `@tickets/T5.md`:
- Line 18: AC3 currently documents an update as POST /api/vehicles/{id} which is
non‑RESTful; change the update endpoint to use PATCH /api/vehicles/{id} (or PUT
if you expect full replacement), keep POST /api/vehicles for creation, and
update any route handlers, router registrations, controller/handler functions
(the existing create and update functions for vehicles), tests, and OpenAPI/docs
accordingly so the update handler (now PATCH/PUT /api/vehicles/{id}) still
assigns company_id from the API key context and performs the same required-field
validation.
In `@tickets/T6.md`:
- Line 18: AC3's current approach of unconditionally updating vehicle.kms when a
refill's kms > vehicle.kms is fragile; update the logic in the refill creation
flow (where total_cost is computed from liters * price_per_liter if omitted and
where kms from the refill record is applied) to: compute total_cost if missing,
persist the refill record with its reported kms/odometer, then in a single
atomic DB operation (transaction or compare-and-set/upsert using SQL MAX or a
versioned update) update vehicle.kms to the greater of existing vehicle.kms and
the persisted refill.kms to avoid race conditions; also add validation for
obviously incorrect kms and an audit trail or flagged field on the refill record
to allow manual corrections—apply the same pattern used/recommended in T7 for
consistency.
In `@tickets/T7.md`:
- Line 20: AC5's simple "if maintenance.kms > vehicle.kms then update
vehicle.kms" is vulnerable to out-of-order entries, concurrent races, and
corrections; update the implementation so that createMaintenance,
updateMaintenance, and deleteMaintenance call a single reconciler function
(recalculateVehicleKms) that computes the vehicle kms as the MAX over all
related events (maintenances + fuelRefills) and writes that value back in a
transaction or using optimistic concurrency (e.g., version check) to avoid
races; additionally add validation in createMaintenance/updateMaintenance to
reject kms that are implausibly lower than the current vehicle kms (or allow but
document the policy), and add a short doc comment describing expected behavior
for out-of-order entries and corrections.
🧹 Nitpick comments (6)
tickets/T5.md (1)
19-19: Clarify the deletion policy for vehicles.AC4 mentions "considerar política de borrado (soft delete o cascade según modelo)" but leaves the decision ambiguous. This could lead to inconsistent implementations or data integrity issues if vehicles have related records (fuel refills, maintenances).
💡 Recommendation
Specify the deletion behavior explicitly. For example:
-- **AC4**: `DELETE /api/vehicles/{id}` elimina el vehículo solo si pertenece a la empresa; considerar política de borrado (soft delete o cascade según modelo). +- **AC4**: `DELETE /api/vehicles/{id}` elimina el vehículo solo si pertenece a la empresa usando soft delete (marca como eliminado sin borrar registros relacionados); alternativamente, retorna error 409 si existen repostajes o mantenimientos asociados.This protects historical data while giving clear implementation guidance.
tickets/T6.md (1)
18-18: Clarifytotal_costcalculation and validation behavior.AC3 states "el backend calcula
total_cost(liters * price_per_liter) si no se envía". This leaves ambiguous what happens if the client does sendtotal_cost:
- Should the backend validate that the sent value matches the calculation?
- Should the backend always recalculate and ignore the client value?
- Could a client send an incorrect
total_costthat gets stored?📋 Recommendation
Specify the behavior explicitly. For data integrity, consider:
-- **AC3**: Al crear un repostaje, el backend calcula `total_cost` (liters * price_per_liter) si no se envía; si `kms` del repostaje es mayor que `vehicle.kms`, se actualiza `vehicle.kms`. +- **AC3**: Al crear un repostaje, el backend **siempre calcula** `total_cost` (liters * price_per_liter) ignorando cualquier valor enviado por el cliente para garantizar consistencia; si `kms` del repostaje es mayor que `vehicle.current_kms`, se actualiza `vehicle.current_kms`.This prevents clients from submitting incorrect financial data.
tickets/T4.md (1)
16-16: Clarify the role and permission model.AC1 states that
GET /api/usersshould only be accessible "solo si la API key tiene permiso; p. ej. admin". However, the specification doesn't clarify:
- Are roles associated with users or API keys?
- Can a company have multiple API keys with different permission levels?
- How is the "admin" role defined and checked?
💡 Recommendation
Add a section or reference explaining the permission model. For example:
## Modelo de permisos - Cada API key está asociada a un **user** específico - Los usuarios tienen un campo `role` (valores: 'admin', 'user', 'viewer') - Solo usuarios con rol 'admin' pueden: - Listar todos los usuarios de la empresa (GET /api/users) - Crear/eliminar usuarios (POST/DELETE /api/users) - Todos los usuarios pueden ver su propio perfil (GET /api/me) - El middleware verifica el rol del usuario asociado a la API keyThis clarifies implementation requirements and prevents ambiguity.
readme.md (2)
653-666: Document pull requests section.The PR documentation section (lines 653-666) is currently a placeholder with empty PR entries. Consider documenting at least this PR (
#101) as the first entry to establish the pattern.📝 Example entry for the current PR
### Pull Request 1 -(Título y descripción; enlace al PR si está en GitHub/GitLab.) +**Título:** Proyecto Final - Entrega 1 - CRM + +**Descripción:** Mejoras documentación: modelo de datos (tablas, campos, tipos), diagrama ER, referencias a trazabilidad, historias de usuario y tickets. + +**URL:** https://github.com/LIDR-academy/AI4Devs-finalproject/pull/101 ### Pull Request 2
5-12: Consider conventional numbering for the index.The index starts with "0. Ficha del proyecto" followed by sections 1-7. While valid, starting at 1 is more conventional for documentation indices. This is a stylistic choice and not a defect.
tickets/T8.md (1)
78-78: Minor: Use explicit path parameter syntax in sequence diagram.Line 78 shows "GET /api/vehicles id report" which could be clearer as "GET /api/vehicles/{id}/report" to match the REST endpoint format used elsewhere in the documentation.
🔧 Suggested diagram update
- Client->>API: GET /api/vehicles id report + Client->>API: GET /api/vehicles/{id}/report
|
|
||
| | Término | Descripción | | ||
| |---------|-------------| | ||
| | **API key** | Clave asociada a una empresa; se envía en el header `Authorization: Bearer {api_key}` o `X-API-Key`. El backend valida la key y filtra datos por la empresa asociada. | |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify the authentication header standard.
The documentation mentions two possible authentication headers: Authorization: Bearer {api_key} or X-API-Key. However, all other ticket specifications (T3, T5, T6, T7, T9) consistently reference only Authorization: Bearer {api_key}. This inconsistency could lead to implementation confusion.
📝 Recommendation
Choose one standard authentication method for the project and document it consistently. Based on the ticket specifications, it appears Authorization: Bearer {api_key} is the preferred approach. Consider:
-| **API key** | Clave asociada a una empresa; se envía en el header `Authorization: Bearer {api_key}` o `X-API-Key`. El backend valida la key y filtra datos por la empresa asociada. |
+| **API key** | Clave asociada a una empresa; se envía en el header `Authorization: Bearer {api_key}`. El backend valida la key y filtra datos por la empresa asociada. |If both methods must be supported, add explicit guidance on when to use each method.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | **API key** | Clave asociada a una empresa; se envía en el header `Authorization: Bearer {api_key}` o `X-API-Key`. El backend valida la key y filtra datos por la empresa asociada. | | |
| | **API key** | Clave asociada a una empresa; se envía en el header `Authorization: Bearer {api_key}`. El backend valida la key y filtra datos por la empresa asociada. | |
🧰 Tools
🪛 LanguageTool
[grammar] ~20-~20: Aquí puede haber un error.
Context: ...da a una empresa; se envía en el header Authorization: Bearer {api_key} o X-API-Key. El backend val...
(QB_NEW_ES)
[grammar] ~20-~20: Aquí puede haber un error.
Context: ... filtra datos por la empresa asociada. | | company_id | Identificador de la em...
(QB_NEW_ES)
🤖 Prompt for AI Agents
In `@glosario.md` at line 20, Update the "API key" glossary entry to standardize
the authentication header to the project-wide choice: replace the dual-header
phrasing with "Authorization: Bearer {api_key}" (as referenced in tickets
T3,T5,T6,T7,T9) and, if both headers must be supported, explicitly document that
"X-API-Key" is legacy/optional and describe when to use it and how the backend
validates the key; edit the line containing the API key glossary entry to
reflect this single standard and add a short sentence about
fallback/compatibility only if both are required.
| ### 0.4. URL del proyecto | ||
|
|
||
| > Puede ser pública o privada, en cuyo caso deberás compartir los accesos de manera segura. Puedes enviarlos a [alvaro@lidr.co](mailto:alvaro@lidr.co) usando algún servicio como [onetimesecret](https://onetimesecret.com/). | ||
| (Indicar URL de la app desplegada si existe.) |
There was a problem hiding this comment.
Complete placeholder sections before final release.
Several sections contain placeholder text that should be filled in:
- Line 32: "Indicar URL de la app desplegada si existe"
- Line 38: "Indicar URL del repo AI4Devs-finalproject"
- Line 64: Missing screenshots/video tutorial
Would you like help generating templates or checklists for completing these sections?
Also applies to: 38-38, 64-64
🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Cambia la palabra o signo.
Context: ...dicar URL de la app desplegada si existe.) Puede ser pública o privada, en cuyo ...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_PUNCTUATION)
[grammar] ~32-~32: Aquí puede haber un error.
Context: ...car URL de la app desplegada si existe.) Puede ser pública o privada, en cuyo cas...
(QB_NEW_ES)
🤖 Prompt for AI Agents
In `@readme.md` at line 32, Replace the placeholder lines in readme.md: update
"Indicar URL de la app desplegada si existe." with the actual deployed app URL
(or remove if none), replace "Indicar URL del repo AI4Devs-finalproject" with
the real repository URL or GitHub link, and add the missing screenshots/video
tutorial section by embedding image files or a hosted video link with captions
and alt text; search for those exact placeholder strings to locate the spots and
ensure links use full URLs or relative paths and that images are included in the
repo (e.g., assets/screenshots) and referenced in the README.
|
|
||
| - **AC1**: `GET /api/vehicles` devuelve listado paginado de vehículos de la empresa; acepta query params `page`, `per_page`, `brand_id`, `search`. | ||
| - **AC2**: `GET /api/vehicles/{id}` devuelve un vehículo por ID solo si pertenece a la empresa; incluye relaciones útiles (brand, lastFuelRefill, etc.) si está definido en el recurso. | ||
| - **AC3**: `POST /api/vehicles` y `POST /api/vehicles/{id}` (update) crean/actualizan vehículo asignando `company_id` desde el contexto de la API key; validación de campos obligatorios (marca, matrícula, etc.). |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use RESTful HTTP methods for updates.
AC3 specifies POST /api/vehicles/{id} for updating vehicles, which is non-standard. RESTful APIs conventionally use PUT (full replacement) or PATCH (partial update) for resource updates.
♻️ Recommended fix
-- **AC3**: `POST /api/vehicles` y `POST /api/vehicles/{id}` (update) crean/actualizan vehículo asignando `company_id` desde el contexto de la API key; validación de campos obligatorios (marca, matrícula, etc.).
+- **AC3**: `POST /api/vehicles` crea vehículos y `PUT /api/vehicles/{id}` (o `PATCH`) actualiza vehículos, asignando `company_id` desde el contexto de la API key; validación de campos obligatorios (marca, matrícula, etc.).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **AC3**: `POST /api/vehicles` y `POST /api/vehicles/{id}` (update) crean/actualizan vehículo asignando `company_id` desde el contexto de la API key; validación de campos obligatorios (marca, matrícula, etc.). | |
| - **AC3**: `POST /api/vehicles` crea vehículos y `PUT /api/vehicles/{id}` (o `PATCH`) actualiza vehículos, asignando `company_id` desde el contexto de la API key; validación de campos obligatorios (marca, matrícula, etc.). |
🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Considere alternativas para este anglicismo (recomendable sobre todo en contextos más formales).
Context: ...está definido en el recurso. - AC3: POST /api/vehicles y `POST /api/vehicles/{i...
(POST)
[style] ~18-~18: Considere alternativas para este anglicismo (recomendable sobre todo en contextos más formales).
Context: ...urso. - AC3: POST /api/vehicles y POST /api/vehicles/{id} (update) crean/actu...
(POST)
🤖 Prompt for AI Agents
In `@tickets/T5.md` at line 18, AC3 currently documents an update as POST
/api/vehicles/{id} which is non‑RESTful; change the update endpoint to use PATCH
/api/vehicles/{id} (or PUT if you expect full replacement), keep POST
/api/vehicles for creation, and update any route handlers, router registrations,
controller/handler functions (the existing create and update functions for
vehicles), tests, and OpenAPI/docs accordingly so the update handler (now
PATCH/PUT /api/vehicles/{id}) still assigns company_id from the API key context
and performs the same required-field validation.
|
|
||
| - **AC1**: `GET /api/fuel-refills` lista repostajes de la empresa con paginación y filtro por `vehicle_id`. | ||
| - **AC2**: `POST /api/fuel-refills` crea repostaje con validación (vehicle_id, refill_date, liters, price_per_liter, fuel_type; kms opcional). Se asigna `company_id` y `created_by` desde el contexto. | ||
| - **AC3**: Al crear un repostaje, el backend calcula `total_cost` (liters * price_per_liter) si no se envía; si `kms` del repostaje es mayor que `vehicle.kms`, se actualiza `vehicle.kms`. |
There was a problem hiding this comment.
Vehicle kilometers update has the same edge case concerns as T7.
Similar to the maintenance ticket (T7), the logic for updating vehicle.kms when fuel refill kms are higher has potential issues with out-of-order data entry, concurrent updates, and data corrections.
Refer to the review comment in T7.md for detailed analysis and suggested improvements. Consider adopting a consistent approach across both fuel refills and maintenances for vehicle kilometer tracking.
🤖 Prompt for AI Agents
In `@tickets/T6.md` at line 18, AC3's current approach of unconditionally updating
vehicle.kms when a refill's kms > vehicle.kms is fragile; update the logic in
the refill creation flow (where total_cost is computed from liters *
price_per_liter if omitted and where kms from the refill record is applied) to:
compute total_cost if missing, persist the refill record with its reported
kms/odometer, then in a single atomic DB operation (transaction or
compare-and-set/upsert using SQL MAX or a versioned update) update vehicle.kms
to the greater of existing vehicle.kms and the persisted refill.kms to avoid
race conditions; also add validation for obviously incorrect kms and an audit
trail or flagged field on the refill record to allow manual corrections—apply
the same pattern used/recommended in T7 for consistency.
| - **AC2**: CRUD completo para maintenance-types y workshops (GET by id, POST, PUT, DELETE) con filtro por empresa. | ||
| - **AC3**: `GET /api/maintenances` lista mantenimientos con filtro por empresa y opcionalmente por `vehicle_id`; paginación. | ||
| - **AC4**: `POST /api/maintenances` crea mantenimiento con validación (vehicle_id, maintenance_type_id, date; workshop_id, kms, cost, next_maintenance_date/km opcionales). Se asigna company_id. | ||
| - **AC5**: Si el mantenimiento tiene `kms` mayor que el vehículo, se actualiza `vehicle.kms`. Reglas de negocio (completado, in_workshop) según modelo existente. |
There was a problem hiding this comment.
Consider edge cases in vehicle kilometers update logic.
AC5 specifies that if a maintenance has kms greater than the vehicle's current kms, the vehicle's kms should be updated. This logic has potential issues:
- Out-of-order data entry: If an older maintenance with lower kms is entered after a newer one, the vehicle's kms won't update, but the data model might be inconsistent.
- Concurrent updates: Multiple simultaneous maintenance creations could race to update vehicle kms.
- Data corrections: If a maintenance record is later corrected or deleted, should vehicle kms be recalculated?
💡 Suggested improvements
Consider either:
- Always store the maximum
kmsfrom all associated records (fuel refills + maintenances) when displaying/reporting vehicle kms - Track vehicle kms separately from maintenance/fuel events and validate that new entries don't have kms lower than current vehicle kms
- Document the expected behavior for out-of-order entries and corrections
Example clarification:
-- **AC5**: Si el mantenimiento tiene `kms` mayor que el vehículo, se actualiza `vehicle.kms`. Reglas de negocio (completado, in_workshop) según modelo existente.
+- **AC5**: Si el mantenimiento tiene `kms` mayor que el `vehicle.current_kms`, se actualiza `vehicle.current_kms` usando una transacción. Los registros ingresados fuera de orden (kms menores al actual) se aceptan pero no actualizan el vehículo. Al eliminar o corregir un mantenimiento, `vehicle.current_kms` se recalcula como el máximo entre todos los registros.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **AC5**: Si el mantenimiento tiene `kms` mayor que el vehículo, se actualiza `vehicle.kms`. Reglas de negocio (completado, in_workshop) según modelo existente. | |
| - **AC5**: Si el mantenimiento tiene `kms` mayor que el `vehicle.current_kms`, se actualiza `vehicle.current_kms` usando una transacción. Los registros ingresados fuera de orden (kms menores al actual) se aceptan pero no actualizan el vehículo. Al eliminar o corregir un mantenimiento, `vehicle.current_kms` se recalcula como el máximo entre todos los registros. |
🤖 Prompt for AI Agents
In `@tickets/T7.md` at line 20, AC5's simple "if maintenance.kms > vehicle.kms
then update vehicle.kms" is vulnerable to out-of-order entries, concurrent
races, and corrections; update the implementation so that createMaintenance,
updateMaintenance, and deleteMaintenance call a single reconciler function
(recalculateVehicleKms) that computes the vehicle kms as the MAX over all
related events (maintenances + fuelRefills) and writes that value back in a
transaction or using optimistic concurrency (e.g., version check) to avoid
races; additionally add validation in createMaintenance/updateMaintenance to
reject kms that are implausibly lower than the current vehicle kms (or allow but
document the policy), and add a short doc comment describing expected behavior
for out-of-order entries and corrections.
1 participant
…dad, glosario
Summary by CodeRabbit