jrief / django-angular

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

Bug in as_ul? #270

Closed racitup closed 7 years ago

racitup commented 7 years ago

I was prodding around on the example form page and got a 500 Internal Server Error when it was rendering {{ form.as_div }}. I think I got the same error a day or two ago on my implementation too. Admittedly I was using a REST api test tool at the time, but still...

screen shot 2016-12-01 at 09 58 32

KeyError at /model_scope/ u'i' Request Method: POST Request URL: http://django-angular.awesto.com/model_scope/ Django Version: 1.7.10 Exception Type: KeyError Exception Value:
u'i' Exception Location: /web/staging/projects/django-angular/djng/forms/angular_base.py in as_ul, line 85 Python Executable: /usr/sbin/uwsgi Python Version: 2.7.5 Python Path:
['.', '', '/web/staging/projects/django-angular', '/web/staging/projects/django-websocket-redis', '/web/staging/virtualenvs/djangular/lib64/python27.zip', '/web/staging/virtualenvs/djangular/lib64/python2.7', '/web/staging/virtualenvs/djangular/lib64/python2.7/plat-linux2', '/web/staging/virtualenvs/djangular/lib64/python2.7/lib-tk', '/web/staging/virtualenvs/djangular/lib64/python2.7/lib-old', '/web/staging/virtualenvs/djangular/lib64/python2.7/lib-dynload', '/usr/lib64/python2.7', '/usr/lib/python2.7', '/web/staging/virtualenvs/djangular/lib/python2.7/site-packages'] Server time: Thu, 1 Dec 2016 10:44:22 +0100

adrienbrunet commented 7 years ago

Hi, I tried but I really can't reproduce your bug. Please note that django 1.7 support has been dropped, could you try with a more recent version of django? (currently supporting 1.8, 1.9, 1.10).

If you can provide some easy steps to reproduce this bug, I'd be happy to investigate more.

I'm closing this for now. Feel free to reopen it if you can reproduce this error.

racitup commented 7 years ago

Hi Adrien,

Hmm, I also wasn't able to reproduce exactly as I first posted, I had to add the CSRFToken to the form body.. must have screen-grabbed the wrong thing somehow, sorry!

This is a bug in the live example server implementation for this module available at http://django-angular.awesto.com/model_scope/

I just tried the steps below and got the Internal Server Error:

  1. Get Chrome and the DHC Restlet Client extension (trial version is okay)
  2. Navigate to the URL above and copy the CSRFToken from the browser developer tools -> Application -> Cookies
  3. Open the DHC client and fill in a scenario as below with your CSRFToken
  4. Hit the play button and wait for the 500 Internal Server Error

screen shot 2017-01-13 at 19 07 53

I'm not able to reopen because of:

you cannot re-open your own issues if a repo collaborator closed them.

Sorry I'm not able to test on my implementation at the moment.

adrienbrunet commented 7 years ago

Thanks for your report. I'll have a look!

adrienbrunet commented 7 years ago

Ok, the reason for this error is that you forgot to add the confirmation_key field which is a hidden field. Let's say you add "&confirmation_key=foo" and it works. You can see the hidden field in the form definition.

@jrief The TupleErrorList implementation does not handle correctly the hidden fields. The resulting error list is:

(u'my_form', u'djng-form-errors', u'$pristine', u'$message', u'invalid', u'$message'), u"(Hidden field confirmation_key) (u'my_form.confirmation_key', u'djng-form-control-feedback djng-field-errors', u'$pristine', u'$pristine', u'invalid', u'This field is required.')")

The pain point is here:

u"(Hidden field confirmation_key) (u'my_form.confirmation_key', u'djng-form-control-feedback djng-field-errors', u'$pristine', u'$pristine', u'invalid', u'This field is required.')"

This is unicode string instead of a tuple.

adrienbrunet commented 7 years ago

This comes from django html output function for forms:

     def _html_output(self, normal_row, error_row, row_ender, help_text_html, errors_on_separate_row):
        "Helper function for outputting HTML. Used by as_table(), as_ul(), as_p()."
        top_errors = self.non_field_errors()  # Errors that should be displayed above all fields.
        output, hidden_fields = [], []

        for name, field in self.fields.items():
            html_class_attr = ''
            bf = self[name]
            # Escape and cache in local variable.
            bf_errors = self.error_class([conditional_escape(error) for error in bf.errors])
            if bf.is_hidden:
                if bf_errors:
                    top_errors.extend(
                        [_('(Hidden field %(name)s) %(error)s') % {'name': name, 'error': force_text(e)}
                         for e in bf_errors])
                hidden_fields.append(six.text_type(bf))
                ...
racitup commented 7 years ago

Hi, I just ran into this bug again.. has there been any progress?

adrienbrunet commented 7 years ago

Have you checked all the hidden fields ? I think this was your issue before... We did not improved how it's handled yet. Any PR is, of course, welcome.

racitup commented 7 years ago

Well at the moment I'm getting this error on my own djng form when rendering an empty form in a template. I'm passing in no instance, no data and no initial. But yes my form does have 2 hidden fields. What would be the workaround?

adrienbrunet commented 7 years ago

Would you provide a minimum failing example ? Or a minimum test for us to investigate further what we could achieve ? I already pointed out where the problem stands but a failing test would be a huge help to debug it quicker. I'm lacking some time to do it all by myself.

racitup commented 7 years ago

No worries, I'll try to debug it myself.

racitup commented 7 years ago

As you say it is caused by the django forms _html_output code above extending strings into a list of SafeTuples for any hidden fields. I've fixed it in angular_base.py and will submit a PR shortly.

adrienbrunet commented 7 years ago

Hooray ! That's some good news. Thanks for your contribution @racitup ! We need more people like you !

jrief commented 7 years ago

@racitup thank a lot! Is the PR ready for merging?

adrienbrunet commented 7 years ago

Thanks for your PR @racitup ! It took 8months to correct this bug but we finally did it ! :+1: Great teamwork (mostly yours) hehe

racitup commented 7 years ago

Thanks Adrien and Jacob. Yes I've tested it on my site and seems to work fine. For the record, the bug happened when data (data=something) was passed to the form with required hidden fields from a view trying to render the form as html, yet there was no data for the hidden fields. Thus when validated, the hidden form fields tried to return an error which the django code forced to a string. So I guess only really applicable to an html POST. The fix code was taken and adapted from the django.forms.forms.BaseForm._html_output() above. So it's basically doing the same thing as before, just in a slightly different order and inserting tuples and not strings to the error list.