mikebridge commented on code in PR #44258: URL: https://github.com/apache/superset/pull/44258#discussion_r4020779727
########## superset/versioning/activity/creation.py: ########## @@ -0,0 +1,175 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Synthetic "starting version" record for the activity stream (sc-120488). + +``operation_type=0`` transactions emit zero change records by design +(see ``versioning/changes/listener.py``), so an entity's creation — +Continuum's INSERT row, an importer's INSERT, or the retroactive +pre-tracking baseline — exists in the versions API but never appears in +the activity timeline. This module derives ONE synthetic record from the +op=0 shadow row and its ``version_transaction`` so the panel can render +the starting version as the oldest entry, previewable and restorable +like any other version (the record carries the same ``version_uuid`` +the ``/versions/`` family resolves). + +Placement and gating live in the orchestrator: the record is appended +only for the PATH entity (it inherits the activity endpoint's access +gate — edit-gated once #44021 lands), +only when the stream is not truncated, and never for +``include="related"``. If retention pruned the op=0 row or its +transaction, no record is synthesized — the timeline simply starts at +the oldest surviving save. +""" + +from __future__ import annotations + +from typing import Any + +import sqlalchemy as sa +from flask_appbuilder import Model + +from superset.extensions import db +from superset.versioning.activity.kinds import USER_FACING_KIND +from superset.versioning.queries import derive_version_uuid + +#: ``kind`` value of the synthetic record. Like ``__meta__``, the dunder +#: name keeps it out of the field-verb vocabulary; renderers dispatch on +#: it explicitly. +CREATION_RECORD_KIND = "__creation__" Review Comment: Confirmed and fixed in `d8d7424c52`. `__creation__` is now in `ACTIVITY_CHANGE_KINDS` (`schemas.py:206`) and the regenerated OpenAPI enum carries it. Pinned with a schema-load test: the synthetic record is dumped and re-loaded through `ActivityRecordSchema`, which failed before the change and passes now. Worth noting the failure was silent from the API's side — the record serialised fine and only the validator rejected it — so without the round-trip test this would have surfaced as a client-side schema mismatch. ########## superset/versioning/activity/creation.py: ########## @@ -0,0 +1,175 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Synthetic "starting version" record for the activity stream (sc-120488). + +``operation_type=0`` transactions emit zero change records by design +(see ``versioning/changes/listener.py``), so an entity's creation — +Continuum's INSERT row, an importer's INSERT, or the retroactive +pre-tracking baseline — exists in the versions API but never appears in +the activity timeline. This module derives ONE synthetic record from the +op=0 shadow row and its ``version_transaction`` so the panel can render +the starting version as the oldest entry, previewable and restorable +like any other version (the record carries the same ``version_uuid`` +the ``/versions/`` family resolves). + +Placement and gating live in the orchestrator: the record is appended +only for the PATH entity (it inherits the activity endpoint's access +gate — edit-gated once #44021 lands), +only when the stream is not truncated, and never for +``include="related"``. If retention pruned the op=0 row or its +transaction, no record is synthesized — the timeline simply starts at +the oldest surviving save. +""" + +from __future__ import annotations + +from typing import Any + +import sqlalchemy as sa +from flask_appbuilder import Model + +from superset.extensions import db +from superset.versioning.activity.kinds import USER_FACING_KIND +from superset.versioning.queries import derive_version_uuid + +#: ``kind`` value of the synthetic record. Like ``__meta__``, the dunder +#: name keeps it out of the field-verb vocabulary; renderers dispatch on +#: it explicitly. +CREATION_RECORD_KIND = "__creation__" + +#: ``creation_kind`` machine values — the API ships these, never display +#: strings; the frontend owns the user-facing copy in ONE constant. +CREATION_KIND_PRE_TRACKING = "pre_tracking" +CREATION_KIND_CREATED = "created" +CREATION_KIND_IMPORTED = "imported" +CREATION_KINDS: tuple[str, ...] = ( + CREATION_KIND_PRE_TRACKING, + CREATION_KIND_CREATED, + CREATION_KIND_IMPORTED, +) + + +def _creation_kind_for(action_kind: str | None) -> str: + # pylint: disable=import-outside-toplevel + from superset.versioning.changes import ACTION_KIND_BASELINE, ACTION_KIND_IMPORT + + if action_kind == ACTION_KIND_BASELINE: + return CREATION_KIND_PRE_TRACKING + if action_kind == ACTION_KIND_IMPORT: + return CREATION_KIND_IMPORTED + # Continuum's own INSERT (ordinary creation), and clones — a clone IS + # a creation from the new entity's point of view. + return CREATION_KIND_CREATED Review Comment: Good catch, and you're right that the staging-only scoping doesn't hold — `VERSION_HISTORY` / `ENABLE_VERSIONING_CAPTURE` have defaulted on since #42801, so master-tracking installs already hold unstamped baselines. Took the neutral option in `d8d7424c52`: a new `unknown` creation kind rather than guessing. `ACTION_KIND_BASELINE` → `pre_tracking`, `ACTION_KIND_IMPORT` → `imported`, `ACTION_KIND_CLONE` → `created` (a clone is a creation from the new entity's side), and anything unstamped → `unknown`. The old catch-all returned `created`, which was the invented provenance you flagged. Flagging the consequence: this changes what existing installs display for unstamped baselines, from "Created" to whatever the client renders for `unknown`. That seemed better than continuing to assert something we can't know, but it is a visible change rather than a pure bug fix. ########## superset/versioning/activity/orchestrator.py: ########## @@ -315,6 +315,38 @@ def get_activity( if q: records = [r for r in records if _record_matches(r, q)] + # Synthetic starting-version row (sc-120488): op=0 transactions emit + # zero change records by design, so the entity's creation — INSERT, + # import, or retroactive pre-tracking baseline — never rides the + # stream above. Appended as the OLDEST entry, and only when the + # stream truly ends here: never on a truncated stream (older records + # exist beyond the clamp, so the end was not reached), never for + # include="related" (it is a self record), and honoring the same + # since/until bounds and search filter as every fetched record. It + # rides the list BEFORE ``total`` so the count endpoint agrees with + # the page contents (+1) and pagination places it on the final page. + # Gating is inherited, not re-implemented: the endpoint access-gated + # the path entity before calling here (edit-gated once #44021 lands), + # and the record only ever describes that same path entity. + if include != "related" and not truncated: Review Comment: Good catch — the guards were there but nothing isolated them, so each could be deleted with the suite staying green. `d8d7424c52` adds cases that each fail when their own conjunct is removed: a truncated-stream case (the one you called out as mattering most — a clamped stream otherwise gets an "Original version" row claiming a start that isn't the start), plus inclusive-`since`, exclusive-`until`, and search-conjunct boundary cases. These pass on the correct implementation and fail against the corresponding single-guard mutation, which is the property that was missing. ########## superset-frontend/src/features/versionHistory/SaveGroupItem.test.tsx: ########## @@ -129,3 +135,78 @@ test('rows of the highlighted group show the active timeline dot', () => { expandGroup(historical); expect(dotBorderColor(current)).not.toBe(dotBorderColor(historical)); }); + +const creationGroup = (overrides: Partial<SaveGroup> = {}): SaveGroup => + makeGroup({ + transactionId: 5, + versionUuid: 'v-created', + issuedAt: '2025-12-05T17:18:00', + changedBy: { id: 1, first_name: 'Ada', last_name: 'Lovelace' }, + creationKind: 'created', + ...overrides, + }); + +test('a chart starting group exposes an explicit preview action', async () => { + const onPreview = jest.fn(); + const group = creationGroup(); + renderItem({ group, onPreview }); + + const button = screen.getByRole('button', { name: 'Preview this version' }); + await userEvent.click(button); + + expect(onPreview).toHaveBeenCalledWith(group); +}); + +test('a dashboard starting group exposes the same preview action via keyboard', async () => { + const onPreview = jest.fn(); + const group = creationGroup({ creationKind: 'pre_tracking' }); + renderItem({ group, onPreview, entityType: 'dashboard' }); + + const button = screen.getByRole('button', { name: 'Preview this version' }); + button.focus(); + await userEvent.type(button, '{enter}', { skipClick: true }); + + expect(onPreview).toHaveBeenCalledWith(group); +}); + +test('the current starting version has nothing to preview', () => { + renderItem({ group: creationGroup(), isCurrent: true }); + + expect( + screen.queryByRole('button', { name: 'Preview this version' }), + ).not.toBeInTheDocument(); +}); + +test('ordinary record-bearing groups do not grow the creation affordance', () => { + renderItem({ + group: creationGroup({ + creationKind: undefined, + records: [ + { + version_uuid: 'v-1', + entity_kind: 'chart', + entity_uuid: 'e-1', + entity_name: 'My chart', + entity_deleted: false, + entity_deletion_state: null, + source: 'self', + transaction_id: 5, + action_kind: null, + issued_at: '2025-12-05T17:18:00', + changed_by: null, + kind: 'metric', + operation: 'add', + path: ['params'], + from_value: null, + to_value: null, + summary: '', + impact: null, + }, + ], + }), + }); + + expect( + screen.queryByRole('button', { name: 'Preview this version' }), + ).not.toBeInTheDocument(); +}); Review Comment: Thanks for re-issuing this — taken in `d8d7424c52`, along with the complementary record-bearing case so each conjunct is pinned independently rather than varying together. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
