Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avances API Rest, cambios schema y scrapers #19

Merged
merged 51 commits into from
Jul 12, 2022
Merged

Avances API Rest, cambios schema y scrapers #19

merged 51 commits into from
Jul 12, 2022

Conversation

nico-mac
Copy link
Member

@nico-mac nico-mac commented Jun 24, 2022

API Rest:

  • Se agrega un router de Courses
  • Se pluralizan nombres de rutas (subject -> subjects, campus -> campuses)
  • Se agrega endpoint /campuses/places
  • Se agrega endpoint /courses para búsqueda de cursos.
  • Se agrega endpoint /courses/:id
  • Se agrega endpoint /courses/:id/banner para cupos (pendiente)
  • Se agrega endpoint /courses/:id
  • Paginación en endpoint /places
  • Se agrega el endpoint /schools/:id/subjects
  • Se agrega endpoint /subjects/:code
  • Se agrega endpoint /courses/:code/sections

Schema:

  • Se distribuye en módulos por eje temático (events, places, subjects). Queda más ordenado especialmente si pensamos agregar más modelos eventualmente (pienso en agregar modelos para los cupos por ejemplo, eje temático banner)
  • Cambios en los modelos de Subject:
    • Se reemplaza RequirementsRelationEnum por un bool need_all_requirements.
    • Se implementa el many-to-many de equivalencias
    • PROPUESTA: Se cambian los prerequisitos en grupos And y Or por una simple lista (many-to-many), se que es menos completo pero es con afán de simplificarlo un poco y pensando en los usos que podría tener al menos en el corto plazo.
    • Se agregan los campos is_active y equivalencies_raw (estas también pueden ser grupos And y Or en realidad)

Scrapers:

  • Se ajustan al nuevo schema.
  • Se ajustan para ser usados en docker.
  • Para el catálogo primero se "descubren" todos los ramos y luego se rellena la información de requisitos y programa de cada uno. Esto permite que cuando se agregen los prerequisitos los modelos ya existan en la BD, y también podría en un futuro abrir la posibilidad de paralelizar ciertos procesos de scrapping.

Que falta:

  • En la API, que se retornen las relaciones correctamente, ej: en lugar de incluir school_id, incluir {school}. Los mismo con las relaciones one-to-many y many-to-many.
  • Hice algunos chanchullos que me tinca que no son muy buenos para poder correr los scrapers en docker, idealmente que alguien me ayude con eso, no tengo experiencia en Docker.
  • Hacer que las búsquedas de la api sean case-insensite y unaccent.
  • Filtro por horario (tengo un truquillo pensado para implementarlo eficientemente, pero lo dejaría para más adelante).
  • Equivalencias y prerequisitos en los scrapers.
  • Búsqueda por profesores en endpoint /courses

@benjavicente
Copy link
Member

PROPUESTA: Se cambian los prerequisitos en grupos And y Or por una simple lista (many-to-many), se que es menos completo pero es con afán de simplificarlo un poco y pensando en los usos que podría tener al menos en el corto plazo.

Ahí surgiría el problema de como se obtiene cuál es el requisito más actualizado. Creo que lo ideal sería configurar SQLAlchemy / SQLModel para que obtenga esa lista por defecto, así se deja el mismo funcionamiento que hay en plataformas como Planner.

@nico-mac nico-mac marked this pull request as ready for review July 5, 2022 23:14
@nico-mac
Copy link
Member Author

nico-mac commented Jul 5, 2022

Ya está resuelto el tema de la estructura de los prerequisitos. Probé los scrapers y pude construir la base de datos con todo el catálogo y lo que está público del buscacursos (tengo el backup, si alguien lo quiere, pida). Diría que conviene hacer el PR antes de seguir alargando la rama.

@nico-mac nico-mac requested review from benjavicente and FarDust July 5, 2022 23:14
Copy link
Member

@benjavicente benjavicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En general se ven muy buenos los cambios 🚀

Creo que se puede mejorar un poco la legibilidad del código, evitando cosas como mucha anidación o funciones "mágicas", lo que se arreglaría fácilmente (creo) con funciones auxiliares y docstrings.

No he probado el código aún, y en unos días no voy a poder seguir revisando hasta luego de unas 3~4 semanas, así que fuera de esta revisión no creo que pueda revisar o aportar más en estas vacaciones 😬

src/api/routes/courses.py Show resolved Hide resolved
src/scrapers/jobs/catalogo.py Outdated Show resolved Hide resolved
src/scrapers/jobs/catalogo.py Show resolved Hide resolved
src/scrapers/catalogo.py Outdated Show resolved Hide resolved
Copy link
Member

@FarDust FarDust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/api/routes/schools.py Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2022

This pull request introduces 2 alerts and fixes 6 when merging ec4e2bc into 05dc492 - view on LGTM.com

new alerts:

  • 2 for Unused import

fixed alerts:

  • 5 for Unused import
  • 1 for Unused local variable

@FarDust
Copy link
Member

FarDust commented Jul 10, 2022

@nico-mac Resolví todos los conflictos en favor de tu PR, no eh logrado correr el proyecto así que si puedes ver que no se rompiera nada estaría OK!

También eliminé las rutas de facultad pues el modelo no existía.

@benjavicente que falta para dar aprobada esta PR? Porque si funciona la PR es mejor pasarla a main y abrir issues con los problemas que identificaste.

@benjavicente
Copy link
Member

Creo que lo importante es que queden algunas funcionalidades de esta PR más documentadas u ordenadas (principalmente lo de sympy).

Pero como dices podríamos unirla y resolverlos aparte.

@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2022

This pull request introduces 2 alerts and fixes 1 when merging 044d917 into b5667ca - view on LGTM.com

new alerts:

  • 2 for Unused import

fixed alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Jul 11, 2022

This pull request fixes 1 alert when merging 1e02813 into e4b6041 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Jul 11, 2022

This pull request fixes 1 alert when merging 6e2947b into e4b6041 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable

@nico-mac
Copy link
Member Author

Hice issues para todos los cambios pendientes y dejé varios comentarios entre la magia de la conversión de requisitos a DNF. Corregí también un par de alertas del bot lgtm.
Con eso listo, ya estarían las condiciones acordadas para el merge.

@nico-mac nico-mac requested a review from benjavicente July 11, 2022 02:22
@nico-mac nico-mac merged commit 17410b6 into main Jul 12, 2022
@nico-mac nico-mac deleted the refactor-schema branch July 12, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants