Skip to content

feat(dashboards): Store dashboard snapshot on PUT when revisions flag is enabled#113065

Open
skaasten wants to merge 6 commits intomasterfrom
skaasten/feat/dashboard-revision-snapshot-on-save
Open

feat(dashboards): Store dashboard snapshot on PUT when revisions flag is enabled#113065
skaasten wants to merge 6 commits intomasterfrom
skaasten/feat/dashboard-revision-snapshot-on-save

Conversation

@skaasten
Copy link
Copy Markdown
Contributor

When the organizations:dashboards-revisions feature flag is enabled, the dashboard PUT endpoint now creates a DashboardRevision snapshot of the current dashboard state before writing the new version, enabling history and revert functionality.

The snapshot is produced by the existing DashboardDetailsModelSerializer (the same serializer used by the GET endpoint), so it captures the full pre-edit state including all widgets and their queries.

A few implementation notes worth calling out:

  • Serialization happens outside the transaction.atomic block. DashboardDetailsModelSerializer calls user_service.serialize_many for the createdBy field, which is a hybrid-cloud RPC call that cannot run inside a transaction. The DashboardRevision.objects.create itself stays inside the transaction, so a failed save rolls back the revision too.
  • After each save, revisions beyond the 10 most recent are deleted to bound storage growth.
  • Prebuilt dashboards that haven't been customised yet (represented as dicts, not DB rows) are skipped — there's no existing state to snapshot.

Refs DAIN-1516

… is enabled

When the organizations:dashboards-revisions feature flag is enabled, the
dashboard PUT endpoint now serializes the current dashboard state (using
the existing DashboardDetailsModelSerializer) into a DashboardRevision
before writing the new version. The snapshot and the save are wrapped in
the same transaction so a failed update rolls back the revision too.

Serialization is intentionally done outside the transaction to avoid
making hybrid-cloud RPC calls (user_service.serialize_many) inside an
atomic block.

Revisions beyond the 10 most recent are deleted after each save to
bound storage growth.

Refs DAIN-1516
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code
Copy link
Copy Markdown

linear-code bot commented Apr 15, 2026

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 15, 2026
…unctions

Move snapshot creation and pruning out of the put() method into two
dedicated functions, _take_dashboard_snapshot and _save_dashboard_revision,
to keep the request handler focused on HTTP concerns.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
skaasten and others added 2 commits April 15, 2026 12:45
…n.create_for_dashboard

The retention policy and schema version are model-level concerns — moving
them out of the endpoint and into a classmethod on DashboardRevision makes
the logic reusable from any callsite without duplicating the pruning rules.

The endpoint retains _take_dashboard_snapshot, which owns the API-layer
concerns: feature flag check and serialization via DashboardDetailsModelSerializer.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pruning and field-correctness tests for DashboardRevision.create_for_dashboard
now live in test_dashboard.py, tested directly without going through HTTP.

The endpoint tests are trimmed to only verify the endpoint wires up to the
model correctly — they no longer re-test the model's own behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rd call site

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@skaasten skaasten marked this pull request as ready for review April 15, 2026 17:33
@skaasten skaasten requested review from a team as code owners April 15, 2026 17:33
@jameskeane
Copy link
Copy Markdown
Contributor

Looks good, a couple questions:

  1. Is doing the serialization outside of the transaction a big deal? We don't really support concurrent edits well right now, but just wondering what potential impact that would have.
  2. In _take_dashboard_snapshot you use serialize vs. constructing a DashboardDetailsSerializer object as in the other places of code. What are the implications of this difference?
  3. If serialize fails what happens? We might want to chat with Ben & Martha about this. Maybe we need to insert a dummy revision that can not be rolled back, but informs the user?

…on fails

If serialize() raises (e.g. user_service RPC failure), the exception was
propagating before the transaction block and causing the entire PUT to fail.
The revision snapshot is secondary to the save; catch and log the exception
so the dashboard update always proceeds.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@skaasten
Copy link
Copy Markdown
Contributor Author

Looks good, a couple questions:

  1. Is doing the serialization outside of the transaction a big deal? We don't really support concurrent edits well right now, but just wondering what potential impact that would have.

There's a race condition - between taking the snapshot and entering the transaction, another user could save the dashboard — so the snapshot we store might not accurately reflect the state just before our save.

  1. In _take_dashboard_snapshot you use serialize vs. constructing a DashboardDetailsSerializer object as in the other places of code. What are the implications of this difference?

We're using the model serializer, used for dashboard api responses. This makes the preview endpoint simple, with more work on the restore endpoint to map it back to a model.

  1. If serialize fails what happens? We might want to chat with Ben & Martha about this. Maybe we need to insert a dummy revision that can not be rolled back, but informs the user?

I've added a new commit to log an error if serialize fails, since it's not as critical as saving the new dashboard record.
This shouldn't happen enough to warrant a special design imo, but happy to chat about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants