Skip to content

fix(revenue-analytics): Use correct field name in persons join#53594

Merged
arthurdedeus merged 3 commits intomasterfrom
fix/revenue-analytics-person-join
Apr 8, 2026
Merged

fix(revenue-analytics): Use correct field name in persons join#53594
arthurdedeus merged 3 commits intomasterfrom
fix/revenue-analytics-person-join

Conversation

@arthurdedeus
Copy link
Copy Markdown
Contributor

@arthurdedeus arthurdedeus commented Apr 7, 2026

Problem

The revenue view join used field_name="person" instead of "persons", silently breaking the person-to-revenue link. Existing
tests passed because they asserted the wrong value or bypassed database_operations entirely.

Part of #52270

Changes

  • Fixed field_name to "persons" in source_templates.py and the corresponding test assertion
  • Added an integration test that queries persons.properties through the customer revenue view after calling
    database_operations() — fails with "Unable to resolve field" if the name is wrong
  • Enriched test CSV data with posthog_person_distinct_id on specific customers, subscriptions, and charges to support person
    resolution testing
  • Added README documenting all test data scenarios

How did you test this code?

New integration test for persons join resolution and existing test suite passes.

Publish to changelog?

No

Docs update

No

LLM context

Co-authored with Claude Code (Opus 4.6).

joining_table_name="persons",
joining_table_key="pdi.distinct_id",
field_name="person",
field_name="persons",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the fix

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this file will only make sense when the whole stack is merged, as the distinct id resolution is coming in the next PR

@arthurdedeus arthurdedeus marked this pull request as ready for review April 7, 2026 17:36
@arthurdedeus arthurdedeus requested a review from a team as a code owner April 7, 2026 17:36
@assign-reviewers-posthog assign-reviewers-posthog bot requested a review from a team April 7, 2026 17:37
Base automatically changed from fix/groups-materialized-view-joins to master April 7, 2026 17:41
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

🎭 Playwright report · View test results →

⚠️ 1 flaky test:

  • Materialize view pane (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 7, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/test/base.py
Line: 77

Comment:
**Removed explanatory comment not made obsolete by the change**

Per the project coding conventions: *"when refactoring or moving code, preserve existing comments unless they are explicitly made obsolete by the change."* The rename of `source``self.source` does not invalidate this comment — it still explains *why* the `ExternalDataSchema` objects are created here (required by `RevenueAnalyticsBaseView` to discover the right tables). Please restore it:

```suggestion
        # Besides the default creations above, also create the external data schema
        # because this is required by the `RevenueAnalyticsBaseView` to find the right tables
        _invoices_schema = ExternalDataSchema.objects.create(
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(revenue-analytics): Use correct fiel..." | Re-trigger Greptile

credential=credential,
)

_invoices_schema = ExternalDataSchema.objects.create(
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.

P2 Removed explanatory comment not made obsolete by the change

Per the project coding conventions: "when refactoring or moving code, preserve existing comments unless they are explicitly made obsolete by the change." The rename of sourceself.source does not invalidate this comment — it still explains why the ExternalDataSchema objects are created here (required by RevenueAnalyticsBaseView to discover the right tables). Please restore it:

Suggested change
_invoices_schema = ExternalDataSchema.objects.create(
# Besides the default creations above, also create the external data schema
# because this is required by the `RevenueAnalyticsBaseView` to find the right tables
_invoices_schema = ExternalDataSchema.objects.create(
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/test/base.py
Line: 77

Comment:
**Removed explanatory comment not made obsolete by the change**

Per the project coding conventions: *"when refactoring or moving code, preserve existing comments unless they are explicitly made obsolete by the change."* The rename of `source``self.source` does not invalidate this comment — it still explains *why* the `ExternalDataSchema` objects are created here (required by `RevenueAnalyticsBaseView` to discover the right tables). Please restore it:

```suggestion
        # Besides the default creations above, also create the external data schema
        # because this is required by the `RevenueAnalyticsBaseView` to find the right tables
        _invoices_schema = ExternalDataSchema.objects.create(
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@arthurdedeus arthurdedeus self-assigned this Apr 7, 2026
@arthurdedeus arthurdedeus force-pushed the fix/revenue-analytics-person-join branch from fc3a1c3 to ee07485 Compare April 7, 2026 17:58
Changed field_name from 'person' to 'persons' in the revenue view
join created by database_operations. Added an integration test that
queries through the join to verify HogQL can resolve the persons
field — would have caught this bug. Enriched test CSV data with
posthog_person_distinct_id on specific customers, subscriptions,
and charges to support resolution testing.
@arthurdedeus arthurdedeus force-pushed the fix/revenue-analytics-person-join branch from ee07485 to c457be8 Compare April 7, 2026 18:07
Copy link
Copy Markdown
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

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

oops

@arthurdedeus arthurdedeus merged commit b1249a3 into master Apr 8, 2026
273 of 280 checks passed
@arthurdedeus arthurdedeus deleted the fix/revenue-analytics-person-join branch April 8, 2026 20:31
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