yourlabs / django-autocomplete-light

A fresh approach to autocomplete implementations, specially for Django. Status: v4 alpha, v3 stable, v2 & v1 deprecated.
https://django-autocomplete-light.readthedocs.io
MIT License
1.8k stars 467 forks source link

GenericForeignKeyModelField filtering on other field doesn't seem to work? #1187

Closed NaturalBornCamper closed 2 years ago

NaturalBornCamper commented 4 years ago

Using the Autocompletion for GenericForeignKey with django-cities-light to make a form with country select, then region select, then city select chained autocompletes,

I can make all 3 fields show up together, but the moment I use the filtering from other fields, it seems to ignore my parameters and sends an invalid AJAX request that obviously fails:

state_or_county = autocomplete.Select2GenericForeignKeyModelField(
    model_choice=[(Region, 'name')],
)

town_city = autocomplete.Select2GenericForeignKeyModelField(
    model_choice=[(City, 'search_names')],  # Works but no filtering
    # model_choice=[(City, 'search_names', [('state_or_county', 'region_id')])],  # Doesn't work
    # model_choice=[(City, 'search_names', [('state_or_county', 'region')])],  # Doesn't work
    field_id='name'  # As a sidenote, doesn't seem to work either
)

The AJAX request I'm getting when using either ('state_or_county', 'region_id') or ('state_or_county', 'region') is: http://localhost:8000/ProspectForm_139965457246160_autocomp?forward={"state_or_county":"61-64"} Which should be: http://localhost:8000/ProspectForm_139965457246160_autocomp?forward={"region_id":"61-64"} The value is fetched correctly from the first value in the Tuple, but it's also trying to filter the cities with the tuple first value instead of the second tuple value. In fact, whatever I put in the second tuple value seems to be completely ignored.

Furthermore, field_id='name' also seems to be completely ignored and the value shown is always the default table model field (display_name). Not sure if I should report this as another bug.

For reference, the django-cities-light Region and City models go as follows:

class AbstractRegion(Base):
    """
    Base Region/State model.
    """

    display_name = models.CharField(max_length=200)
    geoname_code = models.CharField(max_length=50, null=True, blank=True,
                                    db_index=True)

    country = models.ForeignKey(CITIES_LIGHT_APP_NAME + '.Country',
                                on_delete=models.CASCADE)

    class Meta(Base.Meta):
        unique_together = (('country', 'name'), ('country', 'slug'))
        verbose_name = _('region/state')
        verbose_name_plural = _('regions/states')
        abstract = True

    def get_display_name(self):
        return '%s, %s' % (self.name, self.country.name)
class AbstractCity(Base):
    """
    Base City model.
    """

    display_name = models.CharField(max_length=200)

    search_names = ToSearchTextField(
        max_length=4000,
        db_index=INDEX_SEARCH_NAMES,
        blank=True,
        default='')

    latitude = models.DecimalField(
        max_digits=8,
        decimal_places=5,
        null=True,
        blank=True)

    longitude = models.DecimalField(
        max_digits=8,
        decimal_places=5,
        null=True,
        blank=True)

    subregion = models.ForeignKey(CITIES_LIGHT_APP_NAME + '.SubRegion',
                                  blank=True, null=True,
                                  on_delete=models.CASCADE)
    region = models.ForeignKey(CITIES_LIGHT_APP_NAME + '.Region', blank=True,
                               null=True, on_delete=models.CASCADE)
    country = models.ForeignKey(CITIES_LIGHT_APP_NAME + '.Country',
                                on_delete=models.CASCADE)
    population = models.BigIntegerField(null=True, blank=True, db_index=True)
    feature_code = models.CharField(max_length=10, null=True, blank=True,
                                    db_index=True)
    timezone = models.CharField(max_length=40, blank=True, null=True,
                                db_index=True, validators=[timezone_validator])

    class Meta(Base.Meta):
        unique_together = (('region', 'subregion', 'name'),
                           ('region', 'subregion', 'slug'))
        verbose_name_plural = _('cities')
        abstract = True

    def get_display_name(self):
        if self.region_id:
            return '%s, %s, %s' % (self.name, self.region.name,
                                   self.country.name)
        else:
            return '%s, %s' % (self.name, self.country.name)

    def get_timezone_info(self):
        """Return timezone info for self.timezone.

        If self.timezone has wrong value, it returns timezone info
        for value specified in settings.TIME_ZONE.
        """
        try:
            return pytz.timezone(self.timezone)
        except (pytz.UnknownTimeZoneError, AttributeError):
            return pytz.timezone(settings.TIME_ZONE)
jpic commented 3 years ago

I think the problem is: forwarding generic ids of ctypeid-objid breaks because it was just never implemented :joy:

So, it's OPEN for contribution !

@NaturalBornCamper Thank you for the great writeup ! Maybe you want to give it a shot ? Or have you found another solution ?

NaturalBornCamper commented 3 years ago

No worries!

Unfortunately I have already uninstalled and implemented my own autocomplete, but thanks anyways :)

jpic commented 3 years ago

Maybe you were using Select2QuerySetSequenceView, apparently you need to use Select2QuerySetSequenceAutoView for GFK support.

Not sure if that's clearly documented, from what I see both classes could just be merged so that it would work out of the box next time.

@Guilouf Select2QuerySetSequenceAutoView is yours, is BC the reason you didn't want to change Select2QuerySetSequenceView.get_queryset ? If so, let's go ahead and merge the classes to have a single S2QSS view that works with forward OOTB. Otherwise, there might be a proper objection and I'd love to know more about it !

Guilouf commented 3 years ago

Hello @jpic, it's hard to remember but i will take a look at it this afternoon after my working time

jpic commented 3 years ago

@Guilouf thanks for the fast answer, please let me know if you have any objection to merging both classes, also, I think we can maintain BC with something like Select2QuerySetSequenceAutoView = Select2QuerySetSequenceView

Guilouf commented 3 years ago

@jpic Functionality seems to be broken, maybe because of this commit: https://github.com/yourlabs/django-autocomplete-light/commit/053554937475e1a34a5e48de5bf6d66c90e9de45 (reverting made the test page http://127.0.0.1:8000/admin/select2_generic_foreign_key/tmodel/49/change/ working for me) I really struggle with the tests, i tried tox -e py38-dj30 but it gives me 16 passed for 23 errors..

The first thing to address may be the sensibility of the tests, but i can't find a way to run test selectively, maybe you have some tricks ?

In fact, i think the commit is incompatible with querysetSequence, and may also cause problem to Select2QuerySetSequenceView

jpic commented 3 years ago

@Guilouf tests are all passing for me with tox -r -e py38-dj30 and tox -r -e py38-dj31 with a resulting coverage of 90%, have you tried not touching anything while the browser tests are running ? it can fail if there is any interaction by mistake

I'm a bit confused if the commit in question breaks everything, or if it's just when using the new feature.

Is the new feature breaking your own code ?

Guilouf commented 3 years ago

@jpic Actually it was an issue related to selenium and geckodriver, now tests all passing.

The commit in question (https://github.com/yourlabs/django-autocomplete-light/commit/053554937475e1a34a5e48de5bf6d66c90e9de45) is breaking my code, i think it makes BaseQuerySetView not supporting QuerysetSequence objects, i have this error:

django-autocomplete-light/src/dal/views.py", line 148, in get_search_results
    queryset = queryset.filter(reduced)
TypeError: filter() takes 1 positional argument but 2 were given

I couldn't however figuring out exaclty why it's appening

jpic commented 3 years ago

Got something in #1199 but it depends on a contribution to django-querysetsequence, let's hope they release something soonish.

jpic commented 3 years ago

Would love some reviews, even if it's just to say lgtm if that's really how you feel about it: