-
Notifications
You must be signed in to change notification settings - Fork 3
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
Extract order form TreePreloader #665
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrí unas pruebas locales para confirmar lo que suponíamos de que pasarle por parámetro las materias ordenadas es lo mismo que antes no pasar nada:
Antes:
mi-carrera(dev)> TreePreloader.new.preload
Subject Load (15.6ms) SELECT "subjects".* FROM "subjects" WHERE "subjects"."category" IN ('first_semester', 'second_semester', 'third_semester', 'fourth_semester', 'fifth_semester', 'sixth_semester', 'seventh_semester', 'eighth_semester', 'nineth_semester', 'optional', 'extension_module', 'outside_montevideo', 'inactive', 'revalid') ORDER BY CASE WHEN "subjects"."category" = 'first_semester' THEN 1 WHEN "subjects"."category" = 'second_semester' THEN 2 WHEN "subjects"."category" = 'third_semester' THEN 3 WHEN "subjects"."category" = 'fourth_semester' THEN 4 WHEN "subjects"."category" = 'fifth_semester' THEN 5 WHEN "subjects"."category" = 'sixth_semester' THEN 6 WHEN "subjects"."category" = 'seventh_semester' THEN 7 WHEN "subjects"."category" = 'eighth_semester' THEN 8 WHEN "subjects"."category" = 'nineth_semester' THEN 9 WHEN "subjects"."category" = 'optional' THEN 10 WHEN "subjects"."category" = 'extension_module' THEN 11 WHEN "subjects"."category" = 'outside_montevideo' THEN 12 WHEN "subjects"."category" = 'inactive' THEN 13 WHEN "subjects"."category" = 'revalid' THEN 14 END ASC, "subjects"."name" ASC /*application='MiCarrera'*/
Ahora:
- Pasando por parámetro las materias sin ordenar
mi-carrera(dev)> TreePreloader.new.preload
Subject Load (30.4ms) SELECT "subjects".* FROM "subjects" /*application='MiCarrera'*/
- Pasando por parámetro las materias ordenadas
mi-carrera(dev)> TreePreloader.new(Subject.ordered_by_category_and_name).preload
Subject Load (22.9ms) SELECT "subjects".* FROM "subjects" WHERE "subjects"."category" IN ('first_semester', 'second_semester', 'third_semester', 'fourth_semester', 'fifth_semester', 'sixth_semester', 'seventh_semester', 'eighth_semester', 'nineth_semester', 'optional', 'extension_module', 'outside_montevideo', 'inactive', 'revalid') ORDER BY CASE WHEN "subjects"."category" = 'first_semester' THEN 1 WHEN "subjects"."category" = 'second_semester' THEN 2 WHEN "subjects"."category" = 'third_semester' THEN 3 WHEN "subjects"."category" = 'fourth_semester' THEN 4 WHEN "subjects"."category" = 'fifth_semester' THEN 5 WHEN "subjects"."category" = 'sixth_semester' THEN 6 WHEN "subjects"."category" = 'seventh_semester' THEN 7 WHEN "subjects"."category" = 'eighth_semester' THEN 8 WHEN "subjects"."category" = 'nineth_semester' THEN 9 WHEN "subjects"."category" = 'optional' THEN 10 WHEN "subjects"."category" = 'extension_module' THEN 11 WHEN "subjects"."category" = 'outside_montevideo' THEN 12 WHEN "subjects"."category" = 'inactive' THEN 13 WHEN "subjects"."category" = 'revalid' THEN 14 END ASC, "subjects"."name" ASC /*application='MiCarrera'*/
Así que por lo que veo todo bien 👍
Dejo un mini comentario 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
@@ -22,7 +22,7 @@ def render_turbo_stream | |||
if params[:subject_show] == "true" | |||
@subject = @approvable.subject | |||
else | |||
@subjects = TreePreloader.new.preload.select do |subject| | |||
@subjects = TreePreloader.new(Subject.ordered_by_category_and_name).preload.select do |subject| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
si ahora le pasamos siempre los subjects
al TreePreloader
, se puede sacar el default de def initialize(subjects = nil)
y @subjects = subjects || Subject.all
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No me parecía mal dejarlo pero entiendo que si no se usa mejor sacarlo 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 a Santi. Para mi es entendible dejarlo o sacarlo. Como es un contexto controlado donde el TreePreloader
se llama pocas veces, lo podemos sacar en mi opinión. Done
f7c838c
to
6b02d9a
Compare
El
TreePreloader
no tiene porque ordenar las materias. En su lugar, podemos inicializar elTreePreloader
con las materias ordenadas como nos convenga. Esto nos va a ayudar para otros casos de uso donde no queremos que las materias se ordenen por category and name