tests/e2e/tracking-request-methods.spec.ts (2)

333-338: Redundant conditional assertion.

The if (ajaxFallbackUsed) { expect(...).toBe(true) } block is always true when entered, making the expect redundant. Consider either removing the conditional wrapper or making the assertion unconditional if AJAX fallback is the expected behavior.

♻️ Simplify the assertion

       const stat = await waitForStatRow(marker, 15_000);
       expect(stat).toBeTruthy();
       expect(stat!.resource).toContain(marker);

       // When REST is blocked, AJAX should be used as fallback
       // (unless server-side tracking captured it first)
-      if (ajaxFallbackUsed) {
-        expect(ajaxFallbackUsed).toBe(true);
-      }
+      // Note: server-side tracking may capture the hit before client fallback triggers,
+      // so we only verify the pageview was recorded, not necessarily via AJAX.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/tracking-request-methods.spec.ts` around lines 333 - 338, The
conditional wrapper around the assertion is redundant: remove the surrounding
"if (ajaxFallbackUsed) { ... }" and make the assertion unconditional so the test
directly asserts expect(ajaxFallbackUsed).toBe(true); locate the check using the
ajaxFallbackUsed variable in the test case (in
tests/e2e/tracking-request-methods.spec.ts) and replace the if-block with a
single unconditional expect call.

---

124-154: Test title doesn't match assertions.

The test is titled "REST response body contains valid tracking ID with checksum" but the assertion only verifies the response doesn't contain "Internal Server Error". It doesn't actually validate that the response contains a numeric ID or checksum format.

Consider either updating the test title to match what it actually tests, or adding an assertion that validates the response format:

♻️ Option 1: Update title to match behavior

-    test('REST response body contains valid tracking ID with checksum', async ({ page }) => {
+    test('REST response body does not contain server error', async ({ page }) => {

♻️ Option 2: Add format validation assertion

       // If we captured the response, verify it contains a numeric ID
       // REST response wraps the result in JSON — the body should contain a numeric string
       if (restResponseBody) {
         // Response should be parseable (not a 500 error page)
         expect(restResponseBody).not.toContain('Internal Server Error');
+        // Response should contain a numeric tracking ID
+        const parsed = JSON.parse(restResponseBody);
+        expect(typeof parsed === 'string' || typeof parsed === 'number').toBe(true);
+        expect(parseInt(String(parsed), 10)).toBeGreaterThan(0);
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/tracking-request-methods.spec.ts` around lines 124 - 154, The test
named "REST response body contains valid tracking ID with checksum" currently
only checks that restResponseBody doesn't contain 'Internal Server Error';
update behavior to match the title by parsing restResponseBody (captured in the
page.on('response') handler) and asserting the tracking ID and checksum format
(e.g., numeric ID and expected checksum pattern) are present, or alternatively
change the test title to accurately reflect the current assertion; reference the
test function (the test(...) block), the restResponseBody variable, and
waitForStatRow(marker) to locate where to add the parsing/format assertion or
rename the test.

src/Controllers/Rest/TrackingRestController.php (1)

188-206: Consider using Yoda conditions for comparisons.

The coding guidelines specify using Yoda conditions (if ( 'value' === $var )). Lines 192 and 197 use standard comparison order.

♻️ Yoda style refactor

-        if (is_numeric($result) && (int) $result > 0) {
+        if (is_numeric($result) && 0 < (int) $result) {
             return rest_ensure_response((string) $result);
         }

         // Success: checksum-formatted string "<id>.<hash>" from Utils::getValueWithChecksum()
-        if (is_string($result) && preg_match('/^(\d+)\.[0-9a-fA-F]+$/', $result, $matches) && (int) $matches[1] > 0) {
+        if (is_string($result) && preg_match('/^(\d+)\.[0-9a-fA-F]+$/', $result, $matches) && 0 < (int) $matches[1]) {
             return rest_ensure_response($matches[1]);
         }

As per coding guidelines: "Use Yoda Conditions: if ( 'value' === $var )"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Controllers/Rest/TrackingRestController.php` around lines 188 - 206, The
numeric comparisons in the tracking response handling should use Yoda
conditions: update the checks that currently read (int) $result > 0 and (int)
$matches[1] > 0 to place the literal first (e.g. 0 < (int) $result and 0 < (int)
$matches[1]) in the block around Tracker::slimtrack_ajax(), keeping the same
surrounding guards (is_numeric($result), is_string($result) && preg_match(...,
$matches)) and returning via rest_ensure_response or the WP_Error unchanged.

src/Controllers/Rest/TrackingRestController.php (1)

203-208: Consider returning semantically correct HTTP status codes instead of generic 400.

The current implementation returns 400 for all non-success cases, but Ajax.process() returns distinct negative codes with semantic meaning:

Return Value	Meaning	Current	Suggested
-429	Rate limited	400	429
-204	Tracking disabled	400	204
0	Tracking failed	400	400 ✓

Returning 400 for rate limiting may cause the JS tracker to retry aggressively (treating it as a transient failure), and triggering fallback methods when tracking is server-side disabled is wasteful since those will also fail.

If the current behavior is intentional for fallback triggering, consider at minimum passing the original error code in the response data for debugging:

Suggested improvement

+        // Map negative error codes to appropriate HTTP status
+        if (is_int($result) && $result < 0) {
+            $error_code = abs($result);
+            // Preserve semantic meaning for rate limiting
+            if (429 === $error_code) {
+                return new \WP_Error(
+                    'slimstat_rate_limited',
+                    esc_html__('[REST API] Rate limited.', 'wp-slimstat'),
+                    ['status' => 429]
+                );
+            }
+            // Tracking intentionally suppressed (disabled, opt-out, etc.)
+            if (204 === $error_code) {
+                return new \WP_REST_Response(null, 204);
+            }
+        }
+
         // If no valid tracking ID detected, return a non-200 status to trigger fallback tracking methods
         return new \WP_Error(
             'slimstat_tracking_failed',
             esc_html__('[REST API] Tracking failed, falling back to alternative methods.', 'wp-slimstat'),
-            ['status' => 400]
+            ['status' => 400, 'error_code' => is_int($result) ? $result : 0]
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Controllers/Rest/TrackingRestController.php` around lines 203 - 208, The
REST handler in TrackingRestController (the block that currently returns new
\WP_Error('slimstat_tracking_failed', ..., ['status' => 400])) should map the
internal Ajax.process() return codes to semantically correct HTTP statuses:
return 429 when Ajax.process() yields -429 (rate limited), return 204 when it
yields -204 (tracking disabled), and keep 400 for a generic failure (0);
implement this mapping before constructing the \WP_Error so the ['status'] field
uses the mapped HTTP status, and if you prefer to preserve the original code for
debugging also include it in the error data (e.g., ['status'=>mapped,
'code'=>originalCode]) so the client can still see the original numeric result.