Skip to content

6725 sorting forum posts issue #6743

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

Merged
merged 3 commits into from
Jun 30, 2025

Conversation

smithellis
Copy link
Contributor

Nulls rise to the top of the sort order with Postgres - this corrects that issue for Questions by changing nulls to zero.

Comment on lines 290 to 301
# Handle sorting by views specially to treat NULL visit counts as 0
if order == "views":
question_qs = question_qs.annotate(
visits_nulls_as_zero=Coalesce(
"questionvisits__visits", Value(0), output_field=IntegerField()
)
)
question_qs = question_qs.order_by(
"visits_nulls_as_zero" if sort == "asc" else "-visits_nulls_as_zero"
)
else:
question_qs = question_qs.order_by(order_by if sort == "asc" else "-%s" % order_by)
Copy link
Contributor

Choose a reason for hiding this comment

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

Django provides a nice way to deal with the NULL sorting issue via the nulls_first and nulls_last keyword arguments to the asc() or desc() methods of F() instances, so you could do something like the following to cover all cases:

if sort == "asc":
    order_by_expression = F(order_by).asc(nulls_first=True)
else:
    order_by_expression = F(order_by).desc(nulls_last=True)
question_qs = question_qs.order_by(order_by_expression)

There are several examples throughout the code that you can use as reference, for example

return threads_.order_by(F("last_post__created").desc(nulls_last=True)).all()
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @escattone - awesome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised.

@smithellis smithellis requested a review from escattone June 27, 2025 01:36
Copy link
Collaborator

@akatsoulas akatsoulas left a comment

Choose a reason for hiding this comment

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

r+, please squash before merging

@escattone escattone merged commit 0343388 into mozilla:main Jun 30, 2025
2 checks passed
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

Successfully merging this pull request may close these issues.

3 participants