From c8569ce8b9b493a2cc2e0be98a44bc16495cc4d0 Mon Sep 17 00:00:00 2001
From: Jorge Manrubia <jorge@37signals.com>
Date: Thu, 21 Aug 2025 19:12:54 +0200
Subject: [PATCH] Split indexed_by into two filters: indexed by and sort by

https://3.basecamp.com/2914079/buckets/37331921/todos/8877489555#__recording_8987808963
---
 app/controllers/concerns/filter_scoped.rb     |  2 +-
 app/models/card.rb                            | 15 +++++--
 app/models/filter.rb                          |  2 +-
 app/models/filter/fields.rb                   | 19 +++++++--
 app/models/filter/params.rb                   |  2 +
 app/models/filter/summarized.rb               | 10 ++++-
 app/models/user/filtering.rb                  | 12 +++---
 app/views/filters/_settings.html.erb          |  1 +
 app/views/filters/settings/_controls.html.erb |  2 +-
 .../filters/settings/_indexed_by.html.erb     |  8 ++--
 .../filters/settings/_sorted_by.html.erb      | 40 +++++++++++++++++++
 .../filters/settings/_time_window.html.erb    |  2 +-
 app/views/users/show.html.erb                 |  4 +-
 test/fixtures/filters.yml                     |  4 +-
 test/models/filter_test.rb                    |  8 ++--
 15 files changed, 100 insertions(+), 31 deletions(-)
 create mode 100644 app/views/filters/settings/_sorted_by.html.erb

diff --git a/app/controllers/concerns/filter_scoped.rb b/app/controllers/concerns/filter_scoped.rb
index 8e342afe7..6f2dd87de 100644
--- a/app/controllers/concerns/filter_scoped.rb
+++ b/app/controllers/concerns/filter_scoped.rb
@@ -13,7 +13,7 @@ module FilterScoped
   end
 
   private
-    DEFAULT_PARAMS = { indexed_by: "latest" }
+    DEFAULT_PARAMS = { indexed_by: "all", sorted_by: "latest" }
 
     def set_filter
       if params[:filter_id].present?
diff --git a/app/models/card.rb b/app/models/card.rb
index 0d0a458af..a429d7ae6 100644
--- a/app/models/card.rb
+++ b/app/models/card.rb
@@ -20,13 +20,20 @@ class Card < ApplicationRecord
 
   scope :indexed_by, ->(index) do
     case index
+    when "stalled" then stalled
+    when "closing_soon" then closing_soon
+    when "falling_back_soon" then falling_back_soon
+    when "closed" then closed.recently_closed_first
+    else all
+    end
+  end
+
+  scope :sorted_by, ->(sort) do
+    case sort
     when "newest" then reverse_chronologically
     when "oldest" then chronologically
     when "latest" then latest
-    when "stalled" then stalled.chronologically
-    when "closing_soon" then closing_soon.chronologically
-    when "falling_back_soon" then falling_back_soon.chronologically
-    when "closed" then closed.recently_closed_first
+    else latest
     end
   end
 
diff --git a/app/models/filter.rb b/app/models/filter.rb
index 56f9c6be7..8b2a78e21 100644
--- a/app/models/filter.rb
+++ b/app/models/filter.rb
@@ -18,7 +18,7 @@ class Filter < ApplicationRecord
 
   def cards
     @cards ||= begin
-      result = creator.accessible_cards.indexed_by(indexed_by)
+      result = creator.accessible_cards.indexed_by(indexed_by).sorted_by(sorted_by)
       result = result.where(id: card_ids) if card_ids.present?
       result = result.open unless include_closed_cards?
       result = result.by_engagement_status(engagement_status) if engagement_status.present?
diff --git a/app/models/filter/fields.rb b/app/models/filter/fields.rb
index fcdde0346..cf7ce868f 100644
--- a/app/models/filter/fields.rb
+++ b/app/models/filter/fields.rb
@@ -1,13 +1,14 @@
 module Filter::Fields
   extend ActiveSupport::Concern
 
-  INDEXES = %w[ newest oldest latest stalled closing_soon falling_back_soon ]
+  INDEXES = %w[ all stalled closing_soon falling_back_soon ]
+  SORTED_BY = %w[ newest oldest latest ]
 
   delegate :default_value?, to: :class
 
   class_methods do
     def default_values
-      { indexed_by: "latest" }
+      { indexed_by: "all", sorted_by: "latest" }
     end
 
     def default_value?(key, value)
@@ -16,7 +17,7 @@ module Filter::Fields
   end
 
   included do
-    store_accessor :fields, :assignment_status, :indexed_by, :terms,
+    store_accessor :fields, :assignment_status, :indexed_by, :sorted_by, :terms,
       :engagement_status, :card_ids, :creation, :closure
 
     def assignment_status
@@ -27,6 +28,10 @@ module Filter::Fields
       (super || default_indexed_by).inquiry
     end
 
+    def sorted_by
+      (super || default_sorted_by).inquiry
+    end
+
     def engagement_status
       super&.inquiry
     end
@@ -59,4 +64,12 @@ module Filter::Fields
   def default_indexed_by?
     default_value?(:indexed_by, indexed_by)
   end
+
+  def default_sorted_by
+    self.class.default_values[:sorted_by]
+  end
+
+  def default_sorted_by?
+    default_value?(:sorted_by, sorted_by)
+  end
 end
diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb
index 21b0886bd..e7ee5a32b 100644
--- a/app/models/filter/params.rb
+++ b/app/models/filter/params.rb
@@ -4,6 +4,7 @@ module Filter::Params
   PERMITTED_PARAMS = [
     :assignment_status,
     :indexed_by,
+    :sorted_by,
     :engagement_status,
     :creation,
     :closure,
@@ -40,6 +41,7 @@ module Filter::Params
   def as_params
     {}.tap do |params|
       params[:indexed_by]        = indexed_by
+      params[:sorted_by]         = sorted_by
       params[:engagement_status] = engagement_status
       params[:creation]          = creation
       params[:closure]           = closure
diff --git a/app/models/filter/summarized.rb b/app/models/filter/summarized.rb
index e56a2dd10..22bc6bc5e 100644
--- a/app/models/filter/summarized.rb
+++ b/app/models/filter/summarized.rb
@@ -1,15 +1,21 @@
 module Filter::Summarized
   def summary
-    [ index_summary, tag_summary, assignee_summary, creator_summary, stage_summary, terms_summary ].compact.to_sentence
+    [ index_summary, sort_summary, tag_summary, assignee_summary, creator_summary, stage_summary, terms_summary ].compact.to_sentence
   end
 
   private
     def index_summary
-      unless indexed_by.latest?
+      unless indexed_by.all?
         indexed_by.humanize
       end
     end
 
+    def sort_summary
+      unless sorted_by.latest?
+        sorted_by.humanize
+      end
+    end
+
     def tag_summary
       if tags.any?
         "#{tags.map(&:hashtag).to_choice_sentence}"
diff --git a/app/models/user/filtering.rb b/app/models/user/filtering.rb
index 74c436801..448445c0b 100644
--- a/app/models/user/filtering.rb
+++ b/app/models/user/filtering.rb
@@ -44,11 +44,15 @@ class User::Filtering
   def any?
     filter.tags.any? || filter.assignees.any? || filter.creators.any? || filter.closers.any? ||
       filter.stages.any? || filter.terms.any? || filter.card_ids&.any? ||
-      filter.assignment_status.unassigned? || !filter.indexed_by.latest?
+      filter.assignment_status.unassigned? || !filter.indexed_by.all? || !filter.sorted_by.latest?
   end
 
   def show_indexed_by?
-    expanded? || !filter.indexed_by.latest?
+    expanded? || !filter.indexed_by.all?
+  end
+
+  def show_sorted_by?
+    expanded? || !filter.sorted_by.latest?
   end
 
   def show_tags?
@@ -67,10 +71,6 @@ class User::Filtering
     expanded? || filter.closers.any?
   end
 
-  def show_time_window?(name)
-    expanded? || filter.public_send("#{name}_window").present?
-  end
-
   def enable_collection_filtering(&block)
     @collection_filtering_route_resolver = block
   end
diff --git a/app/views/filters/_settings.html.erb b/app/views/filters/_settings.html.erb
index 499878493..1ec499420 100644
--- a/app/views/filters/_settings.html.erb
+++ b/app/views/filters/_settings.html.erb
@@ -2,6 +2,7 @@
   <div class="flex-inline center align-center gap-half">
     <%= render "filters/settings/toggle", user_filtering: user_filtering %>
 
+    <%= render "filters/settings/sorted_by", user_filtering: user_filtering %>
     <%= render "filters/settings/indexed_by", user_filtering: user_filtering %>
     <%= render "filters/settings/tags", user_filtering: user_filtering %>
     <%= render "filters/settings/assignees", user_filtering: user_filtering %>
diff --git a/app/views/filters/settings/_controls.html.erb b/app/views/filters/settings/_controls.html.erb
index 7b24f96c9..0e1d5f096 100644
--- a/app/views/filters/settings/_controls.html.erb
+++ b/app/views/filters/settings/_controls.html.erb
@@ -14,7 +14,7 @@
     <% end %>
   <% end %>
 
-  <%= link_to cards_path(filter.as_params.except(:assignee_ids, :assignment_status, :card_ids, :creator_ids, :closer_ids, :stage_ids, :tag_ids, :terms, :indexed_by, :creation, :closure)), class: "btn btn--remove txt-x-small" do %>
+  <%= link_to cards_path(filter.as_params.except(:assignee_ids, :assignment_status, :card_ids, :creator_ids, :closer_ids, :stage_ids, :tag_ids, :terms, :indexed_by, :sorted_by, :creation, :closure)), class: "btn btn--remove txt-x-small" do %>
     <%= icon_tag "close" %>
     <span class="for-screen-reader">Clear all</span>
   <% end %>
diff --git a/app/views/filters/settings/_indexed_by.html.erb b/app/views/filters/settings/_indexed_by.html.erb
index 71058f4f0..085e85fca 100644
--- a/app/views/filters/settings/_indexed_by.html.erb
+++ b/app/views/filters/settings/_indexed_by.html.erb
@@ -1,15 +1,15 @@
 <% filter = user_filtering.filter %>
 
 <% if user_filtering.show_indexed_by? %>
-  <div class="quick-filter position-relative <%= "default-value" if filter.indexed_by.latest? %>" data-controller="dialog" data-action="keydown.esc->dialog#close click@document->dialog#closeOnClickOutside">
+  <div class="quick-filter position-relative <%= "default-value" if filter.indexed_by.all? %>" data-controller="dialog" data-action="keydown.esc->dialog#close click@document->dialog#closeOnClickOutside">
     <button class="btn input input--select flex-inline txt-x-small" data-action="click->dialog#open:stop">
       <span class="overflow-ellipsis">
-        <%= filter.indexed_by.humanize %>
+        <%= filter.indexed_by == "all" ? "Status" : filter.indexed_by.humanize %>
       </span>
     </button>
 
-    <%= filter_dialog "Sort by…" do %>
-      <strong class="popup__title txt-nowrap">Sort by…</strong>
+    <%= filter_dialog "Filter by…" do %>
+      <strong class="popup__title txt-nowrap">Filter by…</strong>
 
       <ul class="popup__list" data-filter-target="list">
         <% Filter::INDEXES.each do |index| %>
diff --git a/app/views/filters/settings/_sorted_by.html.erb b/app/views/filters/settings/_sorted_by.html.erb
new file mode 100644
index 000000000..d2fe0b247
--- /dev/null
+++ b/app/views/filters/settings/_sorted_by.html.erb
@@ -0,0 +1,40 @@
+<% filter = user_filtering.filter %>
+
+<% if user_filtering.show_sorted_by? %>
+  <div class="quick-filter position-relative <%= "default-value" if filter.sorted_by.latest? %>" data-controller="dialog" data-action="keydown.esc->dialog#close click@document->dialog#closeOnClickOutside">
+    <button class="btn input input--select flex-inline txt-x-small" data-action="click->dialog#open:stop">
+      <span class="overflow-ellipsis">
+        <%= filter.sorted_by.humanize %>
+      </span>
+    </button>
+
+    <%= filter_dialog "Sort by…" do %>
+      <strong class="popup__title txt-nowrap">Sort by…</strong>
+
+      <ul class="popup__list" data-filter-target="list">
+        <% Filter::SORTED_BY.each do |sort| %>
+          <li class="popup__group" data-navigable-list-target="item">
+            <%= form_with url: cards_path, method: :get, class: "full-width", data: { controller: "form" } do |form| %>
+              <% filter.as_params.except(:sorted_by).each do |key, value| %>
+                <%= filter_hidden_field_tag key, value %>
+              <% end %>
+
+              <%= hidden_field_tag :sorted_by, sort %>
+
+              <% if user_filtering.expanded? %>
+                <%= hidden_field_tag :expand_all, user_filtering.expanded? %>
+              <% end %>
+
+              <%= form.button type: "submit", class: "popup__item btn" do %>
+                <span class="overflow-ellipsis flex-item-grow"><%= sort.humanize %></span>
+                <% if filter.sorted_by == sort %>
+                  <%= icon_tag "check", class: "checked flex-item-justify-end" %>
+                <% end %>
+              <% end %>
+            <% end %>
+          </li>
+        <% end %>
+      </ul>
+    <% end %>
+  </div>
+<% end %>
\ No newline at end of file
diff --git a/app/views/filters/settings/_time_window.html.erb b/app/views/filters/settings/_time_window.html.erb
index f7f5a41cc..20590bd40 100644
--- a/app/views/filters/settings/_time_window.html.erb
+++ b/app/views/filters/settings/_time_window.html.erb
@@ -1,5 +1,5 @@
 <% filter = user_filtering.filter %>
-<% if user_filtering.show_time_window?(name) %>
+<% if filter.public_send("#{name}_window").present? %>
   <div class="quick-filter position-relative" data-controller="dialog" data-action="keydown.esc->dialog#close click@document->dialog#closeOnClickOutside">
     <button class="btn input input--select flex-inline txt-x-small" data-action="click->dialog#open:stop">
       <span class="overflow-ellipsis">
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 99a977965..11c712203 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -35,8 +35,8 @@
     </div>
 
     <div class="flex-inline center justify-center flex-wrap gap">
-      <%= link_to "Which cards are assigned to #{ Current.user == @user ? "you" : @user.first_name }?", cards_path(assignee_ids: [@user.id], indexed_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
-      <%= link_to "Which cards were added by #{ Current.user == @user ? "you" : @user.first_name }?", cards_path(creator_ids: [@user.id], indexed_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
+      <%= link_to "Which cards are assigned to #{ Current.user == @user ? "you" : @user.first_name }?", cards_path(assignee_ids: [@user.id], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
+      <%= link_to "Which cards were added by #{ Current.user == @user ? "you" : @user.first_name }?", cards_path(creator_ids: [@user.id], sorted_by: "newest"), class: "btn", data: { turbo_frame: "_top" } %>
     </div>
   </div>
 </div>
diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml
index f68bb6327..18d91f458 100644
--- a/test/fixtures/filters.yml
+++ b/test/fixtures/filters.yml
@@ -2,5 +2,5 @@ jz_assignments:
   creator: david
   tags: mobile
   assignees: jz
-  fields: <%= { indexed_by: :newest }.to_json %>
-  params_digest: <%= Filter.digest_params({ indexed_by: :newest, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %>
+  fields: <%= { indexed_by: :all, sorted_by: :newest }.to_json %>
+  params_digest: <%= Filter.digest_params({ indexed_by: :all, sorted_by: :newest, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }) %>
diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb
index 858f8c583..6dce00f62 100644
--- a/test/models/filter_test.rb
+++ b/test/models/filter_test.rb
@@ -53,7 +53,7 @@ class FilterTest < ActiveSupport::TestCase
 
   test "remembering equivalent filters" do
     assert_difference "Filter.count", +1 do
-      filter = users(:david).filters.remember(indexed_by: "latest", assignment_status: "unassigned", tag_ids: [ tags(:mobile).id ])
+      filter = users(:david).filters.remember(sorted_by: "latest", assignment_status: "unassigned", tag_ids: [ tags(:mobile).id ])
 
       assert_changes "filter.reload.updated_at" do
         assert_equal filter, users(:david).filters.remember(tag_ids: [ tags(:mobile).id ], assignment_status: "unassigned")
@@ -69,7 +69,7 @@ class FilterTest < ActiveSupport::TestCase
   end
 
   test "turning into params" do
-    filter = users(:david).filters.new indexed_by: "latest", tag_ids: "", assignee_ids: [ users(:jz).id ], collection_ids: [ collections(:writebook).id ]
+    filter = users(:david).filters.new sorted_by: "latest", tag_ids: "", assignee_ids: [ users(:jz).id ], collection_ids: [ collections(:writebook).id ]
     expected = { assignee_ids: [ users(:jz).id ], collection_ids: [ collections(:writebook).id ] }
     assert_equal expected, filter.as_params
   end
@@ -122,12 +122,12 @@ class FilterTest < ActiveSupport::TestCase
     filters(:jz_assignments).update!(stages: [], assignees: [], tags: [], collections: [ collections(:writebook) ])
     assert_equal "Newest", filters(:jz_assignments).summary
 
-    filters(:jz_assignments).update!(indexed_by: "stalled")
+    filters(:jz_assignments).update!(indexed_by: "stalled", sorted_by: "latest")
     assert_equal "Stalled", filters(:jz_assignments).summary
   end
 
   test "get a clone with some changed params" do
-    seed_filter = users(:david).filters.new indexed_by: "active", terms: [ "haggis" ]
+    seed_filter = users(:david).filters.new indexed_by: "all", terms: [ "haggis" ]
     filter = seed_filter.with(indexed_by: "closed")
 
     assert filter.indexed_by.closed?