Skip to content

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

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

Closed
NaturalBornCamper opened this issue Oct 16, 2020 · 10 comments

Comments

@NaturalBornCamper
Copy link

NaturalBornCamper commented Oct 16, 2020

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
Copy link
Member

jpic commented Nov 9, 2020

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

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
Copy link
Author

No worries!

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

@jpic
Copy link
Member

jpic commented Nov 9, 2020

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
Copy link
Member

Guilouf commented Nov 10, 2020

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

@jpic
Copy link
Member

jpic commented Nov 10, 2020

@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
Copy link
Member

Guilouf commented Nov 10, 2020

@jpic Functionality seems to be broken, maybe because of this commit: 0535549 (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
Copy link
Member

jpic commented Nov 11, 2020

@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
Copy link
Member

Guilouf commented Nov 16, 2020

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

The commit in question (0535549) 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
Copy link
Member

jpic commented Nov 18, 2020

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

@jpic
Copy link
Member

jpic commented Nov 24, 2020

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

jpic added a commit that referenced this issue May 3, 2021
jpic added a commit that referenced this issue May 3, 2021
jpic added a commit that referenced this issue Nov 23, 2021
jpic added a commit that referenced this issue Nov 23, 2021
@jpic jpic closed this as completed in bd63086 Nov 23, 2021
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

3 participants