[FIX] bedelia_api - Corrige CI, bugs del scraper y de la API - #1
Merged
Conversation
- CI: agrega psycopg2-binary faltante, corre con USE_SQLITE y secret de CI, deja de tragar fallos de tests y flake8 (fallaba en todos los pushes) - settings: fallback de SECRET_KEY para desarrollo, registra el exception handler global, actualiza ENUM_NAME_OVERRIDES a PreviaNodo/PreviaItem - models: PosPreviaItem.__str__ referenciaba un campo inexistente - serializers: agrega items_count faltante en PreviaNodoSerializer - views: elimina decorador extend_schema_view huérfano que se apilaba sobre PreviasViewSet - load_bedelia_data: el modo --dry-run crasheaba con instancias sin guardar; agrega guard de carrera/anio en posprevias - scraper: inicializa vigentes_page, screenshot de error antes de cerrar el navegador, exit code 1 al fallar, tope al loop de expansión de credits, guards contra None/IndexError en paginación y filas, XPath acepta Si/Sí, corrige import de common en posprevias y doble slash en URL - requirements raíz: elimina python-dotenv duplicado que rompía pip install - README y QUICKSTART reescritos para reflejar el código real Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideFixes broken CI by configuring a SQLite-backed Django environment, tightens test/lint behavior, and resolves several runtime bugs in the Django API, management command, and scraper, while updating docs to reflect the current data model and usage. Sequence diagram for scraper failure handling with screenshot and exit codesequenceDiagram
participant Main as main
participant Config as ScraperConfig
participant B as Bedelias
participant D as SeleniumDriver
Main->>Config: ScraperConfig()
Main->>B: Bedelias(config)
Main->>B: run()
B->>D: start_driver()
note over B,D: Scraping operations
B-->>B: run() raises Exception
alt driver exists
B->>D: get_screenshot_as_file("screenshot.png")
end
B->>D: quit()
B-->>Main: Exception propagates
Main->>Main: traceback.print_exc()
Main->>Main: sys.exit(1)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
load_bedelia_data, the dry-run branches now instantiatePlanMateriawithout saving in multiple places; consider centralizing this behavior (e.g., via a helper) and double-check that these unsaved instances are never reused in contexts that expect a primary key to avoid subtle bugs if the loader is extended. - Several scraper paths (e.g., credit tree expansion and requirements expansion) still raise generic
Exceptionon control-flow limits; using more specific/custom exception types and including plan/year context in the message would make failures easier to diagnose and handle upstream.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `load_bedelia_data`, the dry-run branches now instantiate `PlanMateria` without saving in multiple places; consider centralizing this behavior (e.g., via a helper) and double-check that these unsaved instances are never reused in contexts that expect a primary key to avoid subtle bugs if the loader is extended.
- Several scraper paths (e.g., credit tree expansion and requirements expansion) still raise generic `Exception` on control-flow limits; using more specific/custom exception types and including plan/year context in the message would make failures easier to diagnose and handle upstream.
## Individual Comments
### Comment 1
<location path="scraper/common/navigation.py" line_range="109-110" />
<code_context>
- # Click on the info icon for the row that has both the matching year AND "Si" in vigente
- xpath = f'//tr[td[1][text()="{plan_year}"]][td[3][text()="Si"]]//i[contains(@class, "pi-info-circle") or contains(@class, "pi-calendar")]'
+ # Click on the info icon for the row that has both the matching year AND "Si"/"Sí" in vigente
+ xpath = (
+ f'//tr[td[1][text()="{plan_year}"]]'
+ f'[td[3][text()="Si" or text()="Sí" or text()="si" or text()="sí"]]'
</code_context>
<issue_to_address>
**nitpick:** Log message is now slightly misleading given the expanded vigente match conditions.
The XPath now accepts `Si`, `Sí`, `si`, and `sí`, but the log still hardcodes `vigente=Si`. Please update the log to reflect the full set of accepted values, or log the actual matched text, so it accurately describes what the scraper selected.
</issue_to_address>
### Comment 2
<location path="scraper/scraper.py" line_range="76" />
<code_context>
- traceback.print_exc()
+ except Exception as e:
+ last_error = e
+ self.logger.warning(f"Failed to click element after 3 attempts: {last_error}")
return False
</code_context>
<issue_to_address>
**suggestion:** Swallowing the exception completely may make diagnosing click failures harder.
Only `last_error` is logged, without a traceback, before returning `False`. For intermittent Selenium issues, the full stack trace is often needed to debug root causes. Please log the traceback (e.g., via `traceback.format_exc()` or `logger.exception` on the final attempt) so we keep full context while still limiting noise from transient failures.
Suggested implementation:
```python
last_error = None
trys_number += 1
try:
self.scroll_to_element(element)
self.wait_for_element_to_be_clickable(element).click()
return True
except Exception as e:
last_error = e
if last_error:
# Log with full traceback on the final failure to aid debugging
self.logger.exception(
"Failed to click element after 3 attempts", exc_info=last_error
)
else:
self.logger.warning("Failed to click element after 3 attempts")
return False
```
1. The snippet suggests this is inside a loop (e.g., `while trys_number < 3:` or `for _ in range(3):`). Ensure `last_error = None` is declared before that loop, not reinitialized on every iteration. If the loop is above this snippet, move `last_error = None` to the beginning of the method or immediately before the retry loop.
2. If your logger is configured to include tracebacks automatically on `logger.exception()` without `exc_info`, you can simplify the call to `self.logger.exception("Failed to click element after 3 attempts")`. Adjust to match any existing logging conventions in `scraper.py`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+109
to
+110
| # Click on the info icon for the row that has both the matching year AND "Si"/"Sí" in vigente | ||
| xpath = ( |
There was a problem hiding this comment.
nitpick: Log message is now slightly misleading given the expanded vigente match conditions.
The XPath now accepts Si, Sí, si, and sí, but the log still hardcodes vigente=Si. Please update the log to reflect the full set of accepted values, or log the actual matched text, so it accurately describes what the scraper selected.
| traceback.print_exc() | ||
| except Exception as e: | ||
| last_error = e | ||
| self.logger.warning(f"Failed to click element after 3 attempts: {last_error}") |
There was a problem hiding this comment.
suggestion: Swallowing the exception completely may make diagnosing click failures harder.
Only last_error is logged, without a traceback, before returning False. For intermittent Selenium issues, the full stack trace is often needed to debug root causes. Please log the traceback (e.g., via traceback.format_exc() or logger.exception on the final attempt) so we keep full context while still limiting noise from transient failures.
Suggested implementation:
last_error = None
trys_number += 1
try:
self.scroll_to_element(element)
self.wait_for_element_to_be_clickable(element).click()
return True
except Exception as e:
last_error = e
if last_error:
# Log with full traceback on the final failure to aid debugging
self.logger.exception(
"Failed to click element after 3 attempts", exc_info=last_error
)
else:
self.logger.warning("Failed to click element after 3 attempts")
return False
- The snippet suggests this is inside a loop (e.g.,
while trys_number < 3:orfor _ in range(3):). Ensurelast_error = Noneis declared before that loop, not reinitialized on every iteration. If the loop is above this snippet, movelast_error = Noneto the beginning of the method or immediately before the retry loop. - If your logger is configured to include tracebacks automatically on
logger.exception()withoutexc_info, you can simplify the call toself.logger.exception("Failed to click element after 3 attempts"). Adjust to match any existing logging conventions inscraper.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumen
El workflow de GitHub Actions fallaba en todos los pushes desde diciembre (faltaba
psycopg2y no había configuración de base de datos para CI). Además se corrigen bugs reales encontrados en una revisión completa: crashes del scraper, un__str__roto, un serializer inválido, el modo--dry-rundel cargador de datos que crasheaba, y documentación que describía código que ya no existe.Cambios realizados
.github/workflows/deploy.yml): renombrado a "CI"; agregaUSE_SQLITE=trueyDJANGO_SECRET_KEYde CI; los fallos de tests y flake8 ya no se tragan con|| echo/|| true; corre también en pull requests.bedelia/requirements.txt: agregapsycopg2-binary(el Dockerfile ya instalabalibpq-devpero faltaba el paquete).settings.py: fallback de desarrollo paraSECRET_KEY, registraglobal_exception_handler(existía pero nunca se usó),ENUM_NAME_OVERRIDESapuntaba a modelos renombrados (RequisitoNodo→PreviaNodo).models.py:PosPreviaItem.__str__usabamateria_dependiente, campo que no existe (crasheaba admin/shell).serializers/materias.py:PreviaNodoSerializerdeclarabaitems_countsin definirlo (error de DRF al usarlo).views/materias.py: elimina un@extend_schema_viewhuérfano que se apilaba sobrePreviasViewSety pisaba su documentación.load_bedelia_data.py:--dry-runcrasheaba (get_or_createcon instancias sin guardar en Django 5); guard decarrera/aniofaltante en posprevias que envenenaba la transacción.vigentes_pagesin inicializar en__init__; screenshot de error ahora se toma antes de cerrar el navegador (antes se intentaba con la sesión ya cerrada); exit code 1 si falla; tope de intentos al expandir el árbol de credits (antes loop infinito posible); guard de paginador sin ancla activa (AttributeError); filas vacías/incompletas en tablas; XPath de vigente aceptaSíademás deSi; import decommonantes delsys.path.appenden posprevias; doble slash en URL;except:desnudos reemplazados por logging del error real.requirements.txtteníapython-dotenvduplicado con versiones en conflicto —pip install -r requirements.txtfallaba directamente.example_usage.pyy una API de constructor que ya no existen; no mencionaba la API Django). QUICKSTART corregido (importaba modelos inexistentesCarrera/Curso/Previa, 5 links muertos, scriptverify_data.pyinexistente).Validación
python -m compileallsobre todo el repo: sin errores.manage.py check: sin issues.manage.py spectacular: el schema OpenAPI genera con 0 errores (antes fallaban los dos enum overrides).manage.py migrate+load_bedelia_data --dry-runcon los JSON reales del repo: completa sin crashear (antes moría conValueError). Los avisos "materia no encontrada" son inherentes al dry-run (no persiste nada).Notas
eligible_courses.pytrata ambas modalidades igual y no contempla modalidades de inscripción; el filtrounidad_tipode posprevias no filtra las unidades emitidas ni deduplica; el cargador colapsa posprevias por(materia, plan)descartandofecha/descripcionpor fila;go_to_pageno navega a páginas fuera de la ventana visible del paginador si no parte de la página 1.🤖 Generated with Claude Code
Summary by Sourcery
Update CI, documentation, scraper robustness, and Django API behavior to fix crashes and ensure the project can be set up and tested reliably.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation: