Forums order-by-replies should also order by date [#32459]

Following on from this discussion, when forum replies are ordered by the number of replies, the results should also be ordered by the date. Specifically, when the topics in a forum are ordered by ascending number of replies, the list should show the most recent topics first. This will allow unanswered queries to be dealt with more effectively.

The important functions to be modified can be found at the end of the forum.module in the drupal core.

n.b. I will try to create a patch, but do not currently have a decent CVS HEAD installation of Drupal to work against. As soon as I am able to create a patch, I'll reassign this issue to me. I'll add a followup if I cannot do this.

Cheers,
Anj.

Comment	File	Size	Author
#3	tablesort_and_forum_ordering.patch	2.45 KB	anj
#3
#1	forum_order_by_created.patch	1.19 KB	tostinni
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tostinni CreditAttribution: tostinni commented 28 September 2005 at 17:44

Title:	Forms order-by-replies should also order by date	» Formus order-by-replies should also order by date
Status:	Active	» Needs work

File	Size
forum_order_by_created.patch	1.19 KB

Ok, this is a little clunky, but I think it does the job pretty well...
As tablesort_sql doesn't have an "after" attributes, I just add an ORDER BY ", n.created DESC" at the end of the SQL query.
Like this you ever get an order by date created if you order by "Replies" and get the same number of replies, or "Topic" and get various topic with the same name (rare I admin ;) )

Ok this need some improvements, but as the _forum_get_topic_order doesn't allow multiples order by, this is my best simple shot ;)

Comment #2

tostinni CreditAttribution: tostinni commented 28 September 2005 at 17:45

Title:

Formus order-by-replies should also order by date

» Forums order-by-replies should also order by date

Sorry for the bad title...

Comment #3

anj CreditAttribution: anj commented 29 September 2005 at 10:04

File	Size
tablesort_and_forum_ordering.patch	2.45 KB

I've verified that the above patch works, and I think it is safe to always order forums by n.created given that it comes at the end of the list of 'order by's. I've attached my attempt at tidying up the code by allowing an 'after' field on tablesort_sql. The attached patch carries patches for two files, includes/tablesort.inc and modules/forum.module.

Am I heading in the right direction here?

Cheers,
Andy J.

Comment #4

Dries CreditAttribution: Dries commented 29 September 2005 at 15:20

Moshe is the tablesort-master. Moshe? :-)

Doesn't the current tablesort implementation allow two sort keys to be specified?

Comment #5

moshe weitzman CreditAttribution: moshe weitzman commented 29 September 2005 at 16:01

Status:

Needs work

» Reviewed & tested by the community

the patch in #1 looks perfectly clean and reasonable to me. the tablesort code doesn't handle multiple sort conditions, and doesn't need to as shown in patch #1.

Comment #6

anj CreditAttribution: anj commented 29 September 2005 at 16:43

Fair enough. I was just curious whether this would be worth adding to tablesort_sql.inc so that other modules could access it, and whether that approach would make things any neater overall. I'm perfectly happy to use patch #1 keep the change small and localized. Perhaps tostinni could tell us what kind of improvements he had in mind? I'm willing to help with the implementation of any improved approach.

Comment #7

tostinni CreditAttribution: tostinni commented 29 September 2005 at 17:08

As you made, I was thinking in improving tablesort_sql with an after, but I was not sure of the necesity for it as normally you should provide a well ordered sort.
But in case of forum we should eventually need it, I just let it simple...

Btw, I told my patch was clunky, because I think that just added brutally an order by at the end of the sql, may not be so nice as coding. Especially when before you got a _forum_get_topic_order which purpose is to made the job, so I was thinking that modifying this would made it nicer, but it's impossible the way the function is written.
Then the only "ugly" thing I was seeing was that you may have a query like this ... ORDER BY n.sticky DESC, n.created DESC, n.created DESC or ... ORDER BY n.sticky ASC, n.created DESC, n.created DESC there's absolutly no problem with this, but I didn't think it was pretty.

So if this patch made the job and everybody is fine with, let's commit it ;)

Comment #8

anj CreditAttribution: anj commented 29 September 2005 at 19:24

I was worried about multiple n.created statements appearing in the SQL, but as far as I can tell, the table ordering uses the comment_created timestamp, not on the node timestamp, so only appears once at most. I think. In short, I'd support the commit. :)