Add grading stats service for HPPS integration#7931
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new internal “grading stats” abstraction to unify grade totals/averages across legacy comments storage and HPPS tables storage, and updates existing grading/report consumers to use it.
Changes:
- Added
Grading_Stats_Service_Interfaceplus comments-based and tables-based implementations for grade totals and averages. - Wired the new service into
Sensei_Gradingand the Reports Overview services (Courses/Students) viaProgress_Query_Service_Factory. - Added unit tests covering both implementations.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit-tests/internal/services/test-class-tables-based-grading-stats-service.php | Adds unit tests for HPPS tables-based grading stats queries. |
| tests/unit-tests/internal/services/test-class-comments-based-grading-stats-service.php | Adds unit tests for legacy comments-based grading stats queries. |
| includes/reports/overview/services/class-sensei-reports-overview-service-students.php | Switches student-average-grade computation to the new stats service. |
| includes/reports/overview/services/class-sensei-reports-overview-service-courses.php | Switches course-average-grade computation to the new stats service. |
| includes/internal/services/class-tables-based-grading-stats-service.php | Implements grading stats against sensei_lms_progress + sensei_lms_quiz_submissions. |
| includes/internal/services/class-comments-based-grading-stats-service.php | Implements grading stats against wp_comments + wp_commentmeta. |
| includes/internal/services/class-grading-stats-service-interface.php | Defines the consolidated grading stats API. |
| includes/internal/services/class-progress-query-service-factory.php | Adds factory method to select tables vs comments implementation based on HPPS settings. |
| includes/class-sensei-grading.php | Refactors multiple grade stat methods to delegate to the new service. |
| changelog/add-hpps-grading-stats-service | Changelog entry for the internal refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
donnapep
force-pushed
the
add/hpps-grading-stats-service-pr
branch
from
f0f21d5 to
3b3acaa
Compare
|
@merkushin This one addresses Average Grade header values in some reports. |
| "SELECT AVG(course_average) AS courses_average | ||
| FROM ( | ||
| SELECT AVG(cm.meta_value) AS course_average | ||
| FROM %i c | ||
| INNER JOIN %i cm ON c.comment_ID = cm.comment_id | ||
| INNER JOIN %i course ON c.comment_post_ID = course.post_id | ||
| INNER JOIN %i p ON p.ID = course.meta_value | ||
| WHERE c.comment_type = 'sensei_lesson_status' | ||
| AND c.comment_approved IN " . $this->get_graded_statuses_sql() . " | ||
| AND cm.meta_key = 'grade' | ||
| AND course.meta_key = '_lesson_course' | ||
| AND course.meta_value <> '' | ||
| AND EXISTS ( | ||
| SELECT 1 FROM %i cm2 | ||
| WHERE cm2.comment_id = c.comment_ID | ||
| AND cm2.meta_key = 'quiz_answers' | ||
| )", |
There was a problem hiding this comment.
I assume the queries were extracted from the real (production) code, so I don't look at them too precisely 😅
Anyway, need to notice that this condition might cause a performance issue:
p.ID = course.meta_value
—because meta_value doesn't have an index by default.
There was a problem hiding this comment.
This is where I wanted to use parent_post_id from the wp_sensei_lms_progress table, but because a progress row is only created when a student starts on a course, I realized I can't rely on this field in this particular query.
This is going to be a bigger problem when I get to reusable lessons, because I thought I would be able to use parent_post_id for the course-lesson association instead of post meta, but now it seems like I might not be able to. Maybe another new table will be needed for that bit? Not sure, I haven't thought it through fully as I need to get through the HPPS stuff first. 😳
I'll probably need to add some new indexes. I'm just waiting to write all the queries before deciding what's best. Too many architectural decisions. Not my forte! 😅
UPDATE: Oof, I just realized your comment is for the comments-based code, not tables-based. Yes, for the most part the queries were copied out of the old code, though I did change some bits to make them return more reliable data (e.g. instead of checking if a quiz has questions (which can change at any time), I check if the student submitted answers).
There was a problem hiding this comment.
Not sure, I haven't thought it through fully as I need to get through the HPPS stuff first. 😳
I'll probably need to add some new indexes. I'm just waiting to write all the queries before deciding what's best. Too many architectural decisions. Not my forte! 😅
Makes total sense. It's better to get parity between two implementations first.
This is going to be a bigger problem when I get to reusable lessons, because I thought I would be able to use parent_post_id for the course-lesson association instead of post meta, but now it seems like I might not be able to.
I am not sure if I understand why it is a problem. Perhaps I'm missing context.
There was a problem hiding this comment.
@merkushin Let me try to explain with the understanding I have obtained so far. We currently use _lesson_course in post meta to store the course-lesson association, but for reusable lessons I expect we'll want to move away from post meta so that querying will be easier / more performant.
I think we ultimately planned to use the parent_post_id column in the wp_sensei_lms_progress table to store that instead of post meta, probably with a migration which I've done here and here. However, rows are only created in this table if any student has actually made progress. If no student has made progress in a given lesson, then the course-lesson association doesn't exist anywhere (besides in post meta, which we want to eliminate).
So I'm thinking we may need an intermediate table(s) similar to WP Core's term table(s) to store that course-lesson association (maybe the lesson-quiz association too?). In which case perhaps we don't need the parent_post_id column in the progress tables at all? 🤷🏻♀️ Any thoughts on this?
There was a problem hiding this comment.
I think I got it.
It might be a piece of good news or not, depending on what we want to achieve.
Adding parent_post_id might have been a mistake :)
Mostly because it brings the logical course structure to progress.
As you noticed, it might cause complications with using progress in reusable lessons.
Also, I'm 99% sure that it breaks WPML integration :)
And I'm really sorry that I didn't notice it earlier.
Perhaps the proper way of dealing with it is to extract the course structure from the postmeta of courses and lessons. (Actually, besides helping with performance issues, it might later help with solving other issues, including issues in WPML integration and even the simple clarity and observability of the structure :)) I think it is exactly what you called "similar to WP Core's term table(s)".
There was a problem hiding this comment.
Thanks so much for this @merkushin! 🫶 It's great to be able to talk it through with you.
Given this, my plan is to revert the migration and other related changes I've made to integrate parent_post_id, and punt the work of figuring out the course / lesson association out of the HPPS project and into the reusable lessons project.
Create Grading_Stats_Service_Interface with tables-based and comments-based implementations to abstract grade calculation queries behind the HPPS service pattern. This replaces raw SQL in Sensei_Grading (7 methods) and Reports Overview Services (2 methods) with service delegation. The tables-based implementation queries sensei_lms_quiz_submissions.final_grade instead of wp_commentmeta, using the standard quiz-aware join pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace string interpolation of table names with $wpdb->prepare() %i identifier placeholders. This is the proper WordPress approach and eliminates the need for phpcs:disable InterpolatedNotPrepared comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comment_approved IN ('graded','passed','failed') filter to comments-based
get_grade_totals() and get_users_average_grade() to match tables-based behavior.
Add parent_post_id IS NOT NULL filter to tables-based get_courses_average_grade()
to match comments-based non-empty course association requirement.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…yInvalidPropertyFetch errors
…erage Add null check on $result before property access in both implementations of get_courses_average_grade(), consistent with get_grade_totals() pattern. Add tests for: - Status filtering (graded/passed/failed included, others excluded) - Comments-based average-of-averages for get_courses_average_grade - Lessons without quizzes excluded from course average grade Investigation into comment_approved filter behavioral change: The grade comment meta CAN exist with non-graded statuses. In Sensei_Utils::sensei_start_lesson() (line 608-609), when a lesson is completed and has quiz questions, grade meta is set to 0 with status 'complete'. However, this is only an initialization value (grade=0), not a real grade. The status filter (graded/passed/failed) in the new service is therefore a correct tightening that excludes these zero-grade initialization rows. This aligns the comments-based behavior with the tables-based behavior and is effectively a bug fix, not a behavioral regression. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…vice Add short descriptions to @var annotations and reorder phpcs:ignore comments to appear directly before the suppressed line (not before the docblock). Update Psalm baseline for new service files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace duplicated lazy-loading with shared static helpers get_aggregation_service() and get_grading_stats_service(). Remove unused constructor injection for both services. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comments-based queries now require quiz_answers meta to exist, filtering out auto-passed students who never took the quiz. Tables-based get_grade_totals simplified to query quiz progress and submissions directly instead of joining through lesson progress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The quiz_answers EXISTS check makes _quiz_has_questions redundant since having quiz answers implies questions existed. Also simplified tables-based get_users_average_grade to query quiz progress directly, and removed unnecessary COALESCE on quiz status in get_courses_average_grade. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unused _quiz_has_questions meta from comments tests, unused _lesson_course and post_parent from tables tests. Add second lesson to tables AVG-of-AVGs test to match comments test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename WithLessonWithoutQuiz test to WithNoQuizAnswers to reflect what it actually tests. Fix alignment warnings and add missing docblock param. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unused constructor injection and property for grading stats service in both reports overview services. Call factory directly in the single method that needs it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded 'graded', 'passed', 'failed' strings with constants from Quiz_Progress_Interface via a shared helper method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Existing tests create grade meta without quiz_answers, which the new quiz_answers filter excludes. Updated generate_graded_lessons factory helper and assignGrade test helper to include quiz_answers meta for realistic test data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace array spread with array_merge in get_quiz_setting_count and get_quiz_setting_value_count. Psalm 4.x flags array spread of int[] as DuplicateArrayKey on PHP 7.4 since it can't prove the array has only int keys. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify that user_id and lesson_id filters compose correctly without clobbering each other. Mirrors the test in both comments-based and tables-based service test suites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix invalid array shape syntax in docblocks (array { ... } -> array{...})
- Add inline comment explaining the quiz_answers EXISTS clause in
get_grade_totals and get_users_average_grade
- Update get_courses_average_grade docblock to describe how each
implementation enforces the "actually submitted" gate
- Drop redundant '0' === grade_count check (already covered by ! grade_count)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace removed @return variable names with proper descriptions - Use 'float' instead of 'double' (PHP doesn't have a 'double' type) - Return 0.0 instead of int 0 from get_graded_lessons_average_grade zero-count branch for type consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The static cache could return a stale service implementation if the HPPS repository setting is toggled mid-process (e.g., test helpers). Factory instantiation is cheap, so just call it on every access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Per AGENTS.md testing convention, multi-assertion tests should pass a descriptive message to each assertion so failures pinpoint the specific check that failed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix array syntax [] to array() in usage tracking data - Consolidate get_grade_totals query into a single prepare call Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Flip the conditional so the exceptional case (empty) exits early and the main flow handles the positive path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Flip negated checks in get_courses_average_grade and build_post_filter to handle the exceptional case first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
donnapep
force-pushed
the
add/hpps-grading-stats-service-pr
branch
from
e84902a to
287c3e7
Compare
Baseline regen on PHP 7.4 dropped two InvalidScalarArgument occurrences that PHP 8.2 still flags due to int|bool narrowing differences. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WordPress Playground PreviewThe changes in this pull request can previewed and tested using a WordPress Playground instance. 
|
Summary
Grading_Stats_Service_Interfacewith 3 consolidated methods (get_grade_totals,get_courses_average_grade,get_users_average_grade) covering 8 previous use casessensei_lms_quiz_submissions.final_gradeand comments-based implementation with existing SQLSensei_Gradingand grade methods in both Reports Overview ServicesBug Fix
grade=0comment meta (written bySensei_Utils::sensei_start_lesson()on lesson start, not from actual grading) was included in grade averages, dragging them down. The new service filters by grading statuses (graded,passed,failed) to exclude these rows.Setup
After checking out the branch, run
composer dump-autoloadto register the new service classes in the autoload classmap.Test plan
Note that the Average Grade values should either be the same or higher when comparing against trunk, due to the bug fix mentioned above.
🤖 Generated with Claude Code