Actividad 4 - Grupo 5

[dalmamunoz]

https://github.com/DanielUTN/vetsoft

[ivanduva]

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

[dalmamunoz]

https://vetsoft-app-1-0.onrender.com

[ivanduva]

Buenas,
Les vamos a pedir algunos arreglos y cambios:

• El principal problema es que no se agregó el campo ciudad, por lo que el trabajo queda incompleto. Entendemos que Daniel debería ser el encargado de hacerlo.
• En cuanto a la validación de que el teléfono sea numérico, les pedimos que bloqueen los caracteres no numéricos en el form y que lo testeen con un e2e.
• 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 y uno de integración.
• En cuanto a la validación del nombre, en ese PR comentan varios tests. No entendemos bien la razón, pero si lo hicieron porque fallan, no es el procedimiento correcto. Deberían modificarlos para que los tests pasen. Saquen los comentarios y arreglen los que corresponda. Lo mismo para cualquier otro test que hayan comentado.
• Luego de hacer todo, saquen una release.

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

Por otro lado, seguimos viendo problemas en los PRs y las reviews. Además de la casi nula interacción, hay algunas malas prácticas como por ejemplo:

• Lo más grave es que hayan mergeado un PR con los tests fallando: actividad 4 punto 2 regla COM812 DanielUTN/vetsoft#27. Además el PR está mergeado por la misma persona que lo hizo sin ser aprobado por nadie más. Si se hubiera hecho una review, la persona que revisa va a notar rápidamente que los tests no pasan y lo va a comentar. Después vimos que lo revirtieron e hicieron otro PR. Tampoco es una buena práctica. Un revert se hace únicamente en un caso muy grave y como última instancia, por ejemplo cuando hay que poner en producción una versión anterior porque la actual rompe la funcionalidad principal del producto. No creemos que este sea el caso, se podría haber solucionado de otra manera.
• Hay varios PRs poco atómicos que solucionan cuestiones que no están relacionadas entre sí. Por ejemplo: Feature/d103 DanielUTN/vetsoft#16. Ahí vemos que se agregan cuestiones relativas a la CI/CD además del refactor de los tests estáticos. Esto no solo dificulta la review sino que además dificulta solucionar problemas que traigan esos PR luego de mergearlos. En ese mismo PR también se agregan archivos que no tienen que ver con la app, como algunos archivos de configuración de VS code. Tengan cuidado al hacer git add porque evidentemente están agregando múltiples archivos al mismo tiempo. No recomendamos hacer git add .. De cualquier manera, podrían haber agregado esos archivos al .gitignore y al hacer add no se van a agregar.
• Volvemos a notar problemas que ya les corregimos anteriormente. Por ejemplo nombres de commit y títulos de PR que no son apropiados. "actividad 2 COM812" o "actividad 4 punto 2 regla COM812" no hacen referencia a nada relativo a la funcionalidad de la app. Nuevamente les decimos: no es bueno hacer referencia a la consigna en esos lugares. Los nombres de commit deberían ser una síntesis de lo que ese commit agrega en cuanto a funcionalidad. Los títulos de PR, lo mismo.

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

[JoseLlontop]

Buenas, ya se realizaron las correcciones.
https://github.com/DanielUTN/vetsoft
Saludos!

[ivanduva]

Buenas,
Lo mismo que les comentamos con respecto a la Actividad 3. No está bien entrar en el PR de otra persona y comenzar a corregirle todo como hicieron en este: DanielUTN/vetsoft#39. El procedimiento es el de una review como corresponde, con comentarios y feedback entre las personas involucradas.
Por otro lado, lo relativo a la validación de que el teléfono sea numérico no lo corrigió la persona que se encargó de eso sino Joaquín. Lo mismo con respecto a los tests borrados. Por favor lean bien cuando les hacemos una corrección porque decía claramente "Recuerden que los cambios deben ser realizados por las personas que se encargaron de cada issue.". Y repetimos: si alguien del equipo no está conforme con la calidad del código o la funcionalidad que resuelve un PR, hagan una review que es justamente lo que les venimos diciendo en todas las correcciones. En los PRs casi no hay comentarios, cuando tranquilamente podría haber ya que tienen problemas muy visibles.
Otro ejemplo, aca DanielUTN/vetsoft#34 faltaba el test e2e que les pedimos y lo terminan agregando en otro PR. Si se hubiera hecho una review, alguien se hubiera dado cuenta que faltaba y se podría solucionar comentandole a la persona para que lo agregue en ese PR en lugar de tener que hacer otro.
Recuerden que esta materia no se trata de escribir código que funcione, sino de la calidad, de los procedimientos que rodean al desarrollo y del trabajo en un equipo de desarrollo.

Les vamos a pedir lo siguiente, prestando especial atención en la review:

• Dalma: en este commit DanielUTN/vetsoft@2a372b0#diff-4f1074a0e0315101db28e847257792529cebac228be21d9dfc9c57c82873695b se borró el test e2e que había agregado Joaquín sobre que el form permita solo caracteres numéricos en teléfono. Agregá un test sobre eso verificando que al intentar agregar una letra, el input quede vacío.
• Daniel: en la mayoría de los tests que hiciste o modificaste faltaría tomar los valores de las ciudades desde la clase enumerativa del modelo, en lugar de poner a mano "Berisso" por ejemplo. Además te pedimos agregar un test e2e que pruebe que todas las opciones de la clase enumerativa se pueden seleccionar en el form.
• Tomás: te pedimos validar que el nombre no contenga únicamente espacios.
• Joaquín y José: Les pedimos que limiten su participación únicamente a hacer review de los PRs.
• Luego de hacer todo, saquen una release.

Saludos

[JoseLlontop]

Buenas, ya se realizaron las correcciones mencionadas.
https://github.com/DanielUTN/vetsoft

Saludos!

[ivanduva]

Buenas.
El trabajo integrador queda aprobado.
Les dejamos algunos comentarios para que les sirvan en el futuro:

• En el test e2e de este PR Bug fix/ciudad DanielUTN/vetsoft#42 faltó importar de la clase enumerativa "La Plata" y "Ensenada".
• El número de versión que eligieron no es correcto porque en realidad si hacen arreglos muy chiquitos como estos, deberían cambiar el último número. O sea, debería haber quedado v1.5.1. Si quieren leer más sobre el tema, lo pueden hacer acá: https://semver.org/lang/es/

Saludos!