Skip to content

Integrate HPPS in Grading backend#7923

Open
donnapep wants to merge 7 commits intoadd/hpps-reports-grading-migrationfrom
add/grading-service-migration
Open

Integrate HPPS in Grading backend#7923
donnapep wants to merge 7 commits intoadd/hpps-reports-grading-migrationfrom
add/grading-service-migration

Conversation

@donnapep
Copy link
Copy Markdown
Contributor

@donnapep donnapep commented Mar 25, 2026

Summary

Continues the service abstraction from #7920 by migrating the Grading backend so that all grading data access goes through the appropriate service based on HPPS settings.

Depends on #7920 — this PR targets the add/hpps-reports-grading-migration branch.

Changes

  • Add Grading_Listing_Service_Interface with comments-based and tables-based implementations for fetching paginated grading items
  • Add create_grading_listing_service() factory method to Progress_Query_Service_Factory
  • Refactor Sensei_Grading_Main to use the grading listing service for prepare_items() and get_row_data()
  • Wire Sensei_Grading::count_statuses() through the aggregation service instead of raw SQL
  • Convert Sensei_Temporary_User::filter_count_statuses from raw SQL ($args['query']) to structured exclude_user_login_prefixes and include_statuses_override parameters
  • Apply teacher and temp-user restrictions to tables-based grading listing
  • Add per-status count caching to grading listing service to avoid duplicate queries for tab counts
  • Guard against empty post__in intersection and post_id teacher bypass in count restrictions
  • Simplify tables-based effective_status to COALESCE(q.status, p.status) so quiz progress status takes precedence when it exists, aligning with comments-based behavior

Bug Fixes

  • Fix Grading page status tab counts not updating when filtering by a specific lesson
  • Fix teacher role restrictions not applying to tables-based grading listing

Follow-up

Setup

After checking out this branch, run composer dump-autoload to register the new service classes with the autoloader.

Test plan

1. Grading page — listing

  • Navigate to Grading, note the rows shown (student name, course, lesson, status, grade)
  • Comments-based: Rows display correctly with appropriate statuses
  • Tables-based: Rows display correctly, statuses align with comments-based for lessons with real quizzes
  • Filter by course and lesson — rows filter correctly on both backends
  • Search by student name — results filter correctly on both backends
  • Switch between tabs (All, Ungraded, Graded, In Progress) — counts and rows are consistent
  • Pagination works — navigate between pages, verify item counts

2. Grading page — status tab counts

  • Comments-based: Note the counts in the status tabs (All, Ungraded, Graded, In Progress)
  • Tables-based: Counts are consistent between tabs and pagination

Known difference between storage backends

For lessons whose quiz has no questions, comments-based storage shows "Completed" while tables-based shows "Passed". This is because the migration from comments to tables creates quiz progress with passed status for completed lessons regardless of whether the quiz has questions. This is expected and will be addressed in #7926 by filtering out rows with nothing to grade.

This means the Graded count for tables-based storage could potentially be much higher than comments-based storage (since comments storage has Completed status while tables stores has Passed status), though the totals for tables will now equal the "All" value:

Tables
Screenshot 2026-03-26 at 11 00 27 AM

Comments
Screenshot 2026-03-26 at 11 00 32 AM

3. Grading page — actions

  • Click "Grade quiz" on an ungraded item — the grading page loads correctly
  • Click "Review grade" on a graded item — the review page loads correctly
  • Verify the Grade column shows correct percentages (with decimals) for graded items and "N/A" for ungraded/in-progress

4. Grading page — teacher role

Requires a teacher user who owns some courses but not all courses on the site.

  • Comments-based: Log in as teacher, navigate to Grading. Note which rows are visible and the status tab counts
  • Tables-based: Teacher sees only grading rows for lessons in their own courses
  • Status tab counts reflect only the teacher's courses
  • Filter by course — only the teacher's courses appear in the dropdown

Copilot AI review requested due to automatic review settings March 25, 2026 17:23
@donnapep donnapep added this to the 4.26.0 milestone Mar 25, 2026
@donnapep donnapep self-assigned this Mar 25, 2026
@donnapep donnapep force-pushed the add/hpps-reports-grading-migration branch from 867f59c to 0a44259 Compare March 25, 2026 17:29
@donnapep donnapep force-pushed the add/grading-service-migration branch from deff7f5 to 98ed4a4 Compare March 25, 2026 17:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR continues the HPPS service-layer migration by routing Grading backend data access through a new Grading_Listing_Service_Interface, enabling consistent behavior across comments-based and tables-based (HPPS) storage, and refactoring Grading counts/filters to use service abstractions.

Changes:

  • Introduces Grading_Listing_Service_Interface with comments-based and tables-based implementations, plus a factory method in Progress_Query_Service_Factory.
  • Refactors Sensei_Grading_Main to fetch listing rows and (optionally cached) per-status tab counts via the grading listing service.
  • Refactors Sensei_Grading::count_statuses() to delegate to the progress aggregation service and updates temporary-user filtering to use structured args instead of raw SQL.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit-tests/test-class-grading.php Updates/extends Grading unit tests to cover HPPS tables mode and new restriction behavior.
tests/unit-tests/internal/services/test-class-tables-based-grading-listing-service.php Adds unit tests for tables-based grading listing queries, filtering, pagination correction, and cached status counts.
tests/unit-tests/internal/services/test-class-comments-based-grading-listing-service.php Adds unit tests for comments-based grading listing behavior and quiz-answer exclusion.
includes/internal/services/class-tables-based-grading-listing-service.php Adds HPPS tables-based grading listing implementation with per-status count caching.
includes/internal/services/class-comments-based-grading-listing-service.php Adds comments-based grading listing implementation including SQL-level exclusion of “no answers” quiz items.
includes/internal/services/class-grading-listing-service-interface.php Defines the grading listing service contract used by Grading UI.
includes/internal/services/class-progress-query-service-factory.php Adds create_grading_listing_service() factory method for selecting correct backend.
includes/class-sensei-grading-main.php Refactors listing + row rendering to use Grading_Item and the listing service; adds cached-counts path for tabs.
includes/class-sensei-grading.php Routes count_statuses() through aggregation service and adds optional DI for the service.
includes/class-sensei-temporary-user.php Replaces raw SQL mutation with structured exclusion args for temporary users (+ ungraded override).
config/psalm/psalm-baseline.xml Updates Psalm baseline to account for new types/props and new service files.
changelog/* Adds changelog entries for grading fixes/migration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@donnapep donnapep force-pushed the add/hpps-reports-grading-migration branch from 0a44259 to 4984ead Compare March 25, 2026 17:49
Add Grading_Listing_Service_Interface with comments-based and
tables-based implementations. Refactor Sensei_Grading_Main to use the
listing service for prepare_items() and get_row_data(). Add
create_grading_listing_service() to factory. Apply teacher and
temp-user restrictions to tables-based grading. Add per-status count
caching to avoid duplicate queries for tab counts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@donnapep donnapep force-pushed the add/grading-service-migration branch from 98ed4a4 to d4dcece Compare March 25, 2026 17:50
@donnapep donnapep changed the title Migrate Grading backend to HPPS service layer Integrate HPPS in Grading backend Mar 25, 2026
@donnapep donnapep added the HPPS label Mar 25, 2026
donnapep and others added 5 commits March 26, 2026 09:46
The Grading page exclusion filter that hid lessons without quiz
submissions was too aggressive and caused count mismatches between
storage backends. Remove it in favor of showing all lesson progress
rows, and simplify the tables-based effective_status from
COALESCE(CASE WHEN qs.id IS NOT NULL THEN q.status END, p.status)
to COALESCE(q.status, p.status) so quiz progress status always takes
precedence when it exists. This aligns tables-based status display
with comments-based storage for lessons with real quizzes. Filtering
rows with nothing to grade is tracked in #7926.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix offset correction in both listing services to use ceil-based
  last page calculation, avoiding an empty result when total_count
  is an exact multiple of per_page.
- Skip cached status counts when sensei_grading_count_statuses or
  its deprecated alias has listeners, so third-party filters on
  $count_args are respected.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add factory tests for create_grading_listing_service (both HPPS
  on/off) to ensure correct service type is returned.
- Add Utils::log_query_error() and apply to all wpdb query call sites
  so database failures are logged instead of silently returning empty.
- Fix stale sensei_count_statuses_args filter docblock that still
  referenced "comment query" and only "post_in".

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- "completion date" → "activity date" in clauses service (uses updated_at)
- Clarify lesson_completed_count docblock with specific status list
- Add comment explaining why quiz progress join has no submission guard

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Cast $wpdb->get_col() to (array) to prevent TypeError on PHP 8+
  if the query returns null/false.
- Wrap test assertions in try/finally so the sensei_count_statuses_args
  filter is always cleaned up, even if an assertion fails.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@donnapep donnapep requested a review from mdawaffe March 26, 2026 15:06
@donnapep
Copy link
Copy Markdown
Contributor Author

@merkushin And this one builds on top of #7920 for grading page.

Copy link
Copy Markdown
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

A couple nits.

$query = "SELECT COALESCE( q.status, p.status ) AS effective_status, COUNT( * ) AS total FROM {$table} p";
$query .= " LEFT JOIN {$wpdb->postmeta} pm ON pm.post_id = p.post_id AND pm.meta_key = '_lesson_quiz' AND pm.meta_value > 0";
// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table names from wpdb prefix.
$query .= " LEFT JOIN {$submissions_table} qs ON qs.quiz_id = pm.meta_value AND qs.user_id = p.user_id";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we use qs anymore.

$count_base_query = $this->build_base_query( $table, $submissions_table, $count_args );

// Get per-status counts from a single GROUP BY query.
$status_count_query = "SELECT effective_status, COUNT(*) AS total FROM ( $count_base_query ) AS counted GROUP BY effective_status";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the subquery required here? Naively, that seems like it would be inefficient in at least some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants