From 992211350acbacb298f37bf07677338b3b243e2e Mon Sep 17 00:00:00 2001
From: Vinoth Kannan <svkn.87@gmail.com>
Date: Wed, 10 Jan 2024 13:33:30 +0530
Subject: [PATCH] FEATURE: option to sort user and group private messages.
 (#25146)

The UI will be the same as the one we're using in the topic list in "latest", "top" etc.,
---
 .../app/components/basic-topic-list.hbs       |  3 ++
 .../app/controllers/user-topics-list.js       | 40 +++++++++++++++++
 .../discourse/app/lib/utilities.js            |  9 ++++
 .../build-private-messages-group-route.js     | 34 ++++++++------
 .../routes/build-private-messages-route.js    | 45 +++++++++++++------
 .../discourse/app/routes/build-topic-route.js |  8 +---
 .../discourse/app/routes/user-topic-list.js   |  3 ++
 .../app/templates/user-topics-list.hbs        |  3 ++
 .../acceptance/user-private-messages-test.js  | 14 ++++++
 lib/topic_query/private_message_lists.rb      |  2 +-
 spec/requests/list_controller_spec.rb         | 45 +++++++++++++++++++
 11 files changed, 173 insertions(+), 33 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs b/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs
index d5a97b58288..5f4c69f85d4 100644
--- a/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs
+++ b/app/assets/javascripts/discourse/app/components/basic-topic-list.hbs
@@ -8,6 +8,9 @@
       @bulkSelectHelper={{this.bulkSelectHelper}}
       @canBulkSelect={{this.canBulkSelect}}
       @tagsForUser={{this.tagsForUser}}
+      @changeSort={{this.changeSort}}
+      @order={{this.order}}
+      @ascending={{this.ascending}}
     />
   {{else}}
     {{#unless this.loadingMore}}
diff --git a/app/assets/javascripts/discourse/app/controllers/user-topics-list.js b/app/assets/javascripts/discourse/app/controllers/user-topics-list.js
index 3d5d982c66e..bdd68aa1a33 100644
--- a/app/assets/javascripts/discourse/app/controllers/user-topics-list.js
+++ b/app/assets/javascripts/discourse/app/controllers/user-topics-list.js
@@ -1,7 +1,10 @@
+import { tracked } from "@glimmer/tracking";
 import Controller from "@ember/controller";
 import { action } from "@ember/object";
 import { or, reads } from "@ember/object/computed";
+import { isNone } from "@ember/utils";
 import BulkSelectHelper from "discourse/lib/bulk-select-helper";
+import { defineTrackedProperty } from "discourse/lib/tracked-tools";
 import Topic from "discourse/models/topic";
 import {
   NEW_FILTER,
@@ -9,12 +12,20 @@ import {
 } from "discourse/routes/build-private-messages-route";
 import discourseComputed from "discourse-common/utils/decorators";
 
+export const queryParams = {
+  ascending: { replace: true, refreshModel: true, default: false },
+  order: { replace: true, refreshModel: true },
+};
+
 // Lists of topics on a user's page.
 export default class UserTopicsListController extends Controller {
+  @tracked model;
+
   hideCategory = false;
   showPosters = false;
   channel = null;
   tagsForUser = null;
+  queryParams = Object.keys(queryParams);
 
   bulkSelectHelper = new BulkSelectHelper(this);
 
@@ -23,6 +34,13 @@ export default class UserTopicsListController extends Controller {
   @or("currentUser.canManageTopic", "showDismissRead", "showResetNew")
   canBulkSelect;
 
+  constructor() {
+    super(...arguments);
+    for (const [name, info] of Object.entries(queryParams)) {
+      defineTrackedProperty(this, name, info.default);
+    }
+  }
+
   get bulkSelectEnabled() {
     return this.bulkSelectHelper.bulkSelectEnabled;
   }
@@ -54,6 +72,28 @@ export default class UserTopicsListController extends Controller {
     this.pmTopicTrackingState.stopIncomingTracking();
   }
 
+  @action
+  changeSort(sortBy) {
+    if (sortBy === this.resolvedOrder) {
+      this.ascending = !this.resolvedAscending;
+    } else {
+      this.ascending = false;
+    }
+    this.order = sortBy;
+  }
+
+  get resolvedAscending() {
+    if (isNone(this.ascending)) {
+      return this.model.get("params.ascending") === "true";
+    } else {
+      return this.ascending.toString() === "true";
+    }
+  }
+
+  get resolvedOrder() {
+    return this.order ?? this.model.get("params.order") ?? "activity";
+  }
+
   @action
   resetNew() {
     const topicIds = this.selected
diff --git a/app/assets/javascripts/discourse/app/lib/utilities.js b/app/assets/javascripts/discourse/app/lib/utilities.js
index eb87dd093f0..fc69a453740 100644
--- a/app/assets/javascripts/discourse/app/lib/utilities.js
+++ b/app/assets/javascripts/discourse/app/lib/utilities.js
@@ -728,3 +728,12 @@ export function allowOnlyNumericInput(event, allowNegative = false) {
     }
   }
 }
+
+export function cleanNullQueryParams(params) {
+  for (const [key, val] of Object.entries(params)) {
+    if (val === "undefined" || val === "null") {
+      params[key] = null;
+    }
+  }
+  return params;
+}
diff --git a/app/assets/javascripts/discourse/app/routes/build-private-messages-group-route.js b/app/assets/javascripts/discourse/app/routes/build-private-messages-group-route.js
index bc56b27b793..a31a8d80f94 100644
--- a/app/assets/javascripts/discourse/app/routes/build-private-messages-group-route.js
+++ b/app/assets/javascripts/discourse/app/routes/build-private-messages-group-route.js
@@ -1,5 +1,6 @@
 import { capitalize } from "@ember/string";
 import { findOrResetCachedTopicList } from "discourse/lib/cached-topic-list";
+import { cleanNullQueryParams } from "discourse/lib/utilities";
 import createPMRoute from "discourse/routes/build-private-messages-route";
 import I18n from "discourse-i18n";
 
@@ -21,7 +22,7 @@ export default (inboxType, filter) => {
       }
     },
 
-    model() {
+    model(params = {}) {
       const username = this.modelFor("user").get("username_lower");
       const groupName = this.modelFor("userPrivateMessages.group").name;
 
@@ -36,18 +37,25 @@ export default (inboxType, filter) => {
         topicListFilter
       );
 
-      return lastTopicList
-        ? lastTopicList
-        : this.store
-            .findFiltered("topicList", { filter: topicListFilter })
-            .then((topicList) => {
-              // andrei: we agreed that this is an anti pattern,
-              // it's better to avoid mutating a rest model like this
-              // this place we'll be refactored later
-              // see https://github.com/discourse/discourse/pull/14313#discussion_r708784704
-              topicList.set("emptyState", this.emptyState());
-              return topicList;
-            });
+      if (lastTopicList) {
+        return lastTopicList;
+      }
+
+      params = cleanNullQueryParams(params);
+
+      return this.store
+        .findFiltered("topicList", {
+          filter: topicListFilter,
+          params,
+        })
+        .then((topicList) => {
+          // andrei: we agreed that this is an anti pattern,
+          // it's better to avoid mutating a rest model like this
+          // this place we'll be refactored later
+          // see https://github.com/discourse/discourse/pull/14313#discussion_r708784704
+          topicList.set("emptyState", this.emptyState());
+          return topicList;
+        });
     },
 
     afterModel(model) {
diff --git a/app/assets/javascripts/discourse/app/routes/build-private-messages-route.js b/app/assets/javascripts/discourse/app/routes/build-private-messages-route.js
index bb5d6f2f510..26ad188628b 100644
--- a/app/assets/javascripts/discourse/app/routes/build-private-messages-route.js
+++ b/app/assets/javascripts/discourse/app/routes/build-private-messages-route.js
@@ -1,6 +1,7 @@
 import { action } from "@ember/object";
 import { htmlSafe } from "@ember/template";
 import { findOrResetCachedTopicList } from "discourse/lib/cached-topic-list";
+import { cleanNullQueryParams } from "discourse/lib/utilities";
 import UserAction from "discourse/models/user-action";
 import UserTopicListRoute from "discourse/routes/user-topic-list";
 import getURL from "discourse-common/lib/get-url";
@@ -24,7 +25,7 @@ export default (inboxType, path, filter) => {
       ];
     },
 
-    model() {
+    model(params = {}) {
       const topicListFilter =
         "topics/" + path + "/" + this.modelFor("user").get("username_lower");
 
@@ -33,18 +34,25 @@ export default (inboxType, path, filter) => {
         topicListFilter
       );
 
-      return lastTopicList
-        ? lastTopicList
-        : this.store
-            .findFiltered("topicList", { filter: topicListFilter })
-            .then((model) => {
-              // andrei: we agreed that this is an anti pattern,
-              // it's better to avoid mutating a rest model like this
-              // this place we'll be refactored later
-              // see https://github.com/discourse/discourse/pull/14313#discussion_r708784704
-              model.set("emptyState", this.emptyState());
-              return model;
-            });
+      if (lastTopicList) {
+        return lastTopicList;
+      }
+
+      params = cleanNullQueryParams(params);
+
+      return this.store
+        .findFiltered("topicList", {
+          filter: topicListFilter,
+          params,
+        })
+        .then((model) => {
+          // andrei: we agreed that this is an anti pattern,
+          // it's better to avoid mutating a rest model like this
+          // this place we'll be refactored later
+          // see https://github.com/discourse/discourse/pull/14313#discussion_r708784704
+          model.set("emptyState", this.emptyState());
+          return model;
+        });
     },
 
     setupController() {
@@ -65,6 +73,17 @@ export default (inboxType, path, filter) => {
         group: null,
         inbox: inboxType,
       });
+
+      let ascending = userTopicsListController.ascending;
+      if (ascending === "true") {
+        ascending = true;
+      } else if (ascending === "false") {
+        ascending = false;
+      }
+      userTopicsListController.setProperties({
+        ascending,
+      });
+
       userTopicsListController.bulkSelectHelper.clear();
 
       userTopicsListController.subscribe();
diff --git a/app/assets/javascripts/discourse/app/routes/build-topic-route.js b/app/assets/javascripts/discourse/app/routes/build-topic-route.js
index d2fdbaac1bb..0cbe2db285e 100644
--- a/app/assets/javascripts/discourse/app/routes/build-topic-route.js
+++ b/app/assets/javascripts/discourse/app/routes/build-topic-route.js
@@ -4,7 +4,7 @@ import { isEmpty } from "@ember/utils";
 import { queryParams, resetParams } from "discourse/controllers/discovery/list";
 import { disableImplicitInjections } from "discourse/lib/implicit-injections";
 import { setTopicList } from "discourse/lib/topic-list-tracker";
-import { defaultHomepage } from "discourse/lib/utilities";
+import { cleanNullQueryParams, defaultHomepage } from "discourse/lib/utilities";
 import Session from "discourse/models/session";
 import Site from "discourse/models/site";
 import DiscourseRoute from "discourse/routes/discourse";
@@ -60,11 +60,7 @@ export async function findTopicList(
   if (!list) {
     // Clean up any string parameters that might slip through
     filterParams ||= {};
-    for (const [key, val] of Object.entries(filterParams)) {
-      if (val === "undefined" || val === "null") {
-        filterParams[key] = null;
-      }
-    }
+    filterParams = cleanNullQueryParams(filterParams);
 
     list = await store.findFiltered("topicList", {
       filter,
diff --git a/app/assets/javascripts/discourse/app/routes/user-topic-list.js b/app/assets/javascripts/discourse/app/routes/user-topic-list.js
index 438ad20ba2d..ce6ee2c7266 100644
--- a/app/assets/javascripts/discourse/app/routes/user-topic-list.js
+++ b/app/assets/javascripts/discourse/app/routes/user-topic-list.js
@@ -1,3 +1,4 @@
+import { queryParams } from "discourse/controllers/user-topics-list";
 import { setTopicList } from "discourse/lib/topic-list-tracker";
 import ViewingActionType from "discourse/mixins/viewing-action-type";
 import DiscourseRoute from "discourse/routes/discourse";
@@ -6,6 +7,8 @@ export default DiscourseRoute.extend(ViewingActionType, {
   templateName: "user-topics-list",
   controllerName: "user-topics-list",
 
+  queryParams,
+
   setupController(controller, model) {
     setTopicList(model);
 
diff --git a/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs b/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs
index 8363f9de7af..03fd00e1e8a 100644
--- a/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs
+++ b/app/assets/javascripts/discourse/app/templates/user-topics-list.hbs
@@ -50,6 +50,9 @@
       @tagsForUser={{this.tagsForUser}}
       @canBulkSelect={{this.canBulkSelect}}
       @bulkSelectHelper={{this.bulkSelectHelper}}
+      @changeSort={{this.changeSort}}
+      @order={{this.order}}
+      @ascending={{this.ascending}}
     />
 
     <TopicDismissButtons
diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js
index c850b301974..f22b484b8eb 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-private-messages-test.js
@@ -298,6 +298,20 @@ const publishGroupNewToMessageBus = function (opts) {
   );
 };
 
+acceptance("User Private Messages - sorting", function (needs) {
+  withGroupMessagesSetup(needs);
+
+  test("order by posts_count", async function (assert) {
+    await visit("/u/eviltrout/messages");
+
+    assert.ok(exists(".topic-list-header th.posts.sortable"), "is sortable");
+
+    await click(".topic-list-header th.posts.sortable");
+
+    assert.ok(exists(".topic-list-header th.posts.sortable.sorting"), "sorted");
+  });
+});
+
 acceptance(
   "User Private Messages - user with group messages",
   function (needs) {
diff --git a/lib/topic_query/private_message_lists.rb b/lib/topic_query/private_message_lists.rb
index 53bd63c64c3..95d62fffb29 100644
--- a/lib/topic_query/private_message_lists.rb
+++ b/lib/topic_query/private_message_lists.rb
@@ -263,8 +263,8 @@ class TopicQuery
           .joins(
             "LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})",
           )
-          .order("topics.bumped_at DESC")
 
+      result = apply_ordering(result, options) if !options[:skip_ordering]
       result = result.includes(:tags) if SiteSetting.tagging_enabled
       result = result.limit(options[:per_page]) unless options[:limit] == false
       result = result.visible if options[:visible] || @user.nil? || @user.regular?
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index db9d9f02c66..ead20ea2904 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -398,6 +398,25 @@ RSpec.describe ListController do
 
         expect(response.status).to eq(404)
       end
+
+      it "should sort group private messages by posts_count" do
+        topic2 = Fabricate(:private_message_topic, allowed_groups: [group])
+        topic3 = Fabricate(:private_message_topic, allowed_groups: [group])
+        2.times { Fabricate(:post, topic: topic2) }
+        Fabricate(:post, topic: topic3)
+
+        sign_in(Fabricate(:admin))
+
+        get "/topics/private-messages-group/#{user.username}/#{group.name}.json",
+            params: {
+              order: "posts",
+            }
+
+        expect(response.status).to eq(200)
+        expect(response.parsed_body["topic_list"]["topics"].map { |t| t["id"] }).to eq(
+          [topic2.id, topic3.id, topic.id],
+        )
+      end
     end
 
     describe "with unicode_usernames" do
@@ -859,6 +878,32 @@ RSpec.describe ListController do
       json = response.parsed_body
       expect(json["topic_list"]["topics"].size).to eq(1)
     end
+
+    it "sorts private messages by activity" do
+      topic_ids = []
+
+      [1.year.ago, 1.week.ago, 1.month.ago].each do |date|
+        pm =
+          Fabricate(
+            :private_message_topic,
+            user: Fabricate(:user),
+            created_at: date,
+            bumped_at: date,
+          )
+        pm.topic_allowed_users.create!(user: user)
+        topic_ids << pm.id
+      end
+
+      sign_in(user)
+
+      get "/topics/private-messages/#{user.username}.json", params: { order: "activity" }
+
+      expect(response.status).to eq(200)
+      json = response.parsed_body
+      expect(json["topic_list"]["topics"].pluck("id")).to eq(
+        [topic_ids[1], topic_ids[2], topic_ids[0]],
+      )
+    end
   end
 
   describe "private_messages_sent" do