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

Jump to bottom

fix: prevent posting invalid values #1069

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: prevent posting invalid values #1069

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 26 additions & 1 deletion src/settingsFields.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,30 @@ class SettingsFields extends React.Component {
}
}

onUpdateField = (key, value) => {
// `true` if valid, error message string if invalid
const isValid = this.formRef.validateField(
this.formRef.getStateClone(),
key,
value
)

if (isValid === true) {
// Only POST if valid
settingsActions.saveKey(key, value)
} else {
// Update client-side state to invalid value
// to prevent the error UI from persisting when
// changing an invalid value back to the original value
settingsStore.state[key] = value
settingsStore.setState(settingsStore.state)
}
Comment on lines +170 to +176
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a bug after adding the first if clause.

  1. When the user changes the initial form value to an invalid value
  2. And then blurs the field
  3. The field will get and error state and show an error message (CORRECT)
  4. When the user focusses the field again
  5. And then changes the value back to the original value
  6. And then blurs the field
  7. The error message would persist

This was because the form state was not updated to the invalid value, so the state value was still the initial value. And as a result, changing the input's value back to initial state value was not triggering the onUpdateField callback.

Setting the state to the invalid value in the else clause makes sense, because the state should reflect the form-values whether valid or not. However, this does come at a price. The following steps will now produce a POST request, while strictly speaking they shouldn't:

  1. User changes initial value to an invalid value and blurs
  2. User changes invalid value to initial value and blurs
  3. The value is now different, so onUpdateField is triggered. It is also valid, so the value is POSTed too.

Given the constraints of the tools being used here, I think this is probably as good as it will get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably good to point out that strictly speaking this is a "known issue", not a "regression". Since previously invalid values were being posted, resetting to an initial value would also trigger a post.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the constraints of the tools being used here, I think this is probably as good as it will get.

agreed

}

setFormRef = (form) => {
this.formRef = form
}

fieldForMapping({ mapping, fieldBase, key, d2 }) {
switch (mapping.type) {
case 'textfield':
Expand Down Expand Up @@ -361,7 +385,8 @@ class SettingsFields extends React.Component {
<Card className={classes.card} key={this.props.category}>
<FormBuilder
fields={fields}
onUpdateField={settingsActions.saveKey}
onUpdateField={this.onUpdateField}
ref={this.setFormRef}
/>
</Card>
)
Expand Down