Actividad 4 - Grupo 6

[felygl77]

https://github.com/GonzaloEBaez/vetsoft

[ivanduva]

Buenas! Les puedo pedir el link a donde lo deployaron?

[manuel-edlp]

Hola profe, lo deployamos acá: (https://vet-soft-1-4.onrender.com/). Vimos que en el deploy en Render pese a haberlo pasado exitosamente desde flujo de GitHub Actions, se nos rompe por problemas de migraciones, aun cuando localmente funciona. Nos pasa cada vez que mergeamos cualquier rama con main. Siempre se rompen las migraciones y hay que andar haciendo constantemente makemigrations --merge o eliminando algunas y eliminando la bd para volver a crearla.
Sabrías decirnos a que se debe o darnos alguna recomendación de como podríamos hacer para evitar que al fusionar nuestros cambios nos pase esto ?

[ivanduva]

Creo que es porque no están haciendo el migrate en el dockerfile.
Al no funcionar la release, está incompleto. Así que arreglen en un PR y después les seguimos corrigiendo.
En cuanto a lo de las migraciones, eso pasa porque en algún momento hicieron cambios en el modelo sin migrar y después cuando alguien hace cambios sobre el mismo modelo, quedan conflictos en las migraciones. Armen un PR donde hagan makemigrations --merge y metanlo en main antes de seguir haciendo otras correcciones.

La última fecha límite de será el último día de cursada antes de las vacaciones: Lunes 8 de Julio 23:59 hs.

Saludos

[manuel-edlp]

Hola profe, las correcciones ya están hechas.

[ivanduva]

Buenas,
Vemos varios problemas. Principalmente de procedimiento y en los tests, no tanto de funcionalidad. Nos referimos a todo el trabajo con git, los PRs, las revisiones, etc. Así que presten especial atención en los procesos al realizar los cambios y arreglos que les vamos a pedir:

• En cuanto a la validación del email, se permite agregar "@vetsoft.com" sin nada adelante. Arreglen eso. También les pedimos que agreguen un test de unidad, que agreguen uno e2e y que arreglen el de integración porque lo que hace es probar que el campo no cambie cuando se lo edita con un valor erróneo. Eso no es correcto porque no sabemos en donde falla la app ni cuál es el problema específico. En lugar de eso, debería testear que aparezca el error en particular.
• En cuanto a la validación de que el teléfono comience con 54, les pedimos un test de unidad y uno e2e.
• En cuanto a la validación de que el teléfono sea numérico, les pedimos un test de integración y uno e2e.
• En cuanto a la validación del nombre, les pedimos un test e2e.
• En cuanto al campo ciudad, lo mismo que el del email, les pedimos que agreguen uno de unidad, uno e2e y que arreglen el de integración por la misma razón.
• Luego de hacer todo, saquen una release.

Recuerden que los cambios deben ser realizados por las personas que se encargaron de cada issue.

Ahora, en cuanto a lo que les comentabamos de procedimiento:

• La review es casi inexistente. Y esto no lo decimos únicamente porque no hay comentarios o los comentarios que hay son del estilo "Todo bien", que ya explicamos que no son relevantes y que para poner eso, alcanza con poner el approve. Además de esto, hay muchísimos PRs mergeados con la CI fallando, por lo cuál la review también falló en ese sentido. Si alguien revisa un PR y nota que los tests estáticos o dinámicos están fallando, inmediatamente debería hacer ese comentario.
• Hay commits hechos sobre main. Esto es muy peligroso porque no pasa por una instancia formal de review y por lo tanto es poco seguro, puede romper la aplicación. Si se está utilizando el workflow Feature Branch, hay que hacer PRs para cumplir con el proceso.
• Otra cuestión que ya les hemos comentado en otras revisiones: hay muchos commits con mensajes duplicados, lo cual es super confuso. Por ejemplo, en este PR: https://github.com/GonzaloEBaez/vetsoft/pull/29/commits
• Otra medida por la que vemos que la revisión es mala es que varias funcionalidades tuvieron que ser corregidas en otro PR. Por ejemplo: Refactor/d102 GonzaloEBaez/vetsoft#27 o el tema del fix del deploy que lo hicieron en 4 PRs distintos. Como les comentamos en la corrección de la actividad anterior, si hubiera una buena revisión los errores se atajarían en el primer PR, generando un feedback entre el equipo que mejoraría el PR hasta estar listo para mergear.

Tengan esto en cuenta en las correcciones porque aunque resuelvan la funcionalidad podemos seguir pidiendo cambios si vemos que el trabajo no está en forma como para aprobar.

La última fecha límite de será el último día de cursada antes de las vacaciones: Lunes 8 de Julio 23:59 hs.

Saludos

[manuel-edlp]

Hola profe, las correcciones ya fueron hechas.

[ivanduva]

Buenas!
El trabajo integrador queda aprobado.
Igualmente les dejamos algunos comentarios sobre los tests para que les sirva en el futuro:

• El test e2e que agregan acá: Bugfix/validar email cliente GonzaloEBaez/vetsoft#43 tiene demasiados expects sobre el mismo test y está dividido internamente según lo que prueban. Eso lo podrían haber resuelto en varios tests, porque recuerden que si un expect falla, el test no se sigue ejecutando y no sabemos que pasa con los demás.
• El test e2e que agregan acá: Bugfix/validar campo numerico GonzaloEBaez/vetsoft#45 no es e2e porque entra por la API y no incluye a la interfaz, por lo tanto no prueba el sistema entero.

Saludos!