Skip to content

Perform Newest sort type correctly when sorting issues #30644

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.

Sign up for GitHub

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 4 commits into from
Apr 23, 2024
Merged

Perform Newest sort type correctly when sorting issues #30644

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5d39ef
Perform Newest sort type correctly when sorting issues
kemzeb Apr 22, 2024
4136728
Add missing fallthrough
kemzeb Apr 22, 2024
d7b008a
Don't change the default block and replace a line with a fallthrough
kemzeb Apr 23, 2024
340f488
Merge branch 'main' into fix-issue-newest-sort
GiteaBot Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/indexer/issues/dboptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,6 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp
searchOpt.Paginator = opts.Paginator

switch opts.SortType {
case "":
searchOpt.SortBy = SortByCreatedDesc
case "oldest":
searchOpt.SortBy = SortByCreatedAsc
case "recentupdate":
Expand All @@ -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
Copy link
Contributor

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?

Copy link
Member Author

@kemzeb kemzeb Apr 23, 2024 •
edited
Loading

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version LGTM.

case "latest":
fallthrough
default:
searchOpt.SortBy = SortByUpdatedDesc
searchOpt.SortBy = SortByCreatedDesc
Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

@kemzeb kemzeb Apr 23, 2024 •
edited
Loading

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.

}

return searchOpt
Expand Down