Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(app): Add validation to network auth forms #2559

Merged
merged 3 commits into from
Oct 26, 2018
Merged

refactor(app): Add validation to network auth forms #2559

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4816936
refactor(app): Add validation to required network auth fields
Kadee80 Oct 24, 2018
b70e334
Disable submit on pristine and on errors
Kadee80 Oct 25, 2018
fb807ed
fixup: implement change requests
Kadee80 Oct 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 55 additions & 8 deletions app/src/components/RobotSettings/SelectNetwork/ConnectForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,34 @@ export default class ConnectForm extends React.Component<Props, State> {
})
}

getEapFields = (eapMethod: ?WifiEapOption): Array<WifiAuthField> => {
return get(eapMethod, 'options', []).map(field => ({
...field,
name: `eapConfig.${field.name}`,
}))
}

getValidationSchema = (values: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

values should be typed as {[name: string]: string} as in onSubmit on line 45. Should probably consolidate it into type ConnectFormValues or something

let fields = []
const errors = {}
if (this.props.securityType === WPA_PSK_SECURITY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block is shared with render and should probably be consolidated into a this.getFields (values: ConnectFormValues) method. Then we may not need this.getEapFields

fields = WIFI_PSK_FIELDS
} else {
const selectedEapMethod = find(this.props.eapOptions, {
name: values.eapConfig.eapType,
})
fields = this.getEapFields(selectedEapMethod)
}
fields.forEach(f => {
Copy link
Contributor

@mcous mcous Oct 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but this could be a reduce:

return fields.reduce((errors, field) => {
  const {name, displayName, required} = field
  const value = get(values, name, '')
  if (required && !value) {
    set(errors, name, `${displayName} is required`)
  } else if (name === 'psk' && value.length < 8) {
    set(errors, name, 'Password must be at least 8 characters')
  }
  return errors
}, {})

if (f.required && !get(values, f.name)) {
set(errors, f.name, `${f.displayName} is required`)
} else if (f.name === 'psk' && get(values, f.name).length < 8) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we pull psk into a const WPA_PSK_FIELD_NAME up top for use here and in WIFI_PSK_FIELDS. Also could you do me a favor and rename WIFI_PSK_FIELDS to WPA_PSK_FIELDS because I'm pretty sure that was a typo on my part?

set(errors, `${f.name}`, 'Password must be at least 8 characters')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: unnecessary template string for f.name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot, i removed it and then reimplemented it. Will revise

}
})
return errors
}

render () {
const {showPassword} = this.state
const {securityType, eapOptions, close} = this.props
Expand All @@ -70,25 +98,33 @@ export default class ConnectForm extends React.Component<Props, State> {
? `${CONNECT_FIELD_ID_PREFIX}${eapMethodField}`
: ''

// TODO(mc, 2018-10-18): form validation
return (
<Formik
onSubmit={this.onSubmit}
validate={this.getValidationSchema}
render={formProps => {
const {handleChange, handleSubmit, values, setValues} = formProps
const {
handleChange,
handleSubmit,
values,
setValues,
handleBlur,
errors,
touched,
} = formProps

// disable submit if form is pristine or errors present
const disabled = isEmpty(touched) || !isEmpty(errors)

const eapMethod = get(values, eapMethodField)
let fields: Array<WifiAuthField> = []

if (securityType === WPA_PSK_SECURITY) {
fields = WIFI_PSK_FIELDS
} else if (securityType === WPA_EAP_SECURITY) {
const selectedEapMethod = find(eapOptions, {name: eapMethod})
fields = get(selectedEapMethod, 'options', []).map(field => ({
...field,
name: `eapConfig.${field.name}`,
}))
fields = this.getEapFields(selectedEapMethod)
}

return (
<form onSubmit={handleSubmit}>
<FormTable>
Expand Down Expand Up @@ -121,13 +157,16 @@ export default class ConnectForm extends React.Component<Props, State> {
showPassword={!!showPassword[field.name]}
onChange={handleChange}
toggleShowPassword={this.toggleShowPassword}
onBlur={handleBlur}
errors={errors}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than pass the whole errors and touched maps down, can we do what we do with value and showPassword and pull the individual value out of the map here?

touched={touched}
/>
))}
</FormTable>
<BottomButtonBar
buttons={[
{children: 'Cancel', onClick: close},
{children: 'Join', type: 'submit'},
{children: 'Join', type: 'submit', disabled},
]}
/>
</form>
Expand All @@ -137,3 +176,11 @@ export default class ConnectForm extends React.Component<Props, State> {
)
}
}

// Helper function to check for empty objects
function isEmpty (obj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use lodash/isEmpty instead of rolling our own

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough!

for (var key in obj) {
if (obj.hasOwnProperty(key)) return false
}
return true
}
22 changes: 19 additions & 3 deletions app/src/components/RobotSettings/SelectNetwork/ConnectFormField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @flow
import * as React from 'react'

import get from 'lodash/get'
import {InputField, CheckboxField, DropdownField} from '@opentrons/components'
import {FormTableRow} from './FormTable'

Expand All @@ -12,27 +12,43 @@ type Props = {
showPassword: boolean,
onChange: (*) => mixed,
toggleShowPassword: (name: string) => mixed,
errors: {
[string]: string,
},
touched: {
[string]: boolean,
},
onBlur: (SyntheticFocusEvent<*>) => mixed,
}

export const CONNECT_FIELD_ID_PREFIX = '__ConnectForm__'

export default function ConnectFormField (props: Props) {
const {value, showPassword, onChange, toggleShowPassword} = props
const {
value,
showPassword,
onChange,
toggleShowPassword,
onBlur,
touched,
errors,
} = props
const {name, displayName, type, required} = props.field
const id = `${CONNECT_FIELD_ID_PREFIX}${name}`
const label = required ? `* ${displayName}:` : `${displayName}:`
let input = null

if (type === 'string' || type === 'password') {
const inputType = type === 'string' || showPassword ? 'text' : 'password'

input = (
<InputField
id={id}
name={name}
type={inputType}
value={value}
onChange={onChange}
error={get(touched, name) && get(errors, name)}
onBlur={onBlur}
/>
)
} else if (type === 'file') {
Expand Down