Actividad 3 - Grupo 6

[GonzaloEBaez]

https://github.com/GonzaloEBaez/vetsoft.git

[ivanduva]

Buenas!
Hay algunos problemas en la app que deberían corregir para aprobarla:

• En el caso de la dirección del provider, no hay tests específicos sobre la dirección sino que son generales sobre la clase Provider. Los tests están bien pero son más generales sobre la clase Provider. Incluso hay uno de integración sobre el mail, que no está mal, pero no está relacionado con el issue. Cuando se soluciona un issue, se debería agregar tests relativos a la funcionalidad que allí se describe. Además el campo está opcional cuando no se pedía eso en la tarea. Algo que también está raro del form es que al editar se borra el mail.
• En cuanto al precio del producto, al editar no valida y permite agregar precios menores o iguales a 0. Además le sacaron el {% extends 'base.html' %} al template y el form queda sin estilos.
• En cuanto a la dosis del medicamento, lo mismo que la dirección del provider, en los tests e2e y de integración no se testea el atributo en específico. Además al intentar guardar con campos incorrectos, el form borra toda la data que ingresó el usuario, lo cual es un comportamiento poco común. Debería dejar la data y mostrar los errores. El form editar directamente no las muestra y si la data es mala no la guarda. También es un comportamiento raro, debería mostrar las validaciones.
• En el peso de la mascota, se permite agregar 0 y se rompe la app al intentar poner un float.
• En cuanto a la fecha de nacimiento de la mascota, al editar borra el contenido pero pide agregarla si querés guardar con el campo vacío.

Cada problema debería arreglarlo la persona encargada de ese issue.
También vemos algunos problemas de procedimiento que deberían mejorar en las próximas entregas:

• Haber agregado dev les puede haber generado un ruido innecesario. Vemos que al final tuvieron que mergear a mano a main y que hicieron arreglos a mano sobre main que dev no contiene. Además algunos PRs están hechos contra main y otros contra dev. Queda todo muy confuso y propenso a errores. Si les simplifica el proceso, piensen en eliminar dev.
• Hacer esos commits sobre main al final es muy peligroso. Justamente la idea de trabajar con feature branch es que esto no ocurra porque es código que no tiene una instancia de revisión clara como la de un PR.
• Hay muchos indicios de que la review en los PRs no es buena. Además de que no vemos ningún comentario, hay varios problemas claros que se podrían haber atajado. Por ejemplo, el tema de las migraciones, razón por la cual tuvieron que hacer esos commits sobre main. Otro ejemplo es el del PR que apunta a main cuando debería apuntar a dev. Otro más relacionado al funcionamiento es cuando se rompe la app al ingresar un float en el peso de la mascota. Son todos problemas que se ven fácilmente al correr la app o al mirar el PR, no estamos hablando de problemas específicos de código que son más difíciles de detectar.
• El procedimiento de que otra persona distinta a la que venía trabajando sobre el PR haga el merge final y solucioné los conflictos, no es una buena práctica. Cada persona debería solucionar sus propios conflictos en el PR.
• Vemos varios PRs no atómicos. Por ejemplo este PR Agregar atributo "dirección" a la clase de Proveedor GonzaloEBaez/vetsoft#8 soluciona cosas de docker que no tienen nada que ver con el issue. Recuerden que cada PR debería solucionar una funcionalidad acotada

Después vemos un muy buen trabajo en general en cuanto a los tests y los commits. En ese sentido sigan así.
Saludos!

[GonzaloEBaez]

Buenas noches.

Estan subidas las correcciones pertinentes.

https://github.com/GonzaloEBaez/vetsoft.git

[ivanduva]

Buenas,
Queda aprobada la actividad.
Igualmente no quería dejar de comentarles ciertos problemas que vimos:

• Seguimos notando problemas en las reviews de los PR. En algunos casos vemos que se comenta pidiendo modificaciones después de haber hecho el merge. Las reviews están justamente para que eso no pase, tal vez el problema esté en que la persona que crea el PR luego lo mergea. Eso no es una buena práctica, debería mergearlo otro integrante cuando se haya finalizado la review y estén hechos los approves correspondientes. También vimos que revirtieron un PR porque fallaba un test. Esto lo mismo, deberían haber corrido los tests antes de mergearlo, lo repetimos varias veces. Además no había necesidad de revertirlo, una vez cometido el error hacen otro PR. El revert es una última instancia.
• Hay algunos tests que podrían ser mejores. Por ejemplo el unitario test_provider_address chequea que la address creada sea igual a la que se pasó por parámetro buscando al provider por address porque se asume que dos providers no van a tener la misma address. No hay ninguna restricción en el modelo a esto, por lo que tranquilamente podrían haber dos providers con la misma dirección. Podrían haber hecho un test que pruebe el mensaje de error cuando la address está vacía.

Saludos