PR: Enrique San Martin, HF Solutions, enriquesmw@gmail.com, CARGO: Desarrollador Backend Java.#10
PR: Enrique San Martin, HF Solutions, enriquesmw@gmail.com, CARGO: Desarrollador Backend Java.#10enriquesmw wants to merge 20 commits into
Conversation
…leados ; AJAX + HTML
…ara formateo de rut en FRNT
…tar y probar la aplicacion
…ias. Uso correcto de datos y ajustes en el manejo de excepciones
…eflejar los ultimos refacktorings
…a formato canonico XXXXXX-X
…en lugar de valores en duro, manejo de excepciones y normalizacion de clases importandolas en lugar de usar el FQDN
…e nombre, apellido, rut, etc
There was a problem hiding this comment.
Pull request overview
This PR introduces a complete “Gestión de Empleados” solution in the repo: a Spring Boot (Servlet-based) REST API backed by H2/JDBC, a simple HTML/JS UI consuming it via Fetch/AJAX, plus validation/error-handling, logging, and tests.
Changes:
- Implementa API REST con
HttpServletpara listar/crear/eliminar empleados, con persistencia JDBC en H2. - Agrega validaciones de negocio (RUT chileno, salario mínimo, bono/descuentos, duplicidad) y un resolvedor centralizado de excepciones con contrato JSON uniforme.
- Incorpora UI web (HTML/CSS/JS) y una suite de tests unitarios para utilidades/servicios/servlet/excepciones, además de documentación y configuración de logging/DB.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/logback-test.xml | Configuración de logging para tests. |
| src/test/java/cl/previred/desafio/util/RutValidatorTest.java | Tests para validación y formato canónico de RUT. |
| src/test/java/cl/previred/desafio/servlet/EmpleadoServletTest.java | Tests del servlet REST usando mocks y request/response simulados. |
| src/test/java/cl/previred/desafio/service/ValidationServiceTest.java | Tests de reglas de validación de negocio. |
| src/test/java/cl/previred/desafio/service/EmpleadoServiceTest.java | Tests del service de empleados y normalización de RUT/bono. |
| src/test/java/cl/previred/desafio/exception/ApiExceptionResolverTest.java | Tests del mapeo excepción → respuesta HTTP/JSON. |
| src/main/webapp/js/app.js | Lógica frontend: Fetch, validaciones, formateo RUT, render de tabla. |
| src/main/webapp/index.html | Página principal con formulario y tabla de empleados. |
| src/main/webapp/css/styles.css | Estilos de la UI. |
| src/main/resources/schema.sql | DDL para tabla empleados en H2. |
| src/main/resources/logback-spring.xml | Logging runtime con consola + rolling file. |
| src/main/resources/application.properties | Configuración de H2 + inicialización de schema. |
| src/main/java/cl/previred/desafio/util/RutValidator.java | Utilidad de validación/formato de RUT. |
| src/main/java/cl/previred/desafio/servlet/EmpleadoServlet.java | Servlet REST /api/empleados/* con manejo de errores unificado. |
| src/main/java/cl/previred/desafio/service/ValidationService.java | Implementación de EmpleadoValidator con reglas de negocio. |
| src/main/java/cl/previred/desafio/service/EmpleadoValidator.java | Contrato de validación para facilitar mocking/testing. |
| src/main/java/cl/previred/desafio/service/EmpleadoService.java | Orquestación de creación/listado/eliminación y normalización de datos. |
| src/main/java/cl/previred/desafio/repository/EmpleadoRepositoryPort.java | Puerto para repositorio (abstracción). |
| src/main/java/cl/previred/desafio/repository/EmpleadoRepository.java | Implementación JDBC del repositorio. |
| src/main/java/cl/previred/desafio/PreviredDesafioLegacyApplication.java | Bootstrap Spring Boot + @ServletComponentScan. |
| src/main/java/cl/previred/desafio/model/Empleado.java | Modelo de dominio Empleado. |
| src/main/java/cl/previred/desafio/exception/ValidationExceptionList.java | Excepción para múltiples errores de validación. |
| src/main/java/cl/previred/desafio/exception/ValidationException.java | Excepción para un error de validación individual. |
| src/main/java/cl/previred/desafio/exception/TechnicalException.java | Base para excepciones técnicas (infra). |
| src/main/java/cl/previred/desafio/exception/ResourceNotFoundException.java | Excepción para 404 por recurso inexistente. |
| src/main/java/cl/previred/desafio/exception/ResolvedErrorResponse.java | Wrapper status + body de error resuelto. |
| src/main/java/cl/previred/desafio/exception/RepositoryException.java | Excepción para errores de persistencia. |
| src/main/java/cl/previred/desafio/exception/BusinessException.java | Excepción genérica de negocio (no central en el flujo actual). |
| src/main/java/cl/previred/desafio/exception/ApiExceptionResolver.java | Resolver central de excepciones a contrato JSON y códigos HTTP. |
| src/main/java/cl/previred/desafio/dto/ValidationError.java | DTO para error por campo. |
| src/main/java/cl/previred/desafio/dto/ErrorResponseFactory.java | Factory de respuestas de error consistentes. |
| src/main/java/cl/previred/desafio/dto/ErrorResponse.java | DTO de respuesta de error (lista de errores). |
| src/main/java/cl/previred/desafio/dto/EmpleadoRequest.java | DTO de request para creación/actualización. |
| README.md | Instrucciones de ejecución y contrato de errores. |
| pom.xml | Configuración Maven/Spring Boot + dependencias. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @Test | ||
| void invalidFormatExceptionRetorna400ConCampo() { | ||
| InvalidFormatException ex = new InvalidFormatException(null, "valor", Integer.class); |
There was a problem hiding this comment.
El test intenta instanciar InvalidFormatException con un constructor de 3 parámetros que no existe en Jackson (Spring Boot 2.7.x). Esto probablemente no compila. Usa un constructor válido (incluyendo el valor que falló) o crea la excepción vía un factory/helper de Jackson para el test.
| InvalidFormatException ex = new InvalidFormatException(null, "valor", Integer.class); | |
| InvalidFormatException ex = InvalidFormatException.from(null, "valor", "valor", Integer.class); |
| const salario = parseFloat(document.getElementById('salario').value); | ||
| const bono = parseFloat(document.getElementById('bono').value) || 0; | ||
| const descuentos = parseFloat(document.getElementById('descuentos').value) || 0; | ||
|
|
||
| if (!nombre) { | ||
| mostrarError('nombre', 'El nombre es requerido'); | ||
| esValido = false; | ||
| } else if (nombre.length < 2) { | ||
| mostrarError('nombre', 'El nombre debe tener al menos 2 caracteres'); | ||
| esValido = false; | ||
| } else if (nombre.length > 50) { | ||
| mostrarError('nombre', 'El nombre no puede exceder 50 caracteres'); | ||
| esValido = false; | ||
| } else if (!PATTERN_NOMBRE_APELLIDO.test(nombre) || isOnlySymbols(nombre)) { | ||
| mostrarError('nombre', 'El nombre solo puede contener letras y espacios'); | ||
| esValido = false; | ||
| } | ||
|
|
||
| if (!apellido) { | ||
| mostrarError('apellido', 'El apellido es requerido'); | ||
| esValido = false; | ||
| } else if (apellido.length < 2) { | ||
| mostrarError('apellido', 'El apellido debe tener al menos 2 caracteres'); | ||
| esValido = false; | ||
| } else if (apellido.length > 50) { | ||
| mostrarError('apellido', 'El apellido no puede exceder 50 caracteres'); | ||
| esValido = false; | ||
| } else if (!PATTERN_NOMBRE_APELLIDO.test(apellido) || isOnlySymbols(apellido)) { | ||
| mostrarError('apellido', 'El apellido solo puede contener letras y espacios'); | ||
| esValido = false; | ||
| } | ||
|
|
||
| if (!rut) { | ||
| mostrarError('rut', 'El RUT es requerido'); | ||
| esValido = false; | ||
| } else if (!esRutValido(rut)) { | ||
| mostrarError('rut', 'RUT invalido'); | ||
| esValido = false; | ||
| } | ||
|
|
||
| if (!cargo) { | ||
| mostrarError('cargo', 'El cargo es requerido'); | ||
| esValido = false; | ||
| } else if (cargo.length < 2) { | ||
| mostrarError('cargo', 'El cargo debe tener al menos 2 caracteres'); | ||
| esValido = false; | ||
| } else if (cargo.length > 100) { | ||
| mostrarError('cargo', 'El cargo no puede exceder 100 caracteres'); | ||
| esValido = false; | ||
| } else if (isOnlySymbols(cargo)) { | ||
| mostrarError('cargo', 'El cargo no puede ser solo simbolos'); | ||
| esValido = false; | ||
| } else if (!PATTERN_CARGO.test(cargo)) { | ||
| mostrarError('cargo', 'El cargo contiene caracteres invalidos'); | ||
| esValido = false; | ||
| } | ||
|
|
||
| if (!salario) { | ||
| mostrarError('salario', 'El salario es requerido'); | ||
| esValido = false; | ||
| } else if (salario < 0) { | ||
| mostrarError('salario', 'El salario no puede ser negativo'); | ||
| esValido = false; | ||
| } else if (salario < SALARIO_MINIMO) { | ||
| mostrarError('salario', 'Salario debe ser >= $400,000'); | ||
| esValido = false; |
There was a problem hiding this comment.
La validación de salario usa if (!salario); esto trata 0 como “no informado” y además no distingue entre string vacío y NaN. Conviene validar explícitamente Number.isFinite(salario) (o chequear el input.value vacío) antes de aplicar reglas de rango.
| } | ||
|
|
||
| function formatearNumero(num) { | ||
| return num ? num.toString().replace(/\B(?=(\d{3})+(?!\d))/g, '.') : '0'; |
There was a problem hiding this comment.
formatearNumero inserta separadores de miles sobre num.toString() y no maneja decimales (p.ej. 500000.5 se renderiza como "500.000.5"). Si el backend devuelve DECIMAL con parte decimal, la tabla mostrará montos mal formateados. Considera usar Intl.NumberFormat('es-CL') o separar la parte entera/decimal antes de aplicar el regex.
| return num ? num.toString().replace(/\B(?=(\d{3})+(?!\d))/g, '.') : '0'; | |
| if (num === null || num === undefined || num === '') { | |
| return '0'; | |
| } | |
| const numero = Number(num); | |
| if (Number.isNaN(numero)) { | |
| return '0'; | |
| } | |
| return new Intl.NumberFormat('es-CL', { | |
| minimumFractionDigits: 0, | |
| maximumFractionDigits: 20 | |
| }).format(numero); |
| spring.h2.console.enabled=true | ||
| spring.h2.console.path=/h2-console |
There was a problem hiding this comment.
spring.h2.console.enabled=true expone la consola H2 en runtime; si esto se despliega fuera de un entorno local puede permitir acceso no deseado a la BD. Sugerencia: habilitarlo solo en un perfil de desarrollo/test (p.ej. application-dev.properties) o dejarlo deshabilitado por defecto.
| spring.h2.console.enabled=true | |
| spring.h2.console.path=/h2-console | |
| spring.h2.console.enabled=false |
| <dependency> | ||
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
Se agrega junit:junit (JUnit 4) además de spring-boot-starter-test (que ya trae JUnit Jupiter). Esto suele introducir engines duplicados/confusión en el runner. Recomendación: eliminar junit:junit y mantener solo el starter de test (o declarar explícitamente JUnit Jupiter si se quiere aislar dependencias).
| <dependency> | |
| <groupId>junit</groupId> | |
| <artifactId>junit</artifactId> | |
| <scope>test</scope> | |
| </dependency> |
| * <li>Violaciones de contraintes del sistema</li> | ||
| * </ul> | ||
| * | ||
| * <p>En produccion, los detalles de esta excepcion no deben exponerse | ||
| * al cliente para evitar信息披露 de informacion sensible.</p> |
There was a problem hiding this comment.
En el Javadoc hay un typo y texto mezclado que parece accidental ("contraintes" y "evitar信息披露"). Esto afecta la claridad de la documentación y dificulta mantener consistencia de idioma.
| * <li>Violaciones de contraintes del sistema</li> | |
| * </ul> | |
| * | |
| * <p>En produccion, los detalles de esta excepcion no deben exponerse | |
| * al cliente para evitar信息披露 de informacion sensible.</p> | |
| * <li>Violaciones de restricciones del sistema</li> | |
| * </ul> | |
| * | |
| * <p>En produccion, los detalles de esta excepcion no deben exponerse | |
| * al cliente para evitar la divulgacion de informacion sensible.</p> |
| try { | ||
| handler.handle(); | ||
| } catch (IOException ex) { | ||
| LOG.error("Error de entrada/salida en petition HTTP", ex); |
There was a problem hiding this comment.
Typo en el log: "petition HTTP" debería ser "peticion HTTP" (o "petición HTTP").
| LOG.error("Error de entrada/salida en petition HTTP", ex); | |
| LOG.error("Error de entrada/salida en peticion HTTP", ex); |
| * Resuelve una excepcion a su respuesta HTTP correspondiente. | ||
| * | ||
| * @param ex la excepcion a resolver | ||
| * @param requestUri URI de la peticion dondeoccurrio el error |
There was a problem hiding this comment.
En el Javadoc del parámetro requestUri falta un espacio: "dondeoccurrio" debería ser "donde ocurrio" (o "donde ocurrió").
| * @param requestUri URI de la peticion dondeoccurrio el error | |
| * @param requestUri URI de la peticion donde ocurrió el error |
| <appender name="FILE" class="ch.qos.logback.core.rolling.RollingFileAppender"> | ||
| <file>logs/desafio-legacy.log</file> | ||
| <rollingPolicy class="ch.qos.logback.core.rolling.TimeBasedRollingPolicy"> | ||
| <fileNamePattern>logs/desafio-legacy.%d{yyyy-MM-dd}.log</fileNamePattern> | ||
| <maxHistory>30</maxHistory> | ||
| </rollingPolicy> |
There was a problem hiding this comment.
El appender FILE escribe en logs/..., pero el repo no trae esa carpeta. En un entorno limpio/CI esto suele provocar fallos de inicialización del appender (FileNotFoundException) y se pierden logs a archivo. Considera parametrizar la ruta (LOG_PATH) y/o asegurar creación del directorio en startup/build.
Nombre: Enrique San Martin
Empresa: HF Solutions
Correo: enriquesmw@gmail.com
CARGO: Desarrollador Backend Java.