Skip to content

Commit

Permalink
fix: show field error when superset embed id changes from valid to in…
Browse files Browse the repository at this point in the history 
…valid
  • Loading branch information
@HendrikThePendric
HendrikThePendric committed Mar 11, 2025
1 parent ef898c9 commit 4946b6a
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 36 deletions.
7 changes: 3 additions & 4 deletions src/components/DashboardsBar/ConfigureSupersetDashboard/CreateSupersetDashboardModal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const CreateSupersetDashboardModal = ({
const {
hasFieldChanges,
isSupersetEmbedIdValid,
isSupersetEmbedIdFieldTouched,
shouldShowSupersetEmbedIdError,
values,
onChange,
onSupersetEmbedIdFieldBlur,
Expand All @@ -64,9 +64,8 @@ export const CreateSupersetDashboardModal = ({
<ModalContent>
<form onSubmit={handleSubmit} id={FORM_ID}>
<SupersetDashboardFields
isSupersetEmbedIdValid={isSupersetEmbedIdValid}
isSupersetEmbedIdFieldTouched={
isSupersetEmbedIdFieldTouched
shouldShowSupersetEmbedIdError={
shouldShowSupersetEmbedIdError
}
values={values}
onChange={onChange}
Expand Down
12 changes: 4 additions & 8 deletions src/components/DashboardsBar/ConfigureSupersetDashboard/SupersetDashboardFields.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ export const SupersetDashboardFields = ({
submitting,
onChange,
onSupersetEmbedIdFieldBlur,
isSupersetEmbedIdValid,
isSupersetEmbedIdFieldTouched,
shouldShowSupersetEmbedIdError,
}) => {
const supersetEmbedIdFieldHasError =
isSupersetEmbedIdFieldTouched && !isSupersetEmbedIdValid
return (
<>
<InputField
Expand Down Expand Up @@ -63,9 +60,9 @@ export const SupersetDashboardFields = ({
value={values.supersetEmbedId}
disabled={submitting}
name={FIELD_NAME_SUPERSET_EMBED_ID}
error={supersetEmbedIdFieldHasError}
error={shouldShowSupersetEmbedIdError}
validationText={
supersetEmbedIdFieldHasError
shouldShowSupersetEmbedIdError
? i18n.t('Invalid UUID')
: undefined
}
Expand Down Expand Up @@ -96,8 +93,7 @@ export const SupersetDashboardFields = ({
}

SupersetDashboardFields.propTypes = {
isSupersetEmbedIdFieldTouched: PropTypes.bool,
isSupersetEmbedIdValid: PropTypes.bool,
shouldShowSupersetEmbedIdError: PropTypes.bool,
submitting: PropTypes.bool,
values: PropTypes.shape({
code: PropTypes.string,
Expand Down
7 changes: 3 additions & 4 deletions src/components/DashboardsBar/ConfigureSupersetDashboard/UpdateSupersetDashboardModal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const UpdateSupersetDashboardModal = ({ closeModal }) => {
const {
hasFieldChanges,
isSupersetEmbedIdValid,
isSupersetEmbedIdFieldTouched,
shouldShowSupersetEmbedIdError,
values,
onChange,
onSupersetEmbedIdFieldBlur,
Expand Down Expand Up @@ -130,9 +130,8 @@ export const UpdateSupersetDashboardModal = ({ closeModal }) => {
}}
>
<SupersetDashboardFields
isSupersetEmbedIdValid={isSupersetEmbedIdValid}
isSupersetEmbedIdFieldTouched={
isSupersetEmbedIdFieldTouched
shouldShowSupersetEmbedIdError={
shouldShowSupersetEmbedIdError
}
values={values}
onChange={onChange}
Expand Down
27 changes: 20 additions & 7 deletions src/modules/__tests__/useSupersetDashboardFieldsState.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('superset embedded fields state reducer', () => {
initialValues: defaultInitialValues,
values: defaultInitialValues,
isSupersetEmbedIdValid: false,
isSupersetEmbedIdFieldTouched: false,
shouldShowSupersetEmbedIdError: false,
hasFieldChanges: false,
})
})
Expand All @@ -57,11 +57,11 @@ describe('superset embedded fields state reducer', () => {
initialValues: initialValues,
values: initialValues,
isSupersetEmbedIdValid: false,
isSupersetEmbedIdFieldTouched: false,
shouldShowSupersetEmbedIdError: false,
hasFieldChanges: false,
})
})
it('the initial state will have report a valid superset embed ID if a valid UUID was provided', () => {
it('the initial state reports a valid superset embed ID if a valid UUID was provided', () => {
const initialValues = {
...defaultInitialValues,
supersetEmbedId: UUID_V4,
Expand All @@ -70,7 +70,20 @@ describe('superset embedded fields state reducer', () => {
initialValues: initialValues,
values: initialValues,
isSupersetEmbedIdValid: true,
isSupersetEmbedIdFieldTouched: false,
shouldShowSupersetEmbedIdError: false,
hasFieldChanges: false,
})
})
it('the intial state reports that the superset embed ID error should be displayed if invalid UUID is provided', () => {
const initialValues = {
...defaultInitialValues,
supersetEmbedId: 'a-bad-value',
}
expect(createInitialState(initialValues)).toEqual({
initialValues: initialValues,
values: initialValues,
isSupersetEmbedIdValid: false,
shouldShowSupersetEmbedIdError: true,
hasFieldChanges: false,
})
})
Expand Down Expand Up @@ -149,7 +162,7 @@ describe('superset embedded fields state reducer', () => {
const initialState = createInitialState()
expect(
reducer(initialState, { type: SUPERSET_FIELD_BLUR })
).toEqual({ ...initialState, isSupersetEmbedIdFieldTouched: true })
).toEqual({ ...initialState, shouldShowSupersetEmbedIdError: true })
})
})
describe('state updates due to reset field state action', () => {
Expand All @@ -167,7 +180,7 @@ describe('superset embedded fields state reducer', () => {
})
expect(state.values.title).toBe(INITIAL_TITLE)
expect(state.values.supersetEmbedId).toBe(UUID_V1)
expect(state.isSupersetEmbedIdFieldTouched).toBe(true)
expect(state.shouldShowSupersetEmbedIdError).toBe(false)
expect(state.isSupersetEmbedIdValid).toBe(true)
expect(state.hasFieldChanges).toBe(true)

Expand All @@ -188,7 +201,7 @@ describe('superset embedded fields state reducer', () => {
expect(state.initialValues).toEqual(newValues)
expect(state.values).toEqual(newValues)
expect(state.isSupersetEmbedIdValid).toBe(false)
expect(state.isSupersetEmbedIdFieldTouched).toBe(false)
expect(state.shouldShowSupersetEmbedIdError).toBe(true)
// Note that this is by design, the initalValues are also reset
expect(state.hasFieldChanges).toBe(false)
})
Expand Down
36 changes: 23 additions & 13 deletions src/modules/useSupersetDashboardFieldsState.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,18 @@ export const RESET_FIELD_STATE = 'RESET_FIELD_STATE'
const UUID_PATTERN =
/^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i
export const isValidUuid = (string) => UUID_PATTERN.test(string)
export const createInitialState = (initialValues = defaultInitialValues) => ({
initialValues,
values: initialValues,
isSupersetEmbedIdValid: isValidUuid(initialValues.supersetEmbedId),
isSupersetEmbedIdFieldTouched: false,
hasFieldChanges: false,
})
export const createInitialState = (initialValues = defaultInitialValues) => {
const isSupersetEmbedIdValid = isValidUuid(initialValues.supersetEmbedId)

return {
initialValues,
values: initialValues,
isSupersetEmbedIdValid,
shouldShowSupersetEmbedIdError:
!!initialValues.supersetEmbedId && !isSupersetEmbedIdValid,
hasFieldChanges: false,
}
}

export const reducer = (state, { type, payload }) => {
switch (type) {
Expand All @@ -40,14 +45,19 @@ export const reducer = (state, { type, payload }) => {
? payload.checked
: payload.value,
}
const isSupersetEmbedIdFieldChange =
payload.name === FIELD_NAME_SUPERSET_EMBED_ID
const newIsSuperSetEmbedIdValid = isSupersetEmbedIdFieldChange
? isValidUuid(payload.value)
: state.isSupersetEmbedIdValid

return {
...state,
values,
isSupersetEmbedIdValid:
payload.name === FIELD_NAME_SUPERSET_EMBED_ID
? isValidUuid(payload.value)
: state.isSupersetEmbedIdValid,
isSupersetEmbedIdValid: newIsSuperSetEmbedIdValid,
shouldShowSupersetEmbedIdError:
// Show error whenever a valid UUID is changed to an invalid one
state.isSupersetEmbedIdValid && !newIsSuperSetEmbedIdValid,
hasFieldChanges: Object.entries(values).some(
([key, value]) => value !== state.initialValues[key]
),
Expand All @@ -56,7 +66,7 @@ export const reducer = (state, { type, payload }) => {
case SUPERSET_FIELD_BLUR:
return {
...state,
isSupersetEmbedIdFieldTouched: true,
shouldShowSupersetEmbedIdError: !state.isSupersetEmbedIdValid,
}
case RESET_FIELD_STATE:
return createInitialState(payload)
Expand Down Expand Up @@ -85,7 +95,7 @@ export const useSupersetDashboardFieldsState = (
return {
...state,
onChange,
onSupersetEmbedIdFieldBlur: state.isSupersetEmbedIdFieldTouched
onSupersetEmbedIdFieldBlur: state.shouldShowSupersetEmbedIdError
? undefined
: onSupersetEmbedIdFieldBlur,
resetFieldsStateWithNewValues,
Expand Down

0 comments on commit 4946b6a

Please sign in to comment.