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

error when using autocomplete_light.ChoiceField #410

Closed
normal-cock opened this issue May 5, 2015 · 13 comments
Closed

error when using autocomplete_light.ChoiceField #410

normal-cock opened this issue May 5, 2015 · 13 comments
Milestone

Comments

@normal-cock
Copy link

My autocomplete is :

class ProfessionAutocomplete(autocomplete_light.AutocompleteListBase):
    choices = []
    def choices_for_request(self):
        self.choices = ['dae', 'abc', 'daa']
        return super(ProfessionAutocomplete, self).choices_for_request()

Form is :

from django import forms
class RegisterForm(forms.Form):
    profession = autocomplete_light.ChoiceField('ProfessionAutocomplete')

View is :

def test(request):
    if request.method == 'POST':
        # create a form instance and populate it with data from the request:
        f = RegisterForm(request.POST)
        # check whether it's valid:
        if f.is_valid():
            return HttpResponseRedirect('/thanks/')
    else:
        f = RegisterForm()
    return render(request, 'admin_crm/register_document_edit.html', {'form':f})

However, when I post the resquest with 'dae' as the profession, 'f.is_valid()' failed saying that 'dae' is not a valid choice. What the matter?

@jpic
Copy link
Member

jpic commented May 11, 2015

What versions ? (Django, Python, OS, Browser)

Maybe Django is passing unicode and it can't compare with items which are in your str list.

@normal-cock
Copy link
Author

My django version is 1.5.2, python 2.7.5, mac os, firefox browser.
I think it has nothing to do with compare between items in my str list and django unicode, because when I stepped into the error , I found that the choice list of autocomplete_light.ChoiceField, which was actually empty, didn't change when the ProfessionAutocomplete instance's choices field changed due to override of choices_for_request method. So the error seems to be reasonable.
I think this is a bug. What do you think?

@jpic
Copy link
Member

jpic commented May 12, 2015

I think that the problem is that the way you do would require overriding choices_for_values(). So choices_for_values will use choices=[].

Maybe it would be easier for you to make a choices dynamic property with @property decorator instead of setting choices=[]?

@normal-cock
Copy link
Author

I think you haven't got my point.
The choices variable of AutocompleteListBase has been dynamic already. However, the choices variable of ChoiceField, which is assigned at the __init__ period of ChoiceField, is still static. That's why 'dae' is not a valid choice show up.

@jpic
Copy link
Member

jpic commented May 13, 2015

On Wed, May 13, 2015 at 4:44 AM, zhiyajun11 notifications@github.com
wrote:

The choices variable of AutocompleteListBase have been dynamic already.
However, the choices variable of ChoiceField, which is assigned at the
init period of ChoiceField, is still static. That's why 'dae' is not
a valid choice
show up.

I'm sorry, but choices_for_request() is for the autocomplete view to render
a list of choices to the user in JS. validate_values() is for the
autocomplete widget to validate choices, so it should be the root cause of
the validation problem.

http://django-autocomplete-light.readthedocs.org/en/stable-2.x.x/api.html#autocomplete_light.autocomplete.base.AutocompleteInterface.validate_values
http://django-autocomplete-light.readthedocs.org/en/stable-2.x.x/api.html#autocomplete-class-api

As I said, that:

class ProfessionAutocomplete(autocomplete_light.AutocompleteListBase):
    choices = []
    def choices_for_request(self):
        self.choices = ['dae', 'abc', 'daa']
        return super(ProfessionAutocomplete, self).choices_for_request()

Is wrong, because obviously choices is set only in choices_for_value() but then it's empty in validate_values() the next time the autocomplete classe is instanciated. It's a simple interface:

http://django-autocomplete-light.readthedocs.org/en/stable-2.x.x/api.html#autocompleteinterface

If you want to continue breaking the pattern it was designed for, override choices in validation too, and that's validate_values().

Else, maybe set self.choices in constructor or make it a dynamic property.

http://yourlabs.org http://blog.yourlabs.org
Customer is king - Le client est roi - El cliente es rey.

@jpic
Copy link
Member

jpic commented May 13, 2015

The choices variable of AutocompleteListBase has been dynamic already.

That's not how it works. You can't set some instance variable in one request (ajax GET request), and then expect it to still be there in another request (form POST submit). Hopefully Django and autocomplete-light are both thread safe.

@normal-cock
Copy link
Author

def validate(self, value):
    """
    Wrap around Autocomplete.validate_values().
    """
    super(FieldBase, self).validate(value)

    # FIXME: we might actually want to change the Autocomplete API to
    # support python values instead of raw values, that would probably be
    # more performant.
    values = self.prepare_value(value)

    if value and not self.autocomplete(values=values).validate_values():
        raise forms.ValidationError('%s cannot validate %s' % (
            self.autocomplete.__name__, value))

The above code, which is the method of FieldBase of autocomplete-light, shows that it failed at super(FieldBase, self).validate(value) , in which is the validation of django's ChoiceField, before validate_values(). That's my problem and what I mean :).
Maybe I should define my own field to meet my needs.

@jpic
Copy link
Member

jpic commented May 14, 2015 via email

@jpic
Copy link
Member

jpic commented May 15, 2015

I think that fix works, but I can't release it without making a proper security audit.

@jpic jpic modified the milestone: 2.2.0 May 15, 2015
@normal-cock
Copy link
Author

It would probably have been better in the beginning to pass the field
instance to the autocomplete instance in that code and let
Autocomplete.validate_values() call field.validate(value).​ It's a bit
a harsh BC break but might be a good idea for v3.

That's will be fine. But it's not an easy job based on current structure. Hoping for v3 and autocomplete-light is really a nice project. Good luck.

jpic added a commit that referenced this issue May 17, 2015
@jpic
Copy link
Member

jpic commented May 17, 2015

Closing in favor of PR #416 which should be in the next release.

Thanks again for your feedback !

@jpic jpic closed this as completed May 17, 2015
jpic added a commit that referenced this issue May 23, 2015
Fix #410: Removed double validation
@jpic
Copy link
Member

jpic commented May 26, 2015

Could you try with 2.2.0rc1 which was released yesterday ?

Thanks.

@normal-cock
Copy link
Author

The backend seems to go well. But there is a js error in the front-end, which result in the disappearing of prompt box. The error is:

ReferenceError: isOpera is not defined
autocomplete.js line 82

My browser is firefox.

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

2 participants