Code

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#7582 closed (fixed)

Sorting by nullable ForeignKey

Reported by: alerades@… Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords:
Cc: joel@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In the admin site (using latest newforms-admin), it is not possible to sort a table by a ForeignKey with null=True.
See this little example:

class Group(models.Model):
    name = models.CharField(max_length=200, primary_key=True)

class Person(models.Model):
    name = models.CharField(max_length=200, primary_key=True)
    group = models.ForeignKey(Group, blank=True, null=True)

You cannot sort Person by group here.
See also http://groups.google.com/group/django-users/browse_thread/thread/f1f0e50a731834d2/3a44be2ab21e778b

Patch By Karen Tracey <kmtracey@…>

Attachments (1)

nullorder.diff (1.4 KB) - added by Alex Rades 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Alex Rades

comment:1 Changed 6 years ago by jpwatts

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

If I understand correctly, the issue here is that django.contrib.admin isn't making the column re-sortable when a nullable ForeignKey appears in the list_display of a ModelAdmin class.

I've tested the patch and it fixes the above issue by removing the code that explicitly prevents the behavior. It looks to me like that code shouldn't have survived queryset-refactor anyway.

I did notice that the patch as it stands doesn't apply to the top-level directory, so I'll be attaching a version that does, but is otherwise unmodified.

comment:2 Changed 6 years ago by Karen Tracey <kmtracey@…>

Yes, the issue is whether the Admin puts a link in the column header that allows you to sort by the associated field. As mentioned in the list discussion, the restriction has been there since initial import of the code into the current repository. I assume the Admin didn't allow it because the ORM code didn't used to support it. Now it does, I think the Admin restriction can be removed as well. Also mentioned at the tail end of that list discussion is that different DBs may put null values in different places (either first or last), so behavior may not be entirely consistent from DB to DB, but I don't see that as a showstopper for allowing sorting by these fields.

comment:3 Changed 6 years ago by Alex

  • milestone changed from 1.0 alpha to 1.0

This is neither backwards incompatible, nor a feature regression, therefore not needed for the nfa merge(and therefore not 1a).

comment:4 Changed 6 years ago by jpwatts

Nevermind on the new patch; I think the problem was on my end (using git apply instead of patch).

Also, there doesn't seem to be anything in the admin docs indicating that you can't re-sort on nullable ForeignKey fields, so it doesn't look like any changes are required there. In fact, I would argue that if the behavior isn't going to be allowed, that should be documented.

comment:5 Changed 6 years ago by jpwatts

  • Cc joel@… added

comment:6 Changed 6 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0
  • Version changed from newforms-admin to SVN

This is sort of a minor feature request, and as such could easily be bundled with a 1.x release later on. Plus, the last patch is almost two months old, which means it almost certainly won't work on current trunk.

comment:7 Changed 6 years ago by kmtracey

Myself I'd call it removing remnants of an old bug (prohibiting something that wasn't handled correctly before queryset-refactor), not an enhancement. As jpwatts mentions, there's no documentation explaining why these fields are not made sortable in the admin. Thus the current behavior is confusing to the user, since there is no obvious reason for these fields to be treated differently than all the others that are automatically sortable. (Which is why it came up on the user's list in the first place.)

So I'd argue it should be fixed for 1.0, or documented. Fixing it is easier: there's the patch, which does in fact still apply correctly as of [8472].

comment:8 Changed 6 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

(In [9080]) Fixed #7582: Removed checks that prevented null=True ForeignKey fields from being sortable in Admin. Post queryset-refactor there seems no reason to disallow this.

comment:9 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.