kgabryje commented on PR #40466:
URL: https://github.com/apache/superset/pull/40466#issuecomment-4591360324
Thanks for the thorough review @aminghadersohi. All five items addressed in
`abd7f7035f`.
**MEDIUM 1 — guarded mount effect.** Added `if (!encryptedField) return;` at
the top of the `EncryptedField` mount `useEffect`, plus a regression test that
renders with `db={undefined}` and asserts `onParametersChange` is not called.
Four existing tests (unmapped/null/undefined/empty engine, missing db) updated
to assert the new correct behavior.
**MEDIUM 2 — scoped delete-on-empty to a new action type.** Went with your
option (a). New `ActionType.ClearEncryptedExtraKey` does the delete and is
dispatched only from `handlePublicToggle`. `EncryptedExtraInputChange` reverted
to its original master semantics — generic callers that clear a field by
typing-and-deleting now correctly store an empty string, no accidental key
removal. Tests reorganized: `EncryptedExtraInputChange` retains empty strings;
new `ClearEncryptedExtraKey` tests cover delete + no-op + the four
malformed-JSON recovery cases (`'null'`, malformed, JSON primitive, JSON
array). The `handlePublicToggle` integration test was updated to assert
`onClearEncryptedExtraKey` is called and `onEncryptedExtraInputChange` is not.
**NIT 1 — OAuth2ClientField `useEffect` eslint-disable comment.** Expanded
to explain *why* `defaultValue` is excluded (static per-engine default;
depending on it would cause spurious re-syncs when the parent re-renders).
**NIT 2 — `DatabaseConnectionForm` `useEffect` deps.** Added
`db?.parameters` to the dep array so `computeInitialIsPublic` re-runs when
`parameters.service_account_info` / `parameters.oauth2_client_info` arrive
async on edit load. `setIsPublic` is idempotent so any over-runs are no-ops.
Dropped the "intentionally narrowed" wording since we're no longer narrowing.
Also bundled a small Bito follow-up: narrowed both new `JSON.parse` `try { }
catch { }` blocks to `catch (e) { if (!(e instanceof SyntaxError)) throw e; }`
per BITO.md rule 10033 — only swallow the parse's own error instead of catching
everything.
Pre-push: `npm run plugins:build && npm run type && npm run lint` all clean;
85 jest tests pass across the DatabaseConnectionForm / hooks suites and 43 in
the dbReducer suite.
Re-requesting review.
--
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]
