Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up dashboard version history api #4372

Closed
wants to merge 11 commits into from
Closed

Set up dashboard version history api #4372

wants to merge 11 commits into from

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented May 17, 2021

Changes

Set up versions API for #3551

The dashboard insight page is dependent on some of the filter work going on right now, so I'm skipping to the version history in the meanwhile. This is just a basic setup so that I can also start putting it together on the frontend

A version is created whenever:

  • a dashboard item is created
  • a dashboard item is updated

Previous state is set so the user can "roll back" to a previous version of the updated dashboard item. Otherwise it's empty (there's no previous state to a newly created object)

Screen Shot 2021-05-18 at 11 37 11 AM

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious

@timgl timgl temporarily deployed to posthog-pr-4372 May 17, 2021 19:59 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 17, 2021 20:58 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 17, 2021 22:11 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 17, 2021 22:28 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 18, 2021 15:35 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 18, 2021 15:43 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 18, 2021 15:46 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 18, 2021 15:59 Inactive
@timgl timgl temporarily deployed to posthog-pr-4372 May 18, 2021 16:00 Inactive
@liyiy liyiy marked this pull request as ready for review May 18, 2021 16:01
@liyiy liyiy requested review from EDsCODE and macobo May 18, 2021 16:04
@liyiy
Copy link
Contributor Author

liyiy commented May 18, 2021

I am still new to Python and Django, so please feel free to critique away! It will help me learn faster 😀

@timgl
Copy link
Collaborator

timgl commented May 18, 2021

Wouldn't it be a lot simpler to add a 'parent' primary key to DashboardItems, so we can create a chain of version history you can go backwards/forwards in?

@liyiy
Copy link
Contributor Author

liyiy commented May 18, 2021

Wouldn't it be a lot simpler to add a 'parent' primary key to DashboardItems, so we can create a chain of version history you can go backwards/forwards in?

That does sound simpler, but just some clarifying questions then:

When we update a dashboard item, are you saying that we should create two dashboard items out of it and set the original as the parent of the updated? And then if we're looking up the entire history of a dashboard item, wouldn't we be continuously looking up the parent of each dashboard item? (this one I'm a little confused about)

Also, the version history mocks include commenting, I don't know if we'd want to tack that onto dashboard item too?

However, I understand that storing a whole state in a key is not ideal, perhaps that previous_state key could be a previous_instance key that links to the parent dashboard item instead? But then we'd really be creating a new dashboard item every time we're "updating" instead of actually patching over it, in order to maintain its previous state in the database. And then we'd have to point that original dashboard item we're "updating"'s key to the newly created one instead 🤔

Let me know if I'm understanding things correctly or not 🙂

@timgl
Copy link
Collaborator

timgl commented May 18, 2021

I'm not wedded to my approach but it seems easier than copying the state in another object, and we're already creating new DashboardItems any time a user changes something (minus the parent part)

When we update a dashboard item, are you saying that we should create two dashboard items out of it and set the original as the parent of the updated?

Something like this

And then if we're looking up the entire history of a dashboard item, wouldn't we be continuously looking up the parent of each dashboard item?

You'd be able to get the entire history by looking up all DashboardItems that have parent=1

Also, the version history mocks include commenting, I don't know if we'd want to tack that onto dashboard item too?

I think this could be a InsightComment model that links to the parent dashboarditem.

@liyiy
Copy link
Contributor Author

liyiy commented May 18, 2021

I'm not wedded to my approach but it seems easier than copying the state in another object, and we're already creating new DashboardItems any time a user changes something (minus the parent part)

When we update a dashboard item, are you saying that we should create two dashboard items out of it and set the original as the parent of the updated?

Something like this

And then if we're looking up the entire history of a dashboard item, wouldn't we be continuously looking up the parent of each dashboard item?

You'd be able to get the entire history by looking up all DashboardItems that have parent=1

Also, the version history mocks include commenting, I don't know if we'd want to tack that onto dashboard item too?

I think this could be a InsightComment model that links to the parent dashboarditem.

This makes sense! Thanks

@liyiy liyiy closed this May 19, 2021
@liyiy liyiy reopened this May 19, 2021
@liyiy liyiy mentioned this pull request May 19, 2021
5 tasks
@liyiy
Copy link
Contributor Author

liyiy commented May 19, 2021

Closing to implement changes on #4395 instead

@liyiy liyiy closed this May 19, 2021
@paolodamico paolodamico deleted the version-history branch May 20, 2021 22:18
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.

2 participants