mikebridge opened a new pull request, #45349: URL: https://github.com/apache/superset/pull/45349
### SUMMARY **TL;DR:** Version history said "Dataset metric changed" when only a dataset's description or a column label changed. The dataset editor re-sends every column and metric with a rewritten (empty) settings field, and history recorded that rewrite as an edit to every one of them. It doesn't any more, and a history row that covers several kinds of change now gets a neutral headline instead of the first record's verb. Fixes [SC-126760](https://app.shortcut.com/preset/story/126760). **What changed** 1. **Capture: column and metric `extra` is compared, not rewritten** (`superset/versioning/diff.py`) - The dataset editor serializes every column's and metric's `extra` as `"{}"` when nothing is set. Rows created by the backend store `NULL` or differently spaced JSON. - Dataset column and metric diffs now compare `extra` as JSON: `NULL`, `""` and an object that is empty once null-valued keys are dropped (at any depth) all count as unset. Any other JSON compares as canonical text, so spacing and key order don't matter but values and types do. - Real changes (adding certification, changing or removing a warning) are still recorded, with the stored values. - This uses an optional `comparable` hook on the shared list diff, which only the dataset column and metric diffs use. Chart diffs are unchanged. 2. **Capture: an earlier metric-only edit is no longer replayed** (`superset/versioning/changes/shadow_queries.py`) - The previous state of a dataset's columns and metrics was read at the latest *column* version. After a metric-only save, the next save of that dataset therefore reported the earlier metric change again. - The previous state is now read just before the transaction (`transaction_id - 1`), as the function's docstring already described. 3. **Display: neutral headline for a row that spans several kinds of change** (`display.ts`, `RelatedUpdateRow.tsx`, `VersionHistoryPanel.tsx`) - A related row (for example, a dataset used by a chart) collapses one entity's records from a save into one row. It showed the first record's summary, and metric records sort first. - When the row's records have differing summaries, it now reads "Dataset updated: …" (or "Chart updated" / "Dashboard updated"). Rows for deleted entities keep their redacted text. - This also corrects already-stored rows that mix several kinds of change. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF No visual change beyond the headline text. Example: open dataset `my_virtual_ds` in the dataset editor, change its description and one column's label (`country` → "Country"), leave the metrics alone, and save. - **Before:** - A chart using the dataset shows "Dataset metric changed: my_virtual_ds" in its version history. - The dataset records an edit for every column and for the untouched `count` metric (`extra`: `null` → `"{}"`). - **After:** - The chart shows "Dataset updated: my_virtual_ds". - The dataset records the description change and one column edit (`country`), and no metric edit. - **Real change, unchanged behaviour:** setting a warning on the `count` metric still records "Dataset metric changed". ### TESTING INSTRUCTIONS Automated: ```bash pytest tests/unit_tests/versioning/test_dataset_child_extra.py \ tests/unit_tests/versioning/test_shadow_queries.py \ tests/unit_tests/versioning/ cd superset-frontend && npm run test -- src/features/versionHistory/ ``` - `test_dataset_child_extra.py` covers: - the QA repro (description plus one column label, metrics untouched → only the column edit); - equivalence cases: `NULL`/`""`/`"{}"`, null-valued keys, nested null keys, spacing, key order; - real-change controls: certification added, warning changed or removed, unparseable text changed, `[]`/`0` not unset, `true` vs `1`. - `test_shadow_queries.py` covers a metric-only edit followed by a later save not replaying the metric change. It fails on master. - The frontend tests cover the neutral headline, the deleted-entity case and the panel wiring. Manual: with `VERSION_HISTORY` on, follow the example above and check the chart's and the dataset's version history. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [x] Required feature flags: `VERSION_HISTORY` - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API **Review:** an independent review approved this exact commit (0 blocker, 0 high, 0 medium, 1 low). **Known limits and follow-ups:** - **Existing history isn't rewritten.** Changes are recorded when a version is saved, so a save from before this fix that consists only of phantom metric records still reads "Dataset metric changed". Rows that mix kinds are corrected by the new headline. - **Follow-up (low):** null values nested inside *arrays* in `extra` (for example `{"flags":[{"note":null}]}` vs `{"flags":[{}]}`) still count as an edit. The editor's certification and warning shape has no arrays, so this is outside this fix. - The `transaction_id - 1` read is covered by unit tests against a modelled version timeline; an integration test against real version tables would be a good addition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
