Configurable ordering for messageboards #404

timdiggins · 2016-09-17T16:15:56Z

No description provided.

jayroh · 2016-09-17T16:30:15Z

Instead of sorting with ruby what do you think about attaching a scope to messageboards that will order w/SQL? That is - if the messageboads method here returns an AR object and not a collection. Letting SQL do this sort of work would be ideal.

timdiggins · 2016-09-17T17:42:17Z

Totally agree in principle, but I tried and the grouping made it non
trivial so I took the easy way out!

Would like to do a quick fix for this now and then come back to optimise it
later

On Saturday, 17 September 2016, Joel Oliveira notifications@github.com
wrote:

Instead of sorting with ruby what do you think about attaching a scope to
messageboards that will order w/SQL? That is - if the messageboads method
here returns an AR object and not a collection. Letting SQL do this sort of
work would be ideal.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#404 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABH2_Jt4jeOndkX9FHckb_a-HE-U6hdks5qrBWYgaJpZM4J_o0O
.

(I've shortened my email address to tim@red56.uk (three less letters!), but
tim@red56.co.uk will get to me too).

glebm · 2016-09-17T20:07:46Z

app/views/thredded/messageboards/index.html.erb

@@ -7,7 +7,7 @@
      <% if group.name.present? %>
        <h3><%= group.name %></h3>
      <% end %>
-      <%= render group.messageboards %>
+      <%= render group.messageboards.sort_by{|m| - m.updated_at.to_f } %>


This should probably be in the view model or the controller

timdiggins · 2016-09-18T07:30:39Z

Good call @glebm -- moving it to view model made it easy to test. I also realized that Messageboard groups (I don't use them) should be listed in some kind of consistent order on the main page.

I chose alphabetic, but there are better orderings, I'm sure. (we can wait for a PR from someone who uses messageboard groups?)

timdiggins · 2016-09-18T08:38:42Z

So, I didn't realize/remember that the current ordering wasn't emergent, but deliberate (though dependent on database implementation rather than sql). Is it ok to change this ordering for Messageboards in general (or do we need to make another configuration option). Background: I just read that: Within a group, the messageboards are ordered in the order they were created at. Groups themselves are also ordered in the order they were created at.

I think that given the UI, the updated_at would be the least surprising ordering for messageboards. And for Messageboard Groups, the least surprising (though possibly not very helpful) would be ordering by name alphabetically.

glebm · 2016-09-18T11:52:43Z

@timdiggins @jayroh It seems to me that many public forums have deliberate ordering for both groups and messageboards. Perhaps this should be an option?

timdiggins · 2016-09-18T12:18:06Z

@glebm @jayroh I can see that might be useful in some instances, but I think it would still need to be optional -- It wouldn't fit my usage -- as messageboards in my system are many and created semi-automatically and not managed as such (they correspond to "projects" in the main app).

I think this introduction of an ordering will need careful design.

But let's say we introduce a ordering field which is optional, and there's a way to manage and order them, in we still need a tie-breaker for cases where this ordering field/management option isn't used.

I just think that currently the UI and the ordering are not in alignment. This PR fixes it (perfectly for my use case 😉 ) and would like to defer the ordering improvement to a different issue / PR...

Further thoughts on ordering: We could also have configuration for ordering -- this could be complicated to implement (e.g. I'd like everything (messageboards / topics / posts) to be consistently reverse-chronologically ordered, but other people might want messageboards to be ordered by a determined order (or creation order?), topics to be reverse-chronologically ordered and posts to be chronologically ordered)). And then, it's one thing to create a mechanism to allow for multiple orderings, and then the implications (e.g. page on UserTopicReadState) but then these have UI implications not just for now, but for every time any change happens-- quite a big deal.

I think better to have a thredded approach if possible, and then stick to it.

glebm · 2016-09-18T17:52:51Z

A specific order is perfect for my use case, which is why I'm hesitant to accept this unless it's optional. :) Specific order is also the case with traditional messageboards (like phpbb), and other Ruby forum engines like Forem.

For messageboard groups and messageboards, a proper interface would need to be designed carefully, but perhaps for now we could just offer a boolean config setting to enable the reverse chronological order (or maybe separate booleans for messageboard groups and messageboards). We can then deprecate these settings once a proper interface is in place.

I don't think the ordering of topics within a messageboard and posts within a topic needs to be customizable. Can't think of a use case for anything but most recently updated first.

timdiggins · 2016-09-19T07:22:46Z

Ok -- that sounds good -- options would be (maybe same for messageboards & messageboard groups for now):

Thredded.messageboards_order which can be :created_chronological (default) or :updated_reverse_chronological

When we get specific order, the ordering field (when not nil/default) trumps the messageboards_order.

In fact it would be trivial to add this field (Messageboard#ordering, MessageboardGroup#ordering) now, then someone else just needs to build the UI.

However, in response to (which is a side-track):

I don't think the ordering of topics within a messageboard and posts within a topic needs to be customizable. Can't think of a use case for anything but most recently updated first.

Currently posts are chronological. Would you (and @jayroh and others) be happy just reversing this always (as opposed to configuration: #361)

glebm · 2016-09-19T10:03:23Z

Ok -- that sounds good -- options would be (maybe same for messageboards & messageboard groups for now):

Sounds good!

However, in response to (which is a side-track):

Sorry, yeah the posts are currently chronological. This does need to be an option, as most forums out there have posts in chronological order.

timdiggins · 2016-09-20T17:14:03Z

configurable messageboards_order
MessageboardsGroupView responds to messageboards_order
~~add Messageboards#ordering, MessageboardGroup#ordering~~ (see below)

timdiggins · 2016-09-20T18:38:39Z

ok, I've been thinking this through a bit more as I've been coding. I think we should have a Messageboard#ordering column, and then Thredded.messageboards_order can have three options: :first_created, :most_recent_post, :specified -- these determine when the ordering field is updated and how (on creation, update_last_topic, and never respectively). Then the actual order is always based on ordering (and id as a tiebreak). We have an index on Messageboard#ordering. This is more performant and makes more sense.

Another similar configuration option for MessageboardGroups I think (but maybe :alphabetical as another option)

glebm · 2016-09-20T23:35:28Z

@timdiggins Sounds good! Some comments on the naming:

Perhaps position would be a more common name than ordering (as per various gems that default to position for this kind of column). The column should be non-nullable with a default value of 0 or 1 because NULLs last vs first are different in different databases (lesson learned the hard way).

The ordering names should probably match column names for clarity, e.g. created_at_asc, last_post_at_desc, position.

timdiggins · 2016-09-21T06:16:32Z

Messageboards:

add Messageboard#position
calculate Messageboard#position according to Thredded.messageboards_order
last_post_at_desc uses last_topic.last_post_at from update messageboard timestamp when last topic changes #403

Messageboard Groups:

add MessageboardGroup#position
~~Thredded.messageboard_groups_order can be from :created_at_asc or :position~~

timdiggins · 2016-09-21T15:19:53Z

have just discovered in messageboard.rb:

    default_scope { where(closed: false).order(topics_count: :desc) }

I am planning to remove the order(topics_count: :desc) from the default scope and make :topics_count_desc an option for Thredded.messageboards_order

As a separate issue we should remove the default_scope -- see #407

So:

remove order from Messageboard.default_scope
create ordered scope and ensure ordering scope and use everywhere
allow :topics_count_desc as option for Thredded.messageboards_order and respect it

(though in the wrong place)

* creates #position field * ensures default value * fills with correct value depending on messageboard_group * ordered scope respects #position

To prevent nasty surprises later on.

timdiggins · 2016-09-22T06:21:35Z

@glebm Ok, I think this is all done. See if there's anything surprising in there.

The only difference from what was discussed above is that there is no real difference between :position and :created_at_asc -- and I've merged them (created_at provides the default value for :position, so that newly created messageboards with no defined position go to the bottom of the list. see https://github.com/thredded/thredded/pull/404/files#diff-d24aa57ddf38369b21abef6c31f43cae

glebm · 2016-09-22T08:41:43Z

What are the advantages of precalculating the position as opposed to ordering when querying?

Something like:

scope :with_order, ->(order) {
  case order
    when :position
      order(position: :asc)
    when :last_post_at_desc
      left_outer_joins(:last_topic).
        order('COALESCE(last_topic.last_post_at, messageboards.created_at) DESC') # arel here
    when :topics_count_desc
      order(topics_count: :desc)
  end.order(id: :desc)
}

This would support sorting in the UI, e.g. if someone wanted to implement a sort by dropdown, or sortable column tables for an old-school theme where the messageboards are rendered in a table. Also seems simpler to implement / maintain.

timdiggins · 2016-09-22T09:50:39Z

What are the advantages of precalculating the position as opposed to ordering when querying?

@glebm I originally had a similar solution (fe9c189 -- though that was with a different set of options).

However I felt that it was very strange to produce a field (position) that is then sometimes not used. Worse if you want to optimize sorting with indexes, it will be configuration-dependent. This feels like configuration doing the wrong thing. I felt that having the configuration just determine the algorithm to fill position felt cleaner (even if it does involve more lines of code).

There's no problem with using sorting in the UI -- that's exactly what the :position option is for.

I can see that this solution might look more complicated initially. However maintenance comes in many guises -- the algorithm for calculating position is pretty separated from other code and fairly crisp. And if we've got a weird sql error later on when using ordered and maybe a group the more complexity in our ordered will mean the more hard that is to unpick.

Think it's worth ruminating on...

timdiggins · 2016-09-22T12:24:16Z

@glebm totally missed your point initially about sorting in the UI (user chosen sorting as opposed to admin chosen position). Probably tips scales in favour of no precalculation

timdiggins · 2016-09-22T16:19:14Z

@glebm after considering it, I tried it, feels better, and provides more functionality the way you suggested. Have split into multiple scopes, think more readable. The only thing I didn't go for was the arel. I'm sure it's conceptually better, I just can't be bothered to work it out -- given that the sql is really quite readable and AFAICT pretty vanilla sql. Feel free to suggest an arel implementation.

glebm · 2016-09-22T16:35:29Z

@timdiggins No worries, I can't refactor it into Arel later. Will have another look

glebm · 2016-09-22T16:39:23Z

app/models/thredded/messageboard.rb

+        ordered_by_last_post_at_desc
+      when :topics_count_desc
+        ordered_by_topics_count_desc
+      end.order(id: :asc)


just a thought, perhaps we can always order by position, so here this would be

case order when :position self when ... ... end.order(position: :asc, id: :asc)

This way it would serve as a customizable tie-breaker

glebm · 2016-09-22T16:40:17Z

app/models/thredded/messageboard.rb

+      case order
+      when :position
+        ordered_by_position
+      when :created_at_asc


Perhaps we don't need this one?

Useful if you're composing an user-chosen sort (dropddown) -- might want to choose created_at_asc rather than position.

It still orders by position in the ordered_by_created_at_asc though. And position might not be the same as created_at.

glebm · 2016-09-22T16:42:22Z

app/models/thredded/stats.rb

@@ -18,7 +18,8 @@ def posts_count
    private

    def messageboards
-      @messageboards ||= Messageboard.all
+      @ordered = Messageboard.ordered


Seems like this could just be:

@messageboards ||= Messageboard.ordered

glebm · 2016-09-22T16:44:15Z

app/models/thredded/messageboard.rb

+      order(topics_count: :desc)
+    }
+    scope :ordered_by_group, ->(order = Thredded.messageboards_order) {
+      includes(:group).order('thredded_messageboard_groups.position asc').ordered(order)


Perhaps this should be a concerned of Thredded::MessageboardGroup, so that here this would look like:

includes(:group).merge(Thredded::MessageboardGroup.ordered).ordered(order)

And then maybe this scope can be removed (inlined), as it doesn't seem like it's ever useful to order by the group without then grouping by it.

glebm · 2016-09-22T16:47:35Z

lib/thredded.rb


  def self.user_display_name_method
    @@user_display_name_method || user_name_column
  end

+  def self.messageboards_order=(value)


Some simple docs so that folks get warnings in their IDEs if they pass an invalid value:

# @param value [:position, :topics_count_desc, :last_post_at_desc]

done. (all done in fact, I'll push in a second). But one point about thredded.rb @glebm -- I don't get why we're using mattr_accessors, and not cattr_accessors. Thredded is a module and is never included in class and so a mattr_accessor is a weird way of saying cattr_accessor. I think.

@timdiggins

I don't get why we're using mattr_accessors, and not cattr_accessors

I don't know either, though I think cattr_accessor is just an alias for mattr_accessor (http://api.rubyonrails.org/classes/Module.html#method-i-cattr_accessor).

But yeah would love to know if there is a better way. We could also replace all these with:

module Thredded class << self attr_acessor ... # regular instance methods / variables on the metaclass here end end

glebm · 2016-09-22T16:48:14Z

lib/thredded.rb

+    when :position, :topics_count_desc, :last_post_at_desc
+      @@messageboards_order = value # rubocop:disable Style/ClassVars
+    else
+      fail "Unexpected value for Thredded.messageboards_order: #{value}"


this should be an ArgumentError

timdiggins · 2016-09-22T18:29:12Z

over to you @glebm

glebm · 2016-09-22T18:40:01Z

lib/generators/thredded/install/templates/initializer.rb

+# :position            (default) set the position manually (new messageboards go to the bottom, by creation timestamp)
+# :last_post_at_desc   most recent post first
+# :topics_count_desc   most topics first
+# NB: if you change this you should run `Messageboard.recalculate_positions!`


No longer applies

timdiggins · 2016-09-23T07:00:41Z

One last thing @glebm @jayroh, I think the old default ordering was :topics_count (it was not always obvious, but I'm pretty sure that's true). The new default ordering in this PR is :position (defaults to :created_at_asc) -- which is a good default.

Are we happy to go with that change (it's pre 1.0 after all) and just add a note in the release notes? (@glebm you seem to have to do the releases...)

glebm · 2016-09-23T10:27:22Z

@timdiggins Good catch, I hadn't realized that! The new default is OK. I plan to do a release shortly after this has been merged and will emphasize this change in the release notes 🚢

timdiggins · 2016-09-23T10:34:22Z

@glebm Great. Shall I go ahead and merge, or shall we wait for any other reviews (from @jayroh for example)?

jayroh · 2016-09-23T14:35:08Z

I think it looks great, guys. One note - this is a tight enough feature where the 13 commits could probably be squashed down to 1. Otherwise, 👍 from me whenever you're ready to merge.

timdiggins · 2016-09-23T15:54:39Z

great @jayroh merging. (PS: I always squash and merge (now via github) unless there's some really good reason for separate commits and merge commit)

jayroh · 2016-09-23T16:01:53Z

I always squash and merge (now via github) unless there's some really good reason for separate commits and merge commit

By default, I usually do too :). Sometimes, though, there's some valuable info and history in the commits so I will be a little more discriminatory regarding WHAT I squash. I'm guessing you're the same way? :)

In any case - great work here. Glad this is in.

jayroh added the in progress label Sep 17, 2016

glebm reviewed Sep 17, 2016

View reviewed changes

timdiggins mentioned this pull request Sep 19, 2016

update messageboard timestamp when last topic changes #403

Merged

timdiggins changed the title ~~order messageboards with most recent updates first, like topics~~ [WIP] order messageboards with most recent updates first, like topics Sep 21, 2016

timdiggins mentioned this pull request Sep 21, 2016

remove default_scope from messageboard #407

Closed

timdiggins self-assigned this Sep 21, 2016

timdiggins added this to the v0.7.0 milestone Sep 21, 2016

timdiggins added the enhancement label Sep 21, 2016

timdiggins force-pushed the order-messageboards-within-groups-in-index branch from 863b71a to deca7f8 Compare September 21, 2016 23:19

timdiggins changed the title ~~[WIP] order messageboards with most recent updates first, like topics~~ Order messageboards with most recent updates first, like topics Sep 21, 2016

timdiggins added ready and removed in progress labels Sep 21, 2016

timdiggins added 4 commits September 22, 2016 00:33

order messageboards with most recent updates first, like topics

1347569

move ordering into viewmodel

d9accde

move retrieval to view model and ordering to sql

c773158

clarify and remove shadowing

8cd92ba

timdiggins added 2 commits September 22, 2016 00:33

define Thredded.messageboards_order and respect it.

fe9c189

(though in the wrong place)

ensure Messageboard and MessageboardGroup position

dfc2001

* creates #position field * ensures default value * fills with correct value depending on messageboard_group * ordered scope respects #position

timdiggins force-pushed the order-messageboards-within-groups-in-index branch from deca7f8 to dfc2001 Compare September 21, 2016 23:33

timdiggins changed the title ~~Order messageboards with most recent updates first, like topics~~ Configurable ordering for messageboards Sep 21, 2016

check for validty of messageboards_order on assignment in initializer.

52b0792

To prevent nasty surprises later on.

timdiggins added 2 commits September 22, 2016 09:01

make reset support only targeted when set

5508c58

fix missing reset of thredded configuration

5785996

use different ordering sql rather than precalculating position.

fc66a1d

glebm reviewed Sep 22, 2016

View reviewed changes

glebm requested changes Sep 22, 2016

View reviewed changes

refactorings as per discussion

f1639f5

glebm approved these changes Sep 22, 2016

View reviewed changes

remove outdated comment

76434f2

timdiggins merged commit 180da65 into thredded:master Sep 23, 2016

timdiggins deleted the order-messageboards-within-groups-in-index branch September 23, 2016 15:54

jayroh removed the ready label Sep 23, 2016

Configurable ordering for messageboards #404

Configurable ordering for messageboards #404

Conversation

timdiggins commented Sep 17, 2016

jayroh commented Sep 17, 2016

timdiggins commented Sep 17, 2016

Choose a reason for hiding this comment

timdiggins commented Sep 18, 2016

timdiggins commented Sep 18, 2016

glebm commented Sep 18, 2016

timdiggins commented Sep 18, 2016

glebm commented Sep 18, 2016

timdiggins commented Sep 19, 2016 • edited

glebm commented Sep 19, 2016

timdiggins commented Sep 20, 2016 • edited

timdiggins commented Sep 20, 2016

glebm commented Sep 20, 2016

timdiggins commented Sep 21, 2016 • edited

timdiggins commented Sep 21, 2016 • edited

timdiggins commented Sep 22, 2016

glebm commented Sep 22, 2016 • edited

timdiggins commented Sep 22, 2016

timdiggins commented Sep 22, 2016

timdiggins commented Sep 22, 2016

glebm commented Sep 22, 2016

glebm Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glebm Sep 22, 2016 • edited

Choose a reason for hiding this comment

glebm Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glebm Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdiggins commented Sep 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdiggins commented Sep 23, 2016

glebm commented Sep 23, 2016 • edited

timdiggins commented Sep 23, 2016

jayroh commented Sep 23, 2016

timdiggins commented Sep 23, 2016

jayroh commented Sep 23, 2016

timdiggins commented Sep 19, 2016 •

edited

timdiggins commented Sep 20, 2016 •

edited

timdiggins commented Sep 21, 2016 •

edited

timdiggins commented Sep 21, 2016 •

edited

glebm commented Sep 22, 2016 •

edited

glebm Sep 22, 2016 •

edited

glebm Sep 22, 2016 •

edited

glebm Sep 22, 2016 •

edited

glebm Sep 22, 2016 •

edited

glebm commented Sep 23, 2016 •

edited