From 0d9f9ab7e1235b018e1a409f00d3f77f1852dbed Mon Sep 17 00:00:00 2001 From: POBrien333 <954979+POBrien333@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:15:05 +0200 Subject: [PATCH] Fix course filter reporting zero activity for everyone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filtering the student list by course showed every student as having no activity, no completions and no active days, even where they plainly had. $wpdb->prepare() substitutes placeholders in the order they appear in the SQL text, but base_query() pushed its parameters in the order the clauses were assembled. The course filter is appended to the activity subquery's WHERE clause, which in the finished SQL comes *after* the three window comparisons in the SELECT list. So with a course filter applied the bindings shifted by one: completions_recent event_time >= 8131 <- the course ID active_days_long event_time >= activity WHERE course_id = 1781259752 <- a unix timestamp No row has a course_id equal to a timestamp, so the activity subquery returned nothing, every student left-joined to NULL and the list read as all zeroes. The placeholder count still matched, which is why nothing errored and why the unfiltered list — the only shape exercised during development — was always correct. Parameters are now pushed in SQL-text order, with a comment explaining why the assembly order and the binding order differ. This is the second placeholder-ordering bug in this file, so it also adds tests/placeholder-order.php: a dependency-free regression test that substitutes parameters the way prepare() does and asserts each value is the right kind of value — a timestamp comparison gets a timestamp, a course filter gets a post ID. It reports 9 failures against the 0.1.1 code and passes on this one. A CI workflow runs it, plus a lint pass, on PHP 7.4 and 8.3. Co-Authored-By: Claude Opus 5 --- .distignore | 1 + .gitattributes | 1 + .github/workflows/test.yml | 26 +++++ CHANGELOG.md | 15 +++ includes/Query.php | 14 ++- readme.txt | 9 +- student-activity-masterstudy.php | 4 +- tests/placeholder-order.php | 168 +++++++++++++++++++++++++++++++ 8 files changed, 234 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/test.yml create mode 100644 tests/placeholder-order.php diff --git a/.distignore b/.distignore index 1c9fc35..9fe2938 100644 --- a/.distignore +++ b/.distignore @@ -11,5 +11,6 @@ /.gitignore /.gitattributes /.distignore +/tests /README.md /CHANGELOG.md diff --git a/.gitattributes b/.gitattributes index 39d8f77..981588d 100644 --- a/.gitattributes +++ b/.gitattributes @@ -13,5 +13,6 @@ /.gitignore export-ignore /.gitattributes export-ignore /.distignore export-ignore +/tests export-ignore /README.md export-ignore /CHANGELOG.md export-ignore diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..dcae7e9 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,26 @@ +name: Tests + +on: + push: + branches: [ main ] + pull_request: + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + php: [ '7.4', '8.3' ] + steps: + - uses: actions/checkout@v4 + + - uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + coverage: none + + - name: Lint every PHP file + run: find . -name '*.php' -not -path './.git/*' -print0 | xargs -0 -n1 php -l + + - name: Query placeholder regression test + run: php tests/placeholder-order.php diff --git a/CHANGELOG.md b/CHANGELOG.md index bbbb1ea..83aeecf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to this project are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this project adheres to [Semantic Versioning](https://semver.org/). +## [0.1.2] — 2026-09-10 + +### Fixed +- Filtering the student list by course reported every student as having no + activity at all. `$wpdb->prepare()` fills placeholders in SQL-text order, but + the parameters were pushed in the order the clauses were assembled, so the + course ID landed in a timestamp comparison and a timestamp landed in the course + filter — which matched no rows. The unfiltered list was never affected. + +### Added +- `tests/placeholder-order.php`, a dependency-free regression test asserting that + every substituted value is the right *kind* of value across six query shapes. + It fails against the 0.1.1 code and passes against this one. +- A CI workflow running that test plus a lint pass on PHP 7.4 and 8.3. + ## [0.1.1] — 2026-08-27 Housekeeping release. No functional change: no SQL was altered, and the only diff --git a/includes/Query.php b/includes/Query.php index c4f9d52..249dc07 100644 --- a/includes/Query.php +++ b/includes/Query.php @@ -360,13 +360,25 @@ private static function base_query( array $args ): array { if ( $course_id > 0 ) { $activity_where .= ' AND course_id = %d'; - $params[] = $course_id; } + /* + * prepare() fills placeholders in the order they appear in the SQL TEXT, + * not the order the clauses were assembled in PHP. The three window + * comparisons live in the SELECT list, which precedes the WHERE clause, + * so they must be pushed first — even though $activity_where was built + * above. Getting this backwards silently substitutes a course ID into a + * timestamp comparison and a timestamp into the course filter, which + * matches no rows at all. + */ $params[] = $short_since; $params[] = $short_since; $params[] = $long_since; + if ( $course_id > 0 ) { + $params[] = $course_id; + } + $activity_sub = "SELECT user_id, MAX(event_time) AS last_activity, MIN(event_time) AS first_activity, diff --git a/readme.txt b/readme.txt index 774e08f..79b8ea1 100644 --- a/readme.txt +++ b/readme.txt @@ -4,7 +4,7 @@ Tags: masterstudy, lms, student activity, elearning, engagement Requires at least: 6.0 Tested up to: 7.1 Requires PHP: 7.4 -Stable tag: 0.1.1 +Stable tag: 0.1.2 License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html @@ -71,5 +71,12 @@ A small number of historical usermeta records point at courses that have since b == Changelog == += 0.1.2 = +* Fixed: filtering the student list by course showed every student as having no activity. Query parameters were bound out of order, so the course filter compared against a timestamp and matched nothing. The unfiltered list was never affected. +* Added a regression test and CI covering the query builder. + += 0.1.1 = +* Housekeeping. Corrected several ineffective phpcs:ignore comments, removed a dead method and fixed an invalid readme header. No functional change. + = 0.1.0 = * First public release. diff --git a/student-activity-masterstudy.php b/student-activity-masterstudy.php index 946a83b..33a2775 100644 --- a/student-activity-masterstudy.php +++ b/student-activity-masterstudy.php @@ -3,7 +3,7 @@ * Plugin Name: Student Activity for MasterStudy LMS * Plugin URI: https://github.com/POBrien333/student-activity-masterstudy * Description: Answers "is this student actually active?" — records every lesson view, including the re-watches MasterStudy does not track, and reports last-active date, activity streaks and membership status in one admin screen. - * Version: 0.1.1 + * Version: 0.1.2 * Requires at least: 6.0 * Requires PHP: 7.4 * Author: Patrick O'Brien @@ -32,7 +32,7 @@ exit; } -define( 'MSSA_VERSION', '0.1.1' ); +define( 'MSSA_VERSION', '0.1.2' ); define( 'MSSA_FILE', __FILE__ ); define( 'MSSA_PATH', plugin_dir_path( __FILE__ ) ); define( 'MSSA_URL', plugin_dir_url( __FILE__ ) ); diff --git a/tests/placeholder-order.php b/tests/placeholder-order.php new file mode 100644 index 0000000..06da741 --- /dev/null +++ b/tests/placeholder-order.php @@ -0,0 +1,168 @@ +prepare() fills placeholders in the order they appear in the SQL TEXT, + * not the order the clauses were assembled in PHP. Query::base_query() builds + * its WHERE clauses before the SELECT list that precedes them, so it is easy to + * push parameters in the wrong order — and the failure is silent, because the + * placeholder count still matches. A course ID lands in a timestamp comparison, + * a timestamp lands in the course filter, and the query quietly returns nothing. + * + * That shipped once (0.1.1: filtering by course showed every student as having + * no activity). This asserts the substituted values are the right *kind* of + * value, which catches the whole class rather than one instance. + * + * Run with plain PHP, no dependencies: + * + * php tests/placeholder-order.php + * + * @package StudentActivityForMasterStudy + */ + +define( 'ABSPATH', __DIR__ . '/' ); +define( 'DAY_IN_SECONDS', 86400 ); + +$root = dirname( __DIR__ ) . '/'; + +class Mssa_Test_Wpdb { + public $prefix = 'wp_'; + public $users = 'wp_users'; + public $usermeta = 'wp_usermeta'; + public $posts = 'wp_posts'; + + public function esc_like( $text ) { + return addcslashes( (string) $text, '_%\\' ); + } +} + +$GLOBALS['wpdb'] = new Mssa_Test_Wpdb(); + +function get_option( $name, $default = false ) { + return $default; +} +function wp_parse_args( $args, $defaults = array() ) { + return array_merge( $defaults, (array) $args ); +} +function __( $text, $domain = null ) { + return $text; +} +function esc_html__( $text, $domain = null ) { + return $text; +} + +require $root . 'includes/Settings.php'; +require $root . 'includes/Schema.php'; +require $root . 'includes/Query.php'; + +use StudentActivityForMasterStudy\Query; + +/** Substitute placeholders in SQL-text order, exactly as $wpdb->prepare() does. */ +function mssa_fake_prepare( string $sql, array $args, ?int &$used = null ): string { + $i = 0; + $out = preg_replace_callback( + '/%[dsf]/', + function ( $m ) use ( &$i, $args ) { + $value = $args[ $i ] ?? '<>'; + ++$i; + + return '%d' === $m[0] ? (string) (int) $value : "'" . $value . "'"; + }, + $sql + ); + $used = $i; + + return $out; +} + +$failures = array(); +$checks = 0; + +function mssa_assert( bool $ok, string $what, array &$failures ): void { + global $checks; + ++$checks; + + if ( ! $ok ) { + $failures[] = $what; + } +} + +$cases = array( + 'no filters' => array(), + 'course filter' => array( 'course_id' => 8131 ), + 'course + search' => array( 'course_id' => 8131, 'search' => 'ana' ), + 'course + single user' => array( 'course_id' => 8131, 'user_id' => 414 ), + 'search only' => array( 'search' => 'ana' ), + 'single user only' => array( 'user_id' => 414 ), +); + +$method = new ReflectionMethod( Query::class, 'base_query' ); +$method->setAccessible( true ); + +foreach ( $cases as $label => $overrides ) { + $args = array_merge( Query::default_args(), $overrides ); + $base = $method->invoke( null, $args ); + + $used = 0; + $sql = mssa_fake_prepare( $base['sql'], $base['params'], $used ); + + mssa_assert( + count( $base['params'] ) === $used, + sprintf( '%s: %d placeholders but %d parameters', $label, $used, count( $base['params'] ) ), + $failures + ); + + mssa_assert( + false === strpos( $sql, '<>' ), + $label . ': ran out of parameters part-way through the query', + $failures + ); + + // A timestamp comparison must receive something that looks like a timestamp. + preg_match_all( '/event_time >= (\d+)/', $sql, $times ); + + foreach ( $times[1] as $value ) { + mssa_assert( + (int) $value > 1000000000, + sprintf( '%s: event_time compared against %s, which is not a timestamp', $label, $value ), + $failures + ); + } + + // A course filter must receive something that looks like a post ID. + preg_match_all( '/course_id = (\d+)/', $sql, $courses ); + + foreach ( $courses[1] as $value ) { + mssa_assert( + (int) $value > 0 && (int) $value < 100000000, + sprintf( '%s: course_id compared against %s, which looks like a timestamp', $label, $value ), + $failures + ); + } + + // The long window must reach further back than the short one. + if ( count( $times[1] ) >= 3 ) { + mssa_assert( + (int) $times[1][2] < (int) $times[1][0], + $label . ': the long activity window is not older than the short one', + $failures + ); + } + + printf( " %-22s %2d placeholders, %d parameters\n", $label, $used, count( $base['params'] ) ); +} + +echo "\n"; + +if ( $failures ) { + echo "FAILED (" . count( $failures ) . " of {$checks} checks):\n"; + + foreach ( $failures as $f ) { + echo ' - ' . $f . "\n"; + } + + exit( 1 ); +} + +echo "OK — {$checks} checks passed across " . count( $cases ) . " query shapes.\n"; +exit( 0 );