Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
## Unreleased

- Added stat cards to the group view
- Added stat cards to the student view
- Added stat cards to organization view
- Added a clear icon to search input and removed dead student performance view
Expand Down
33 changes: 33 additions & 0 deletions app/controllers/groups_controller.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# rubocop:disable Metrics/ClassLength
class GroupsController < HtmlController
include Pagy::Method

Expand All @@ -22,6 +23,8 @@ def show
lesson_url: lesson_path(Lesson.find_by(id: summary.lesson_id))
}
end
@current_average_score = @group_summaries.last&.dig(:average_mark)
populate_skill_growth
end

def new
Expand Down Expand Up @@ -107,6 +110,35 @@ def confirm_enrollments

private

def populate_skill_growth
growths = average_growth_per_skill
@most_improved_skill = growths.min_by { |g| [-g[:growth], g[:skill_name], g[:skill_id]] }
@least_improved_skill = growths.min_by { |g| [g[:growth], g[:skill_name], g[:skill_id]] }
end

def average_growth_per_skill
deltas_by_skill = Hash.new { |hash, key| hash[key] = [] }
marks_by_student_and_skill.each do |(_student_id, skill_id, skill_name), marks|
next if marks.size < 2

deltas_by_skill[[skill_id, skill_name]] << (marks.last - marks.first)
end

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

marks = Hash.new { |hash, key| hash[key] = [] }
active_student_ids = @group.active_students.pluck(:id)
@group.valid_grades
.where(student_id: active_student_ids)
.joins(:skill)
.order('lessons.date ASC')
.pluck(:student_id, 'skills.id', 'skills.skill_name', :mark)
.each { |student_id, skill_id, skill_name, mark| marks[[student_id, skill_id, skill_name]] << mark }
marks
end

def group_params
params.require(:group).permit :group_name, :mlid, :chapter_id
end
Expand All @@ -121,3 +153,4 @@ def new_params
params.permit :chapter_id
end
end
# rubocop:enable Metrics/ClassLength
2 changes: 1 addition & 1 deletion app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def name_or_email(user)
end

def lesson_student_average_grade
t :student_not_graded
t :not_graded
end

def student_mini_thumbnail(student)
Expand Down
1 change: 1 addition & 0 deletions app/models/enrollment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Enrollment < ApplicationRecord

scope :by_student, ->(student_id) { where student_id: }
scope :by_group, ->(group_id) { where group_id: }
scope :active, ->(as_of = Time.zone.now) { where('active_since <= ? AND (inactive_since IS NULL OR inactive_since > ?)', as_of, as_of) }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


validates :active_since, presence: true
validates :inactive_since, comparison: { greater_than: :active_since, message: I18n.t(:enrollment_end_before_start) }, allow_nil: true
Expand Down
24 changes: 24 additions & 0 deletions app/models/group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,28 @@ def students_with_grades_outside_enrollment
SQL
)
end

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
end

def valid_grades
Grade.joins(:lesson)
.where(deleted_at: nil, lessons: { group_id: id, deleted_at: nil })
.where(
<<~SQL.squish
EXISTS (
SELECT 1
FROM enrollments e
WHERE e.student_id = grades.student_id
AND e.group_id = lessons.group_id
AND lessons.date BETWEEN e.active_since AND COALESCE(e.inactive_since, 'infinity')
)
SQL
)
end
end
28 changes: 22 additions & 6 deletions app/views/groups/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,29 @@ end %>
<% end %>

<section>
<div class="w-1/2">
<%= render CommonComponents::Card.new(title: t(:average_performance_for_last_30_lessons).capitalize) do |card| %>
<% card.with_card_content do %>
<div id="group-chart" class="ct-chart ct-octave bg-white full-width"></div>
<% end %>
<% end %>
<div class="flex gap-4">
<div class="w-1/2">
<%= render CommonComponents::Card.new(title: t(:average_performance_for_last_30_lessons).capitalize) do |card| %>
<% card.with_card_content do %>
<div id="group-chart" class="ct-chart ct-octave bg-white full-width"></div>
<% end %>
<% end %>
</div>
<div class="w-1/2">
<div class="mt-6">
<%= render CommonComponents::StatCards.new(
label: t(:overview).capitalize,
columns: 2,
stats: [
{ title: t(:nr_of_active_students), value: @group.active_students.count },
{ title: t(:current_average_score_for_group), value: @current_average_score&.round(2) || t(:not_graded) },
{ title: t(:most_improved_skill), value: @most_improved_skill&.dig(:skill_name) || t(:not_graded) },
{ title: t(:least_improved_skill), value: @least_improved_skill&.dig(:skill_name) || t(:not_graded) }
]
) %>
</div>
</div>
</div>
</section>
<turbo-frame id="students-table">
<%= render GroupEnrolledStudentsComponent.new(students: @group.students, group: @group, students_with_invalid_grades: @students_with_invalid_grades) %>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, switch to @group.active_students after making the changes from above

Expand Down
6 changes: 3 additions & 3 deletions app/views/students/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ end %>
label: t(:student_statistics),
stats: [
{ title: t(:nr_of_lessons_present), value: @nr_of_lessons_present },
{ title: t(:total_average_score), value: @total_average_score&.round(2) || t(:student_not_graded) },
{ title: t(:most_improved_skill), value: @most_improved_skill&.dig(:skill_name) || t(:student_not_graded) },
{ title: t(:least_improved_skill), value: @least_improved_skill&.dig(:skill_name) || t(:student_not_graded) }
{ title: t(:total_average_score), value: @total_average_score&.round(2) || t(:not_graded) },
{ title: t(:most_improved_skill), value: @most_improved_skill&.dig(:skill_name) || t(:not_graded) },
{ title: t(:least_improved_skill), value: @least_improved_skill&.dig(:skill_name) || t(:not_graded) }
]
) %>
</div>
Expand Down
3 changes: 2 additions & 1 deletion config/locales/en.yml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

student_not_graded reads as 'Not Graded'. this should be then just not_graded, didn't catch this in the other PRs

Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ en:
student_grades: Student Grades
student_graded: Student graded.
student_graded_text: Student %{student} graded.
student_not_graded: Not Graded
not_graded: Not Graded
add_grade: Add Grade
graded_vs_students_in_group: Graded / Students in Group
back_to_subjects: Back to All Subjects
Expand Down Expand Up @@ -409,6 +409,7 @@ en:
nr_of_users_with_role: Number of users with a role within the Organization
nr_of_lessons_present: Number of lessons present
total_average_score: Total average score
current_average_score_for_group: Current average score for group
most_improved_skill: Most improved skill
least_improved_skill: Least improved skill
overview: Overview
Expand Down
33 changes: 33 additions & 0 deletions spec/controllers/groups_controller_spec.rb

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,39 @@

it { should respond_with 200 }
end

context 'group statistics' do
context 'when the group has graded lessons' do
before :each do
subject = create :subject_with_skills, skill_names: %w[Memorization Grit], organization: @group.chapter.organization
create :lesson_with_grades, group: @group, subject:, date: 2.days.ago,
student_grades: { @student1.id => { 'Memorization' => 1, 'Grit' => 3 } }
create :lesson_with_grades, group: @group, subject:, date: 1.day.ago,
student_grades: { @student1.id => { 'Memorization' => 3, 'Grit' => 6 } }

get :show, params: { id: @group.id }
end

it 'assigns the current average score and the most/least improved skill' do
expect(assigns(:current_average_score)).to be_a(Numeric)
expect(assigns(:most_improved_skill)[:skill_name]).to eq 'Grit'
expect(assigns(:least_improved_skill)[:skill_name]).to eq 'Memorization'
end
end

context 'when the group has no graded lessons' do
before :each do
@ungraded_group = create :group
get :show, params: { id: @ungraded_group.id }
end

it 'assigns nil for the current average score and the skill growth statistics' do
expect(assigns(:current_average_score)).to be_nil
expect(assigns(:most_improved_skill)).to be_nil
expect(assigns(:least_improved_skill)).to be_nil
end
end
end
end

describe '#edit' do
Expand Down
32 changes: 32 additions & 0 deletions spec/features/group_features_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,36 @@
expect(page).to have_selector('span.group > .tooltip', visible: :all, text: expected_alert_text)
end
end

describe 'Group statistics' do
it 'shows active student count, current average, and most/least improved skill for the current roster only' do
group = create :group, group_name: 'Growth Stats Group'
subject = create :subject_with_skills, skill_names: %w[Memorization Grit], organization: group.chapter.organization

active_student = create :student, organization: group.chapter.organization
create :enrollment, student: active_student, group: group, active_since: 1.month.ago.to_date
second_active_student = create :student, organization: group.chapter.organization
create :enrollment, student: second_active_student, group: group, active_since: 1.month.ago.to_date

# This student already left the group. Their grades were valid while they were enrolled, and show a huge
# improvement in Memorization -- if they were wrongly counted, that swing would hijack "most improved".
left_student = create :student, organization: group.chapter.organization
create :enrollment, student: left_student, group: group, active_since: 1.year.ago.to_date, inactive_since: 1.month.ago.to_date
create :lesson_with_grades, group: group, subject:, date: 11.months.ago, student_grades: { left_student.id => { 'Memorization' => 1 } }
create :lesson_with_grades, group: group, subject:, date: 10.months.ago, student_grades: { left_student.id => { 'Memorization' => 7 } }

create :lesson_with_grades, group: group, subject:, date: 2.days.ago,
student_grades: { active_student.id => { 'Memorization' => 1 }, second_active_student.id => { 'Grit' => 1 } }
create :lesson_with_grades, group: group, subject:, date: 1.day.ago,
student_grades: { active_student.id => { 'Memorization' => 2 }, second_active_student.id => { 'Grit' => 3 } }

visit "/groups/#{group.id}"

stat_values = all('dd.tracking-tight').map(&:text)
expect(stat_values[0]).to eq '2' # active students: left_student excluded
expect(stat_values[1]).to eq '2.5' # average of the two active students' marks on the most recent lesson
expect(stat_values[2]).to eq 'Grit' # most improved, among active students only (delta +2, vs Memorization's +1)
expect(stat_values[3]).to eq 'Memorization' # would lose to Memorization's +6 if left_student were wrongly counted
end
end
end
29 changes: 29 additions & 0 deletions spec/models/group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,34 @@
expect(result).not_to include @students[4]
end
end

describe '#active_students' do
before :each do
@group = create :group
@currently_enrolled = create :student, organization: @group.chapter.organization
create :enrollment, group: @group, student: @currently_enrolled, active_since: 1.week.ago

@already_left = create :student, organization: @group.chapter.organization
create :enrollment, group: @group, student: @already_left, active_since: 1.year.ago.to_date, inactive_since: 1.month.ago.to_date

@not_yet_started = create :student, organization: @group.chapter.organization
create :enrollment, group: @group, student: @not_yet_started, active_since: 1.month.from_now.to_date

@deleted_student = create :student, organization: @group.chapter.organization, deleted_at: Time.zone.now
create :enrollment, group: @group, student: @deleted_student, active_since: 1.week.ago
end

it 'returns only students with an open enrollment covering the given date who are not deleted' do
result = @group.active_students

expect(result).to contain_exactly @currently_enrolled
end

it 'accepts an as_of date to check enrollment at a different point in time' do
result = @group.active_students(as_of: 6.months.ago)

expect(result).to contain_exactly @already_left
end
end
end
end
Loading