search

jrief / django-angular

Let AngularJS play well with Django
http://django-angular.awesto.com/
MIT License
1.23k stars 294 forks source link

Don't mark fields as pristine when setErrors is run #260

Closed omarkhan closed 6 years ago

omarkhan commented 8 years ago

Background

I have noticed bugs when using django-angular with a submitted form that fails server-side validation:

This pull request fixes these issues by:

jrief commented 8 years ago

Thanks for your pull-request. Strange that I never encountered this on my testing/demo page, but I have to admit, that I often did not combine server side, with client side validation. In general, I wanted to switch from $pristine to $untouched for form fields, because it gives a better user experience. Please be patient with merging, because I'm busy on another project right now.

omarkhan commented 8 years ago

This does not fix the case when setErrors is called on a submitted form for a dirty field that fails client-side validation. I still get a double error message in this case. I have no idea how to fix this - any thoughts?

omarkhan commented 8 years ago

On a related note, I have noticed that fields that fail server-side validation are not marked as $invalid on the client-side. This can cause weird styling bugs, where a field displays an error message but does not get a red border (or any other styles applied to invalid fields) until the user modifies it.

antoviaque commented 7 years ago

@jrief @jkosir Any chance this and #259 could get a review? We would like to get rid of our forked version we need to maintain for these fixes : ) Thank you!

jkosir commented 7 years ago

I'm not involved much in the form validation part of this project. Quickly went through the PRs, looks good to me, but let's wait for @jrief to comment. Thanks for contributing!

antoviaque commented 7 years ago

@jkosir You're welcome - thanks to you for having a look : )

jrief commented 7 years ago

@omarkhan since I never encountered this problem before myself, please can you retry with all supplied HTML widgets. The demo for testing is part of the django-angular project.

Please supply 1-3 lines of text for the changelog.

antoviaque commented 7 years ago

@jrief Sure - since Omar isn't with us anymore, we'll assign someone else to this to answer you. Thank you!

itsjeyd commented 7 years ago

@jrief I tested locally using the demo, and wasn't able to reproduce the issues mentioned in the description of this PR. At least not directly: I never saw both a tick and a cross on the same field, and error messages were never duplicated. However, when submitting a form with field contents that passed client-side validation but failed server-side validation, I found that the error message was displayed at the top of the form, but the offending field still showed a green checkmark. That behavior seemed slightly confusing to me, but it might be intended for this particular demo? In any case, I got this behavior on all relevant branches: jrief:master, open-craft:master, and open-craft:set-errors.

I also tested our code again using jrief:master and open-craft:master. (Skipped open-craft:set-errors since it doesn't include the commit from #259 that got merged recently.) Here is what I found:

Fields that pass client-side validation but fail server-side have both a tick and a cross: ...

  • Can reproduce:
  • On open-craft:master@5f3e54f (includes changes from this PR): no
  • On open-craft:master@063aad2 (does not include changes from this PR): yes
  • On jrief:master: yes

When setErrors is used to add the error to the field, the error appears twice: ...

  • Can reproduce:
  • On open-craft:master@5f3e54f (includes changes from this PR): no
  • On open-craft:master@063aad2 (does not include changes from this PR): yes
  • On jrief:master: yes

Note: If I'm interpreting his comment correctly, Omar mentioned here that the changes in this PR actually don't fix the second issue. I'm assuming that the reason why it didn't come up when testing our code with open-craft:master@5f3e54f is that we have some custom logic for displaying error messages in our code base.

To sum up, I didn't get the exact same behavior mentioned in the PR description when working with the demo, but the issues still affect our code (when using a version of django-angular that does not include the changes from this PR).

itsjeyd commented 7 years ago

@jrief Have you had a chance to look at my response to your latest comment? Please let me know if there is anything else you want us to test.

I rebased this branch (from 0ee92541bdcbd2858d6f259b89cfa8d1892a390e), and updated the Changelog.

adrienbrunet commented 7 years ago

@jrief Looks pretty good to me. Have you tried it?

jrief commented 7 years ago

ups, sorry, not yet

itsjeyd commented 7 years ago

@jrief Have you had a chance to look at this again?

I just rebased this branch (from 6071d2b2e3e311e4fff9563ff6cab51d1245e0b9) to resolve conflicts that had come up recently. Please let me know if you would want the changes to the Changelog to be added to the "latest (master) - not released yet" section (they are currently part of a separate section, "0.8.5").

Thanks!

jrief commented 7 years ago

@itsjeyd thanks for reminding me. Will try to look at it soon.

itsjeyd commented 7 years ago

Thanks for the update, @jrief!

jrief commented 7 years ago

Today I had some time looking at this pull request. Something which now doesn't work anymore:

The form will not validate, because some fields are missing. These fields are mentioned in errors of the response dict. However they are not rendered.

If you do the same test with version 0.8.4, this feature works fine.

Do you want to investigate on this further?

itsjeyd commented 7 years ago

@jrief Thanks for testing the changes, and apologies for the delay. I just wanted to let you know that we are still planning on following up on your feedback; we just didn't get around to it yet.

itsjeyd commented 7 years ago

@jrief Another update: We are probably going to change the approach to form validation that we're using in our code and may not need the changes from this PR anymore. In any case, this is low priority for us at the moment, so it could take a while until we make a final decision.

adrienbrunet commented 6 years ago

@itsjeyd Some updates should handle your problem (see #314). If you still have this problem, feel free to repopen it !