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

Sort groups by name in group index view #11162

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

cnk
Copy link
Member

@cnk cnk commented Oct 31, 2023

Fixes #11159

git bisect tells me that the sort on the group index view broke at commit 27aabc1 I would have thought that the new code would use the default_ordering in the IndexView but it clearly does not. Removing it and moving the ordering to the ViewSet restores ordering by name as the default sort order.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?

Please describe additional details for testing this change.

Footnotes

  1. Development Testing

@squash-labs
Copy link

squash-labs bot commented Oct 31, 2023

Manage this branch in Squash

Test this branch here: https://cnkissue-11159-9y2if.squash.io

@cnk cnk requested a review from laymonage October 31, 2023 06:13
@laymonage laymonage added this to the 5.2 milestone Oct 31, 2023
@laymonage laymonage self-assigned this Oct 31, 2023
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Comment on lines 1331 to 1333
# This group should display after the default groups but will display
# before them if default_ordering is lost.
Group.objects.create(name="Photographers")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can move this to the test_default_ordering test directly, as it's not needed for the others

Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although thinking about why this happened in the first place, I'm wondering whether we should've made it so that these attributes fall back to the view's attribute by default instead of having its own default in the ModelViewSet (which is set to None).

This applies to other attributes e.g. list_per_page though, so I'm happy to accept this fix for now.

@laymonage
Copy link
Member

I think this happens for Locale and Site too, so maybe I should rethink. Will get back to this in an hour!

@laymonage
Copy link
Member

It seems we lucked out...

  • Locale defines a Meta.ordering that's the same as the IndexView.default_ordering
    class Meta:
    ordering = [
    "language_code",
    ]
    default_ordering = "language_code"
  • Site calls order_by in the default manager's get_queryset(), not quite the same IndexView.default_ordering because it's case-insensitive, but I think that's OK (and the IndexView's ordering should probably be case-insensitive too anyway):
    class SiteManager(models.Manager):
    def get_queryset(self):
    return super().get_queryset().order_by(Lower("hostname"))
    default_ordering = "hostname"

With how close the release is, I'm inclined to leave them out of this PR. Will see if we can better handle this in 6.0.

@laymonage laymonage merged commit 7cdc496 into wagtail:main Oct 31, 2023
20 checks passed
@gasman
Copy link
Collaborator

gasman commented Oct 31, 2023

Thanks @cnk and @laymonage! For 6.0 I think we should change the behaviour of ModelViewSet so that leaving ordering as None on the viewset causes it to be omitted from the get_index_view_kwargs, and do the same for any other attributes that might reasonably be left out of the viewset. (If we really want to be fancy we could define a custom 'undefined' value to distinguish between a developer leaving it unset on a ModelViewSet subclass, and explicitly setting it to None...)

@laymonage
Copy link
Member

Thanks @gasman I agree, I think omitting it from the view kwargs would be the way to do it.

Cherry-picked to stable/5.2.x in 20fde5a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Group index page lost default ordering in 5.2
3 participants