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.

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

Add ability to save changes to changeset regardless of validation status, and then validate on changeset.execute() #59

Closed
acburdine opened this issue Jul 9, 2016 · 6 comments

Comments

@acburdine
Copy link
Contributor

So after building out all of Ghost's new validation functionality using this, I ran into an issue with this library that forced me to re-write a private method (which is bad practice 😁 ). Essentially, I found out that my changeset didn't contain the most recent changes, because only valid changes were actually persisted to the changeset.

This isn't a bug, because it says in the readme:

The idea behind a changeset is simple: it represents a set of valid changes to be applied onto any Object

Here's the specific use-case for why this broke my refactor: the validation flow in Ghost works by validating when the particular input is de-focused, and then all inputs are validated when the form is submitted. After all changes are validated (using the changeset.validate) method, then the changes are persisted to the underlying model.

The problem arises because if the user de-focuses an input without the content of said input being valid, the actual input value doesn't exist in the changeset, resulting in an incorrect error message displayed (e.g. the please enter a value error message other than the invalid format for value message).

After reading through the Ecto Changeset documentation, it seems that the changes are set on the changeset regardless of validation status (it differentiates "casting" from "validation"). I could be reading this wrong though.

I understand that not everyone would necessarily want to use ember-changeset this way, but I think there could be an ability to do both at the same time, based on how the changeset is created initially (maybe a third parameter passed to the changeset constructor that enables/disables this new method of behavior?)

In terms of code changes, it's pretty minimal.

Thoughts?

/cc @poteto

@poteto
Copy link
Collaborator

poteto commented Jul 9, 2016

The problem arises because if the user de-focuses an input without the content of said input being valid, the actual input value doesn't exist in the changeset, resulting in an incorrect error message displayed (e.g. the please enter a value error message other than the invalid format for value message).

I'm not sure if I'm misunderstanding, but in my mind this doesn't feel like how ember-changeset works - I'm assuming on blur / focus out you run some kind of action to changeset.validate(propertyName). If you then do {{#if changeset.error.propertyName}}, the validation attribute on that object should be the right error message (not that it is blank). For example, if you try out the changeset demo you'll see that it behaves in the way I just described it, less the focus out bit.

In ember-changeset I'm implicitly casting when the value is set, which I think is more robust on the client side. There isn't really any gain to be had from splitting it up.

If I could trouble you to make a simple twiddle to illustrate, that would be very helpful!

Edit: Added the focus out action in the demo. Try focusing on the age or job fields without modifying the value, then hit tab

@dustinfarris
Copy link
Contributor

dustinfarris commented Jul 10, 2016

@acburdine I had a similar issue using ember-changeset with ember-paper where a paper-input component matches the value passed to it exactly—typing in, say, an email field, would result in nothing showing up because on the first keystroke, the input is not yet a valid email address, so the changeset would not update the attribute, and paper-input would thus reset the input value to blank.

I don't think this is a shortcoming of ember-changeset, but rather a quirk of my use-case when trying to integrate with another (very opinionated) library.

I worked around this by creating an intermediary component to handle the value of the input (versus the value of the changeset attribute). This is specific to my situation, but maybe you could modify to work for you.

x-paper-input.js

export default Ember.Component.extend({
  errors: [],

  inputValue: Ember.computed.oneWay('value'),

  actions: {
    updateValue(value) {
      this.set('inputValue', value);
      this.sendAction('onChange', value);
    }
  }
});

x-paper-input.hbs

{{paper-input value=inputValue onChange=(action "updateValue") errors=errors}}

Usage

{{#with (changeset model myValidations) as |changeset|}}
  {{x-paper-input 
      value=changeset.email 
      onChange=(action (mut changeset.email))
      errors=changeset.error.email.validation
  }}
{{/with}}

@acburdine
Copy link
Contributor Author

acburdine commented Jul 10, 2016

@poteto I went and looked at the updated twiddle, and was able to reproduce my error. Apologies for perhaps not explaining it well at first.

To reproduce the error on that twiddle, you must:

  • Have a validatePresence and another type of validation in the field
  • Start off with a blank value
  • Fill out the input with an invalid value
  • tab/focus out

After which the error message will also include <value> cannot be blank as well as the second validation, even though there is still a value in the field. (In the case of that twiddle, I had the user's last name be initially set to a blank string, then typed one letter, then tabbed out.)

Not sure if this is in any way related to #60

@poteto
Copy link
Collaborator

poteto commented Jul 12, 2016

@acburdine I believe this is fixed, try it out again on the demo!

@acburdine
Copy link
Contributor Author

@poteto Sorry for not replying sooner. I tested it out in the demo and it fixed the issue! However, I need to apply the update to my PR in Ghost to verify it there, once that's done I'll go ahead and close this 😄

Thanks for getting it fixed so quickly!

@acburdine
Copy link
Contributor Author

@poteto Finally got around to updating ember-changeset in my PR, this issue has been fixed 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants