Skip to content

feat(analytics): Placements — player block_source emit + per-video section - #2041

Merged
subodhr258 merged 11 commits into
developfrom
feat/analytics-placements
Jul 28, 2026
Merged

feat(analytics): Placements — player block_source emit + per-video section#2041
subodhr258 merged 11 commits into
developfrom
feat/analytics-placements

Conversation

@subodhr258

@subodhr258 subodhr258 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

What

godam side of Placements (rtCamp/godam-plugin-wp#22): the player emits block_source on analytics events, and the per-video Analytics page gains a Placements section. Consumes rtCamp/godam-analytics#237.

Player emit

  • rtgodam_get_block_source_from_context() maps the existing godam_context to the placement slug (shoppable-video / wc-product-gallery / product-reels / reel-pop; empty → video-block; unknown passes through). One mapper, no per-surface instrumentation.
  • The player template stamps data-block-source (always) + data-host-post-id (embeds only) on the <video>; explicit block_source/host_post_id shortcode atts override (used by the embed page).
  • analytics.js: type=1 batch entries are now [videoId, jobId, blockSource] triples; every type=2 path (track + keepalive teardown) carries block_source; type=3 flushes best-effort.
  • Gallery/iframe attribution: gallery view.js appends host_post_id + block_source=video-gallery to the video-embed iframe URL; video-embed.php sanitizes + threads them; buildAnalyticsRequestBody overrides post_id with hostPostId (>0) so lightbox plays attribute to the hosting page.

Placements section (per-video analytics)

  • Block-source sections (auto-select first) → per-page rows: "Displayed on" (title + permalink), Edit Page (hidden without the edit cap; "Deleted page" fallback), Views / Plays / Play Rate / Avg Watch Time (derived client-side from raw primitives; full numbers).
  • Own DateRangePicker (cache-key dedup via conditional spread), loading/error/empty states.
  • Proxy enrichment: fetch_analytics_data adds title/permalink/edit_url/is_deleted per row (≤100 lookups); key stays absent on old microservices.
  • Top Videos: new "Placements" column (placements_count).

Testing

  • Jest: Placements helpers (17) + picker suite (8) → 25 passed. Lint clean; PHPCS (main-checkout vendor, worktree lacks vendor/) exit 0.
  • Manual (needs Fixes for minor issues #237 running): play a video via different surfaces (block / gallery lightbox / Woo gallery), run /process-analytics, open the video's analytics → sections group by surface, gallery plays attribute to the host page; check Top Videos "Placements" column.

Automation impact

New test ids: godam-placements-section, godam-placements-list-item-<block_source>, godam-placements-row, godam-placements-daterange-toggle.

Deploy order

godam-analytics#237 first (store) → this PR (emit + consume) → godam-for-woo#184 (Reel Pop tuple).

Rebased onto develop after #2028 merged, so the date-range plumbing is inherited (no vendored copy) and the card's picker is live end to end: startDate/endDateuseFetchAnalyticsDataQuerystart_date/end_date → microservice range-sums the placements. Verified against the dev site: a past window returns 0 placements, today returns 4.

Review fixes

Two adversarial review rounds (8 dimensions, 3-lens refutation per finding).

Round 1

  • enrich_placements gates on is_post_publicly_viewable() + current_user_can('edit_post'), so the public /analytics/fetch route no longer leaks private/draft/pending titles or permalinks to anonymous callers. Verified live: a private page returns "unavailable"/no permalink for anon, and the real title + Edit link for a capable admin.
  • Rows past the 100-lookup cap get a constant-cost Post #<id> fallback (PHP) plus a getPlacementTitle() guard (JS), so no row renders nameless.
  • The new Placements column was added to the Top Videos CSV export (header + row).

Round 2

  • Untrusted block_source as an object key (MED, security). Values like toString, valueOf or constructor resolved to inherited Object.prototype members, making the label lookup truthy and the bucket a function — so groupPlacementsByBlockSource threw a TypeError inside the card's useMemo and white-screened the whole per-video Analytics page. Reachable by an anonymous caller (the embed page takes block_source from a query arg; the microservice normalizes but never rejects it). Buckets are now a null-prototype map and both the grouping and the label lookup use an own-property check.
  • Trashed pages (MED). WordPress maps edit_post on a trashed post to its pre-trash status, so current_user_can passed and a trashed page rendered as a live row with an Edit link that wp-admin/post.php refuses to open (HTTP 409). Trashed posts now route to the unavailable state regardless of the edit capability.
  • Host-post attribution (MED, data-integrity). hostPostId was read once per page and applied to every batched type=1 entry, so a single host-stamped player re-attributed every other video's page_load on that page. It is carried per queue entry now and the flush groups by it (post_id is one top-level field per request); the type=3 flush reads it from that video's own element.
  • Unavailable rows keep their ID (Post #12 (unavailable)) so several of them stay distinguishable.
  • video-embed.php uses mb_substr for the block_source cap — a byte-wise cut could split a multibyte character, after which esc_attr() dropped the value entirely.
  • enrich_placements primes the post cache in one query instead of up to 100 uncached get_post() calls per request on a public endpoint.
  • The empty state distinguishes "no placements in this date range" (with a View all time reset) from the plain no-data state, instead of showing the same message whenever the range is narrowed.

Tests: 76 passed (Placements helpers 33 incl. 13 hostile-key cases; emit-path helpers 19). ESLint clean; PHPCS exit 0; husky lint:php + lint:js pass on the staged diff.

Emit path coverage: groupBatchByHostPostId moved into analytics-helpers.js (analytics.js has top-level side effects, so it is not importable in jest) and covered by a new 19-case spec — block_source defaulting/passthrough, post_id overridden only when hostPostId parses > 0 (untouched for 0/null/undefined/negative/non-numeric), triple passthrough per event type, both bail conditions, and the grouping regression itself: entries with different host attributions land in separate requests.

Automation impact

Test ids: godam-placements-section, godam-placements-list-item-<block_source>, godam-placements-row, godam-placements-daterange-toggle, godam-placements-reset-range.

Deploy order

godam-analytics#237 first (store; run its alembic migration before the app restarts) → this PR (emit + consume) → godam-for-woo#184 (Reel Pop tuple + WC gallery attribution).

No catch-all section, no historical-data note

The card shows only real, labelled placements. Anything it cannot attribute to a surface — an empty block_source (unattributed) or a slug with no label — is dropped rather than collected into an "Other" section: those plays are already reported in the video's overall metrics above the card, so repeating them here would double-report them.

  • groupPlacementsByBlockSource drops unlabelled rows; the 'other' key is gone from SECTION_ORDER, and getBlockSourceLabel returns '' for anything unknown instead of falling back to "Other".
  • Removed the "Placement analytics starts collecting after this update…" note. The no-data state is now simply "No placement data for this video yet"; the range-scoped empty state (with its View all time reset) is unchanged.
  • Hostile block_source values stay safe: the null-prototype bucket map and own-property label check remain, and such rows are now dropped instead of bucketed (tests still assert no throw for Object.prototype key names).

The microservice excludes unattributed rows at the read layer too, so the per-video sections and the dashboard's Placements count agree.

Demo Recording

https://app.godam.io/web/video/6821bb3iu9

Added a note at the bottom:

Screenshot 2026-07-27 at 7 33 30 PM

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

🔍 WordPress Plugin Check Report

⚠️ Status: Passed with warnings

📊 Report

🎯 Total Issues ❌ Errors ⚠️ Warnings
23 0 23

⚠️ Warnings (23)

📁 composer.json (1 warning)
📍 Line 🔖 Check 💬 Message
0 missing_composer_json_file The "/vendor" directory using composer exists, but "composer.json" file is missing.
📁 readme.txt (2 warnings)
📍 Line 🔖 Check 💬 Message
0 mismatched_plugin_name Plugin name "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" is different from the name declared in plugin header "GoDAM".
0 trademarked_term The plugin name includes a restricted term. Your chosen plugin name - "GoDAM - Organize WordPress Media Library & File Manager with Unlimited Folders for Images, Videos & more" - contains the restricted term "wordpress" which cannot be used at all in your plugin name.
📁 tests/bootstrap.php (7 warnings)
📍 Line 🔖 Check 💬 Message
28 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "wp_strip_all_tags".
48 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "wp_prepare_attachment_for_js".
63 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "get_post_meta".
77 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "get_post_mime_type".
88 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "wp_get_attachment_url".
99 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "get_the_title".
111 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "date_i18n".
📁 assets/build/blocks/godam-gallery-v2/render.php (2 warnings)
📍 Line 🔖 Check 💬 Message
15 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$inner_block_video_ids".
23 WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound Global variables defined by a theme/plugin should start with the theme/plugin prefix. Found: "$inner_block_video_ids".
📁 assets/build/css/main.css (1 warning)
📍 Line 🔖 Check 💬 Message
0 EnqueuedStylesScope This style is being loaded in all contexts.
📁 assets/src/libs/analytics.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/07/27/demo-post-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle analytics-library) is loaded in the footer. Consider a defer or async script loading strategy instead.
📁 assets/build/js/main.min.js (5 warnings)
📍 Line 🔖 Check 💬 Message
0 EnqueuedScriptsScope This script is being loaded in all frontend contexts.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880 (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/2026/07/27/demo-post-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-page-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.
0 NonBlockingScripts.NoStrategy This script on http://localhost:8880/demo-attachment-post/ (with handle rtgodam-script) is loaded in the footer. Consider a defer or async script loading strategy instead.

🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check

@subodhr258
subodhr258 marked this pull request as ready for review July 24, 2026 12:22
Copilot AI review requested due to automatic review settings July 24, 2026 12:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds analytics placement attribution end-to-end (player emits block_source / embed attribution) and surfaces it in the per-video Analytics UI via a new Placements section, plus a Placements count column in Top Videos.

Changes:

  • Emit block_source / host_post_id from the player/embed surfaces and thread these into analytics requests (including batched page_load).
  • Add per-video “Placements” analytics card with date-range scoping, grouping, and derived metrics.
  • Enrich placement rows server-side with WP page metadata and expose a “Placements” column in Top Videos.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
pages/dashboard/components/TopVideosTable.js Adds Placements column and updates empty/loading row colSpans.
pages/analytics/Analytics.js Mounts the new Placements section on the per-video analytics page.
pages/analytics/components/Placements/index.js Implements Placements UI, grouping helpers, derived metrics, and range-scoped query.
pages/analytics/components/Placements/placements.test.js Adds unit coverage for Placements helper functions.
pages/analytics/components/DateRangePicker/index.js Adds shared analytics DateRangePicker component + helper exports.
pages/analytics/components/DateRangePicker/index.test.js Adds unit coverage for DateRangePicker helper functions.
pages/analytics/components/DateRangePicker/style.scss Adds styling for the analytics date-range picker popover/calendar.
inc/helpers/custom-functions.php Adds rtgodam_get_block_source_from_context() and threads new embed params into shortcode generation.
inc/templates/godam-player.php Stamps data-block-source and optional data-host-post-id on the <video> element.
inc/templates/video-embed.php Accepts host_post_id/block_source from iframe URL and passes into embed content generation.
inc/classes/shortcodes/class-godam-player.php Adds shortcode attrs block_source and host_post_id.
inc/classes/rest-api/class-analytics.php Enriches microservice placement rows with WP title/permalink/edit_url and deletion status.
assets/src/js/godam-player/analytics.js Threads block_source/hostPostId through type=1/2/3 analytics paths (including batch/flush).
assets/src/js/godam-player/analytics-helpers.js Adds block_source + host_post_id override logic to request body builder.
assets/src/js/godam-player/video-analytics-plugin.js Passes new attribution fields into buildAnalyticsRequestBody.
assets/src/blocks/godam-gallery-v2/view.js Appends host_post_id and block_source=video-gallery to embed iframe URL for attribution.

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

Comment thread pages/analytics/components/DateRangePicker/index.js Outdated
Comment thread pages/analytics/components/DateRangePicker/index.js
Comment thread pages/analytics/components/DateRangePicker/index.js
Comment thread pages/analytics/components/DateRangePicker/index.js
Comment thread inc/classes/rest-api/class-analytics.php
Comment thread inc/templates/godam-player.php
Comment thread pages/analytics/components/Placements/index.js
Comment thread pages/analytics/components/Placements/index.js
…acements section

godam side of Placements (godam-plugin-wp#22), consuming godam-analytics#237.

Player emit:
- rtgodam_get_block_source_from_context() maps godam_context to the
  placement slug (shoppable-video / wc-product-gallery / product-reels /
  reel-pop; empty = video-block; unknown passes through). The player
  template stamps data-block-source (always) and data-host-post-id (embeds
  only) on the <video>; an explicit block_source shortcode att overrides.
- analytics.js sends [videoId, jobId, blockSource] triples in the type=1
  batch and block_source on every type=2 path (track + keepalive teardown);
  type=3 layer flushes carry it best-effort.
- Embed-iframe attribution: gallery view.js appends host_post_id +
  block_source=video-gallery to the video-embed iframe URL; video-embed.php
  sanitizes and threads them as shortcode atts; buildAnalyticsRequestBody
  overrides body.post_id with hostPostId (>0), so gallery plays attribute
  to the hosting page, not the embed page.

Placements UI (per-video analytics):
- New Placements card: block-source sections (auto-select first) with
  per-page rows - "Displayed on" title/permalink, Edit Page (hidden without
  edit cap; "Deleted page" fallback), Views / Plays / Play Rate / Avg Watch
  Time (derived client-side from raw primitives). Own DateRangePicker
  (dedup via conditional-spread args). Loading / error / collecting-data
  states; godam-placements-* test ids. Mounted below "Views across the
  video".
- Proxy: fetch_analytics_data enriches microservice placements rows with
  title/permalink/edit_url/is_deleted (capped 100 lookups); leaves the key
  absent for old microservices.
- Top Videos: "Placements" column (placements_count).

Tests: Placements helpers (17) + vendored picker suite (8) - 25 passed.
Lint + PHPCS clean.

Depends on #2028 for live date-range params (until it merges, non-All-Time
picks refetch all-time data); the vendored picker commit is dropped on
rebase after #2028 lands.
@subodhr258
subodhr258 force-pushed the feat/analytics-placements branch from 4977663 to 82502b5 Compare July 24, 2026 17:43
…attribution

- Untrusted block_source as an object key (security): a value like 'toString',
  'valueOf' or 'constructor' resolved to an inherited Object.prototype member,
  making the label lookup truthy and the bucket a function, so
  groupPlacementsByBlockSource threw a TypeError inside the card's useMemo and
  white-screened the whole per-video Analytics page. The value is reachable by
  an anonymous caller (the embed page takes block_source from a query arg and
  the microservice normalizes but never rejects it). Buckets are now a
  null-prototype map and both the grouping and the label lookup use an
  own-property check. +13 jest cases covering those exact key names.
- Trashed pages: WordPress maps edit_post on a trashed post to its pre-trash
  status, so current_user_can passed and a trashed page rendered as a live row
  with an Edit link that wp-admin refuses to open (HTTP 409). Trashed posts now
  route to the unavailable state regardless of the edit capability.
- Unavailable rows keep their ID ("Post #12 (unavailable)") so several of them
  stay distinguishable instead of repeating one constant string.
- Host-post attribution: hostPostId was read once per page and applied to every
  batched type=1 entry, so a single host-stamped player re-attributed every
  other video's page_load on that page. It is now carried per queue entry and
  the flush groups by it (post_id is one top-level field per request); the
  type=3 flush reads it from that video's own element.
- video-embed.php uses mb_substr for the block_source cap: a byte-wise cut
  could split a multibyte character, after which esc_attr() dropped the value
  entirely.
- enrich_placements primes the post cache in one query instead of issuing up to
  100 uncached get_post() calls per request on a public endpoint.
- Empty state distinguishes "no placements in this date range" (with a "View all
  time" reset) from "collecting placement data", instead of telling the user the
  feature has not started collecting whenever they narrow the range.

Tests: 57 passed (Placements helpers now 33). ESLint clean, PHPCS exit 0.
Known gap: the emit path (buildAnalyticsRequestBody / queue grouping) still has
no unit test; tracked for a follow-up.
@subodhr258 subodhr258 changed the title feat(analytics): Placements — player block_source emit + per-video section (depends on #2028) feat(analytics): Placements — player block_source emit + per-video section Jul 26, 2026
Closes the test gap the re-review flagged: the attribution logic had no unit
coverage even though the UI helpers were fully covered.

- Moved the pure `groupBatchByHostPostId` from analytics.js into
  analytics-helpers.js (the shared, importable module — analytics.js has
  top-level DOM/library side effects, so it is not directly testable).
- New analytics-helpers.test.js (19 cases): block_source defaults to '' and
  passes through verbatim; post_id is overridden only when hostPostId parses
  > 0 and is left untouched for 0 / null / undefined / negative / non-numeric;
  numeric strings coerce; type=1 video_ids triples pass through unchanged while
  other types send none; both bail conditions return { endpoint: null,
  body: null }.
- Grouping is pinned by the regression that motivated it: entries with
  different host attributions land in separate groups, so one host-stamped
  player can no longer drag the rest of the page's videos into its post_id
  override. Also covers wire-triple shape and non-positive/unparseable ids.

77 tests passing across the analytics areas. ESLint clean.
…note

The card now shows only real, labelled placements. Anything it cannot attribute
to a surface — an empty block_source (unattributed) or a slug with no label — is
dropped rather than collected into an "Other" section: those plays are already
reported in the video's overall metrics above the card, so repeating them here
would double-report them.

- groupPlacementsByBlockSource drops unlabelled rows; the 'other' key is gone
  from SECTION_ORDER and getBlockSourceLabel no longer has an "Other" fallback
  (it returns '' for anything unknown).
- Removed the "Placement analytics starts collecting after this update. Data
  appears as new plays come in." note; the no-data state is now just
  "No placement data for this video yet". The range-scoped empty state (with its
  "View all time" reset) is unchanged.

Hostile block_source values are still safe: the null-prototype bucket map and
own-property label check remain, and those rows are now dropped instead of
bucketed. Tests updated accordingly (they assert dropping, and still assert no
throw for Object.prototype key names).

The microservice also excludes unattributed rows at the read layer, so the
per-video sections and the dashboard's Placements count agree.

76 jest tests passing. ESLint clean.
…slugs

Round-3 review (MEDIUM, confirmed): the previous fix conflated two different
things under "don't show unattributed" -- the truly empty block_source
(unattributed, no surface at all) and a non-empty slug this build simply has
no friendly label for yet. Only the former should be hidden (it's already
reported in the video's overall metrics above the card); the latter is a
real, attributed placement, and dropping it made the card disagree with the
dashboard's placements_count, which only excludes the empty case -- a real
placement became invisible with no UI trace.

groupPlacementsByBlockSource now drops only rows whose block_source is empty.
A non-empty unmapped slug gets its own section, labelled with the raw slug,
sunk after the known SECTION_ORDER sections (sorted by row count). The
null-prototype bucket map keeps this safe for hostile keys ('toString' etc.)
-- they're now kept as ordinary sections instead of dropped, since they are
just as real (or fake) as any other unmapped slug and are only ever rendered
as text.

Tests updated to match: unmapped-but-real slugs are asserted present with the
slug as their label; hostile-key tests now assert they're kept, not dropped.
33 jest passing. ESLint clean; build green.
formatWatchTime floors to whole seconds, so a real but sub-second Avg Watch
Time (now correctly computed after the analytics#237 page-load-scoping fix)
rendered as a bare "0s" -- indistinguishable from no watch time at all. Show
"<1s" instead when the average is positive but under a second.
Placement attribution is forward-only (block_source is captured at ingestion,
no historical backfill), so the per-placement breakdown can legitimately show
less than the video's all-time totals in the Insights above it. Adds a small
footnote clarifying that, shown whenever the card has resolved (data or the
empty state) -- not during loading/error, where it would just add noise.
…zation

- Placements row key drops `index`: (post_id, block_source) is already unique
  per row (the microservice aggregates by that exact pair), so including index
  bought nothing and could cause needless remounts when a range change
  re-sorts rows by plays desc.
- Auto-select effect reads the previous selectedKey via the functional
  setSelectedKey(prev => ...) updater instead of closing over the outer
  variable, so it only needs `sections` as a dependency -- removes the
  eslint-disable-next-line react-hooks/exhaustive-deps with no behavior
  change (verified live: section switching still works).
- godam-player.php now caps block_source the same way video-embed.php already
  does (mb_substr(...,100,'UTF-8') after sanitize_text_field) instead of only
  sanitizing with no length cap -- block_source is ultimately user-controlled
  (threaded from a public GET param on the embed page into this shortcode
  attribute), so both paths should bound it identically.

Note: 4 additional Copilot comments on this PR (DateRangePicker DST/off-by-one
and month-grid issues) are about code from already-merged PR #2028, inherited
via the rebase onto develop -- confirmed 0 lines of DateRangePicker/index.js
are in this PR's diff (git diff origin/develop...HEAD). Left untouched; belongs
to a follow-up against develop, not this PR.

33 jest passing, PHPCS clean, verified live in Chrome (section switching intact).
@subodhr258
subodhr258 requested a review from KMchaudhary July 28, 2026 06:12
@KMchaudhary

Copy link
Copy Markdown
Collaborator

Code review — Placements (#2041 + godam-for-woo#184, reviewed together)

Reviewed both halves of the feature end-to-end (emit → embed threading → proxy enrichment → render). Overall it's carefully built and well-tested — the null-prototype bucket map, per-entry hostPostId grouping, multibyte block_source cap, cache-key dedup, and the TopVideos column/CSV integrity all check out, and no page-breaking bug survived. Findings below, ranked.

Security / info-disclosure

  • [MED · new] enrich_placements leaks metrics of non-public pages. Title/permalink are gated for anonymous callers, but the row's post_id + views/plays/page_load/play_time still pass through the public /analytics/fetch route (permission_callback => __return_true). An anonymous caller can enumerate the private/draft page IDs a video is placed on, plus play counts. The "unavailable" branch rewrites the title but never strips the primitives. — inc/classes/rest-api/class-analytics.php (~L516)
  • [MED · pre-existing, same function] post_details is worse and ungated. The sibling post_views → post_details enrichment returns real get_the_title() + get_permalink() for every post — private/draft/pending included — with no viewability/capability check, on the same public route. This PR adds exactly the protection to placements that this adjacent block still lacks; worth fixing them together. — inc/classes/rest-api/class-analytics.php (~L642)
  • [LOW] Attacker-controlled block_source rendered as an admin label. block_source is accepted from the public embed page (?block_source=) and only length-capped (100), never allowlisted. An unmapped value is rendered verbatim as a section label in the admin Placements card. React escapes it (no XSS), but it's stored text / UI spoofing in the admin surface. — pages/analytics/components/Placements/index.js (getBlockSourceLabel, ~L65)

Correctness

  • [LOW–MED · godam-for-woo#184] get_the_ID() for host_post_id in block-render filters. class-wc-featured-video-gallery-compat.php::get_video_embed_url() (~L111) reads get_the_ID() inside render_block_woocommerce/* callbacks instead of the block context postId ($block->context['postId']). In FSE / query-loop contexts get_the_ID() may not be the product (or may be false), so lightbox plays misattribute — the same class of bug this PR fixes for the classic gallery. (The classic class-wc-featured-video-gallery.php path is safe because it runs inside the single-product loop.)
  • [LOW] Hardcoded version string. The disclaimer reads reflects activity since v2.1.0, but the plugin is at 2.0.0 (godam.php / RTGODAM_VERSION). Nothing ties the string to the actual release version. — pages/analytics/components/Placements/index.js (~L474)

Cleanup / altitude

  • [LOW] getPlayRate(plays, pageLoad) duplicates calculatePlayRate(pageLoad, plays) from pages/analytics/helper.js with the arguments in reversed order — a real footgun for the next caller. Consider reusing the existing helper. — Placements/index.js (~L134)
  • [LOW] Host-post-id parsing is copy-pasted 4× with two "none" encodings (id > 0 ? id : null in elementHostPostId/getPageLoadVideoInfo vs parseInt(...) || 0 in the two type=2 heatmap paths). Only harmless because buildAnalyticsRequestBody treats both as "no override". One shared helper would remove the divergence. — assets/src/js/godam-player/analytics.js (~L102)
  • [LOW] Shared godam_context patched with an override. The featured-gallery lightbox reuses godam_context='godam-video-product-gallery' (core maps it to shoppable-video) and then compensates with an explicit block_source='wc-product-gallery'. Two placements sharing one context means every new emit site must remember the override or silently fold into shoppable-video; a 1:1 context→placement mapping would be more robust. — godam-for-woo class-wc-featured-video-gallery.php (~L357)

Deploy-order note (not a code bug)

Both PRs convert all type=1 tuples to 3-element triples, which older microservices reject with HTTP 400 — so shipping either plugin PR before godam-analytics#237 breaks all page_load analytics, not just placements. The documented order (analytics#237 → #2041 → godam-for-woo#184) is mandatory; worth enforcing in release automation.

…cements

Human review (both PRs reviewed together): enrich_placements gated title/
permalink for private/draft/pending/trashed pages, but the row's raw post_id
and engagement metrics (views/plays/page_load/play_time) still passed through
unchanged on the public `/analytics/fetch` route (permission_callback =>
__return_true). An anonymous caller could enumerate the private/draft page
IDs a video is placed on, plus their play counts.

Split the "post exists but this caller can't see it" case from "post_id
doesn't resolve to anything real":
- Real-but-inaccessible post: post_id -> 0, title -> generic "Unavailable"
  (no ID), views/plays/page_load/play_time -> 0. Every such row is now
  intentionally identical, so nothing distinguishes one hidden page from
  another.
- Truly nonexistent post_id: unchanged behavior (numbered "Post #%d (deleted)"
  label + real metrics) -- there's no real, currently-existing resource being
  described, so nothing sensitive is exposed.

FE: since redacted rows now share post_id=0, the row key falls back to
`redacted-${block_source}-${index}` for that case only (avoids a duplicate-key
collision when a section has more than one hidden page); the normal case is
unchanged.

Also, from the same review: godam-player.php now caps block_source length
(mb_substr, 100 chars) instead of only sanitizing with no length cap, matching
video-embed.php's existing cap for the same user-controlled value.

Verified live: flipped a real placement's host post to private and confirmed
an anonymous /analytics/fetch call now returns post_id 0 / all-zero metrics
for it, while unrelated public placements are untouched; restored the post
afterward. 77 jest passing, PHPCS clean.

Noted but out of scope (flagged separately): the same review found the
sibling post_details/post_views enrichment block in this file leaks real
titles+permalinks for ALL posts with no viewability/capability gate at all --
confirmed pre-existing (git blame predates this branch entirely), not part of
this PR's diff. Also noted: reusing the existing calculatePlayRate() helper
instead of a parallel getPlayRate() with swapped arg order, and consolidating
4 copy-pasted host-post-id parsing call sites -- both low-risk cleanups,
deferred to keep this fix focused on the security issue.
@subodhr258
subodhr258 merged commit a5e8e86 into develop Jul 28, 2026
2 of 4 checks passed
@subodhr258
subodhr258 deleted the feat/analytics-placements branch July 28, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants