[FIX] bedelia_api - Corrige semántica de modalidades, filtro de posprevias y paginación - #2
Merged
Merged
Conversation
Reviewer's GuideFixes modality semantics for course vs exam requirements, corrects unidad_tipo filtering and duplication in the posprevias endpoint, adds a proper Bedelías-reported date field to PosPreviaItem and its loader/serializer, and makes the scraper paginator robust for pages outside the visible window. Sequence diagram for updated scraper paginator navigation in go_to_pagesequenceDiagram
participant Scraper
participant BrowserPaginator
Scraper->>BrowserPaginator: go_to_page(page)
BrowserPaginator->>BrowserPaginator: wait.until(invisibility_of_element_located)
loop until target_xpath visible or steps == 50
BrowserPaginator->>BrowserPaginator: try_find_element(target_xpath)
alt [target not found]
BrowserPaginator->>BrowserPaginator: wait_for_element_to_be_visible(ui-state-active)
BrowserPaginator->>BrowserPaginator: scroll_to_element_and_click(ui-paginator-next/ui-paginator-prev)
BrowserPaginator->>BrowserPaginator: wait_for_page_to_load()
end
end
BrowserPaginator->>BrowserPaginator: scroll_to_element_and_click(target_xpath)
BrowserPaginator->>BrowserPaginator: wait_for_page_to_load()
Flow diagram for unidad_tipo filtering and flattening in posprevias endpointflowchart LR
A[Request with unidad_tipo] --> B[get_queryset]
B --> C[filter PosPreviaItem by unidades_dependientes.tipo]
C --> D[apply distinct on queryset]
D --> E[list]
E --> F[iterate posprevia_item.unidades_dependientes]
F --> G[filter unidades by unidad_tipo]
G --> H[emit flattened entries in response]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
- eligible_courses: un requisito de examen ya no se satisface con solo el curso aprobado; las modalidades de inscripción (course/exam_enrollment) ahora son satisfacibles en vez de excluir el curso permanentemente - API posprevias: el filtro unidad_tipo ahora filtra también las unidades emitidas (antes devolvía todos los tipos) y deduplica con distinct() - PosPreviaItem: nuevo campo fecha con la fecha real de Bedelías; el serializer ya no reporta la fecha de importación como "fecha" - scraper go_to_page: navega por bloques del paginador cuando la página objetivo no está en la ventana visible (antes timeout garantizado) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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
go_to_page, the hard-coded step limit of 50 and the reliance on parsingaria-label/text could be made more robust by extracting this logic into a helper with clearer bounds or retries and handling unexpected label formats explicitly. - When parsing
fechainload_bedelia_data, invalid date strings are silently dropped; consider logging or otherwise surfacing these cases so data issues from Bedelías can be traced rather than failing quietly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `go_to_page`, the hard-coded step limit of 50 and the reliance on parsing `aria-label`/text could be made more robust by extracting this logic into a helper with clearer bounds or retries and handling unexpected label formats explicitly.
- When parsing `fecha` in `load_bedelia_data`, invalid date strings are silently dropped; consider logging or otherwise surfacing these cases so data issues from Bedelías can be traced rather than failing quietly.
## Individual Comments
### Comment 1
<location path="scraper/common/usetable.py" line_range="73-78" />
<code_context>
self.wait.until(EC.invisibility_of_element_located((By.XPATH, '//div[@id="j_idt22_modal"]')))
- self.scroll_to_element_and_click(self.wait_for_element_to_be_clickable((By.XPATH, f'//a[@aria-label="Page {page}"]')))
+ steps = 0
+ while not self.try_find_element((By.XPATH, target_xpath)) and steps < 50:
+ steps += 1
+ current = self.wait_for_element_to_be_visible(
+ (By.XPATH, '//a[contains(@class, "ui-state-active")]')
+ )
+ current_label = current.get_attribute("aria-label") or current.text.strip()
+ current_page = int(current_label.replace("Page", "").strip() or 1)
+ arrow = "ui-paginator-next" if page > current_page else "ui-paginator-prev"
</code_context>
<issue_to_address>
**issue:** Current page parsing can raise ValueError if the label format changes or is non-numeric.
This relies on `current_label.replace("Page", "").strip()` always being numeric. If the aria-label/text changes (translation, extra text, or no number), `int()` will raise and break pagination. Please make the conversion more defensive (e.g., helper that safely defaults to page 1 or regex to extract the numeric portion) so pagination remains robust to label changes.
</issue_to_address>
### Comment 2
<location path="scraper/common/usetable.py" line_range="80-69" />
<code_context>
+ )
+ current_label = current.get_attribute("aria-label") or current.text.strip()
+ current_page = int(current_label.replace("Page", "").strip() or 1)
+ arrow = "ui-paginator-next" if page > current_page else "ui-paginator-prev"
+ self.scroll_to_element_and_click(
+ self.wait_for_element_to_be_clickable((By.XPATH, f'//a[contains(@class,"{arrow}")]'))
+ )
+ self.wait_for_page_to_load()
+ self.scroll_to_element_and_click(self.wait_for_element_to_be_clickable((By.XPATH, target_xpath)))
self.wait_for_page_to_load()
</code_context>
<issue_to_address>
**issue:** Loop relies on next/prev arrows always being present, which can cause hangs at paginator boundaries.
At the first or last page, the chosen arrow link may be disabled or absent, causing `wait_for_element_to_be_clickable` to block until timeout and making the bounded loop fail slowly or incompletely. Consider detecting paginator boundaries (e.g., arrow not found or not enabled) and breaking early with a clear error instead of continuing to loop.
</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
+73
to
+78
| while not self.try_find_element((By.XPATH, target_xpath)) and steps < 50: | ||
| steps += 1 | ||
| current = self.wait_for_element_to_be_visible( | ||
| (By.XPATH, '//a[contains(@class, "ui-state-active")]') | ||
| ) | ||
| current_label = current.get_attribute("aria-label") or current.text.strip() |
There was a problem hiding this comment.
issue: Current page parsing can raise ValueError if the label format changes or is non-numeric.
This relies on current_label.replace("Page", "").strip() always being numeric. If the aria-label/text changes (translation, extra text, or no number), int() will raise and break pagination. Please make the conversion more defensive (e.g., helper that safely defaults to page 1 or regex to extract the numeric portion) so pagination remains robust to label changes.
| self.wait_for_page_to_load() | ||
| # The paginator only renders a window of ~10 page links. If the target | ||
| # link is not visible from the current position, step through blocks | ||
| # with the next/prev arrows until it appears (bounded to avoid looping). |
There was a problem hiding this comment.
issue: Loop relies on next/prev arrows always being present, which can cause hangs at paginator boundaries.
At the first or last page, the chosen arrow link may be disabled or absent, causing wait_for_element_to_be_clickable to block until timeout and making the bounded loop fail slowly or incompletely. Consider detecting paginator boundaries (e.g., arrow not found or not enabled) and breaking early with a clear error instead of continuing to loop.
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
Resuelve los cuatro hallazgos que habían quedado documentados como pendientes en el PR anterior: la lógica curso-vs-examen de
eligible_courses.py, el filtrounidad_tipodel endpoint de posprevias que no filtraba, la fecha de importación reportada como fecha del dato, y la paginación del scraper que no alcanzaba páginas fuera de la ventana visible.Cambios realizados
eligible_courses.py: un requisito de modalidadexamahora solo se satisface con el examen aprobado (antes bastaba el curso, incorrecto en Udelar); un examen aprobado sigue satisfaciendo requisitos de curso (implica la materia aprobada). Las modalidadescourse_enrollment/exam_enrollmentahora se satisfacen con la aprobación correspondiente (antes no matcheaban nunca y excluían el curso permanentemente).views/materias.py):unidad_tipofiltra también las unidades emitidas en ellist()(antes solo acotaba losPosPreviaItemy devolvía unidades de todos los tipos) y agregadistinct()al join M2M que duplicaba filas.PosPreviaItem.fecha(nuevo campo, migración incluida) guarda la fecha que reporta Bedelías; el serializer la expone en el mismo formatodd/mm/yyyy(antes devolvíafecha_creacion, o sea la fecha de importación a la base).usetable.py):go_to_pageavanza/retrocede por bloques del paginador con las flechas hasta que el link de la página objetivo aparece, con tope de intentos (antes, saltar a una página fuera de la ventana visible desde una página distinta de la 1 hacía timeout de 60s garantizado).Validación
compileall, flake8 (selectores del CI) ymanage.py check: sin errores.migrate+ carga REAL completa de los JSON del repo en SQLite: 22 planes, 2407 materias, 106.744 relaciones de posprevias.?materia_code=TI09&unidad_tipo=EXAMENdevuelve solo filas EXAMEN (antes mezclaba CURSO), 0 duplicados, yfecha= 01/01/2007 (la fecha real de Bedelías, no la de hoy).eligible_courses.pyejecutado antes y después con elapproved.csvdel repo: salida idéntica (con estos datos ambas semánticas coinciden), confirmando que el cambio no rompe el caso actual.Notas
load_bedelia_datapara poblar el campofecha(quedanully el serializer lo tolera mientras tanto).🤖 Generated with Claude Code
Summary by Sourcery
Adjust Bedelías modality semantics, posprevia filtering, date handling, and scraper pagination to match real-world behaviour and data.
New Features:
Bug Fixes:
Enhancements: