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

Sorting by "Nearest Due Date" puts all issues without a due date first. #8383

Closed
2 of 7 tasks
bobemoe opened this issue Oct 5, 2019 · 13 comments
Closed
2 of 7 tasks
Labels

Comments

@bobemoe
Copy link
Contributor

bobemoe commented Oct 5, 2019

  • Gitea version (or commit ref): 1.10.0+dev-369-g1a2d7207e
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

When sorting by nearest due date you'd expect the overdue/imminent issues to be first in the list. If on a repo with lots of issues you can find that the issues you are looking for are actually on the last page.

https://try.gitea.io/bobemoe/test/issues?q=&type=all&sort=nearduedate&state=open&labels=0&milestone=0&assignee=0

This leaves no way to actually sort them to the top of the list.

When sorting like this, should we:

  • filter out any issues without a due date
  • or ensure they are always at the bottom of the list
@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 5, 2019

I'm going to try and do a PR for this.. never used GO before, but looking at the code this might be quite simple? ;)

@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 5, 2019

Not quite as simple as fist thought :/

Going with the first option, "filter out any issues without a due date", the above commit works, however it does not update the counts on the "open/closed" buttons.

I guess the result set is counted before it's sorted? This raises the question of should I be mixing filters into the sort function?

If its cool to do that, where should I be adapting to get the correct count?

2019-10-05-133936_1192x484_scrot

@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 5, 2019

OK, I've found what looks like a much better place to put the code; in setupSession, which is used by CountIssuesByRepo as well as Issues. The issues are still filtered as as expected but the issue count open/closed is still not correct. I can't understand why its not working!?

@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 5, 2019

OK, so a bit of debugging shows CountIssuesByRepo isn't actually getting called :(

So where are these counts coming from? Wherever it is it can't be using setupSession...

@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 5, 2019

I think the counts come from GetRepoIssueStats but that function hasn't got IssuesOptions available.

I think I'm beat :(

@guillep2k
Copy link
Member

Perhaps you can try a different route. I'm not entirely sure if this is possible, given the compatibility constraints, but for example, this kind of usage is valid:

OrderBy("case when xxxx then yyy else zzz end").

I imagine that something like this can be done (note: it's wrong SQL, it's just to convey the idea):

OrderBy("case when due_date is null then '3000-12-31' else due_date end").

This at least will send all the issues without due date to the back of the query.

The problems this may pose (if you're interested in following it) are:

  • Finding out the maximum date for all supported databases (example) and make sure you don't exceed the maximum common denominator.
  • Finding a bulletproof way of building the date value in xorm in a way that is compatible with all engines, as Engine.Orderby() doesn't accept any parameters. Perhaps there's a helper function for that? In the database/sql package? Not sure. It must be a locale-independent method. Perhaps the ISO-8601 date format is supported by all (e.g. 3000-12-31T23:59:59Z).

@bobemoe
Copy link
Contributor Author

bobemoe commented Oct 6, 2019

Thanks for the input :) This seems to work for:

sess.OrderBy("issue.deadline_unix == 0, issue.deadline_unix")

I'm not sure if using ORDER BY CASE WHEN THEN would be more efficient? I'm sure either way will mean the deadline index cant be used for the ordering - if it even exists and was used in the first place I'm not sure.

Are there any tests I can run to ensure compatibility?

@guillep2k
Copy link
Member

Are there any tests I can run to ensure compatibility?

Yes, there are the integration tests. Currently the example in the Hacking Gitea page points out only to sqlite (which is embedded in the code, so always available), but to test other databases you'd have to install them yourself.

But for a single statement like this I suggest try using one of the online pages like SQL Fiddle, db-fiddle, sqltest, etc. The list of engines to test is currently: MSSQL, Postgres, MySQL/MariaDB and SQLite.

@lunny lunny added the type/bug label Oct 7, 2019
@bagasme
Copy link
Contributor

bagasme commented Oct 9, 2019

When doing tests, consider when end-user install distro version of Postgres or MySQL/MariaDB instead of latest upstream version and ensure that your SQL statement works correctly.

@vincentdavis
Copy link

I would really like to see this fixed in 1.11
I can cheer but not sure I can be of much more help.

@bobemoe
Copy link
Contributor Author

bobemoe commented Nov 14, 2019

I can submit my one-liner fix as a PR if you like? But I haven't had any time to run all the tests against all the DB's.

@guillep2k
Copy link
Member

This was fixed in #8871 I believe.

@bobemoe
Copy link
Contributor Author

bobemoe commented Nov 14, 2019

Fantastic! Yep, I can confirm the fix is working :) Thanks

@bobemoe bobemoe closed this as completed Nov 14, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants