feat(2629): Add stat cards to group view - #2645
Conversation
There was a problem hiding this comment.
student_not_graded reads as 'Not Graded'. this should be then just not_graded, didn't catch this in the other PRs
There was a problem hiding this comment.
This whole section can be under one context called something like 'group statistics'. Also, just have one happy path and one unhappy path spec here testing that they're present, and that they're not if the group hasn't been graded. This controller spec class should test specific controller actions, it can get bloated if we test every single variable and its permutations. You can add one feature spec to test that an average and most/least improved skill are calculated correctly on screen
| @@ -1,4 +1,6 @@ | |||
| ## Unreleased | |||
|
|
|||
| - added stat cards to the group view | |||
There was a problem hiding this comment.
Capitalize first letter
| lesson_url: lesson_path(Lesson.find_by(id: summary.lesson_id)) | ||
| } | ||
| end | ||
| @nr_of_active_students = active_student_count |
There was a problem hiding this comment.
So...this is something that's in reality missing from the group. What we can do here is introduce a method for lazy loading active students in the actual group model. So something similar to what you're doing:
def active_students(as_of: Time.zone.today)
Student
.joins(:enrollments)
.merge(enrollments.active(as_of))
.where(enrollments: { group_id: id }, students: { deleted_at: nil })
.distinct
endThen just remove this line and use that in the group view. Also we should add a spec for this so we know it calculates correctly.
|
|
||
| private | ||
|
|
||
| def active_student_count |
There was a problem hiding this comment.
Remove this whole method after doing the above
| scope :by_group, ->(group_id) { where group_id: } | ||
| # Enrollments that are open at `as_of`: started on/before it and not yet ended (inactive_since is | ||
| # exclusive, matching Student#active_enrollment? and Student.unenrolled_for_organization). | ||
| scope :active, ->(as_of = Time.zone.now) { where('active_since <= ? AND (inactive_since IS NULL OR inactive_since > ?)', as_of, as_of) } |
There was a problem hiding this comment.
This is fine just remove the comments, the scope is already self-explanatory (Also, these comments seem AI generated, although I'm all for using tools to get the job done, try to refrain from leaving everything up to an agent)
| label: t(:overview).capitalize, | ||
| columns: 2, | ||
| stats: [ | ||
| { title: t(:nr_of_active_students), value: @nr_of_active_students }, |
There was a problem hiding this comment.
Change to @group.active_students.count after making the changes from above
| </div> | ||
| </section> | ||
| <turbo-frame id="students-table"> | ||
| <%= render GroupEnrolledStudentsComponent.new(students: @group.students, group: @group, students_with_invalid_grades: @students_with_invalid_grades) %> |
There was a problem hiding this comment.
Also here, switch to @group.active_students after making the changes from above
| deltas_by_skill.map { |(skill_id, skill_name), deltas| { skill_id:, skill_name:, growth: deltas.sum.to_f / deltas.size } } | ||
| end | ||
|
|
||
| def marks_by_student_and_skill |
There was a problem hiding this comment.
This whole calculation seeks for all grades connected to a group, which is incorrect. We want the non-deleted grades for the current active students within lessons of this group, since that's what we're showing. So here you would need to again add some checks to see if the grades were valid, for active students only, and are not deleted. Instead of trying to do this whole thing through ruby, maybe create a separate db view called like 'group_statistics' or something, and see if it's faster that way. I'm not sure if it will be, but try and let me know.
2 participants
Add the following stat cards to the group view
Number of active students
The current average score for the group
Most improved skill
Least improved skill