-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Perform Newest sort type correctly when sorting issues #30644
Conversation
modules/indexer/issues/dboptions.go
Outdated
default: | ||
searchOpt.SortBy = SortByUpdatedDesc | ||
searchOpt.SortBy = SortByCreatedDesc |
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.
Is it alright to change the default block here? My reasoning for changing this was because only invalid sort types would resort in executing the default block (as expected) and users/web handlers should be passing correct sort type anyways. Alternatively I could just add a case: "" || "latest"
instead of changing this block.
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 am not sure whether it is right to change the "default" behavior.
I think the old behavior should be kept as-is if it is not clear.
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.
Gotcha, I made the necessary changes and decided to change the second-to-last case statement to just use a fallthrough
since it's right above the default
block and they both do the same thing.
I think the default sort order should be updated time. |
Thanks for the review @yp05327!
Gotcha, I didn't have any other reason for changing this other than it was convenient and of the thought that users/the web client should be passing sort types that we support anyways.
I'm a tad confused by this. I'm sure there is something I'm missing here, but when reading this line from a function that the affected route handler uses and from looking at this template file here, to me it looks like we do since we just snatch the sorted list and pass it on to the template engine for it to use.
Just to make sure I understand this, do you mean I should update the switch statement I changed? Something like: switch opts.SortType {
case "", "latest":
searchOpt.SortBy = SortByCreatedDesc
. . .
case "recentupdate":
fallthrough
default:
searchOpt.SortBy = SortByUpdatedDesc
} |
modules/indexer/issues/dboptions.go
Outdated
@@ -87,8 +85,10 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp | |||
case "priority", "priorityrepo", "project-column-sorting": | |||
// Unsupported sort type for search | |||
searchOpt.SortBy = SortByUpdatedDesc |
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.
Sorry, I misunderstood, plz ignore my comment above.
Your fix is right, it think it is better to fix it here.
Unsupported sort type should be same to default?
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.
No worries!
Another review was made and it was determined that we shouldn't change the default block. However, I did replace the only line of this case block with a fallthrough
since it's the same as the default
block.
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.
The latest version LGTM.
* origin/main: Interpolate runs-on with variables when scheduling tasks (go-gitea#30640) Initial support for colorblindness-friendly themes (go-gitea#30625) Fix flash message for flex-container (go-gitea#30657) Perform Newest sort type correctly when sorting issues (go-gitea#30644) Fix project name wrapping, remove horizontal margin on header (go-gitea#30631) Add a db consistency check to remove runners that do not belong to a repository (go-gitea#30614) Fix wrong table name (go-gitea#30557) Fix compare api swagger (go-gitea#30648) [skip ci] Updated translations via Crowdin Fix queue test (go-gitea#30646) Enable jquery-related eslint rules that have no violations (go-gitea#30632) Enable more `revive` linter rules (go-gitea#30608) Remove obsolete CSS text classes (go-gitea#30576) Hide diff stats on empty PRs (go-gitea#30629) [skip ci] Updated licenses and gitignores Use correct hash for "git update-index" (go-gitea#30626) Fix repo home UI when there is no repo description (go-gitea#30552) Fix dropdown text ellipsis (go-gitea#30628) fix(api): refactor branch and tag existence checks (go-gitea#30618) Add --skip-db option to dump command (go-gitea#30613)
* giteaofficial/main: Fix a panic bug when head repository deleting (go-gitea#30674) Fix some bug on migrations (go-gitea#30647) Fix checkbox field markup (go-gitea#30666) Avoid doubled border for the PR info segment (go-gitea#30663) Interpolate runs-on with variables when scheduling tasks (go-gitea#30640) Initial support for colorblindness-friendly themes (go-gitea#30625) Fix flash message for flex-container (go-gitea#30657) Perform Newest sort type correctly when sorting issues (go-gitea#30644) Fix project name wrapping, remove horizontal margin on header (go-gitea#30631) Add a db consistency check to remove runners that do not belong to a repository (go-gitea#30614) Fix wrong table name (go-gitea#30557) Fix compare api swagger (go-gitea#30648) [skip ci] Updated translations via Crowdin Fix queue test (go-gitea#30646) Enable jquery-related eslint rules that have no violations (go-gitea#30632) Enable more `revive` linter rules (go-gitea#30608) Remove obsolete CSS text classes (go-gitea#30576) Hide diff stats on empty PRs (go-gitea#30629)
…low_dispatch_event * origin/main: (62 commits) Add test for go-gitea#30674 (go-gitea#30679) Fix border-radius of header+segment boxes (go-gitea#30667) Fix a panic bug when head repository deleting (go-gitea#30674) Fix some bug on migrations (go-gitea#30647) Fix checkbox field markup (go-gitea#30666) Avoid doubled border for the PR info segment (go-gitea#30663) Interpolate runs-on with variables when scheduling tasks (go-gitea#30640) Initial support for colorblindness-friendly themes (go-gitea#30625) Fix flash message for flex-container (go-gitea#30657) Perform Newest sort type correctly when sorting issues (go-gitea#30644) Fix project name wrapping, remove horizontal margin on header (go-gitea#30631) Add a db consistency check to remove runners that do not belong to a repository (go-gitea#30614) Fix wrong table name (go-gitea#30557) Fix compare api swagger (go-gitea#30648) [skip ci] Updated translations via Crowdin Fix queue test (go-gitea#30646) Enable jquery-related eslint rules that have no violations (go-gitea#30632) Enable more `revive` linter rules (go-gitea#30608) Remove obsolete CSS text classes (go-gitea#30576) Hide diff stats on empty PRs (go-gitea#30629) ...
Should resolve go-gitea#30642. Before this commit, we were treating an empty `?sort=` query parameter as the correct sorting type (which is to sort issues in descending order by their created UNIX time). But when we perform `sort=latest`, we did not include this as a type so we would sort by the most recently updated when reaching the `default` switch statement block. This commit fixes this by considering the empty string, "latest", and just any other string that is not mentioned in the switch statement as sorting by newest.
Backport #30644 by kemzeb Should resolve #30642. Co-authored-by: Kemal Zebari <[email protected]>
Should resolve #30642.
Before this commit, we were treating an empty
?sort=
query parameter as the correct sorting type (which is to sort issues in descending order by their created UNIX time). But when we performsort=latest
, we did not include this as a type so we would sort by the most recently updated when reaching thedefault
switch statement block.This commit fixes this by considering the empty string, "latest", and just any other string that is not mentioned in the switch statement as sorting by newest.