-
-
Notifications
You must be signed in to change notification settings - Fork 264
Feature/improve api test coverage #2527
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
base: main
Are you sure you want to change the base?
Changes from all commits
10adfe2
87f450a
e6ac712
31c4af8
d2e2ae1
4aa9cb4
394e4e9
81459c6
32d310d
4d91bab
6de85f2
1689530
9c0c191
593395f
c3ced64
951e409
8c10abe
2322d40
f8912b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,10 @@ def list_events( | |
| ), | ||
| ) -> list[Event]: | ||
| """Get all events.""" | ||
| return EventModel.objects.order_by(ordering or "-start_date", "-end_date") | ||
| if ordering and ordering.lstrip("-") == "end_date": | ||
| secondary = "-start_date" if ordering.startswith("-") else "start_date" | ||
| return EventModel.objects.order_by(ordering, secondary, "id") | ||
| return EventModel.objects.order_by(ordering or "-start_date", "-end_date", "id") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use event name for tie break. |
||
|
|
||
|
|
||
| @router.get( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,7 +89,9 @@ def list_issues( | |
| if filters.state: | ||
| issues = issues.filter(state=filters.state) | ||
|
|
||
| return issues.order_by(ordering or "-created_at", "-updated_at") | ||
| if ordering: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reasoning behind this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used id so that pagination becomes deterministic. Also, the earlier version had a double ordering bug, if a user provides ordering=updated_at, this would result in order_by("updated_at", "-updated_at"), which is contradictory. same with other files too. |
||
| return issues.order_by(ordering, "id") | ||
| return issues.order_by("-created_at", "-updated_at", "id") | ||
|
|
||
|
|
||
| @router.get( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,10 @@ class MilestoneFilter(FilterSchema): | |
| def list_milestones( | ||
| request: HttpRequest, | ||
| filters: MilestoneFilter = Query(...), | ||
| ordering: Literal["created_at", "-created_at", "updated_at", "-updated_at"] | None = None, | ||
| ordering: Literal["created_at", "-created_at", "updated_at", "-updated_at"] | None = Query( | ||
| None, | ||
| description="Ordering field", | ||
| ), | ||
| ) -> list[Milestone]: | ||
| """Get all milestones.""" | ||
| milestones = MilestoneModel.objects.select_related("repository", "repository__organization") | ||
|
|
@@ -91,7 +94,9 @@ def list_milestones( | |
| if filters.state: | ||
| milestones = milestones.filter(state=filters.state) | ||
|
|
||
| return milestones.order_by(ordering or "-created_at", "-updated_at") | ||
| if ordering: | ||
| return milestones.order_by(ordering, "id") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason why you change the user provided ordering params? |
||
| return milestones.order_by("-created_at", "-updated_at", "id") | ||
|
|
||
|
|
||
| @router.get( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we want to rely on user provided ordering here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the special logic for end_date because I thought when two events end on the same date, sorting by start_date seemed a bit more meaningful (longer events come first).
e.g. if Event A runs June 1-15 and Event B runs June 10-15, ordering by end_date, start_date would show A first.
But I am a bit in the middle whether we should add semantic logic or keeping it simple? would like a suggestion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arkid15r , I’d appreciate your input on this whenever you have a moment.