Fixes #27536: Allow removing optional password fields in connection edit form#27589
Fixes #27536: Allow removing optional password fields in connection edit form#27589miantalha45 wants to merge 20 commits intoopen-metadata:mainfrom
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
1 similar comment
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
@miantalha45 can you please also upload a screen recording of the fix |
|
sure @ulixius9 let me share it. |
There was a problem hiding this comment.
Hey @miantalha45, Thanks for the contribution. Can you please
- Address the comments from Gitar-bot?
- Add playwright e2e coverage for the new scenarios? You can use the existing ServiceForm.spec.ts to add the test case
- Attach the screen recording for the changes
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
sure @aniketkatkar97 Let me do all these changes. I will let you know once I have done. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
| const isMaskedValue = useMemo( | ||
| () => ALL_ASTERISKS_REGEX.test(props.value), | ||
| [props.value] | ||
| ); | ||
|
|
||
| const wasOriginallyMasked = useRef(isMaskedValue); | ||
|
|
||
| const [isEditing, setIsEditing] = useState(!isMaskedValue); |
There was a problem hiding this comment.
wasOriginallyMasked is initialized only on first render. If props.value is populated asynchronously (e.g., from undefined to a masked value) this widget will never enter the saved-indicator state. Consider updating the ref when isMaskedValue becomes true (e.g., wasOriginallyMasked.current ||= isMaskedValue) so the indicator reliably appears once a masked value arrives.
| <Button | ||
| data-testid={`password-update-btn-${props.id}`} | ||
| size="small" | ||
| type="link" | ||
| onClick={handleUpdate}> | ||
| {t('label.update')} | ||
| </Button> | ||
| {!props.required && ( | ||
| <Button | ||
| danger | ||
| data-testid={`password-remove-btn-${props.id}`} | ||
| icon={<CloseCircleOutlined />} | ||
| size="small" | ||
| type="link" | ||
| onClick={handleRemove}> | ||
| {t('label.remove')} | ||
| </Button> |
There was a problem hiding this comment.
The saved-indicator actions ignore disabled/readonly: users can click Update/Remove even when the widget is disabled/read-only, which still flips local state (and Remove triggers onChange). The action buttons should be disabled (or hidden) when props.disabled or props.readonly is true, and the handlers should no-op in that case.
| "passed-x-checks": "Passed {{count}} checks", | ||
| "password-error-message": "Password must be a minimum of 8 and a maximum of 56 characters long and contain at least one uppercase character (A-Z), one lowercase character (a-z), one number, and one special character (such as !, %, @, or #)", | ||
| "password-pattern-error": "Password must be of minimum 8 and maximum 56 characters, with one special , one upper, one lower case character", | ||
| "password-saved": "Password is saved. Enter a new value to update or remove it.", |
There was a problem hiding this comment.
This helper text says users can "update or remove" the saved password, but the Remove action is intentionally hidden when the field is required. Please adjust the message to avoid promising an unavailable action (e.g., make it conditional based on required, or use wording that only mentions Update).
| "password-saved": "Password is saved. Enter a new value to update or remove it.", | |
| "password-saved": "Password is saved. Enter a new value to update it.", |
| it('Should return to saved indicator after clicking Cancel', async () => { | ||
| render(<PasswordWidget {...mockProps} />); | ||
|
|
||
| const passwordInput = screen.getByTestId( | ||
| 'password-input-widget-root/password' | ||
| ); | ||
| fireEvent.click(screen.getByTestId('password-update-btn-root/password')); | ||
| fireEvent.click(screen.getByTestId('password-cancel-edit-btn-root/password')); | ||
|
|
||
| fireEvent.focus(passwordInput); | ||
| expect(screen.getByTestId('password-update-btn-root/password')).toBeInTheDocument(); | ||
| expect(screen.queryByTestId('password-input-widget-root/password')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Test coverage doesn’t currently validate the core Cancel semantics after an edit/remove (i.e., that the edited value is reverted in form state, not just that the indicator is shown again). Add a test that: click Update, type a new value, click Cancel, and assert the original masked value is restored (and/or onChange is called with the original value as appropriate).
| const handleRemove = useCallback(() => { | ||
| props.onChange(''); | ||
| setIsEditing(true); | ||
| }, [props]); | ||
|
|
||
| const handleUpdate = useCallback(() => { | ||
| setIsEditing(true); | ||
| }, []); | ||
|
|
||
| const handleCancelEdit = useCallback(() => { | ||
| setIsEditing(false); | ||
| props.onChange(props.value); | ||
| }, [props]); |
There was a problem hiding this comment.
handleCancelEdit does not restore the original masked value when the user has typed a new value (or clicked Remove). Since props.value will already be the edited value by the time Cancel is clicked, calling props.onChange(props.value) keeps the edited value in the form state while hiding the input again. Store the original value (e.g., in a ref when entering edit mode) and restore that on Cancel (or avoid mutating form state until the user confirms).
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
1 similar comment
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 4 resolved / 4 findingsEnables removal of optional password fields in the connection edit form while resolving state mismatch issues, improper secret deletion handling, and stale commented code. Locale keys have also been updated to ensure proper translation support. ✅ 4 resolved✅ Bug: Cancel does not revert form value, causing UI/state mismatch
✅ Edge Case: deleteSecretInternal may throw on non-existent secrets (AWS/GCP/Azure)
✅ Quality: Commented-out code should be removed, not left in place
✅ Quality: Locale files use untranslated English for password-saved key
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Problem
Password fields in the connection edit form don't return values from the API (expected, for security). However, this meant users had no way to:
Fixes #27536
Solution
When a password field contains a masked value (
*********) from the API, instead of showing a confusing empty input, we now show a saved password indicator with explicit actions:Changes
PasswordWidget.tsx— new saved/edit/remove state machine for masked password fieldspassword-widget.less— styles for the saved password indicatoren-us.json+ all 17 locale files — newmessage.password-savedi18n keyPasswordWidget.test.tsx— updated and expanded tests (9 → 13 tests)Recording
Screen.Recording.2026-04-22.at.5.00.45.PM.mov
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
SecretsManagerto handle null password values as explicit removal requests.ExternalSecretsManagerandDBSecretsManagerto delete existing secrets when an empty string is provided.DBSecretsManagerTestandKubernetesSecretsManagerTestto verify secret removal logic.This will update automatically on new commits.