Activity kits: server-side download tracking

[Piyopiyo-Kitsune]

Problem

get_clicks() in Jetpack Stats only tracks outbound (external-domain) clicks. Since the activity kit ZIP files are hosted on learn.wordpress.org (same domain), they are never captured — download counts always showed 0.

Solution

Replace the Jetpack click approach with a thin REST redirect endpoint that increments a post meta counter before forwarding the browser to the actual file.

GET /wp-json/activity-kits/v1/download/{slug}
→ 302 Location: https://learn.wordpress.org/.../activity-kit.zip

This works regardless of file host domain, requires no JS, and persists across page loads.

Changes

inc/activity-kit-rest.php

• New route GET activity-kits/v1/download/{slug} (public, no auth required)
  • Resolves the kit by slug, fetches the ZIP attachment URL
  • Increments _activity_download_count post meta
  • Returns WP_REST_Response( null, 302, [ 'Location' => $zip_url ] )
• Fix view stats — switch from get_top_posts() to get_total_post_views() so recently published kits with low traffic aren't omitted from results (same fix as Fix activity kit stats: switch to get_total_post_views() for per-kit view counts #3591, included here since this branch is off trunk)
• Remove get_jetpack_download_clicks() — now dead code
• Stats endpoint reads downloads from _activity_download_count post meta directly, removing the Jetpack clicks dependency entirely

inc/post-meta.php

• Register _activity_download_count meta (integer, default 0, show_in_rest => false — admin-read-only via REST stats endpoint)

patterns/single-activity-kit-content.php

• Both download buttons now point at the /download/{slug} endpoint
• Removed the window._stq JS tracking block — it was tracking clicks to internal REST URLs (meaningless), and server-side counting replaces it entirely

Testing

1. Load any published activity kit page
2. Click Download kit — you should be redirected to the ZIP file
3. Visit the Stats page — the download count for that kit should increment

🤖 Written with Claude Code

[Copilot]

Pull request overview

This PR replaces client-side / Jetpack outbound-click tracking for Activity Kit ZIP downloads with a server-side REST redirect endpoint that increments a per-kit download counter in post meta, ensuring same-domain downloads are counted reliably.

Changes:

• Adds a public REST download endpoint that redirects to the ZIP while incrementing _activity_download_count.
• Updates the stats implementation to fetch per-kit views via Jetpack’s get_total_post_views() and reads downloads directly from post meta.
• Updates the single activity kit pattern to link download buttons to the new REST endpoint and removes the old JS tracking block.

Reviewed changes

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

File	Description
wp-content/themes/pub/wporg-learn-2024/patterns/single-activity-kit-content.php	Points download buttons at the new REST redirect endpoint and removes JS click tracking.
wp-content/plugins/wporg-learn/inc/post-meta.php	Registers _activity_download_count post meta used for server-side download tracking.
wp-content/plugins/wporg-learn/inc/activity-kit-rest.php	Adds the download redirect route and updates stats logic (views via Jetpack per-ID query; downloads via post meta).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Piyopiyo-Kitsune]

Context on the tracking approach

This PR uses a REST redirect endpoint for download tracking, which is architecturally different from what dd32 flagged as non-viable in #3496 — but similar enough that I want to flag it proactively rather than wait for a review surprise.

What was rejected in #3496: A POST /activity-kits/v1/track endpoint called client-side as a fire-and-forget beacon — an extra REST request layered on top of the real action. Dion's fix was to use Jetpack Stats instead, which #3496 shipped with.

Why Jetpack still doesn't capture downloads: Dion noted in #3496 that "The dedicated File Downloads report is structurally unavailable for this site" because WordPress.org serves uploads from /files/ rather than /wp-content/uploads/, so Jetpack's automatic click tracker skips them entirely. The _stq.push() beacon approach that shipped in #3496 has the same limitation — get_clicks() in WPCOM_Stats only returns outbound (external-domain) clicks, so same-domain file downloads never register. That's why the download count has shown 0 since launch.

How this PR differs: The REST endpoint here is the download mechanism — both download buttons now point at /wp-json/activity-kits/v1/download/{slug}, which increments a post meta counter and 302-redirects to the file. There's no extra tracking request; the user can't reach the file without going through the endpoint. The counter is a single integer per kit (not a row-per-event table), so the storage footprint is minimal.

The open question: This still bootstraps WordPress + REST + a update_post_meta write on every download. That's lighter than the original rejected approach, but heavier than a pure Jetpack solution. @dd32 — given that Jetpack's click tracker can't cover /files/ paths, is a REST redirect endpoint like this acceptable on WordPress.org infrastructure, or would you prefer a different mechanism (e.g., a plain PHP handler via rewrite rule that bypasses the REST bootstrap)?

Happy to rework the approach based on feedback before this gets further into review.

🤖 Written with Claude Code

[obenland]

Ran a review on this PR — six inline comments below, plus two findings in files this PR doesn't touch:

1. Library grid/search cards still bypass the counter entirely. wp-content/themes/pub/wporg-learn-2024/inc/block-hooks.php (~line 274) and wp-content/plugins/wporg-learn/views/block-activity-kit-card.php (lines 89–93) still link straight at wp_get_attachment_url() with the now-orphaned data-post-id/data-track-download attributes. This PR deletes the only consumer of those attributes (the _stq click script) and removes the Jetpack clicks aggregation, so every download from the Activity Library grid or search results is counted nowhere — the dashboard silently under-reports. Suggest one get_download_url( $kit_id ) helper in the plugin used by all four link sites, and dropping the dead attributes.

2. The stats page still hard-gates on Jetpack, making download counts unreachable exactly when they'd matter. wp-content/plugins/wporg-learn/js/activity-kit-stats/index.js (~line 402): render() early-returns on the jetpackAvailable flag before fetchStats() is ever called, with a message saying download counts require a Jetpack connection — which this PR makes false. The recorded _activity_download_count values are never displayed when Jetpack is disconnected. Suggest always fetching and showing views as "—" when the response carries jetpack_unavailable (which is currently dead payload no JS reads).

Also worth noting: historical Jetpack-clicks download data is lost with no backfill — the new counter starts at zero for every kit, so the dashboard will show a cliff.

[obenland]

This route regex is narrower than what WordPress allows in post_name, so some kits get a dead Download button (REST 404 JSON instead of the ZIP) — a regression vs. the old direct link.

sanitize_title_with_dashes() keeps underscores, and non-Latin titles become percent-encoded slugs (utf8_uri_encode()) — relevant here since activity kits are explicitly multilingual and the bulk importer sets post_title with no slug control. A kit slugged trivia_night or %e3%81%82… renders an href that WP_REST_Server never matches, and the user gets rest_no_route. The sanitize_callback => 'sanitize_title' can't help — sanitization runs only after a route matches.

Fix options: widen to (?P<slug>[^/]+) and rawurlencode() the slug when building the URL, or route on post ID instead.

[Piyopiyo-Kitsune]

Fixed — route now uses (?P<id>\d+) (post ID) rather than a slug pattern. The template builds the URL with $kit_id directly, which is always a plain integer and has no encoding ambiguity.

[obenland]

This compare-and-swap loop unfortunately doesn't prevent lost increments in either scenario it targets:

1. $prev_value = 0 degrades to a blind write. update_metadata() only adds the meta_value WHERE clause if ( ! empty( $prev_value ) ), so the 0→1 increment is unconditional — two concurrent first downloads both write 1.
2. For counts > 0, a genuine CAS failure can't recover. When $wpdb->update matches zero rows it returns before wp_cache_delete, so the loser's get_post_meta() re-reads the same stale cached value on every retry — all 5 iterations issue the identical failing UPDATE and the increment is silently dropped.

A single atomic query avoids both:

$updated = $wpdb->query( $wpdb->prepare(
	"UPDATE {$wpdb->postmeta} SET meta_value = meta_value + 1 WHERE post_id = %d AND meta_key = %s",
	$kit_post->ID,
	'_activity_download_count'
) );
if ( ! $updated ) {
	add_post_meta( $kit_post->ID, '_activity_download_count', 1, true );
}
wp_cache_delete( $kit_post->ID, 'post_meta' );

(The comment above the loop would need updating to match, since the concurrency claim it documents doesn't hold.)

[Piyopiyo-Kitsune]

Replaced entirely. The new code issues a single UPDATE … SET meta_value = meta_value + 1 with add_post_meta as a fallback for the first-download case, then calls wp_cache_delete. The UPDATE is atomic at the DB level with no read; the comment above it now documents both failure modes of the old CAS approach (zero-skips-WHERE and stale-cache-on-retry) so the reasoning is clear.

[obenland]

get_total_post_views() is being passed the old get_top_posts() range mapping, but the underlying /stats/views/posts endpoint accepts only post_ids/num/date/offset (checked against Jetpack 13.3.1 as pinned in composer.lock, and the WPCOM endpoint docs): period is ignored and num is capped at 30 days. So "90 days" silently returns ~30 days of views, and "All time" (period=month, num=36) also returns ~30 days — wrong by orders of magnitude, with no error. post_ids is additionally capped at 100 IDs, so once the library exceeds 100 kits the overflow IDs silently report 0 views.

Suggest mapping ranges within the num <= 30 constraint (or summing multiple windowed calls) and chunking post_ids.

[Piyopiyo-Kitsune]

Fixed. period is no longer passed. num is capped per window at 30 (7d → 1 call of 7, 30d → 1 call of 30, 90d/all → 3 calls of 30 offset by 0/30/60 days, results summed). post_ids is chunked in groups of 100 and the inner loop issues one call per chunk per window. The rangeLabel() in the JS now shows 'All time (max 90 days)' to be honest about the ceiling.

[obenland]

Style nit: three stacked // lines — per the project comment conventions this should be a single /* */ block with * continuations. (Its content also needs a rewrite per the CAS comment below, since the claim it documents doesn't hold.)

[Piyopiyo-Kitsune]

Rewritten as a single /* */ block. Also took the opportunity to document the two specific failure modes of the old CAS approach that made the comment's original claim incorrect.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

wp-content/plugins/wporg-learn/js/activity-kit-stats/index.js:403

• Similarly, this overwrites the localized summary label with hardcoded English (“Total Downloads …”). Preserve the existing translated label and only append the suffix when needed.

			const dlLabel = summaryDownloads.closest( '.ak-summary-box' )
				? summaryDownloads.closest( '.ak-summary-box' ).querySelector( '.ak-stat-label' )
				: null;
			if ( dlLabel ) {
				dlLabel.textContent = activeRange === 'all' ? 'Total Downloads' : 'Total Downloads (all time)';

wp-content/plugins/wporg-learn/inc/activity-kit-rest.php:154

• The atomic increment treats any falsey $updated value as “no row yet”, which (a) hides SQL errors (wpdb::query can return false) and (b) can lose counts on concurrent first downloads: two requests can both see 0 rows updated, one insert succeeds and the other insert fails (unique), resulting in only one increment recorded. Handle false separately, and if the insert fails, fall back to an UPDATE increment.

	if ( ! $updated ) {
		// No row yet — insert with an initial count of 1.
		add_post_meta( $kit_post->ID, '_activity_download_count', 1, true );
	}
	wp_cache_delete( $kit_post->ID, 'post_meta' );

wp-content/plugins/wporg-learn/inc/activity-kit-rest.php:57

• PR description documents the download endpoint as /download/{slug}, but the implementation registers /download/{id} and the theme builds URLs using the numeric post ID. Please align the PR description (or switch the route to slug) so API consumers and testers aren’t misled.

	register_rest_route(
		'activity-kits/v1',
		'/download/(?P<id>\d+)',
		array(
			'methods'             => 'GET',

wp-content/plugins/wporg-learn/js/activity-kit-stats/index.js:395

• Setting thDownloads.textContent to hardcoded English strings overwrites the already-localized header text rendered by PHP (see inc/activity-kit-stats-page.php:277-279), which regresses i18n for non-English admins. Preserve the existing translated base label and only append the “(all time)” suffix.

This issue also appears on line 399 of the same file.

			const arrow = thDownloads.querySelector( '.ak-sort-arrow' );
			thDownloads.textContent = activeRange === 'all' ? 'Downloads' : 'Downloads (all time)';
			if ( arrow ) {
				thDownloads.appendChild( arrow );
			}

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

wp-content/plugins/wporg-learn/js/activity-kit-stats/index.js:78

• The UI label/comment claims the 'all' range is capped at 90 days, but the updated server implementation of get_jetpack_post_views() sums multiple 30-day windows (currently 6 windows = ~180 days). This makes the UI misleading and can confuse interpretation of stats; align the label/comment with the server behavior (or reduce the server-side windows to match the stated cap).

			// 'All time' is capped at 90 days by the WPCOM /stats/views/posts API
			// (30-day max per call; see get_jetpack_post_views() in activity-kit-rest.php).
			all: 'All time (max 90 days)',
		};
		return labels[ activeRange ] || 'All time (max 90 days)';

wp-content/plugins/wporg-learn/inc/activity-kit-rest.php:231

• In handle_stats(), this per-kit get_post_meta() call can become an N+1 pattern when many kits are returned. A concrete improvement is to prime the meta cache once for the kit IDs before building $results (e.g., update_meta_cache('post', $kit_ids) or equivalent), then read from cache via get_post_meta without repeated queries.

		if ( 'both' === $metric || 'downloads' === $metric ) {
			$data['downloads'] = (int) get_post_meta( $kit_post->ID, '_activity_download_count', true );
		}

wp-content/themes/pub/wporg-learn-2024/patterns/single-activity-kit-content.php:14

• wp_get_attachment_url( $zip_id ) is called inline solely as an existence check, which duplicates work and makes the condition harder to read. Consider assigning the attachment URL to a variable once (and reusing it if needed) so the condition and intent are clearer.

$kit_id       = get_the_ID();
$duration     = get_post_meta( $kit_id, '_activity_duration', true );
$zip_id       = (int) get_post_meta( $kit_id, '_activity_zip_id', true );
// Link at the counting endpoint, not the file; route on ID since slugs may not URL-encode cleanly.
$download_url = ( $zip_id && wp_get_attachment_url( $zip_id ) ) ? rest_url( 'activity-kits/v1/download/' . $kit_id ) : '';

wp-content/plugins/wporg-learn/js/activity-kit-stats/index.js:394

• Setting thDownloads.textContent replaces all existing header content (including any hidden/screen-reader-only elements or other markup that might be present besides .ak-sort-arrow). A safer approach is to update only a dedicated label node/element (or insert/update a text node while preserving other children) so future accessibility markup in the table header isn’t accidentally removed.

			const arrow = thDownloads.querySelector( '.ak-sort-arrow' );
			thDownloads.textContent = activeRange === 'all' ? 'Downloads' : 'Downloads (all time)';
			if ( arrow ) {
				thDownloads.appendChild( arrow );
			}

[obenland]

Suggested change

	'description' => __( 'Number of times this activity kit ZIP has been downloaded via the tracked download endpoint.', 'wporg-learn' ),
	'description' => 'Number of times this activity kit ZIP has been downloaded via the tracked download endpoint.',

I don't think this is user-facing

[Piyopiyo-Kitsune]

Applied in 12284c9 — removed the __() wrapper.

🤖 Written with Claude Code