adopted-ember-addons / ember-changeset-validations

Validations for ember-changeset
http://bit.ly/ember-changeset-demo
Other
218 stars 100 forks source link

"validatePresence on" not updating #193

Open Redsandro opened 5 years ago

Redsandro commented 5 years ago

validatePresence({ presence: true, on: 'email' })

Version

2.1.0

This is probably me misunderstanding the behavior of the validator.

Steps to reproduce

export default {
    email: [
        validatePresence(true),
        validateFormat({ type: 'email' }),
    ],
    password: [
        validatePresence({ presence: true, on: 'email' }),
    ],
}

Expected Behavior

Empty password validates fine, but

  1. turns invalid once an email is entered, or
  2. turns invalid after focused after email is entered.

Actual Behavior

Empty password stays valid, focused or not. Once you type in a password and then remove it again then the validator fails (as expected).

snewcomer commented 5 years ago

From my pov, 1 && 2 are features that some/many users wouldn't want since eagerly displaying error messages would be optically annoying. What do you think? If you really wanted this behaviour, we could try to figure out something, but would have to be implemented on your end. Lmk your thoughts!

Redsandro commented 5 years ago

@snewcomer I'm experimenting a bit as I've run into limitations with cp-validations and am looking to replace it. I don't currently need this, and I agree with the optical annoyance in this scenario. However, doesn't this mean that the on: functionality is broken nonetheless?

Perhaps a better example would be an address with streetname, apt number, and postal code. The address is optional, but if the user enters the streetname e.g. for future deliveries, you want apt number and postal code to no longer allow empty.

However, if either apt number or postal code had been previously (accidentally) focused, they will stay validated and accepted as empty. They will stay green, and only on form submit will those fields (provided they are given the allowEmpty=false treatment) popup a tool tip saying "This field cannot be empty" (while still being marked as accepted in the template). That's optically confusing.

Now I can hide the apt number and postal code until streetname is no longer empty, but it would be a workaround because the on: is not really doing what I would expect.

I am open to the possibility that I am misunderstanding the use cases of 'on:'. :relaxed:

allthesignals commented 4 years ago

I don't see on documented anywhere 🤔 https://offirgolan.github.io/ember-validators/docs/classes/Presence.html

I'd like to validate presence conditionally.

Redsandro commented 4 years ago

@allthesignals Not sure what is the latest trend, but this issue was based on version 2.1.0. The expected validatePresence behavior was documented in this https://github.com/poteto/ember-changeset-validations/pull/191 based on issue https://github.com/poteto/ember-changeset-validations/issues/189.

snewcomer commented 4 years ago

@Redsandro In response to this issue, if you had a minimal reproduction that I could take a look at, I think we could close this issue once and for all 👍

Redsandro commented 4 years ago

@snewcomer unfortunately I'm not using this currently.

The issue was that on wasn't working as advertised for validatePrecense. It was more than a year ago. I'm not sure the feature is officially supported anymore.

We can close this, although @allthesignals seems to be looking for exactly this feature. Perhaps these days there's a different approach to achieving conditional precense.

allthesignals commented 4 years ago

I've been working on something that could achieve this, but I'm having trouble with the re-validation issue you're seeing.

My use case is conditional presence validation but the condition is more specific: it should only validate presence when the value of the on field equals something specific.

This has been a matter of updating these lines to something like this, and it works okay:

   const hasTargetValues = targets.some((target) => changes[target] || (changes[target] === undefined && content[target]));
    const hasMatchingWith = options.withValue && targets.some((target) => (changes[target] || content[target]) === options.withValue);

    if ((targets && !hasTargetValues) || !hasMatchingWith) {
      return true;
    }

I forked the original twiddle, updated to the latest ember/addon: https://ember-twiddle.com/40a98999c925a2c81ffe87882585de0f?openFiles=validations.user.js%2C

You can see that it's necessary to focus into the conditionally validated field to get it to validate. With input fields, this isn't as much of a problem for the user because they can just click it. It will be confusing but not a disaster.

However, if you're showing/hiding the conditional validated field, it's a problem because there's no way to re-validate everything.

@snewcomer @Redsandro I'm curious if you can think of an approach to get around this problem? If we can solve it as-is with the addon, that would also help with the extended behavior I'm working towards.

Is there some way to get it to re-validate anything specified in on? I don't think validators will have access to that sort of API...

EDIT: Here's a nicer implementation of the "validatePresenceIf" behavior which actually builds on top of the current validatePresence :)