search

trabajo-profesional-fiuba / assignment-service

Backend Service in charge of assignments, users, students and more.
https://tpp-g4-fiuba.azurewebsites.net/api/docs
MIT License
1 stars 0 forks source link

#277 Agrego endpoints para sincronizar los slots de disponibilidad del admin, grupo y tutor #281

Closed celedituro closed 3 weeks ago

celedituro commented 4 weeks ago

Closes #277

alejovillores commented 4 weeks ago

Renombrar rama

celedituro commented 4 weeks ago

Che todo bien pero mencioné 30 veces en la reunión en qué consistía ese PUT. No me parece mal lo que planteas pero plantéalo antes porfa porque básicamente queres que borre todo y haga todo de nuevo con otra lógica.

Por otro lado, funciona y es un MVP!!

El El mié, 16 oct 2024 a la(s) 11:34 a. m., Alejo Villores < @.***> escribió:

@.**** requested changes on this pull request.

Esto no es un update sino que es un overwrite, estas borrando lo que habia y agregando lo nuevo (sin updatear).

Bulk update -> https://docs.sqlalchemy.org/en/20/orm/queryguide/dml.html#orm-queryguide-bulk-update

No me parece que sea un PUT un POST porque estas agregando nuevos recursos al sistema. Incluso, se podria modificar los que ya estan para que refleje esto.

Los nombres de las funciones bulkupdate.. no reflejan un bulk update.

Lo que podemos hacer, es en vez de editar como un PUT, que sea un "sobreescribir" de las fechas.

cc @vickyylopezz https://github.com/vickyylopezz, @ivanpfaab https://github.com/ivanpfaab Que les parece?

In src/api/dates/repository.py https://github.com/trabajo-profesional-fiuba/assignment-service/pull/281#discussion_r1803177119 :

  • """
  • Deletes existing slots that are not in updated list and add the new ones.

No se si esta bien el hecho de borrar lo que no esta para updatear.

Un update deberia agregar lo que no esta y updatear lo que esta. Aquello que no esta en slots_to_update no deberia ser borrado. Deberia borrarse mediante otro endpoint

In src/api/dates/repository.py https://github.com/trabajo-profesional-fiuba/assignment-service/pull/281#discussion_r1803179518 :

  • existing_slots = session.query(DateSlot).all()
  • existing_slot_ids = {(slot.slot, slot.period_id) for slot in existing_slots}

Pedi el id si solo usa los ids, no todo el obj

In src/api/dates/repository.py https://github.com/trabajo-profesional-fiuba/assignment-service/pull/281#discussion_r1803188703 :

  • for slot in slots_to_update:
  • if (slot["slot"], slot["period_id"]) not in existing_slot_ids:
  • new_slot = DateSlot(**slot)
  • session.add(new_slot)

Aca agregas los nuevos slots que no habia antes (usa un bulk insert si ya tenes los diccionarios armdos, no un session.add(new_slot) ). Pero en que momento haces un bulk_update??

In src/api/dates/repository.py https://github.com/trabajo-profesional-fiuba/assignment-service/pull/281#discussion_r1803191853 :

  • for slot, tutor_id, period_id in slots_to_delete:
  • delete_stmt = delete(TutorDateSlot).where(
  • TutorDateSlot.period_id == period,
  • TutorDateSlot.tutor_id == tutor_id,
  • TutorDateSlot.slot == slot,
  • )
  • session.execute(delete_stmt)

Hay alguna manera de evitar el for?

— Reply to this email directly, view it on GitHub https://github.com/trabajo-profesional-fiuba/assignment-service/pull/281#pullrequestreview-2372598023, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQAEFLPASRAWYNEMCCQFUSDZ3Z2PHAVCNFSM6AAAAABQAN3WLWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZSGU4TQMBSGM . You are receiving this because you were assigned.Message ID: <trabajo-profesional-fiuba/assignment-service/pull/281/review/2372598023@ github.com>

alejovillores commented 4 weeks ago

@celedituro

Ok. Dejalo pero cambia los nombres de bulk_update pq no son bulk updates, ponele override o algo que represente mejor lo que la funcion hace

alejovillores commented 4 weeks ago

Por favor pone el # de la issue en el titulo