mikebridge opened a new pull request, #42839:
URL: https://github.com/apache/superset/pull/42839

   ### SUMMARY
   
   Editing a semantic view's **Description** saved correctly but re-opening the 
Edit datasource modal always showed the field empty, so the user had no way to 
confirm the save and would reasonably conclude it was lost.
   
   The Details tab read `description`/`cache_timeout` from a prop, and both 
callers hand the modal their own copy of the datasource. In Explore that copy 
is never refreshed after a write — `DatasourceControl` passes the *pre-edit* 
`datasource` straight back into the store via `handleDatasourceSave` — so the 
prop is stale from the first save onward. There was nothing to fall back to 
either: `SemanticViewRestApi` omits `"get"` from `include_route_methods`, so 
`GET /api/v1/semantic_view/<id>` answers **405**, as the bug report observed.
   
   So the modal had no read path to the values it had just written.
   
   This PR serves both editable fields from `/<pk>/structure` — the one read 
endpoint the modal already calls — and hydrates the form from that response. 
The prop still seeds the form while the request is in flight and remains the 
fallback if it fails, so a `/structure` error degrades to today's behaviour 
rather than to a blank field.
   
   Two notes for reviewers:
   
   - The structure fetch gains a cancellation guard. That is new exposure 
introduced here, not a pre-existing bug: the response previously only wrote to 
`structure` state, and now also writes to *form* state, so a response for a 
superseded view could otherwise clobber what the user is editing.
   - The endpoint already returned `name`, so returning model scalars is not a 
new kind of thing — but the drift is now explicit enough that the docstring has 
to explain it. The cleaner long-term fix is a real detail route, which would 
also close the 405; that needs `show_columns` and its own security review, so 
it is left as a follow-up.
   
   The addition is purely additive. The other two `/structure` consumers 
(`ColumnSelect.tsx`, `FiltersConfigForm.tsx`) destructure only 
`dimensions`/`name`/`metrics` and are unaffected — verified by reading them, 
not by assuming structural typing covers it.
   
   Disclosure: this change was developed with AI assistance (Claude), on behalf 
of and reviewed by @mikebridge.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Not captured: this checkout has no live semantic-layer environment (the 
local Superset health check is unverified and the semantic-layer extensions are 
not set up here), so the flow could not be exercised end to end. The change is 
covered by unit tests with controls run both ways — see below.
   
   **Before:** re-opening Edit datasource shows Description empty, however many 
times it was saved.
   **After:** re-opening shows the persisted Description and Cache timeout.
   
   ### TESTING INSTRUCTIONS
   
   Automated:
   
   ```bash
   # backend
   pytest tests/unit_tests/semantic_layers/            # 392 passed
   
   # frontend
   cd superset-frontend
   npx jest src/features/semanticViews                                      # 
24 passed
   npx jest src/explore/components/controls/DatasourceControl 
src/pages/DatasetList   # 134 passed
   ```
   
   Controls were run in both directions:
   
   - Reverting `superset/semantic_layers/api.py` fails the two new backend 
assertions with `KeyError: 'description'`.
   - Reverting `SemanticViewEditModal.tsx` fails both new hydration tests.
   - Of the three new frontend tests, only two discriminate. `falls back to the 
caller prop when the structure fetch fails` passes against pre-fix code as well 
— it pins the fallback contract and should not be read as proving the fix.
   
   Manual, with `SEMANTIC_LAYERS` enabled:
   
   1. Open Explore on a semantic view 
(`/explore/?datasource_type=semantic_view&datasource_id=<id>`).
   2. More menu → **Edit datasource** → **Details** tab → set Description to 
`roundtrip test` → **Save**.
   3. Re-open More → **Edit datasource**.
   4. **Expected:** Description shows `roundtrip test`. On `master` it is empty.
   5. Repeat from the Datasources list (edit action on a semantic-view row) — 
that path should behave identically.
   6. Confirm `GET /api/v1/semantic_view/<id>/structure` now includes 
`description` and `cache_timeout`.
   
   QA note: this closes TC28 (SemanticViewEditModal round-trip) for the 
Snowflake, BigQuery and Cube suites, which need re-running against a real 
environment.
   
   ### ADDITIONAL INFORMATION
   
   - [ ] Has associated issue:
   - [x] Required feature flags: `SEMANTIC_LAYERS`
   - [x] 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
   


-- 
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]

Reply via email to