[subodhr258]

Do not merge until rtCamp/godam-analytics#365 (item 1 of rtCamp/godam-analytics#364) is live on the analytics service the site posts to, and the visitor-IP check passes (a browser event sent through the live proxy is stored with the visitor's own address and country). After that change the service takes the address of browser-sent events (page load, heatmap, layer, Reel Pop and Shoppable events) from the request itself, accepts events with no location_ip, and ignores the field when the request has an address. Events the store's server posts (assisted add to cart, order revenue) keep their payload value. This PR does nothing useful before that.

Review packet

In one line: The page params (the videoAnalyticsParams object printed on every front-end page) and the analytics events no longer carry the locationIP / location_ip field; the analytics service works out the location from the request itself. No issue; planned in rtCamp/godam-analytics#364 (item 1).

1. Criteria and proof

Acceptance criterion	Proof
The page params have no locationIP (before: the address the server saw for the request)	PHP test VisitorLocationParamTest::test_page_params_carry_no_location_field; capture S01
The analytics request body has no location_ip key (before: it held that address)	JS test sends no location field, even when the page still carries one; capture S01
A page cached by an older plugin version, which still carries locationIP, does not make the player send it	same JS test (its fixture keeps locationIP: '10.0.0.1' on purpose)
The page and the request keep the other fields (user id, site id, token, post details, browser language)	PHP test test_page_params_keep_what_analytics_still_needs; JS test still sends the browser language
Plays are still recorded and the country still resolves, with the analytics change in place	capture S02

2. Before and after

A signed-in subscriber opens a page with videos on godam-dev in Chrome and plays one for 6 seconds. Before is develop (2.4.0) with the current analytics service; after is this branch with the analytics change from rtCamp/godam-analytics#365 (its head cce9ea8 when tested). The test machine has no proxy in front of the analytics service, so the capture adds X-Forwarded-For: 8.8.8.8 to the analytics requests, the way the proxy does in production. Captures are verbatim from the browser and from the analytics service's database.

What was read	Before	After
locationIP in videoAnalyticsParams	8.8.8.8	not present
location_ip in the play request	8.8.8.8	not a key
The address in the page source, 12 pages	1 each	0 each
Address stored for the 6 events of the run	8.8.8.8, 6 of 6	8.8.8.8, 6 of 6 (taken from the request by the service)
Country stored	US, 6 of 6	US, 6 of 6
Video 203 plays, page loads, country	1, 1, USA 1	1, 1, USA 1

3. Tests

• Added: tests/php/VisitorLocationParamTest.php. It builds the page params and asserts there is no locationIP key (test_page_params_carry_no_location_field), and that userId, siteId, endpoint, token and postId are still there. Each test runs in its own process, and loads its stubs there, because loading godam-transcoder-functions.php registers hooks and EarlyTranslationHooksTest checks exactly that on first load.
• Added: two tests in assets/src/js/godam-player/analytics-helpers.test.js (no location_ip key and no trace of the address in the body even when the page still carries locationIP; the browser language is still sent).
• Added: tests/stubs/template-tags.php, the page-tag stubs the PHP test needs (is_single, get_the_title and similar). It is identical to the file in the sibling PR rtCamp/godam#2213 (stops sending the signed-in viewer's email), so the two merge cleanly in either order.
• Changed: tests/stubs/engagement-functions.php. The current-user stub now also returns ID and data->user_email (an empty data when nobody is signed in), as WordPress does. Same change as Stop sending the signed-in viewer's email with video analytics #2213. The Engagement tests that use it still pass: 426 PHP tests, 1408 assertions, in normal, reversed and random order.
• Revert check: with the fix reverted, test_page_params_carry_no_location_field fails with "Failed asserting that an array does not have the key 'locationIP'", and the JS test fails with Expected path: not "location_ip", Received value: "10.0.0.1".

4. Risk flags

No risk flags fired on #2214. Nothing in the diff touched the watched areas.

5. Independent review

A separate review pass saw only the task and the diff.

Finding	Grade	What I did
The stub file was loaded at the top of the test file, so its stand-in functions and the RTGODAM_ANALYTICS_BASE constant reached every other test	Valid	Fixed before this PR was opened: loaded inside setUp(), in the separate process. The suite passes in normal, reversed and random order.
The merge condition should name what the analytics release must do, not only "is live"	Valid	Added at the top of this description, with the analytics PR that does it.
Pages cached by an older version keep the old params until the cache expires, and older plugin versions keep sending location_ip	Valid, outside this diff	No code change. Listed in section 6.
Unique-viewer counts may shift once at cutover, because the same visitor can map to a different address string (for example IPv4 versus IPv6)	Valid	Nothing to change here. Listed in section 6 as a figure to watch after the analytics release.
The PHP test only checks the key name, not an address value	Valid, limited by the test environment	In a command-line test the plugin's server-variable reader returns nothing, so no address can be injected. The JS test does check the value, and the capture on godam-dev checks the real page and request.
Other code that reads the removed field	Valid question, checked	Only the godam-for-woo add-on reads locationIP (two senders; they will send an empty location_ip, which the service ignores for browser-sent events). rtgodam_get_user_ip() had no other caller in the plugin. No PHP code posts the address to the analytics service.
userId stays in the same object	By design	Out of scope here. #2213 removes the email from the same object.
Test names and comments in the first version described the purpose in detail	Valid	Renamed to neutral names and comments before this PR was opened.
The new JS tests sit next to lines the sibling PR also edits, so the two would conflict in the test file	Valid	Fixed in 0db62af: the tests moved to the end of the file and the shared fixture lines are untouched. One conflict is left, in analytics-helpers.js (see Merging).
The test annotations stop working in PHPUnit 12	Overstated	composer.json pins phpunit/phpunit to ^9.6. Revisit on an upgrade.

6. Not verified

• The real proxy chain in production (Cloudflare and the proxies behind it). The capture adds the X-Forwarded-For header itself. If a proxy in front of the analytics service does not pass it, the service sees no usable address and the country is empty, so the analytics environment has to be checked for this before release.
• The analytics change (rtCamp/godam-analytics#365) was tested from its pull request branch, not from a release.
• The godam-for-woo add-on sends location_ip too. After this PR its Reel Pop events carry an empty one (seen in capture S02, type 4): the service ignores it for browser events and stores the request's address. Its server-sent events (order revenue) are not part of this change. Checked by reading its code and by that one capture.
• Pages cached by an earlier version keep the old params, and older plugin versions keep sending location_ip, until caches expire or sites update. The analytics release has to cope with both (it does: for browser events it ignores the field when the request has an address).
• Unique-viewer counts and country figures may move once when the service starts using the request's address (IPv4 versus IPv6, proxies). Compare a week before and after.
• Tested as a signed-in subscriber, with Chrome driven by a script. Not tested: a logged-out visitor or a site behind a page cache.

7. Scope

None. rtgodam_get_user_ip() is kept, though nothing in the plugin calls it any more, in case an add-on does; its docblock now says so (e10c7bb).

Merging

#2213 (stops sending the signed-in viewer's email) is merged, and develop is merged into this branch. The one conflict, in analytics-helpers.js where the two removed lines sat next to each other, was resolved by keeping both removals, so this diff now only removes locationIP and location_ip.

What changed

• rtgodam_get_localize_array() no longer sets locationIP.
• buildAnalyticsRequestBody() no longer reads locationIP or sends location_ip.

How to check by hand

1. Open a page that has a GoDAM video and view the page source. Search for videoAnalyticsParams: the object has no locationIP.
2. Open DevTools, Network, filter on analytics, play the video. The request payload has no location_ip key.
3. After the analytics release is live, check the stored play still has a country.

[github-actions]

🔍 WordPress Plugin Check Report

⚠️ Status: Passed with warnings

📊 Report

🎯 Total Issues	❌ Errors	⚠️ Warnings
15	0	15

---

⚠️ Warnings (15)

📁 inc/classes/class-video-sitemap.php (1 warning)

📍 Line	🔖 Check	💬 Message
620	WordPress.DB.SlowDBQuery.slow_db_query_meta_key	Detected usage of meta_key, possible slow query.

📁 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.

📁 inc/classes/class-video-sitemap-core-provider.php (1 warning)

📍 Line	🔖 Check	💬 Message
73	WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound	Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "wp_sitemaps_index_entry".

📁 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/10/20/hello-world/ (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/sample-page/ (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/10/20/hello-world/ (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/sample-page/ (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

[KMchaudhary]

Code review

Verdict: LGTM on the code — but release ordering is the real gate. The diff itself is minimal and correct: locationIP is dropped from the page params and location_ip from the request body, nothing else in the plugin produced that value, and the tests + revert check cover it.

1. ⚠️ Do not release before godam-analytics#365 is live (the key risk)

This is already flagged at the top of the description, and it's the thing to hold the line on. Until the service derives the visitor IP from the request itself, shipping this makes every browser event arrive with no location, and the pre-#365 service can't backfill it — country resolution goes empty site-wide. Per the release thread, analytics #365 is item 1 and lands first, with this PR after it. Worth an explicit checkbox on the release checklist so the ordering can't slip. (Old cached JS still sending location_ip is fine — the new service ignores it for browser events.)

2. rtgodam_get_user_ip() is now dead code — admin/godam-transcoder-functions.php:784

The line you removed (733) was its only caller anywhere in inc//admin//assets/src. After this PR the function is defined but unused. Your own independent-review note already spotted it had no other caller — so just make the call explicit: either delete it, or if you're keeping it as a retained public helper (it's a prefixed global an add-on could theoretically call), drop a one-line note to that effect so it doesn't read as an oversight.

3. Expected merge conflict with #2213 (acknowledged) — analytics-helpers.js

In the buildAnalyticsRequestBody destructure, emailId (removed by #2213) and locationIP (removed here) are adjacent lines, so whichever PR lands second will conflict here. You've called this out under Merging; resolution is trivial (keep both removals). Flagging only so the second merge isn't a surprise.

Notes

• The template-tags.php and engagement-functions.php stub additions are intentionally identical to Stop sending the signed-in viewer's email with video analytics #2213 so the two merge in any order — nice.
• The unique-viewer count shift at cutover (IPv4 vs IPv6 string for the same visitor) is correctly parked as a figure to watch, not a code concern.
• PHP test asserts only the key's absence (the CLI env can't inject a real address); the JS test and the godam-dev capture cover the value. Reasonable given the environment.

_{🤖 Automated review via Claude Code (/code-review, high effort).}

[subodhr258]

@KMchaudhary on your comment:

Thanks Kuldip.

1. Release order: agreed. It's a checkbox in the 2.5.0 release ticket's dependency list, and this merges only after godam-analytics#365 is live and the real-IP check on the develop server passes.
2. rtgodam_get_user_ip(): kept on purpose, since it's a prefixed public helper another plugin may call. No rtCamp repo calls it. Its docblock says so now, in e10c7bb.
3. Conflict with Stop sending the signed-in viewer's email with video analytics #2213: resolved in a3bef22 by keeping both removals, and the PR merges cleanly again.

[subodhr258]

Thanks for the review. Taking the three points in order:

1. Release order: agreed. The merge condition stays at the top of the description (merge only after rtCamp/godam-analytics#365 is live and the visitor-IP check passes). No code change.
2. rtgodam_get_user_ip(): kept as a public helper, and e10c7bb now says so in its docblock.
3. Conflict with Stop sending the signed-in viewer's email with video analytics #2213: that PR is merged and develop is merged into this branch (a3bef22). Both removals are kept, so the diff now only removes locationIP and location_ip.