Opened 6 years ago

Closed 3 years ago

#29010 closed Bug (fixed)

Allow customizing the autocomplete search results based on the querying model

Reported by: Muslu Y. Owned by: Johannes Maron
Component: contrib.admin Version: 2.0
Severity: Normal Keywords: ForeignKey, get_search_results, search_fields
Cc: Johannes Maron, Étienne, Gordon Wrigley, Andy Grabow Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have A,B,C models. A and B models are using C model's ForeignKey for autocomplete.

How can i find which model is querying?

Change History (32)

comment:1 by Muslu Y., 6 years ago

Summary: Which models is using search_fields to get_search_resultsWhich models is querying search_fields to autocomplete?

comment:3 by Muslu Y., 6 years ago

I think Autocomplete is not enough right now.

models.py

class A(models.Model):
    a_name = models.CharField(max_length=100)

class B(models.Model):
    b_name   =   models.ManyToManyField(A)
    
class C(models.Model):
    c_name   =   models.ManyToManyField(A)

admin.py

class AAdmin(admin.ModelAdmin):
    search_fields       =   ['a_name']

    def get_search_results(self, request, queryset, search_term):
        queryset, use_distinct  =   super().get_search_results(request, queryset, search_term)
        
        ## how can i learn which models (B or C) querying for a_name?
        ## because i want to separately filter for every model


class BAdmin(admin.ModelAdmin):
    autocomplete_fields    =   ['b_name']
    
class CAdmin(admin.ModelAdmin):
    autocomplete_fields    =   ['c_name']

comment:4 by Tim Graham, 6 years ago

Cc: Johannes Maron added
Component: Database layer (models, ORM)contrib.admin
Summary: Which models is querying search_fields to autocomplete?Allow customizing the autocomplete search results based on the querying model

I see. I guess it might be possible to add a hint of the model where the query is coming from in a GET parameter. That would be untrusted though.

comment:5 by Tim Graham, 6 years ago

Triage Stage: UnreviewedAccepted

comment:6 by Johannes Maron, 6 years ago

Triage Stage: AcceptedSomeday/Maybe

Hi there,

currently you don't explicitly know who is calling this method, since it's all called from the same view. Both Widgets call the same view as the view is on AAdmin.
So this is conceptually not supported. That doesn't mean it doesn't work. You can always check https://en.wikipedia.org/wiki/HTTP_referer

To add a bit more context here, we did have an intermediate solution that would have made this easier. A view per widget. We ultimately dropped that approach to decrease complexity. A decision I still support.

My suggestion would be use an external library like django-select2 or django-autocomplete if you want to implement more sophisticated logic.
Please also keep in mind, the admin is not recommend to be used for sofistikated user interfaces.

Anyhow, at the moment I would strongly advice against adding a "hint" to the request. Since it's not safe against request forging and can lead to unintended security issues.

in reply to:  6 ; comment:7 by David W. Lloyd, 6 years ago

Replying to Johannes Hoppe:

To add a bit more context here, we did have an intermediate solution that would have made this easier. A view per widget. We ultimately dropped that approach to decrease complexity. A decision I still support.

Was there no in-between option? This severely limits the usefulness of the widget... as a separate-but-related issue:

  • the new autocomplete widget ignores any filtering done in the "limit_choices_to" ORM definition, unlike a regular select field; not very DRY, since limit_choices_to can, with other widgets, be used to filter lookups effectively... inconsistent, unintuitive behavior. If you're not going to fix it, perhaps update the autocomplete documentation to mention that limit_choices_to is completely ignored...
  • the new autocomplete widget ties its ordering of results to the ModelAdmin in question, which is also unintuitive if your ModelAdmin has a default ordering other than alphabetical... for example, if you want the ModelAdmin to default to showing the newest entries, your autocomplete results will ALSO be ordered thus, but such sorting is extremely confusing and unlikely to be helpful in a type-ahead scenario

Your proposal to use django-select2 or DAL almost makes me wonder: why add autocomplete to the admin at all, then? It's 2018, this is a standard design pattern for administrative backends, and to omit the ability to filter based on referring entity, to ignore limit_choices_to, and to tightly couple autocomplete result sorting to default ModelAdmin result sorting seems highly counterintuitive in a pretty common set of use cases.

My suggestion would be use an external library like django-select2 or django-autocomplete if you want to implement more sophisticated logic.
Please also keep in mind, the admin is not recommend to be used for sofistikated user interfaces.

Based on the packages out there targeting the admin & the extensive articles on customizing it, that recommendation just seems optimistic. Django's built-in admin is hailed as one of its selling points, and the fact that it HAS a strong built-in solution has probably discouraged the creation of third-party standalone packages targeting scaffolding & building admin backends - why reinvent the wheel? This recommendation seems to ignore reality - many folks are using the admin to build complex interfaces, the autocomplete in 2.0 *can* really help us all out, but it's a little half-baked at the moment. Baking it some more, to allow for filtering based on relation and decoupled sorting, seems like a high-value enhancement...

in reply to:  7 ; comment:8 by Johannes Maron, 6 years ago

Replying to David W. Lloyd:

Replying to Johannes Hoppe:

To add a bit more context here, we did have an intermediate solution that would have made this easier. A view per widget. We ultimately dropped that approach to decrease complexity. A decision I still support.

Was there no in-between option? This severely limits the usefulness of the widget... as a separate-but-related issue:

I agree, but this feature took ten years to be implemented. We decreased scope to decrease complexity. Now that it's out we can see what additional features get requested and chose to work on those.

  • the new autocomplete widget ignores any filtering done in the "limit_choices_to" ORM definition, unlike a regular select field; not very DRY, since limit_choices_to can, with other widgets, be used to filter lookups effectively... inconsistent, unintuitive behavior. If you're not going to fix it, perhaps update the autocomplete documentation to mention that limit_choices_to is completely ignored...

I agree this is a problem. I would much welcome a fix here to. It seems we all missed in the reviews.

  • the new autocomplete widget ties its ordering of results to the ModelAdmin in question, which is also unintuitive if your ModelAdmin has a default ordering other than alphabetical... for example, if you want the ModelAdmin to default to showing the newest entries, your autocomplete results will ALSO be ordered thus, but such sorting is extremely confusing and unlikely to be helpful in a type-ahead scenario

The sorting is only there for pagination purposes but it can be used to improve results. I use the widgets on full text indexes too, which go way beyond a simple typeahead.

Your proposal to use django-select2 or DAL almost makes me wonder: why add autocomplete to the admin at all, then? It's 2018, this is a standard design pattern for administrative backends, and to omit the ability to filter based on referring entity, to ignore limit_choices_to, and to tightly couple autocomplete result sorting to default ModelAdmin result sorting seems highly counterintuitive in a pretty common set of use cases.

Go ahead an decouple it. That's an easy one. You add a new method that by defaults calls the current soring method. Please open a separate issue for that one. I would be happy to review this feature.

My suggestion would be use an external library like django-select2 or django-autocomplete if you want to implement more sophisticated logic.
Please also keep in mind, the admin is not recommend to be used for sofistikated user interfaces.

Based on the packages out there targeting the admin & the extensive articles on customizing it, that recommendation just seems optimistic. Django's built-in admin is hailed as one of its selling points, and the fact that it HAS a strong built-in solution has probably discouraged the creation of third-party standalone packages targeting scaffolding & building admin backends - why reinvent the wheel? This recommendation seems to ignore reality - many folks are using the admin to build complex interfaces, the autocomplete in 2.0 *can* really help us all out, but it's a little half-baked at the moment. Baking it some more, to allow for filtering based on relation and decoupled sorting, seems like a high-value enhancement...

Please not that these 3rd party librarie have been around a lot longer than the new admin feature – no wheel reinvention happening. Furthermore they address a lot more use cases they are meant for user interaction (none-admins).
It's a pretty big hypophysis that "many folks are using the admin to build complex interfaces", even though this is discouraged. In general I would kindly ask you to watch your tone. This feature is a result of a lot of hard work by "many folks". Phrases like "half-baked" may hurt people's feelings.

You have to chose here, you either: Love it, change it or leave it.

comment:9 by Carlton Gibson, 6 years ago

Hi. Just seconding Joe's comment about tone.

Can we please make sure we breathe before posting and are respectful of each other and the thought, time and energy that goes into work on Django from all the volunteers.

Thank you.

in reply to:  8 comment:10 by David W. Lloyd, 6 years ago

Replying to Johannes Hoppe:

Replying to Johannes Hoppe:

  • the new autocomplete widget ignores any filtering done in the "limit_choices_to" ORM definition, unlike a regular select field; not very DRY, since limit_choices_to can, with other widgets, be used to filter lookups effectively... inconsistent, unintuitive behavior. If you're not going to fix it, perhaps update the autocomplete documentation to mention that limit_choices_to is completely ignored...

I agree this is a problem. I would much welcome a fix here to. It seems we all missed in the reviews.

I think the problem is that any fix would almost have to do what the feature request here is proposing - if the view doesn't know where the relation is coming from, it can't consult the ORM field and get the correct limit_choices_to filter to apply. If the referring model were passed in, one way or another, this filtering could be accomplished. Rather than doing a view per widget, having the ModelAdmin provide its underlying model to any autocomplete view being referenced seems like it might do the trick?

Go ahead an decouple it. That's an easy one. You add a new method that by defaults calls the current soring method.
Please open a separate issue for that one. I would be happy to review this feature.

Will do, thanks!

It's a pretty big hypophysis that "many folks are using the admin to build complex interfaces", even though this is discouraged. In general I would kindly ask you to watch your tone. This feature is a result of a lot of hard work by "many folks". Phrases like "half-baked" may hurt people's feelings.

You have to chose here, you either: Love it, change it or leave it.

Understood; I'm new here and this wasn't the right tone, I apologize. I do love 95% of it and I hope I can help change the other five.

Version 0, edited 6 years ago by David W. Lloyd (next)

comment:11 by Carlton Gibson, 6 years ago

Is this really a Someday/Maybe, rather than a wontfix as it stands?
No doubt we evolve the capabilities here but this doesn't look at all actionable with the current design.

As per comment on #29700, I'd (currently/initially) favour pointing users to subclassing AutocompleteJsonView and seeing what they come up with.
I suspect good ideas would come out of there...

comment:12 by Johannes Maron, 6 years ago

Is there a ticket now for the limit_choices_to issue yet? I'd really like to fix that one.

in reply to:  12 comment:13 by David W. Lloyd, 6 years ago

Replying to Johannes Hoppe:

Is there a ticket now for the limit_choices_to issue yet? I'd really like to fix that one.

I can create a ticket, but isn't it the same problem as this ticket? Once the AutocompleteView knows which model to get "limit_choices_to" from, the original issue being presented here would *also* be addressed at the same time - unless I'm missing something?

I'll gladly create a separate ticket for limit_choices_to if you prefer, I just thought that the solution to that problem would end up fixing this issue as well...

comment:14 by Johannes Maron, 6 years ago

Well not necessarily. I did have a go at this yesterday evening. limit_choices_to could be fixed, without a larger refactoring.
I would use a couple of strange API's though. I am not yet happy with my solution.

in reply to:  14 comment:15 by David W. Lloyd, 6 years ago

Replying to Johannes Hoppe:

Well not necessarily. I did have a go at this yesterday evening. limit_choices_to could be fixed, without a larger refactoring.
I would use a couple of strange API's though. I am not yet happy with my solution.

I'm curious how limit_choices_to can be accessed without also then knowing the referring model... so I'll log the ticket and link this one :)

comment:16 by Johannes Maron, 5 years ago

Owner: changed from nobody to Johannes Maron
Status: newassigned

comment:17 by Johannes Maron, 5 years ago

Triage Stage: Someday/MaybeAccepted
Type: New featureBug

comment:18 by Johannes Maron, 5 years ago

Has patch: set

comment:19 by Florian Apolloner, 5 years ago

Patch needs improvement: set

comment:20 by Johannes Maron, 4 years ago

Patch needs improvement: unset

comment:21 by Étienne, 4 years ago

Cc: Étienne added

comment:22 by Andy Grabow, 3 years ago

I have tested that feature branch - works like a charm!

To me the added tests also look good, there is just one thin missing:
admin_views/models.py:646
There is a prepared field for ManyToMany (related_answers) tests, but the matching test case is missing.

With that test case checked in, this feature could finally go live. :)

comment:23 by Carlton Gibson, 3 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

The patch is looking good. I've raised a possible adjustment on the ticket, as Andy says it looks like there a test missing for the M2M case, and it'll docs.

Joe: if you don't have capacity for this now, say, and either I or someone else can pick it up to finish.
Good work!

comment:24 by Gordon Wrigley, 3 years ago

Cc: Gordon Wrigley added

comment:25 by Johannes Maron, 3 years ago

Hi Clarton, I have this wedding on Friday, in which I have an important part to play ;) I'll get to it next week, I hope. As always, I don't mind help with the docs until then. Best Joe

comment:26 by Carlton Gibson, 3 years ago

OK, thanks for the update. No problem Joe. This is a good win, and it doesn’t need much now, so if you have some time over the next few weeks, that’s plenty. Enjoy the wedding! 😀

comment:27 by Gordon Wrigley, 3 years ago

It would be really nice if it were easy to inject additional values into the widget at form construction time and then filter on them in get_search_results.

With the current patch I believe that would require:
1: overriding build_attrs to pass the value along
2: overriding autocomplete.js to pass the value along

Regarding these:
1: is not a particularly big deal, but could be tidied up with something like a get_search_args.
2: this is annoying just because it's outside the python, it would be nice if it was a wild card pass, perhaps the fields could be bundled into a dict or some such.

After than you can pull the values off the request in get_search_results, which is maybe not elegant, but works fine.

In terms or what values to pass along the most useful and the ones on my mind are the model and pk of the admin page the autocomplete is resident on.
The page specifically because when there's dynamic (can add and remove) inlines involved the model and pk of the inlined object is not always useful.
As an example if you consider polls and choices this is required for a "default_choice" field which is only allowed to choose from the polls existing choices.

comment:28 by Andy Grabow, 3 years ago

Cc: Andy Grabow added

comment:29 by Johannes Maron, 3 years ago

Ok Carlton, honeymoon's over. I addressed all review comments.

Gordon, interesting idea, why don't you start a new issue for that, since it seems to be a separate issue.

comment:30 by Johannes Maron, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:31 by Johannes Maron, 3 years ago

Needs documentation: unset

comment:32 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:33 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 3071660a:

Fixed #29010, Fixed #29138 -- Added limit_choices_to and to_field support to autocomplete fields.

  • Fixed #29010 -- Added limit_choices_to support to autocomplete fields.
  • Fixed #29138 -- Allowed autocomplete fields to target a custom to_field rather than the PK.
Note: See TracTickets for help on using tickets.
Back to Top