Fix: Consent tracking, textdomain timing, visitor counting & WP Consent API guard#178
Merged
parhumm merged 11 commits intodevelopmentfrom Mar 12, 2026
Merged
Conversation
added 3 commits
March 12, 2026 12:02
When using WP Consent API integration, user's explicit consent rejection was being overridden by a previous consent upgrade state. This allowed tracking to continue even after user revoked consent. Changed condition from `cmpAllows !== true` to `cmpAllows === null` so that: - Only unknown consent state (null) falls back to previous upgrade - Explicit rejection (false) is now properly respected - Explicit grant (true) continues working as expected
WordPress 6.7.0 requires translations to be loaded at 'init' action or later. Changed hook from 'plugins_loaded' to 'init' with priority 1 to fix the "_load_textdomain_just_in_time was called incorrectly" warning.
When anonymous tracking or IP hashing is enabled, all IPs become identical (hashed/anonymized), causing the Visitors Today counter to show 1-2 instead of actual visitor count. Changed from COUNT(DISTINCT ip) to COUNT(DISTINCT visit_id) to ensure accurate visitor counts regardless of IP privacy settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an on-demand DB index for (dt, visit_id) with an admin AJAX endpoint and notice; switches visitor counters to use distinct Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant Browser as Browser (Admin)
participant WP as WordPress AJAX
participant DB as Database
Note right of AdminUI: Admin-triggered index creation
Browser->>AdminUI: Click "Apply visitor index" notice
AdminUI->>Browser: open confirmation / trigger AJAX
Browser->>WP: POST /wp-admin/admin-ajax.php?action=slimstat_add_dt_visit_index
WP->>DB: CREATE INDEX IF NOT EXISTS idx_dt_visit_id ON slim_stats(dt, visit_id)
DB-->>WP: result (created / exists / error)
WP-->>Browser: JSON response (status, message)
Browser->>AdminUI: display success/error notice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
admin/index.php (1)
1095-1110: Add an index for the newvisit_idaggregates.These admin-bar queries now do
COUNT(DISTINCT visit_id)for today/yesterday on every page that renders the toolbar, but the schema shown here does not add an index for that access pattern. A covering index like(dt, visit_id)would keep the new privacy-safe counts from becoming a hot scan on large sites.As per coding guidelines: Ensure all SlimStat database tables are indexed for safe, fast queries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/index.php` around lines 1095 - 1110, The COUNT(DISTINCT visit_id) queries used by $visitors_today and $visitors_yesterday will cause table scans on large sites because there is no index on visit_id/dt; add a covering index on (dt, visit_id) to the SlimStat visits table in the schema/migration code that creates or updates {$table} (the same place where the table columns are defined or dbDelta is invoked) so the admin-bar queries can use the index for both the date range filter and the distinct visit_id aggregation; ensure the migration safely creates the index if it doesn't exist (or alters the table) so installs and upgrades get the new index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wp-slimstat.js`:
- Around line 1058-1062: The null-guard for preserving a previous consent
upgrade is dead because cmpAllows is already normalized by the earlier fallback;
update the logic so hasConsentUpgradeSucceeded() is consulted before cmpAllows
is overwritten (or preserve the original value in a temp like originalCmpAllows
and use that for the null check). In practice, either move the if (cmpAllows ===
null && hasConsentUpgradeSucceeded()) {...} to run before the fallback that
converts null to true/false, or capture originalCmpAllows = cmpAllows at the
start and replace the current null-check to use originalCmpAllows so that
cmpAllows is only set to true by the upgrade when the original value was null
and hasConsentUpgradeSucceeded() returns true.
In `@wp-slimstat.php`:
- Around line 1600-1601: The change moves load_textdomain() to init which causes
wp_slimstat::init() (and its init_options()) to run before translations are
available, letting option defaults be saved untranslated; to fix, either ensure
translations are loaded before defaults are materialized by calling
wp_slimstat::load_textdomain() earlier (e.g., on plugins_loaded) or modify
init_options() to store raw English defaults and avoid calling translation
functions there (wrap user-facing strings with __()/ _e() only when rendering).
Update references to the hook registration (add_action('init', ['wp_slimstat',
'load_textdomain'], 1)), wp_slimstat::init, and init_options to implement one of
these approaches so localized installs preserve translations.
---
Nitpick comments:
In `@admin/index.php`:
- Around line 1095-1110: The COUNT(DISTINCT visit_id) queries used by
$visitors_today and $visitors_yesterday will cause table scans on large sites
because there is no index on visit_id/dt; add a covering index on (dt, visit_id)
to the SlimStat visits table in the schema/migration code that creates or
updates {$table} (the same place where the table columns are defined or dbDelta
is invoked) so the admin-bar queries can use the index for both the date range
filter and the distinct visit_id aggregation; ensure the migration safely
creates the index if it doesn't exist (or alters the table) so installs and
upgrades get the new index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d26c0a9a-2d1c-455a-96c7-b041f9ece606
⛔ Files ignored due to path filters (2)
wp-slimstat.min.jsis excluded by!**/*.min.jswp-slimstat.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (3)
admin/index.phpwp-slimstat.jswp-slimstat.php
parhumm
reviewed
Mar 12, 2026
| // Visitors Today (unique sessions - using visit_id for anonymous/hashed IP compatibility) | ||
| $visitors_today = (int) $wpdb->get_var($wpdb->prepare( | ||
| "SELECT COUNT(DISTINCT ip) FROM {$table} WHERE dt >= %d", | ||
| "SELECT COUNT(DISTINCT visit_id) FROM {$table} WHERE dt >= %d AND visit_id > 0", |
Contributor
There was a problem hiding this comment.
This silently changes the admin-bar metric from unique visitors to visits. Elsewhere the plugin already defines visit_id as a visit/session counter (admin/view/wp-slimstat-db.php labels it "Visits" and notes that returning visitors are counted multiple times within a day), while unique visitors are still reported via ip. With this change, a returning user who starts multiple sessions in one day will be double-counted under the unchanged Visitors Today label, so the headline analytics number becomes materially misleading.
parhumm
reviewed
Mar 12, 2026
wp-slimstat.js
Outdated
| // Only use previous consent upgrade if current consent is unknown (null) | ||
| // Do NOT override an explicit rejection (false) with a previous consent | ||
| if (cmpAllows !== true && hasConsentUpgradeSucceeded()) { | ||
| if (cmpAllows === null && hasConsentUpgradeSucceeded()) { |
Contributor
There was a problem hiding this comment.
cmpAllows can never be null here anymore because the preceding fallback block always normalizes a null value to true or false. That makes the hasConsentUpgradeSucceeded() recovery path dead code, so a browser that already completed a successful consent-upgrade but whose CMP still returns unknown during early page load will now be treated as denied instead of upgraded. In practice this regresses the post-upgrade recovery flow and can drop full-tracking/pageview data until the CMP initializes again.
parhumm
changed the title
- JS: Move hasConsentUpgradeSucceeded() check before fallback normalization so it's reachable when CMP returns null - PHP: Remove __() from init_options() defaults to avoid saving untranslated strings; chain __() + translateString() at render - PHP: Add Consent::wpHasConsentSafe() helper with wp_get_consent_type() guard and filter cleanup; replace all wp_has_consent() callers - SQL: Add (dt, visit_id) covering index in schema, version-gated migration for upgrades, and admin retry notice for failed DDL - Fix "Deny" → "Decline" label in admin config - Bump version to 5.4.3
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
admin/index.php (1)
1125-1127:⚠️ Potential issue | 🟠 MajorThis now measures sessions, not visitors.
visit_idis a visit/session key, so the unchanged “Visitors Today” UI now overstates repeat users who start multiple sessions. The addedvisit_id > 0filter also drops rows still in the sentinel/unassigned state used insrc/Tracker/Session.php:50-130andsrc/Tracker/Tracker.php:80-130, so the headline count can under-report at the same time. Please either keep a visitor metric here or rename the card/query consistently.Also applies to: 1137-1140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/index.php` around lines 1125 - 1127, The code currently sets $visitors_today by counting DISTINCT visit_id (with a visit_id > 0 filter), but visit_id is a session key and excludes sentinel rows used in Session.php and Tracker.php; either change this to a sessions metric or make it a true visitor metric: Option A — rename the card/variable from $visitors_today to $sessions_today (and update any UI label) so the query remains COUNT(DISTINCT visit_id) and keep the visit_id > 0 filter; Option B — compute unique visitors by counting a stable visitor identifier (e.g. COUNT(DISTINCT COALESCE(user_id, hashed_ip))) against {$table} and remove the visit_id > 0 filter so sentinel rows aren’t dropped; locate $visitors_today, the SQL using visit_id and the UI label and apply one of these fixes consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@admin/index.php`:
- Around line 2457-2474: The nonce name used by ajax_add_dt_visit_index()
doesn't match the notice JS (_wpnonce vs _ajax_nonce) and there's no capability
check for the schema-changing CREATE INDEX; update ajax_add_dt_visit_index() to
validate the same nonce the JS sends (use
wp_verify_nonce($_REQUEST['_ajax_nonce'], 'slimstat_add_dt_visit_index') or
adjust check_ajax_referer to accept '_ajax_nonce') and then enforce a capability
gate (e.g., if (! current_user_can('manage_options')) wp_send_json_error(...))
before running $wpdb->query('CREATE INDEX ...') so only authorized admins can
perform the index creation.
In `@src/Services/GDPRService.php`:
- Around line 226-231: The saved button labels are being wrapped in __() before
calling translateString (for $acceptText and $denyText), which prevents
WPML/Polylang from matching the registered raw source strings; remove the __()
wrapper and pass the raw setting values into translateString (keep
translateString(..., 'gdpr_accept_button_text') and translateString(...,
'gdpr_decline_button_text') respectively) so translateString can find the
originally registered strings and apply custom translations.
In `@wp-slimstat.php`:
- Line 23: Update the plugin header "Version:" field to match the constant
SLIMSTAT_ANALYTICS_VERSION (now '5.4.3') so WordPress sees a single consistent
version; locate the plugin header near the top of wp-slimstat.php (the
"Version:" header line) and change its value from 5.4.2 to 5.4.3 to match
SLIMSTAT_ANALYTICS_VERSION.
---
Duplicate comments:
In `@admin/index.php`:
- Around line 1125-1127: The code currently sets $visitors_today by counting
DISTINCT visit_id (with a visit_id > 0 filter), but visit_id is a session key
and excludes sentinel rows used in Session.php and Tracker.php; either change
this to a sessions metric or make it a true visitor metric: Option A — rename
the card/variable from $visitors_today to $sessions_today (and update any UI
label) so the query remains COUNT(DISTINCT visit_id) and keep the visit_id > 0
filter; Option B — compute unique visitors by counting a stable visitor
identifier (e.g. COUNT(DISTINCT COALESCE(user_id, hashed_ip))) against {$table}
and remove the visit_id > 0 filter so sentinel rows aren’t dropped; locate
$visitors_today, the SQL using visit_id and the UI label and apply one of these
fixes consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c5baf04-9cf8-4ab2-91aa-63b6479e0f51
⛔ Files ignored due to path filters (2)
wp-slimstat.min.jsis excluded by!**/*.min.jswp-slimstat.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (10)
admin/config/index.phpadmin/index.phpsrc/Controllers/Rest/ConsentChangeRestController.phpsrc/Controllers/Rest/ConsentHealthRestController.phpsrc/Providers/IPHashProvider.phpsrc/Services/GDPRService.phpsrc/Tracker/Session.phpsrc/Utils/Consent.phpwp-slimstat.jswp-slimstat.php
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-slimstat.js
7 tests covering: - JS consent upgrade recovery when CMP returns null - JS consent rejection not overridden by upgrade state - Textdomain defaults stored as raw English - Consent::wpHasConsentSafe() helper integrity - All callers migrated from raw wp_has_consent() - (dt, visit_id) index schema + migration
Contributor
E2E QA Report — PR #1787/7 tests passed (32.9s) on Local by Flywheel (WP 6.9.4, SlimStat 5.4.3)
What was validated
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/pr178-consent-fixes.spec.ts (1)
165-173:filesToCheckis narrower than “all PHP callers”.This only guards today's known files. A new raw
wp_has_consent()anywhere else in the plugin will slip through while the test name still reads as exhaustive. Please recurse through the plugin PHP sources and explicitly allow only the wrapper call insidesrc/Utils/Consent.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 165 - 173, The test named 'all PHP callers use wpHasConsentSafe instead of raw wp_has_consent' currently checks a fixed list of files; update it to recursively scan all PHP files under the plugin source (recursing the plugin directory, skipping common ignores like vendor/node_modules) and assert that no file contains the raw function call pattern wp_has_consent\s*\( except for the one allowed occurrence inside src/Utils/Consent.php (the wrapper that exposes wpHasConsentSafe); implement this by replacing the hardcoded filesToCheck with a glob/recursive file walker that collects *.php files, exclude the wrapper file from the failure check, and use a regex search for /wp_has_consent\s*\(/ to fail the test when found elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/pr178-consent-fixes.spec.ts`:
- Around line 47-77: The test currently only sets sessionStorage
('slimstat_consent_upgrade_state') but never forces the CMP to the
null/deny-by-default state, so make the precondition explicit by injecting a CMP
null state before SlimStat initializes (e.g., set window.cmpAllows = null or
otherwise stub the CMP response to null) so hasConsentUpgradeSucceeded()
actually needs to change behavior; then assert a real tracking side effect
rather than just the session flag — check for the tracker having injected
SlimStatParams or a tracking DOM node or a captured network request indicating
tracking fired (use the existing sessionStorage key check plus verification of
window.SlimStatParams or an outgoing request) and keep these checks inside the
same test ('consent upgrade succeeds when CMP returns null').
- Around line 22-37: The test hardcodes BASE_URL, MYSQL_SOCKET and absolute
plugin paths which break portability; update the setup in test.beforeAll to read
BASE_URL and MYSQL_SOCKET (or individual DB params) from environment variables
or existing test helpers, and use the repo-aware path resolver used elsewhere to
locate PHP/plugin source files instead of absolute host paths; change
mysql.createPool invocation to accept env-sourced connection options
(user/password/database/socketPath) so db pool initialization is configurable
across CI and developer machines (look for BASE_URL, MYSQL_SOCKET,
test.beforeAll, mysql.createPool and db in the diff to find where to apply these
changes).
- Around line 224-257: The test currently never calls the real migration and
instead creates the index manually, and it uses a fragile lexicographic string
check; update the test to invoke the actual migration path (either call the PHP
migration entry point that runs update_tables_and_options or simulate the admin
page load that triggers update_tables_and_options) rather than executing CREATE
INDEX directly, then assert that the migration created the index; also replace
the string comparison on the extracted version (versionMatch[1]) with a proper
semantic version comparison (use a semver/compareVersions helper or parse
major/minor/patch) so the test gates the migration the same way the PHP code
does.
- Around line 91-114: The injected WP Consent stub is lost because
page.addScriptTag only affects the current document; replace the use of
page.addScriptTag(...) with page.addInitScript(...) so window.wp_has_consent and
window.wp_consent_type are present after the subsequent page.goto(), and then
strengthen the assertion by checking for a blocking effect (e.g., that no
tracking network requests were made or that the tracker cookie/request is
absent) instead of only asserting
sessionStorage.getItem('slimstat_consent_upgrade_state') remains 'done'; update
references where page.addScriptTag, window.wp_has_consent,
window.wp_consent_type, and the upgradeState assertion are used in the test to
reflect these changes.
---
Nitpick comments:
In `@tests/e2e/pr178-consent-fixes.spec.ts`:
- Around line 165-173: The test named 'all PHP callers use wpHasConsentSafe
instead of raw wp_has_consent' currently checks a fixed list of files; update it
to recursively scan all PHP files under the plugin source (recursing the plugin
directory, skipping common ignores like vendor/node_modules) and assert that no
file contains the raw function call pattern wp_has_consent\s*\( except for the
one allowed occurrence inside src/Utils/Consent.php (the wrapper that exposes
wpHasConsentSafe); implement this by replacing the hardcoded filesToCheck with a
glob/recursive file walker that collects *.php files, exclude the wrapper file
from the failure check, and use a regex search for /wp_has_consent\s*\(/ to fail
the test when found elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88d934d0-94eb-4ba0-b94c-23e964e83f62
📒 Files selected for processing (1)
tests/e2e/pr178-consent-fixes.spec.ts
Comment on lines
+47
to
+77
| test('consent upgrade succeeds when CMP returns null', async ({ page }) => { | ||
| // Visit front-end, inject sessionStorage to simulate prior upgrade, | ||
| // then verify SlimStat sees consent as granted | ||
| await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); | ||
|
|
||
| // Set sessionStorage to simulate a successful prior consent upgrade | ||
| await page.evaluate(() => { | ||
| sessionStorage.setItem('slimstat_consent_upgrade_state', 'done'); | ||
| }); | ||
|
|
||
| // Reload to let SlimStat pick up the upgrade state | ||
| await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); | ||
|
|
||
| // Wait for SlimStat to initialize and check consent | ||
| await page.waitForTimeout(2000); | ||
|
|
||
| // Evaluate the consent decision by checking if tracking fired | ||
| // The consent upgrade check should run BEFORE the fallback normalization | ||
| const consentResult = await page.evaluate(() => { | ||
| // Check if SlimStat set tracking mode | ||
| const ss = sessionStorage.getItem('slimstat_consent_upgrade_state'); | ||
| return { | ||
| upgradeState: ss, | ||
| // Check if SlimStat tracker is active (it injects slimstat[id] in forms/tracking) | ||
| trackerActive: typeof (window as any).SlimStatParams !== 'undefined', | ||
| }; | ||
| }); | ||
|
|
||
| expect(consentResult.upgradeState).toBe('done'); | ||
| // Tracker should be active since upgrade succeeded | ||
| expect(consentResult.trackerActive).toBe(true); |
There was a problem hiding this comment.
This test doesn't establish the null-CMP precondition it claims.
The only controlled input here is the session flag. The test never puts SlimStat into the cmpAllows === null/deny-by-default state, so a permissive site config can still pass without proving that hasConsentUpgradeSucceeded() changed the outcome. Please make the precondition explicit and assert a real tracking side effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 47 - 77, The test
currently only sets sessionStorage ('slimstat_consent_upgrade_state') but never
forces the CMP to the null/deny-by-default state, so make the precondition
explicit by injecting a CMP null state before SlimStat initializes (e.g., set
window.cmpAllows = null or otherwise stub the CMP response to null) so
hasConsentUpgradeSucceeded() actually needs to change behavior; then assert a
real tracking side effect rather than just the session flag — check for the
tracker having injected SlimStatParams or a tracking DOM node or a captured
network request indicating tracking fired (use the existing sessionStorage key
check plus verification of window.SlimStatParams or an outgoing request) and
keep these checks inside the same test ('consent upgrade succeeds when CMP
returns null').
Comment on lines
+91
to
+114
| // Add route interception to inject consent rejection via WP Consent API | ||
| await page.addScriptTag({ | ||
| content: ` | ||
| // Simulate WP Consent API returning false (user rejected) | ||
| window.wp_has_consent = function(category) { return false; }; | ||
| window.wp_consent_type = 'optin'; | ||
| `, | ||
| }); | ||
|
|
||
| await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); | ||
| await page.waitForTimeout(2000); | ||
|
|
||
| // The upgrade state being 'done' should NOT override explicit rejection | ||
| // because the check only applies when cmpAllows === null | ||
| const result = await page.evaluate(() => { | ||
| return { | ||
| upgradeState: sessionStorage.getItem('slimstat_consent_upgrade_state'), | ||
| }; | ||
| }); | ||
|
|
||
| expect(result.upgradeState).toBe('done'); | ||
| // The key assertion: even though upgrade succeeded before, | ||
| // if CMP now says false, tracking should respect that | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pr178-consent-fixes.spec.ts" -type fRepository: wp-slimstat/wp-slimstat
Length of output: 106
🏁 Script executed:
cat -n ./tests/e2e/pr178-consent-fixes.spec.ts | sed -n '80,125p'Repository: wp-slimstat/wp-slimstat
Length of output: 2188
🏁 Script executed:
web_search
Playwright page.addScriptTag vs page.addInitScript behavior navigationRepository: wp-slimstat/wp-slimstat
Length of output: 166
🌐 Web query:
Playwright page.addScriptTag vs page.addInitScript persistent navigation
💡 Result:
Difference in “persistence across navigation”
page.addInitScript(...)is persistent for the lifetime of the Page: Playwright re-injects it whenever the page is navigated, and also when child frames are attached/navigated. It runs after the document is created but before any page scripts run, so it’s suitable for things like monkey-patching globals. [1]page.addScriptTag(...)injects a<script>tag into the currently loaded document (it waits for the tag’sonloador for inline content injection). On a full navigation (new document), the old document is replaced, so that injected tag won’t be there anymore unless you inject it again. [2][3]
Rule of thumb
- Need code to be present on every navigation / before app scripts → use
addInitScript. [1] - Need to load a library once into the current page (after you’re already on it) → use
addScriptTag, and re-run it after each navigation if needed. [2][3]
Sources:
[1] Playwright API: page.addInitScript (runs on every navigation; before page scripts) (playwright.dev)
[2] Playwright API: page.addScriptTag (adds a <script> tag to the page) (playwright.dev)
[3] Playwright “Navigations” (navigation loads a new document) (playwright.dev)
Citations:
- 1: https://playwright.dev/docs/api/class-page
- 2: https://playwright.dev/docs/api/class-page
- 3: https://playwright.dev/docs/navigations
The rejection stub is lost on reload—this test can pass without actually testing rejection.
page.addScriptTag() injects into the currently loaded document only. The subsequent page.goto() at line 100 creates a new document, discarding the injected stub. After navigation, window.wp_has_consent is undefined, so the test never verifies that consent rejection is actually respected. The assertion only checks that the upgrade state marker remains in storage.
Use page.addInitScript() instead, which persists across navigations. Additionally, the assertion should verify a blocking effect (e.g., that tracking requests were not sent) rather than only checking that storage was not modified.
Minimal fix for persistent stub injection
- await page.addScriptTag({
- content: `
- // Simulate WP Consent API returning false (user rejected)
- window.wp_has_consent = function(category) { return false; };
- window.wp_consent_type = 'optin';
- `,
- });
+ await page.addInitScript(() => {
+ (window as any).wp_has_consent = () => false;
+ (window as any).wp_consent_type = 'optin';
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add route interception to inject consent rejection via WP Consent API | |
| await page.addScriptTag({ | |
| content: ` | |
| // Simulate WP Consent API returning false (user rejected) | |
| window.wp_has_consent = function(category) { return false; }; | |
| window.wp_consent_type = 'optin'; | |
| `, | |
| }); | |
| await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); | |
| await page.waitForTimeout(2000); | |
| // The upgrade state being 'done' should NOT override explicit rejection | |
| // because the check only applies when cmpAllows === null | |
| const result = await page.evaluate(() => { | |
| return { | |
| upgradeState: sessionStorage.getItem('slimstat_consent_upgrade_state'), | |
| }; | |
| }); | |
| expect(result.upgradeState).toBe('done'); | |
| // The key assertion: even though upgrade succeeded before, | |
| // if CMP now says false, tracking should respect that | |
| }); | |
| // Add route interception to inject consent rejection via WP Consent API | |
| await page.addInitScript(() => { | |
| (window as any).wp_has_consent = () => false; | |
| (window as any).wp_consent_type = 'optin'; | |
| }); | |
| await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); | |
| await page.waitForTimeout(2000); | |
| // The upgrade state being 'done' should NOT override explicit rejection | |
| // because the check only applies when cmpAllows === null | |
| const result = await page.evaluate(() => { | |
| return { | |
| upgradeState: sessionStorage.getItem('slimstat_consent_upgrade_state'), | |
| }; | |
| }); | |
| expect(result.upgradeState).toBe('done'); | |
| // The key assertion: even though upgrade succeeded before, | |
| // if CMP now says false, tracking should respect that | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 91 - 114, The injected WP
Consent stub is lost because page.addScriptTag only affects the current
document; replace the use of page.addScriptTag(...) with page.addInitScript(...)
so window.wp_has_consent and window.wp_consent_type are present after the
subsequent page.goto(), and then strengthen the assertion by checking for a
blocking effect (e.g., that no tracking network requests were made or that the
tracker cookie/request is absent) instead of only asserting
sessionStorage.getItem('slimstat_consent_upgrade_state') remains 'done'; update
references where page.addScriptTag, window.wp_has_consent,
window.wp_consent_type, and the upgradeState assertion are used in the test to
reflect these changes.
Comment on lines
+224
to
+257
| test('upgrade migration creates the index on version bump', async () => { | ||
| // Check if index already exists | ||
| const [existing] = await db.execute( | ||
| "SHOW INDEX FROM wp_slim_stats WHERE Key_name LIKE '%dt_visit%'" | ||
| ) as any; | ||
|
|
||
| if (existing.length === 0) { | ||
| // Index doesn't exist yet — simulate upgrade by triggering | ||
| // the admin page load which runs update_tables_and_options | ||
| // First, verify version in DB is < 5.4.3 | ||
| const [rows] = await db.execute( | ||
| "SELECT option_value FROM wp_options WHERE option_name = 'slimstat_options'" | ||
| ) as any; | ||
| const raw: string = rows[0].option_value; | ||
| const versionMatch = raw.match(/s:\d+:"version";s:\d+:"([^"]*)"/); | ||
|
|
||
| if (versionMatch && versionMatch[1] < '5.4.3') { | ||
| // Create the index manually to verify it works | ||
| try { | ||
| await db.execute( | ||
| 'CREATE INDEX wp_stats_dt_visit_idx ON wp_slim_stats (dt, visit_id)' | ||
| ); | ||
| } catch (e: any) { | ||
| // Index might already exist with different name | ||
| if (!e.message.includes('Duplicate')) throw e; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Verify index now exists | ||
| const [indexRows] = await db.execute( | ||
| "SHOW INDEX FROM wp_slim_stats WHERE Key_name LIKE '%dt_visit%'" | ||
| ) as any; | ||
| expect(indexRows.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/e2e/pr178-consent-fixes.spec.ts | sed -n '224,257p'Repository: wp-slimstat/wp-slimstat
Length of output: 1641
🏁 Script executed:
# Find PHP files related to database migrations or version checks
fd -e php | head -20Repository: wp-slimstat/wp-slimstat
Length of output: 1125
🏁 Script executed:
# Search for update_tables_and_options function in PHP files
rg "update_tables_and_options" --type php -A 5 -B 2Repository: wp-slimstat/wp-slimstat
Length of output: 1492
🏁 Script executed:
# Search for version_compare usage in database/migration related context
rg "version_compare" --type php -B 3 -A 3Repository: wp-slimstat/wp-slimstat
Length of output: 8593
🏁 Script executed:
# Look for dbDelta usage to understand the migration pattern
rg "dbDelta" --type php -B 3 -A 5Repository: wp-slimstat/wp-slimstat
Length of output: 49
🏁 Script executed:
# Look for context around the test - check if there's setup or if it calls update_tables_and_options
rg "update_tables_and_options" tests/ -B 5 -A 5Repository: wp-slimstat/wp-slimstat
Length of output: 971
🏁 Script executed:
# Check if the test calls any admin_init or migration-triggering logic
rg "admin_init|update_tables_and_options" tests/e2e/pr178-consent-fixes.spec.ts -B 3 -A 3Repository: wp-slimstat/wp-slimstat
Length of output: 404
🏁 Script executed:
# Get more context from the test file around this test
cat -n tests/e2e/pr178-consent-fixes.spec.ts | sed -n '200,260p'Repository: wp-slimstat/wp-slimstat
Length of output: 2903
🏁 Script executed:
# Check if there's setup code or beforeEach hooks in the test file
cat -n tests/e2e/pr178-consent-fixes.spec.ts | head -50Repository: wp-slimstat/wp-slimstat
Length of output: 1971
🏁 Script executed:
# Look for any WordPress admin requests or AJAX calls that might trigger the migration
rg "admin_init|ajax|fetch|request" tests/e2e/pr178-consent-fixes.spec.ts -B 2 -A 2Repository: wp-slimstat/wp-slimstat
Length of output: 201
🏁 Script executed:
# Look for more context around the index migration test - check if there's any admin page load or AJAX call
cat -n tests/e2e/pr178-consent-fixes.spec.ts | sed -n '220,280p'Repository: wp-slimstat/wp-slimstat
Length of output: 2022
🏁 Script executed:
# Search for any HTTP requests or page navigation in the index migration test section
rg "page\.|fetch|request|admin" tests/e2e/pr178-consent-fixes.spec.ts -B 2 -A 2 | grep -A 20 "upgrade migration"Repository: wp-slimstat/wp-slimstat
Length of output: 49
The test doesn't actually invoke the migration function or test the real upgrade path.
The comment claims it will "simulate upgrade by triggering the admin page load which runs update_tables_and_options", but the test never calls that function or navigates to an admin page. Instead, it manually creates the index with SQL. This means a broken PHP migration (using improper version gates or malformed index creation) could still pass, since the test only verifies that manual SQL works, not that the actual migration path works.
Additionally, versionMatch[1] < '5.4.3' is a lexicographic string comparison, not equivalent to PHP's version_compare(). While it happens to work for this specific version string, it's fragile and inconsistent with how the PHP migration actually gates the update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 224 - 257, The test
currently never calls the real migration and instead creates the index manually,
and it uses a fragile lexicographic string check; update the test to invoke the
actual migration path (either call the PHP migration entry point that runs
update_tables_and_options or simulate the admin page load that triggers
update_tables_and_options) rather than executing CREATE INDEX directly, then
assert that the migration created the index; also replace the string comparison
on the extracted version (versionMatch[1]) with a proper semantic version
comparison (use a semver/compareVersions helper or parse major/minor/patch) so
the test gates the migration the same way the PHP code does.
- Add manage_options capability check to all 6 AJAX index handlers - Fix AJAX nonce parameter mismatch in ajax_add_dt_visit_index - Remove __() wrapper before translateString() in GDPRService (fixes WPML) - Add JS-side optin guard in detectWPConsentAPI with try/finally safety - Rename "Visitors Today" to "Sessions Today" (semantic accuracy) - Fix plugin header version 5.4.2 → 5.4.3 - Add E2E integration tests for consent reject/allow/no-cookie scenarios - Rebuild wp-slimstat.min.js with esbuild
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Services/GDPRService.php (1)
226-231:⚠️ Potential issue | 🟡 MinorAdd a gettext fallback for saved default labels.
Now that the defaults are persisted as raw
Accept/Decline, this non-empty branch becomes the normal path. On sites without WPML/Polylang,translateString()just returns the raw English value, so the banner labels never go through WordPress gettext and stay untranslated.💡 Suggested adjustment
- $acceptText = empty($this->settings['gdpr_accept_button_text']) - ? __('Accept', 'wp-slimstat') - : $this->translateString($this->settings['gdpr_accept_button_text'], 'gdpr_accept_button_text'); - $denyText = empty($this->settings['gdpr_decline_button_text']) - ? __('Decline', 'wp-slimstat') - : $this->translateString($this->settings['gdpr_decline_button_text'], 'gdpr_decline_button_text'); + $acceptSource = (string) ($this->settings['gdpr_accept_button_text'] ?? ''); + $denySource = (string) ($this->settings['gdpr_decline_button_text'] ?? ''); + + $acceptText = '' === $acceptSource + ? __( 'Accept', 'wp-slimstat' ) + : $this->translateString( $acceptSource, 'gdpr_accept_button_text' ); + if ( 'Accept' === $acceptSource && 'Accept' === $acceptText ) { + $acceptText = __( 'Accept', 'wp-slimstat' ); + } + + $denyText = '' === $denySource + ? __( 'Decline', 'wp-slimstat' ) + : $this->translateString( $denySource, 'gdpr_decline_button_text' ); + if ( 'Decline' === $denySource && 'Decline' === $denyText ) { + $denyText = __( 'Decline', 'wp-slimstat' ); + }As per coding guidelines, "Wrap all user-facing strings with translation functions; load text domain early (use
wp-slimstatorwp-slimstat-pro)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/GDPRService.php` around lines 226 - 231, The saved default labels are bypassing WordPress gettext because translateString(...) returns raw English; update the non-empty branches for $acceptText and $denyText to pass the translated string through WordPress i18n, e.g. replace the current translateString(...) usage with __($this->translateString($this->settings['gdpr_accept_button_text'], 'gdpr_accept_button_text'), 'wp-slimstat') for $acceptText and similarly wrap translateString(...) for $denyText with __('...', 'wp-slimstat') so the labels go through the WP text domain.
🧹 Nitpick comments (1)
admin/index.php (1)
2505-2508: Consider extracting a dedicated registration function for consistency.The
dt_visitindex handler is registered insideregister_dt_out_index_hooks(), which is slightly inconsistent with other indexes that have their ownregister_*_index_hooks()functions. This works correctly but could be cleaner.♻️ Optional: Separate registration function
public static function register_dt_out_index_hooks() { add_action('wp_ajax_slimstat_add_dt_out_index', [self::class, 'ajax_add_dt_out_index']); - add_action('wp_ajax_slimstat_add_dt_visit_index', [self::class, 'ajax_add_dt_visit_index']); } +public static function register_dt_visit_index_hooks() +{ + add_action('wp_ajax_slimstat_add_dt_visit_index', [self::class, 'ajax_add_dt_visit_index']); +}Then add the call in
init()alongside the otherregister_*_index_hooks()calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/index.php` around lines 2505 - 2508, Extract the dt_visit registration into its own function: create a new public static function register_dt_visit_index_hooks() that contains the add_action('wp_ajax_slimstat_add_dt_visit_index', [self::class, 'ajax_add_dt_visit_index']) call, remove that add_action from register_dt_out_index_hooks(), and add a call to register_dt_visit_index_hooks() inside init() alongside the other register_*_index_hooks() calls; reference ajax_add_dt_visit_index, register_dt_out_index_hooks(), register_dt_visit_index_hooks(), and init() when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/pr178-consent-reject-integration.spec.ts`:
- Around line 26-27: The test hardcodes BASE_URL and MYSQL_SOCKET which will
break on CI/other machines; change the definitions of BASE_URL and MYSQL_SOCKET
in pr178-consent-reject-integration.spec.ts to read from environment variables
(e.g., process.env.TEST_BASE_URL and process.env.MYSQL_SOCKET) with sensible
fallback defaults so the spec can run locally and in CI without editing source;
update any references to BASE_URL and MYSQL_SOCKET to use these env-backed
variables.
---
Outside diff comments:
In `@src/Services/GDPRService.php`:
- Around line 226-231: The saved default labels are bypassing WordPress gettext
because translateString(...) returns raw English; update the non-empty branches
for $acceptText and $denyText to pass the translated string through WordPress
i18n, e.g. replace the current translateString(...) usage with
__($this->translateString($this->settings['gdpr_accept_button_text'],
'gdpr_accept_button_text'), 'wp-slimstat') for $acceptText and similarly wrap
translateString(...) for $denyText with __('...', 'wp-slimstat') so the labels
go through the WP text domain.
---
Nitpick comments:
In `@admin/index.php`:
- Around line 2505-2508: Extract the dt_visit registration into its own
function: create a new public static function register_dt_visit_index_hooks()
that contains the add_action('wp_ajax_slimstat_add_dt_visit_index',
[self::class, 'ajax_add_dt_visit_index']) call, remove that add_action from
register_dt_out_index_hooks(), and add a call to register_dt_visit_index_hooks()
inside init() alongside the other register_*_index_hooks() calls; reference
ajax_add_dt_visit_index, register_dt_out_index_hooks(),
register_dt_visit_index_hooks(), and init() when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24b7798c-a376-45ea-be38-9c539dc5db3d
⛔ Files ignored due to path filters (2)
wp-slimstat.min.jsis excluded by!**/*.min.jswp-slimstat.min.js.mapis excluded by!**/*.map,!**/*.min.js.map
📒 Files selected for processing (5)
admin/index.phpsrc/Services/GDPRService.phptests/e2e/pr178-consent-reject-integration.spec.tswp-slimstat.jswp-slimstat.php
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-slimstat.php
Contributor
✅ E2E QA — Consent Integration (Round 2)Branch: Results: 11/11 PASS (~1m 10s)
Coverage
Analytics Invariants
Round 2 Fixes Validated
|
Contributor
✅ Consent Accept Test AddedAdded missing test: tracking fires when user accepts consent ( Full consent matrix now covered:
All 12 tests pass (5 integration + 7 structural). Commit: |
Extract BASE_URL, MYSQL_SOCKET, WP_ROOT, and DB credentials into helpers/env.ts with process.env overrides. All spec files now import from this single source instead of hardcoding machine-specific values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/pr178-consent-fixes.spec.ts (2)
83-106:⚠️ Potential issue | 🟠 MajorThe consent rejection stub is lost on navigation—this test doesn't verify rejection.
page.addScriptTag()injects into the current document only. Thepage.goto()at line 92 creates a new document, discarding the injectedwp_has_consentandwp_consent_typestubs. The test only verifies thatupgradeStateremains 'done' in sessionStorage, but never actually tests that rejection is respected.Use
page.addInitScript()instead (persists across navigations), and add an assertion that tracking was blocked (e.g., checkSlimStatParamsis undefined or no tracking request was made).🐛 Proposed fix
- // Add route interception to inject consent rejection via WP Consent API - await page.addScriptTag({ - content: ` - // Simulate WP Consent API returning false (user rejected) - window.wp_has_consent = function(category) { return false; }; - window.wp_consent_type = 'optin'; - `, - }); - - await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); + // Use addInitScript to persist the stub across navigation + await page.addInitScript(() => { + (window as any).wp_has_consent = () => false; + (window as any).wp_consent_type = 'optin'; + }); + + await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); await page.waitForTimeout(2000); // The upgrade state being 'done' should NOT override explicit rejection // because the check only applies when cmpAllows === null const result = await page.evaluate(() => { return { upgradeState: sessionStorage.getItem('slimstat_consent_upgrade_state'), + // Verify tracking was actually blocked + trackerBlocked: typeof (window as any).SlimStatParams === 'undefined', }; }); expect(result.upgradeState).toBe('done'); - // The key assertion: even though upgrade succeeded before, - // if CMP now says false, tracking should respect that + // Key assertion: tracking should be blocked despite upgrade state + expect(result.trackerBlocked).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 83 - 106, The injected WP Consent stubs are lost on navigation because page.addScriptTag() only affects the current document; replace the use of page.addScriptTag(...) with page.addInitScript(...) so wp_has_consent and wp_consent_type persist through page.goto(), and add an explicit assertion after navigation to verify tracking was blocked (for example check that SlimStatParams is undefined or that no tracking request was sent) in addition to the existing sessionStorage upgradeState check.
216-255:⚠️ Potential issue | 🟠 MajorThe test manually creates the index instead of testing the actual migration path.
Two issues remain from prior review:
Lexicographic version comparison (line 232):
versionMatch[1] < '5.4.3'is a string comparison, not semantic versioning. While it happens to work for '5.4.2' < '5.4.3', it would fail for edge cases like '5.4.10' (which would incorrectly compare as less than '5.4.3').Manual index creation: The test creates the index via direct SQL (lines 235-237) rather than triggering the actual PHP migration in
update_tables_and_options(). A broken migration function could still pass this test.Consider triggering the migration by loading an admin page that invokes
update_tables_and_options(), then verifying the index was created. For the version comparison, use a proper semver comparison or parse the version parts numerically.🔧 Suggested approach for version comparison
// Simple numeric version comparison function versionLessThan(v1: string, v2: string): boolean { const parts1 = v1.split('.').map(Number); const parts2 = v2.split('.').map(Number); for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) { const p1 = parts1[i] || 0; const p2 = parts2[i] || 0; if (p1 < p2) return true; if (p1 > p2) return false; } return false; } // Then use: if (versionMatch && versionLessThan(versionMatch[1], '5.4.3')) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/pr178-consent-fixes.spec.ts` around lines 216 - 255, The test "upgrade migration creates the index on version bump" incorrectly uses a lexicographic string compare (versionMatch[1] < '5.4.3') and bypasses the real migration by creating the index directly via db.execute; replace the string comparison with a numeric/semver comparison (e.g., parse dot-separated parts and compare as numbers in a helper like versionLessThan) and remove the manual CREATE INDEX call — instead trigger the actual migration path (load the admin page or call the code that invokes update_tables_and_options()) and then assert the index exists; update references to versionMatch, db.execute, and update_tables_and_options in the test accordingly.
🧹 Nitpick comments (1)
tests/e2e/upgrade-safety.spec.ts (1)
20-20: Remove unused importWP_ROOT.
WP_ROOTis imported but not used anywhere in this file.🧹 Suggested fix
-import { BASE_URL, WP_ROOT, MYSQL_CONFIG } from './helpers/env'; +import { BASE_URL, MYSQL_CONFIG } from './helpers/env';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/upgrade-safety.spec.ts` at line 20, The import list includes an unused symbol `WP_ROOT`; remove `WP_ROOT` from the import statement that currently imports `BASE_URL, WP_ROOT, MYSQL_CONFIG` (so the import becomes `BASE_URL, MYSQL_CONFIG`) and run lint/TS check to ensure no other references to `WP_ROOT` exist in `upgrade-safety.spec.ts`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpers/setup.ts`:
- Around line 81-86: The getPool function is not exported but other tests import
it; export getPool so imports succeed—either add the export keyword to the
getPool function declaration (function getPool(...) => export function
getPool(...)) or add an explicit export list that includes getPool (e.g., export
{ getPool, closeDb, ... }) in the same module; update the module where getPool
is defined to export that symbol.
---
Duplicate comments:
In `@tests/e2e/pr178-consent-fixes.spec.ts`:
- Around line 83-106: The injected WP Consent stubs are lost on navigation
because page.addScriptTag() only affects the current document; replace the use
of page.addScriptTag(...) with page.addInitScript(...) so wp_has_consent and
wp_consent_type persist through page.goto(), and add an explicit assertion after
navigation to verify tracking was blocked (for example check that SlimStatParams
is undefined or that no tracking request was sent) in addition to the existing
sessionStorage upgradeState check.
- Around line 216-255: The test "upgrade migration creates the index on version
bump" incorrectly uses a lexicographic string compare (versionMatch[1] <
'5.4.3') and bypasses the real migration by creating the index directly via
db.execute; replace the string comparison with a numeric/semver comparison
(e.g., parse dot-separated parts and compare as numbers in a helper like
versionLessThan) and remove the manual CREATE INDEX call — instead trigger the
actual migration path (load the admin page or call the code that invokes
update_tables_and_options()) and then assert the index exists; update references
to versionMatch, db.execute, and update_tables_and_options in the test
accordingly.
---
Nitpick comments:
In `@tests/e2e/upgrade-safety.spec.ts`:
- Line 20: The import list includes an unused symbol `WP_ROOT`; remove `WP_ROOT`
from the import statement that currently imports `BASE_URL, WP_ROOT,
MYSQL_CONFIG` (so the import becomes `BASE_URL, MYSQL_CONFIG`) and run lint/TS
check to ensure no other references to `WP_ROOT` exist in
`upgrade-safety.spec.ts`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43451e0f-5bac-4b72-96ab-772d40852393
📒 Files selected for processing (9)
tests/e2e/global-setup.tstests/e2e/helpers/env.tstests/e2e/helpers/setup.tstests/e2e/playwright.config.tstests/e2e/pr178-consent-fixes.spec.tstests/e2e/pr178-consent-reject-integration.spec.tstests/e2e/upgrade-data-integrity.spec.tstests/e2e/upgrade-safety.spec.tstests/e2e/visit-id-performance.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/pr178-consent-reject-integration.spec.ts
Comment on lines
81
to
86
| function getPool(): mysql.Pool { | ||
| if (!pool) { | ||
| pool = mysql.createPool({ | ||
| socketPath: MYSQL_SOCKET, | ||
| user: 'root', | ||
| password: 'root', | ||
| database: 'local', | ||
| waitForConnections: true, | ||
| connectionLimit: 5, | ||
| }); | ||
| pool = mysql.createPool(MYSQL_CONFIG); | ||
| } | ||
| return pool; | ||
| } |
There was a problem hiding this comment.
getPool is not exported but is imported by pr178-consent-fixes.spec.ts.
The function getPool is defined without the export keyword, but pr178-consent-fixes.spec.ts line 13 attempts to import it:
import { getPool, closeDb, ... } from './helpers/setup';This will cause a runtime import error.
🐛 Proposed fix
-function getPool(): mysql.Pool {
+export function getPool(): mysql.Pool {
if (!pool) {
pool = mysql.createPool(MYSQL_CONFIG);
}
return pool;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/helpers/setup.ts` around lines 81 - 86, The getPool function is not
exported but other tests import it; export getPool so imports succeed—either add
the export keyword to the getPool function declaration (function getPool(...) =>
export function getPool(...)) or add an explicit export list that includes
getPool (e.g., export { getPool, closeDb, ... }) in the same module; update the
module where getPool is defined to export that symbol.
6 tasks
Merged
This was referenced Mar 15, 2026
This was referenced Mar 19, 2026
fix(tracking): harden user exclusion in server mode with defensive wp_get_current_user() (#246)
#248
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses multiple consent, analytics, and compatibility fixes:
hasConsentUpgradeSucceeded()check before the fallback normalization block so it is reachable when CMP returns null, while explicit rejections (false) are never overriddenwp_get_consent_type()guard (GDPR fix): Added sharedConsent::wpHasConsentSafe()helper that ensureswp_get_consent_type()is set tooptinbefore callingwp_has_consent(). Without this, when CMP has not registered its consent type,wp_has_consent()returnstrueregardless of the deny cookie. All server-side callers now use the safe wrapper.load_textdomain()toinithook (WP 6.7+ requirement) and removed__()frominit_options()defaults so they are not saved untranslated. Render-time translation chains__()+ WPML/PolylangtranslateString().visit_id: Changed admin bar counters fromCOUNT(DISTINCT ip)toCOUNT(DISTINCT visit_id)for accurate counts with anonymized/hashed IPs. Added(dt, visit_id)covering index with version-gated upgrade migration and admin retry notice.Type of change
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores