-
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
semesters to planner #615
semesters to planner #615
Conversation
da39889
to
54a107e
Compare
method: :delete, | ||
data: { controller: "autosave-check" }, | ||
class: 'd-flex', | ||
locale: false, |
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.
creo que este parámetro debería ser local
. Asi que esto ahora se esta ignorando y se puede sacar
@@ -0,0 +1,5 @@ | |||
class AddSemesterToPlannedSubjects < ActiveRecord::Migration[8.0] | |||
def change | |||
add_column :planned_subjects, :semester, :integer |
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.
debería ser nullable este campo?
@planned_subjects, @not_planned_subjects = TreePreloader.new.preload.partition do |subject| | ||
current_student.approved?(subject) || current_user.planned?(subject) | ||
end | ||
@planned_subjects = current_user.planned_subjects.includes(:subject).order(:semester) |
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.
Al no estar usando el Treepreloader no estas cargando los prerrequisitos de estas subjects. Y las que tenes planeadas estamos chequeando si se puede cursar o no en la view asi que se generan N+1 ahi.
|
||
<div class="mdc-deprecated-list-group__subheader mdc-typography--subtitle2"> | ||
<% planned_approvable_ids = planned_subjects.map { |subject| [subject.course.id, subject.exam&.id] }.flatten.compact %> | ||
<% planned_approvable_ids = planned_subjects.map { |planned_subject| [planned_subject.subject.course.id, planned_subject.subject.exam&.id] }.flatten.compact %> |
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.
Vuelvo a comentar que me parece que en lugar de hacer esto, deberiamos hacer un metodo nuevo que acepta subjects y te diga los creditos para esas subjects.
<h3 class="mdc-deprecated-list-group__subheader mdc-typography--subtitle2"> | ||
Materias planeadas | ||
</h3> | ||
<%= render partial: "credits_counter", locals: { planned_subjects: @planned_subjects } %> |
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.
Para mi el partial credits counter deberia tomar subjects no planned subjects, luego le pasas las que quieras.
1ce7233
to
7a4573c
Compare
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.
Bien metida!! 💪
Un buen esfuerzo para seguir avanzando hacia la versión final del planificador. Este laburo aporta pila de valor dado que ahora se pueden separar por semestre y se puede entender cuántos créditos por semestre tenés planificados 👀
Dejo un par de comentarios con algunas sugerencias!
|
||
<div class="mdc-deprecated-list-group__subheader mdc-typography--subtitle2"> | ||
<% planned_approvable_ids = planned_subjects.map { |subject| [subject.course.id, subject.exam&.id] }.flatten.compact %> | ||
<% planned_approvable_ids = subject_plans.map { |subject_plan| [subject_plan.subject.course.id, subject_plan.subject.exam&.id] }.flatten.compact %> |
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.
Esto está generando ahora un N + 1 en la tabla de Approvable la primera vez de se muestran los créditos planeados:
El tema es que deberíamos también hacer un preload de la tabla de approvables
cuando cargamos los subject_plans
, algo así por ejemplo:
subjct_plans = current_user.subject_plans.includes(subject: [:course, :exam])
Cabe mencionar que antes esto no pasaba porque estábamos haciendo el map
a partir de planned_subjects
que se obtenía de lo que devuelve TreePreloader
que ya precarga los approvables
.
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.
Con cargar el course y exam no es suficiente, hay que cargar los prerequisitos tambien. Agregue el commit 788f31f que usa el tree preloader para cargar los prerequisitos de las subjects planificadas. Con eso ya no esta el N+1.
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.
Fua yo siento que evitaría lo más posible seguir añadiendo complejidad al TreePreloader
que me parece ya bastante complejo de entender 😕
Estuve pensando algo así:
@planned_subjects =
TreePreloader
.new(user.planned_subjects.select('subjects.*', subject_plans [:semester]).joins(:subject_plans))
.preload
La única razón, si no entiendo mal, por la que en este PR se cambió para usar los subject_plans
en vez de una colección de subjects
es porque se precisa agruparlos por semestre lo cual esta propuesta soportaría – se puede hacer directamente:
@planned_subjects.group_by(&:semester)
Notése que esta propuesta implica añadir también un has_many
through
en los users:
has_many :planned_subjects, through: :subject_plans, source: :subject
Creo que eso debería andar (haciendo los cambios pertinentes ya que ahora lo que sale del controller es una colección de subjects
). Qué les parece?
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.
Los cambios del TreePreloader los puse en este PR, así movemos la discusión para ahí.
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.
Les parece entonces, para no bloquear este PR, en darle para adelante por ahora con la solución que no cambia nada en el TreePreloader
(o si alguien tiene otra solución para proponer también!). Después con el PR de @RenzoMinelli igual las cosas cambien seguramente.
|
||
<%= render partial: "credits_counter", locals: { planned_subjects: @planned_subjects } %> | ||
<% if @not_planned_approved_subjects.any? %> | ||
<h3 class="mdc-deprecated-list-group__subheader mdc-typography--subtitle2">Materias aprobadas sin semestre asignado</h3> |
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.
Quizás en un futuro podamos hacer que si sabemos a qué semestre pertenece la materia ya la añadimos en ese semestre en el planificador
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.
me gusta la idea si implementamos una manera mas facil de quitar y añadir materias planeadas (un drag and drop e.g.). Puede ser una buena aproximación agregar las materias al semestre que pertenecen pero no sabemos si efectivamente el usuario las hizo en ese por lo que el usuario podria querer cambiarlas de lugar, lo cual en este momento es medio trabajoso. Además habría que implementar una alerta que diga que hicimos esa suposición así el usuario puede ver y confirmar que es correcto.
stopPropagation(e) { | ||
e.preventDefault(); |
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.
Probablemente haya que empezar a pensar en otro nombre para este controlador 🤔
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! estuve intentando crear otro controlador pero no me hacía sentido. Cambiar el nombre de este en un futuro me parece bien
def semester_display_name(semester) | ||
case semester | ||
when 1 then 'Primer semestre' | ||
when 2 then 'Segundo semestre' | ||
when 3 then 'Tercer semestre' | ||
when 4 then 'Cuarto semestre' | ||
when 5 then 'Quinto semestre' | ||
when 6 then 'Sexto semestre' | ||
when 7 then 'Séptimo semestre' | ||
when 8 then 'Octavo semestre' | ||
when 9 then 'Noveno semestre' | ||
when 10 then 'Décimo semestre' | ||
end | ||
end |
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.
Estamos introduciendo el concepto de 'semestre'` cuando ya tenemos el concepto de 'categoría' que para una de las cosas que se usa es para marcar el semestre al cual pertenece la materia en la página principal
No me parece un blocker para este PR, pero seamos consientes de que esto quizá vuelva el código más confuso y difícil de entender.
Quizás en el futuro podamos extraer el concepto de semestre del campo de category
y añadirlo como una columna por sí mismo en los subjects
también 🤔
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.
planteé esto en la descripción del PR. Me parece algo a tener en cuenta si. Sin embargo, me parece que son cosas diferentes. Un semestre debería ser un número sin más y las categorías puede haber otras un poco más extrañas como outside_montevideo
. Me parece que en un futuro lo que va a haber es un mapping de uno a otro que no lo veo mal. No creo que complejice el código.
<%= | ||
form.select( | ||
:semester, | ||
options_for_select((1..10).map{|i| ["Sem. #{i}", i]}), | ||
{}, | ||
data: { action: "click->autosave-check#stopPropagation" } | ||
) | ||
%> |
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.
Probaste usar los dropdown menus de Material UI? Capaz intentaría darle una vichada y si no es mucho laburo intentaría usarlos a ver cómo queda 🤔
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.
probé sin éxito. Yo diría de mergear este PR así como está y en la Hackathon de mañana dividir el trabajo en
- cambiar lo de mostrar todas las materias a un solo campo con un dropdown selector
- pasar los/el dropdown selector al MaterialUI menu
Que decis? me parece que el objetivo de este PR era hacer funcional la división por semestres
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.
Posteate el diff de lo que probaste así lo usamos como punto de arranque (o incluso podemos verlo por arriba a ver si nos damos cuenta de qué es lo que está fallando)
ccfb6c9
to
788f31f
Compare
Co-authored-by: Santiago Rodriguez <46354312+santiagorodriguez96@users.noreply.github.com>
5dc28e8
to
a75157c
Compare
4e0841f
to
170445d
Compare
Co-authored-by: Santiago Rodriguez <46354312+santiagorodriguez96@users.noreply.github.com>
0298103
to
7e3adfc
Compare
Purpose
Segunda iteración del planner
Detalles
semester
aplanned_subjects
.Open questions
semester: 1, year: 2025
en caso de que quieramos modelar algo más general en el futuro y no hardcodear X semestres.category
usa strings y tenemos valores del estilofirst_semester
. Tandría sentido matchear estos valores ensemester
por alguna razón? Creo que no por lo que sigo prefiriendo elint
pero capaz hay cosas que no tengo en cuenta.Sem. 1
por que poner elsemester_display_name
ocupaba mucho espacio y algunos nombres de materias quedaban muy cortados. Algunas sugerencia para mejorar esto?