Add ability to sort comments

[ludwiczakpawel]

This piggy-backs #3001.

---

Description

• As a user I want to be able to sort comments.
• [Stretch goal] As an admin I want to define default sorting option for discussions on my Forem.

1. Admin: community-level

Admin of a community should have a setting somewhere in /admin to set default sorting for their community discussions.

2. User-level

User should be able to set/overwrite default sorting for comments. Something like:

3. User, post-level

User should be able to overwrite global settings and his own preference for particular post only.

Importance, order of work:

Above I ordered it from the most global (community-level) to the most granular type of settings (user- & post-level). But implementation wise I think we should build it in the opposite order (post-level first, then user-level and finally community-level) because this feature seems like something that would be the most useful for users in general rather than admins.

Caveats

• For the scope of this project, I don't think we need to consider anything but top-level comments.

[github-actions]

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

Feature requests that require more discussion may be closed. Read more about our feature request process on forem.dev.

To our amazing contributors: issues labeled type: bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem/oss. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

[amywtlin]

@jeremyf out of all 3, 3 would be a good candidate for CE pod to tackle if anyone has time or would like to change things up a little!

[vishaldeepak]

any chance this issue is open for dev ?

[jeremyf]

@vishaldeepak as in you'd be up for tackling this? If so, I'd love to hear your approach.

[vishaldeepak]

@jeremyf Thinking about point no 3 first.(Backend)
Looks like sorting things should be quite straightforward.
From the UI we can ask for another parameter that defines the sorting sequence. Then we would add another parameter to the following method

def self.tree_for(commentable, limit = 0, sorting_sequence="score DESC")
    # make change here
    commentable.comments
      .includes(user: %i[setting profile])
      .arrange(order: sorting_sequence)
      .to_a[0..limit - 1]
      .to_h
end

All sorts should be

• "created_at desc"
• "created_at asc"
• "score desc"

As for the cache we might want to add a new parameter

<%= render partial: "articles/comment_tree", collection: Comment.tree_for(@article, @comments_to_show_count), as: :comment_node, cached: proc { |comment, _sub_comments| [comment, sort_order] } %>

Point No 2
Should be similar to other customization settings.

On the front-end side we could refresh the page when user wants to change the sort or dynamically load the comments view again.

Anything else I should be aware of/or missing?

[vishaldeepak]

@jeremyf can I start given the above?

[jeremyf]

Go for it! I'll assign this to you and me both, so we have a core member tracking this. Please ping me if you get stuck or need help with something. Your approach looks good.

[jeremyf]

@vishaldeepak checking in on where you're at with this.

[vishaldeepak]

@jeremyf Was too busy. A need a litte more time. Maybe il have a PR in a weeks time.

[djuber]

catching up on this now to determine if the work in #16686 closes this issue (or just part of it).

It looks like the user controlled sorting was added, but the admin option to set a community default sort order is an open goal. Do we want to open a new more focused issue about admin controlled default comment sort option, or refine the description to highlight that the original goals have been partly met?

I left a comment proposing Settings::UserExperience.comment_default_sort_order as the admin facing customization to add and what that looks like in the controller on the PR - it might be a small win to add a setting with a default value to the user experience customization in admin.

[vishaldeepak]

#16686 Only covers Step 3. Step 1 and Step 2 remain. Maybe have news goals for each? I'd like to try my hand on them as well if open :-)

[djuber]

@vishaldeepak thanks for the clarification! I didn't read far enough down in the description (I was looking at the first few lines outlining the goals). Yes, I think the existing UserSetting for font selection, and the admin UserExperience setting for community default font is a good template to consider on how to add a default sort order.

If you peek at Users::Setting.config_font, Users::Setting.resolved_font_name, and Settings::UserExperience.default_font, that pattern captures one approach for this that makes sense to me (and I think adding a UI for both user and admin options completes the remaining two steps).

[djuber]

#17084 (caching?)

#17082 (mobile)
#17081 (mobile)

#17080 (caching, fixed)

Looks like these are related. the last one is something tied to DEV.to caching specifically, so wouldn't have been seen in reviewer testing, and the first one also is caching related. The other two appear to be something we could have caught better as reviewers by previewing the changes with mobile as well as desktop.

[vishaldeepak]

@djuber @jeremyf Looks like the code has been revereted. How should I raise PR for the fixes?

[jeremyf]

@vishaldeepak in DEV things were breaking with this PR. I think you can build from the previous work (open a new PR). I'm uncertain of why it was breaking, I was just the one to issue the revert.

[aitchiss]

Following our chat @vishaldeepak, I've had an investigate and think I can summarise the issues we had when #16686 was first merged:

Caching issues:

• We had an edge caching bug where the sort order parameter was being ignored - this has been fixed already - see "comments_sort" param needs to be added to safe list #17080
• There was another fragment caching issue - detailed here Comments area not paying attention to comments_order #17084. The issue details how we need to work around it, so hopefully this would be a small tweak to your original work

These caching issues can be so difficult to spot in development, and we all missed it when reviewing too. I think when this work is next ready for review we can deploy it to one of our "canary" instances which would allow us to test in a production-like environment and make sure all is well 🙂

UI issues:

• The whole option should be clickable, not just the text in the title - see Inconsistent clickable area on comment sort dropdown #17082 (I think this is what you have been working on already)
• Mobile layout should be tweaked - see "Top Comments" is too big for mobile #17081. I think this might just be a case of applying some responsive classes so the text is smaller on smaller screens

With these points in mind, are you happy to continue working on this feature? If you're up for it, I think you could use your previous code from #16686, update from main branch, and add these small tweaks in.

[vishaldeepak]

thanks @aitchiss , i'l work on this

[vishaldeepak]

@aitchiss I've done all that was required though Im running into random error. Described here - #18349 (comment)
Will need help from the team.

[amywtlin]

@mirie Can someone from the team help with this?