Course searching fixes #4
Conversation
…oday's date in the get_course_ctgs function
…classifications, and reorder featured list
…red `get_course_ctgs` function to fix variable naming issues. Added `classify_and_sort_courses` function to classify courses based on state and date, including handling for cases with no featured courses. Updated tests to support `start_date` parameter in `CourseFactory.create` calls.
course_classification/helpers.py
Outdated
|
|
||
| # Sorting each state group by proximity to the present | ||
| def sort_key_start(course): |
There was a problem hiding this comment.
Por orden del código estas funciones deberían quedar afuera de la función
course_classification/helpers.py
Outdated
| return float('inf') | ||
|
|
||
| def sort_key_end(course): |
course_classification/helpers.py
Outdated
| return sorted_courses | ||
|
|
||
|
|
course_classification/helpers.py
Outdated
| Classify and sort courses based on their state and proximity to the current date. | ||
| """ | ||
| # Classify courses by state |
There was a problem hiding this comment.
esta comentario quedo como flotando por así decirlo
sort_key function. Minor format fixes
There was a problem hiding this comment.
En general veo que el código está ok, aunque todavía tiene puntos de optimización. El código de la función classify_and_sort_courses es bien caro en tiempo y espacio, y creo que empeorará considerablemente el desempeño de la presentación de cursos featured y por categorías, el cual ya tiene un problema de escala importante. Ese problema se va a ir mostrando gradualmente a medida que crezca la cantidad de cursos.
Los pedidos de este review tienen más que ver con facilitar la comprensión del funcionamiento.
course_classification/helpers.py
Outdated
| if ret_courses['featured']: | ||
| return ret_courses, mc_courses | ||
| del ret_courses['featured'] | ||
| if featured_courses: |
There was a problem hiding this comment.
Esta condición quedaría más clara si explicitara el no-vacío del arreglo. Algo como len(featured_courses) > 0.
course_classification/helpers.py
Outdated
| mc_courses[course.id] = {'name': course_class.MainClass.name,'logo': course_class.MainClass.logo,'days_left': days_left} | ||
| else: | ||
| if course_class.MainClass: | ||
| days_left = (course_start - today).days |
There was a problem hiding this comment.
Este código está duplicado. Todo el if course_class.MainClass: puede ir 1 sola vez fuera del if course_class.is_featured_course: porque se hace idénticamente en el else.
Ojo que esto significa, además, que antes el mc_courses sólo incluía cursos "featured" por tanto hay un cambio de comportamiento en la vista asociada y ahora se agregan todos los cursos, lo cual tiene un impacto en en el uso de memoria del contexto.
Además, tiene un impacto en 2 condiciones para mostrar el curso en https://github.com/open-uchile/open-uchile-theme/blob/main/lms/templates/course.html#L35 y https://github.com/open-uchile/open-uchile-theme/blob/main/lms/templates/course.html#L39 que ahora buscan en un diccionario más grande.
course_classification/helpers.py
Outdated
| categories = CourseCategory.objects.all().order_by('sequence') | ||
| if categories.exists(): | ||
| for main in categories: | ||
| ret_courses[main.id] = {'id':main.id,'name':main.name, 'seq':main.sequence,'show_opt':main.show_opt, 'courses':[]} |
There was a problem hiding this comment.
Para qué es necesario agregar el id de la categoría al objeto si el mismo id está en la llave para indexarlo?
There was a problem hiding this comment.
Confunde mucho que el nombre de la variable para almacenar las categorías se llame ret_courses cuando almacena categorías. Dado que el caso es un else del caso "featured" se podría usar otra variable para evitar esa confusión.
course_classification/helpers.py
Outdated
| # If no featured courses, proceed to build categories | ||
| categories = CourseCategory.objects.all().order_by('sequence') | ||
| if categories.exists(): | ||
| for main in categories: |
There was a problem hiding this comment.
Si se puede renombrar esta variable iterable en el for, sería de ayuda para entender el código. Como está ahora toda categoría es una "main", lo cual es raro. Podría ser cat o ctg.
| pass | ||
| # Apply classification and sorting to courses in each category | ||
| for key in ret_courses: | ||
| if key != 'featured': |
There was a problem hiding this comment.
Me parece que con tu refactor está condición pasa a ser siempre verdadera.
|
|
||
| # Combine the lists | ||
| sorted_courses = ( |
There was a problem hiding this comment.
No entendí para qué trabajar los grupos por separado si dp se van a juntar todos; Qué hace este "combine" exactamente? concatena los arreglos uno tras otro? me hace dudar el paréntesis redondo en el envoltorio.
| mc_courses= {} | ||
| today = timezone.now() # Get timezone-aware current datetime |
There was a problem hiding this comment.
No tengo claro si esta fecha está en UTC o no, y me parece que debería.
Se mejora la lógica en
helpers.pypara clasificar cursos según su estado y fecha, incluyendo casos sin cursos destacados y ordenándolos por proximidad a la fecha actual. También se refactorizan nombres de variables y se actualizan los tests para soportar parámetros de fecha en las pruebas.