feat(insights): dashboards insights many-to-many another attempt #9416
Conversation
|
Would be really nice with this better data model We'd need to bring across two tests posthog/posthog/api/test/test_dashboard.py Line 175 in 9228e3e and posthog/posthog/api/test/test_insight.py Line 273 in 9228e3e Hopefully whatever implementation mistake I made that caused queries to explode with data model in this shape isn't in this implementation. I saw different query performance between tests that used the API to do setup and tests that used the ORM to do setup too |
|
UI for adding insight to dashboards |
…are reported correctly
This reverts commit 4d48a16.
|
It currently handles colours and layouts per dashboard. But still only allows an insight to be on one dashboard at a time (in the UI) The dashboard, its time filters, and the displayed insights can get out of sync (as demonstrated in the GIF) but that's the case on master too. The next step after merging this would be to move filters_hash, and possibly last refreshed time, onto the DashboardTile so that we can have a cache of the insight in each state (with dashboard filters and standalone with its own). Migrating to that (en masse) will be tricky because Django's bulk create doesn't trigger the Model's save method or any pre/post save signals |
There was a problem hiding this comment.
I can't approve my own PR, but looks good! Great work @pauldambra
Left some comments and nits, but for me this could be merged... assuming it won't interfere with the release.
| @@ -27,7 +30,7 @@ export function SaveToDashboardModal({ visible, closeModal, insight }: SaveToDas | |||
|
|
|||
| async function save(event: MouseEvent | FormEvent): Promise<void> { | |||
| event.preventDefault() | |||
| updateInsight({ ...insight, dashboard: dashboardId }, () => { | |||
| updateInsight({ ...insight, dashboards: [...(insight.dashboards ?? []), dashboardId] }, () => { | |||
There was a problem hiding this comment.
Nit: I think this will not be exposed to anyone, but we could just do dashboards: [dashboardId] to make sure there's always just one per insight. We'll change this soon anyway, so 🤷
| @@ -458,7 +458,7 @@ export const insightLogic = kea<insightLogicType>({ | |||
| ], | |||
| }), | |||
| selectors: { | |||
| /** filters for data that's being displayed, might not be same as savedInsight.filters or filters */ | |||
| /** filters for data that's being displayed, might not be same as savedInsight. filters or filters */ | |||
| for dashboard in Dashboard.objects.filter(id__in=[d.id for d in dashboards]).all(): | ||
| if dashboard.team != insight.team: | ||
| raise serializers.ValidationError("Dashboard not found") | ||
| DashboardTile.objects.create(insight=insight, dashboard=dashboard) |
There was a problem hiding this comment.
Should we add color and layouts here as well?
There was a problem hiding this comment.
I think they're both default values at this point so we don't need them...
| ("id", models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), | ||
| ("dashboard", models.ForeignKey(on_delete=models.deletion.CASCADE, to="posthog.dashboard")), | ||
| ("insight", models.ForeignKey(on_delete=models.deletion.CASCADE, to="posthog.insight")), | ||
| ("layouts", models.JSONField(default=dict)), | ||
| ("color", models.CharField(blank=True, max_length=400, null=True)), |
There was a problem hiding this comment.
Should we somehow add team_id here as well? It'll make some code more complicated, but other... uhm, safer... and shardable? I'm not sure.
There was a problem hiding this comment.
I think it's too much complication for now. We'd need to explicitly pass it everywhere when it's always equal to both the dashboard and insight team ids.
Easy to add in future too
Problem
WIP
Cleaner attempt at #9408
See #9331 for mini-RFC
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?