aminghadersohi commented on code in PR #45843:
URL: https://github.com/apache/superset/pull/45843#discussion_r4429800957
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -1522,13 +1540,14 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
if (dbFetched) {
setDB({
type: ActionType.Fetched,
+ isEditMode,
payload: dbFetched,
});
// keep a copy of the name separate for display purposes
// because it shouldn't change when the form is updated
setDbName(dbFetched.database_name);
}
- }, [dbFetched]);
+ }, [dbFetched, isEditMode]);
Review Comment:
My earlier reply covered only the Close path and missed the successful-save
path. Fixed in 8271d43868fd2001d5f52f0fdb109bd74d8f321a: the resource-copy
effect ignores updates while the modal is closed and rejects records for a
different edit target. It still consumes resource changes while an open
multi-step create is in progress; reopening alone does not replay the cached
resource.
Regression test: 'saving edit then creating uses POST without the old id'.
Companion tests: 'closing edit then creating uses POST without the old id'
and 'X edit then creating uses POST without the old id' in
superset-frontend/src/features/databases/DatabaseModal/index.test.tsx. Each
case checks the picker, a POST without an id and no PUT to the old connection,
then create-after-create and reopening an existing edit. It also checks that
the disabled backdrop does not close the dialog.
Before: with the final Finish test and the modal source from
f875d424a8f02311fd263fe1fcd39425a17c0155, Jest exits 1: `Expected length: 0 /
Received length: 1` for PUT /api/v1/database/10. The same test fails with the
modal source from master at d27687c4cfc8c6e737d66918db72bace8693097d, so this
save-path behavior predates this change.
After: `npx jest src/features/databases/DatabaseModal src/pages/DatabaseList
--runInBand` — 12 suites, 228 tests passed. Branch-scoped pre-commit passes.
Siblings checked:
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1539 —
fixed in 8271d43868fd2001d5f52f0fdb109bd74d8f321a (resource-copy effect; closed
saves/creates cannot restore an id).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1091 —
fixed in 8271d43868fd2001d5f52f0fdb109bd74d8f321a (successful edit save calls
updateResource, then onClose).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1117 —
fixed in 8271d43868fd2001d5f52f0fdb109bd74d8f321a (successful SQLAlchemy-form
create caches the created record, then closes).
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:924 —
fixed in 8271d43868fd2001d5f52f0fdb109bd74d8f321a (shared reset for Close and
all modal onHide callbacks at 2087, 2131 and 2328, including X/keyboard).
- superset-frontend/src/pages/DatabaseList/index.tsx:559 — fixed in
8271d43868fd2001d5f52f0fdb109bd74d8f321a (onHide clears databaseId and show
together; reproduced by the mounted harness).
- superset-frontend/src/views/CRUD/hooks.ts:531 — not affected:
updateResource must retain its cache for other consumers; the modal gates
copying that cache after close.
- superset-frontend/src/views/CRUD/hooks.ts:478 — not affected:
createResource still caches the created id; the open multi-step flow needs it
and the modal ignores it after close.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1175 —
not affected: fetchDB still fetches the requested edit target on open; the
regression reopens an edit and checks its loaded name.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1343 —
not affected: Back from step 3 explicitly refetches the created resource while
the modal remains open.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1351 —
not affected: Back to picker resets the reducer without changing the resource
cache, so it does not trigger the resource-copy effect.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1147 —
not affected: import save and the import-completion effect at 1496 close
through onClose but do not populate dbFetched.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1904 —
not affected: the dataset/SQL Lab CTA fetches a created connection into local
storage; any completion after close is ignored by the guarded form effect.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:1554 —
not affected: available-engine, import-field and SSH effects at 1554-1598 do
not copy a fetched database id; validation callbacks update validation state,
not the form resource.
- superset-frontend/src/features/databases/DatabaseModal/index.tsx:2085 —
not affected: backdrop dismissal is disabled with maskClosable=false in all
three modal variants (also 2150 and 2340).
--
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]
