Enable GeoIP resolution in PostHog client initialization

[JSv4]

Summary

This PR enables GeoIP resolution in the PostHog client by explicitly setting disable_geoip=False during initialization. By default, PostHog's server-side SDKs disable GeoIP resolution, but we want to allow PostHog to resolve geographic data from the server's IP address.

Key Changes

• config/telemetry.py: Added disable_geoip=False parameter when initializing the PostHog client in _get_posthog_client()
• opencontractserver/users/apps.py: Added posthog.disable_geoip = False configuration in the ready() method to ensure GeoIP is enabled for the module-level PostHog instance
• opencontractserver/tests/test_telemetry.py: Added comprehensive test test_posthog_client_initialized_with_geoip_enabled() to verify that the PostHog client is initialized with GeoIP enabled

Implementation Details

The changes ensure that GeoIP resolution is enabled in both initialization paths:

1. When creating a new PostHog client instance in the telemetry module
2. When configuring the module-level PostHog instance in the users app

The test validates that the disable_geoip parameter is explicitly set to False during client initialization, preventing regression of this behavior.

https://claude.ai/code/session_01YGVkYhBaCeunwnYGWxLoVN

[claude]

PR Review: Enable GeoIP resolution in PostHog client initialization

Overall this is a small, focused change with clear intent and adequate test coverage. A few things worth addressing before merging:

---

Issues

1. Outdated mock accessor style — test_telemetry.py:84

The new test uses:

init_kwargs = self.mock_posthog_class.call_args[1]

call_args[1] is the old-style way of accessing keyword arguments from a MagicMock. Since Python 3.8+, unittest.mock exposes call_args.kwargs which is explicit and immune to positional-vs-keyword argument ordering issues. The other test assertions in this file already use call_args[1] on capture so this is consistent with the existing pattern, but it would be worth standardising on call_args.kwargs in the new test at minimum:

init_kwargs = self.mock_posthog_class.call_args.kwargs

2. Missing CHANGELOG.md update

Per the project's CLAUDE.md conventions, CHANGELOG.md should be updated for production code changes including architecture/configuration changes. This PR intentionally changes telemetry behavior and should have a note in the [Unreleased] section under ### Changed.

---

Observations (non-blocking)

Dual configuration paths are intentional but undocumented

The PR touches two separate PostHog configuration mechanisms:

• config/telemetry.py — the custom Posthog singleton instance used by record_event/arecord_event
• opencontractserver/users/apps.py — the module-level posthog instance (the import posthog; posthog.capture() pattern from the official Django integration)

These are genuinely independent so both changes are necessary. A brief comment in apps.py explaining why disable_geoip is set redundantly (in case future maintainers wonder why the same setting appears in two places) would help.

GeoIP resolves the server's IP, not end-user IPs

The PR description is accurate — this resolves geographic data from the server's outgoing IP, not from end users. For self-hosted deployments this reveals where the installation is hosted. Given that OpenContracts is AGPL and often self-hosted in sensitive environments (legal industry), it is worth confirming that operators are aware of what GeoIP data PostHog receives. The existing TELEMETRY_ENABLED opt-out handles this at the feature level, but the in-code comments do not mention the geographic data specifically.

---

Test Coverage

The new test correctly verifies disable_geoip=False is passed at construction time. The setUp/tearDown reset cycle ensures the singleton is rebuilt fresh for each test, so there is no risk of the PostHog class mock not being called. The test logic is sound.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

PR Review: Enable GeoIP Resolution in PostHog Client

The change itself is minimal and the intent is clear. A few items worth discussing before merging:

---

Potentially Dead Configuration (apps.py change)

The module-level posthog.disable_geoip = False in apps.py may have no effect. Searching the backend Python code, there are no posthog.capture() calls using the module-level posthog client — all telemetry events flow exclusively through the Posthog class instance in config/telemetry.py. The posthog.api_key / posthog.host settings were already there before this PR, so this is a pre-existing pattern, but it's worth verifying whether the module-level client is actually used for event capture anywhere. If it isn't, the apps.py change (and its dedicated test class) are configuring a code path that never fires.

---

GeoIP Resolves the Server's IP, Not the User's

For server-side PostHog events, disable_geoip=False causes PostHog to geolocate the server's IP, not individual users'. The PR description acknowledges this ("resolve geographic data from the server's IP address"), but for self-hosted enterprise deployments this will always report the datacenter location. If user-level geo is ever the intent, a different approach is needed ($ip property on events). Worth adding a comment near the setting so future maintainers understand what data is actually being collected.

---

Test Isolation: Missing tearDown in UsersAppGeoIPTestCase

# test_telemetry.py, line 142
posthog.disable_geoip = True  # mutates global module state
app_config.ready()            # sets it back to False
self.assertFalse(posthog.disable_geoip)
# No tearDown restores posthog.disable_geoip

The test leaves the global posthog.disable_geoip as False after it runs — which happens to match the production value, so other tests aren't currently affected. But proper isolation requires restoring state in tearDown. Something like:

def setUp(self):
    import posthog as posthog_module
    self._original_disable_geoip = getattr(posthog_module, 'disable_geoip', True)

def tearDown(self):
    import posthog as posthog_module
    posthog_module.disable_geoip = self._original_disable_geoip

---

Side Effects from Calling ready() Directly in Tests

test_ready_sets_geoip_enabled calls app_config.ready(), which also triggers _warm_pipeline_registry() — discovering and loading all parsers, embedders, thumbnailers, and post-processors. That's meaningful startup overhead in a focused unit test. Mocking the method out keeps the test targeted:

with patch.object(app_config, '_warm_pipeline_registry'):
    app_config.ready()

---

Missing Guard Before Accessing call_args.kwargs

In test_posthog_client_initialized_with_geoip_enabled:

init_kwargs = self.mock_posthog_class.call_args.kwargs  # AttributeError if mock wasn't called
self.assertIn("disable_geoip", init_kwargs)

If record_event returns early (e.g., installation lookup fails unexpectedly), call_args is None and this raises AttributeError rather than a clear assertion failure. Adding self.mock_posthog_class.assert_called_once() before the kwargs check gives a more useful failure message.

---

Minor: Assertion Style Inconsistency

The new test uses call_args.kwargs while the existing test_record_event_success uses call_args[1]. Both work correctly; call_args.kwargs is the more readable modern form, so it may be worth updating the existing test to match for consistency.

---

Privacy / Disclosure Note (Non-blocking)

With disable_geoip=False, PostHog now receives the server's IP on every telemetry event. For an enterprise AGPL platform where operators may run on internal infrastructure, this could expose deployment location data. Worth ensuring the telemetry opt-in documentation mentions that geographic data is included when telemetry is enabled.

---

Summary: The core config/telemetry.py change is correct and the regression test for it is solid. Main concerns are: (1) whether the apps.py module-level path is actually active and sending events, (2) the test isolation gap in UsersAppGeoIPTestCase, and (3) the pipeline-warming side effect during the test. The privacy/geo documentation point is non-blocking but worth addressing before the next release.