Merge pull request #205 from basecamp/simplify-filters

Simplify filters

app/controllers/bubbles_controller.rb

@@ -34,7 +34,7 @@ class BubblesController < ApplicationController
 
   private
     def set_filter
-      @filter = Current.user.filters.build params.permit(*Filter::PERMITTED_PARAMS)
+      @filter = Current.user.filters.from_params params.permit(*Filter::PERMITTED_PARAMS)
     end
 
     def set_bubble

app/controllers/filters_controller.rb

@@ -3,7 +3,7 @@ class FiltersController < ApplicationController
 
   def create
     @filter = Current.user.filters.remember filter_params
-    redirect_to bubbles_path(@filter.to_params)
+    redirect_to bubbles_path(@filter.as_params)
   end
 
   def destroy
@@ -24,7 +24,7 @@ class FiltersController < ApplicationController
       if request.referer == root_url
         redirect_to root_path
       else
-        redirect_to bubbles_path(@filter.to_params)
+        redirect_to bubbles_path(@filter.as_params)
       end
     end
 end

app/models/filter.rb

@@ -5,26 +5,22 @@ class Filter < ApplicationRecord
   has_one :account, through: :creator
 
   class << self
+    def from_params(params)
+      find_by_params(params) || build(params)
+    end
+
     def remember(attrs)
       create!(attrs)
     rescue ActiveRecord::RecordNotUnique
-      find_by!(params_digest: digest_params(attrs)).tap(&:touch)
+      find_by_params(attrs).tap(&:touch)
     end
-
-    def digest_params(params)
-      Digest::MD5.hexdigest params.to_h.sort.to_json
-    end
-  end
-
-  def empty?
-    as_params.blank?
   end
 
   def bubbles
     @bubbles ||= begin
       result = creator.accessible_bubbles.indexed_by(indexed_by)
       result = result.active unless indexed_by.popped?
-      result = result.unassigned if assignments.unassigned?
+      result = result.unassigned if assignment_status.unassigned?
       result = result.assigned_to(assignees.ids) if assignees.present?
       result = result.assigned_by(assigners.ids) if assigners.present?
       result = result.in_bucket(buckets.ids) if buckets.present?
@@ -37,6 +33,10 @@ class Filter < ApplicationRecord
     end
   end
 
+  def empty?
+    self.class.normalize_params(as_params).blank?
+  end
+
   def cacheable?
     buckets.exists?
   end

app/models/filter/fields.rb

@@ -3,36 +3,39 @@ module Filter::Fields
 
   INDEXES = %w[ most_discussed most_boosted newest oldest popped ]
 
+  delegate :default_value?, to: :class
+
   class_methods do
-    def default_fields
+    def default_values
       { indexed_by: "most_active" }
     end
+
+    def default_value?(key, value)
+      default_values[key.to_sym].eql?(value)
+    end
   end
 
   included do
-    store_accessor :fields, :indexed_by, :assignments, :terms
+    store_accessor :fields, :assignment_status, :indexed_by, :terms
+
+    def assignment_status
+      super.to_s.inquiry
+    end
 
     def indexed_by
       (super || default_indexed_by).inquiry
     end
 
-    def assignments
-      super.to_s.inquiry
-    end
-
     def terms
       Array(super)
     end
   end
 
   def default_indexed_by
-    default_fields[:indexed_by]
+    self.class.default_values[:indexed_by]
   end
 
   def default_indexed_by?
-    indexed_by == default_indexed_by
+    default_value?(:indexed_by, indexed_by)
   end
-
-  private
-    delegate :default_fields, to: :class, private: true
 end

app/models/filter/params.rb

@@ -1,34 +1,44 @@
 module Filter::Params
   extend ActiveSupport::Concern
 
-  PERMITTED_PARAMS = [ :indexed_by, :assignments, bucket_ids: [], assignee_ids: [], assigner_ids: [], tag_ids: [], terms: [] ]
+  PERMITTED_PARAMS = [ :assignment_status, :indexed_by, assignee_ids: [], assigner_ids: [], bucket_ids: [], tag_ids: [], terms: [] ]
+
+  class_methods do
+    def find_by_params(params)
+      find_by params_digest: digest_params(params)
+    end
+
+    def digest_params(params)
+      Digest::MD5.hexdigest normalize_params(params).to_json
+    end
+
+    def normalize_params(params)
+      params.to_h.compact_blank.reject(&method(:default_value?)).sort
+    end
+  end
 
   included do
     before_save { self.params_digest = self.class.digest_params(as_params) }
   end
 
+  # +as_params+ uses `resource#ids` instead of `#resource_ids`
+  # because the latter won't work on unpersisted filters.
   def as_params
-    @as_params ||= {
-      terms: terms,
-      tag_ids: tags.ids,
-      indexed_by: indexed_by,
-      bucket_ids: buckets.ids,
-      assignments: assignments,
-      assignee_ids: assignees.ids,
-      assigner_ids: assigners.ids
-    }.reject { |k, v| default_fields[k] == v }.compact_blank
+    {}.tap do |params|
+      params[:indexed_by]        = indexed_by
+      params[:assignment_status] = assignment_status
+      params[:terms]             = terms
+      params[:tag_ids]           = tags.ids
+      params[:bucket_ids]        = buckets.ids
+      params[:assignee_ids]      = assignees.ids
+      params[:assigner_ids]      = assigners.ids
+    end.compact_blank.reject(&method(:default_value?))
   end
 
-  def to_params
-    ActionController::Parameters.new(as_params).permit(*PERMITTED_PARAMS).tap do |params|
-      params[:filter_id] = id if persisted?
-    end
-  end
-
-  def params_without(key, value)
-    to_params.tap do |params|
-      params[key].delete(value) if params[key].is_a?(Array)
-      params.delete(key) if params[key] == value
+  def as_params_without(key, value)
+    as_params.tap do |params|
+      params[key].delete(value.to_i) if params[key].is_a?(Array)
+      params.delete(key) if params[key] == value.to_s
     end
   end
 end

app/models/filter/resources.rb

@@ -17,6 +17,6 @@ module Filter::Resources
   end
 
   def buckets
-    creator.buckets.where id: bucket_ids
+    creator.buckets.where id: super.ids
   end
 end

app/models/filter/summarized.rb

@@ -3,10 +3,6 @@ module Filter::Summarized
     [ index_summary, tag_summary, assignee_summary, assigner_summary, terms_summary ].compact.to_sentence + " #{bucket_summary}"
   end
 
-  def plain_summary
-    summary.remove(/<\/?mark>/)
-  end
-
   private
     def index_summary
       indexed_by.humanize
@@ -21,7 +17,7 @@ module Filter::Summarized
     def assignee_summary
       if assignees.any?
         "assigned to #{assignees.pluck(:name).to_choice_sentence}"
-      elsif assignments.unassigned?
+      elsif assignment_status.unassigned?
         "assigned to no one"
       end
     end

app/views/bubbles/_filters.html.erb

@@ -17,32 +17,32 @@
     </button>
 
     <div class="flex-inline center align-center gap-half">
-      <%= filter_chip_tag filter.indexed_by.humanize, filter.params_without(:indexed_by, filter.indexed_by) unless filter.default_indexed_by? %>
+      <%= filter_chip_tag filter.indexed_by.humanize, filter.as_params_without(:indexed_by, filter.indexed_by) unless filter.default_indexed_by? %>
 
       <% filter.tags.each do |tag| %>
-        <%= filter_chip_tag tag.hashtag, filter.params_without(:tag_ids, tag.id) %>
+        <%= filter_chip_tag tag.hashtag, filter.as_params_without(:tag_ids, tag.id) %>
       <% end %>
 
       <% filter.assignees.each do |assignee| %>
-        <%= filter_chip_tag "for #{assignee.name}", filter.params_without(:assignee_ids, assignee.id) %>
+        <%= filter_chip_tag "for #{assignee.name}", filter.as_params_without(:assignee_ids, assignee.id) %>
       <% end %>
 
-      <% if filter.assignments.present? %>
-        <%= filter_chip_tag filter.assignments.humanize, filter.params_without(:assignments, filter.assignments) %>
+      <% if filter.assignment_status.present? %>
+        <%= filter_chip_tag filter.assignment_status.humanize, filter.as_params_without(:assignment_status, filter.assignment_status) %>
       <% end %>
 
       <% filter.assigners.each do |assigner| %>
-        <%= filter_chip_tag "by #{assigner.name}", filter.params_without(:assigner_ids, assigner.id) %>
+        <%= filter_chip_tag "by #{assigner.name}", filter.as_params_without(:assigner_ids, assigner.id) %>
       <% end %>
 
       <% if filter.buckets.many? %>
         <% filter.buckets.each do |bucket| %>
-          <%= filter_chip_tag "in #{bucket.name}", filter.params_without(:bucket_ids, bucket.id) %>
+          <%= filter_chip_tag "in #{bucket.name}", filter.as_params_without(:bucket_ids, bucket.id) %>
         <% end %>
       <% end %>
 
       <% filter.terms.each do |term| %>
-        <%= filter_chip_tag %Q("#{term}"), filter.params_without(:terms, term) %>
+        <%= filter_chip_tag %Q("#{term}"), filter.as_params_without(:terms, term) %>
       <% end %>
     </div>
   </div>
@@ -67,7 +67,7 @@
 
       <div class="pad fill-shade border-radius margin-block-end">
         <%= form_tag bubbles_path, class: "flex align-center gap input input--actor txt-small fill-white border-radius", method: :get do %>
-          <% @filter.to_params.each do |key, value| %>
+          <% @filter.as_params.each do |key, value| %>
             <%= filter_hidden_field_tag key, value %>
           <% end %>
 
@@ -140,8 +140,8 @@
           <menu class="filter__menu">
             <li class="filter__label"><strong>Assigned to…</strong></li>
             <li>
-              <%= label_tag "assignments_unassigned", class: "btn filter__button" do %>
-                <%= check_box_tag "assignments", "unassigned", filter.assignments.unassigned?, id: "assignments_unassigned",
+              <%= label_tag "assignment_status_unassigned", class: "btn filter__button" do %>
+                <%= check_box_tag "assignment_status", "unassigned", filter.assignment_status.unassigned?, id: "assignment_status_unassigned",
                     hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignee_ids[],assigner_ids[]" } %>
                 <span>No one</span>
                 <%= image_tag "close.svg", aria: { hidden: true }, size: 24 %>
@@ -150,7 +150,7 @@
             <li>
               <%= label_tag "assignee_ids_me", class: "btn filter__button" do %>
                 <%= check_box_tag "assignee_ids[]", Current.user.id, filter.assignees.include?(Current.user), id: "assignee_ids_me",
-                    hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %>
+                    hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignment_status" } %>
                 <span>Me</span>
                 <%= image_tag "close.svg", aria: { hidden: true }, size: 24 %>
               <% end %>
@@ -159,7 +159,7 @@
               <li>
                 <%= label_tag "assignee_ids_#{user.id}", user.name, class: "btn filter__button" do %>
                   <%= check_box_tag "assignee_ids[]", user.id, filter.assignees.include?(user), id: "assignee_ids_#{user.id}",
-                      hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %>
+                      hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignment_status" } %>
                   <span><%= user.name %></span>
                   <%= image_tag "close.svg", aria: { hidden: true }, size: 24 %>
                 <% end %>
@@ -172,7 +172,7 @@
             <li>
               <%= label_tag "assigner_ids_me", class: "btn filter__button" do %>
                 <%= check_box_tag "assigner_ids[]", Current.user.id, filter.assigners.include?(Current.user), id: "assigner_ids_me",
-                    hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %>
+                    hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignment_status" } %>
                 <span>Me</span>
                 <%= image_tag "close.svg", aria: { hidden: true }, size: 24 %>
               <% end %>
@@ -181,7 +181,7 @@
               <li>
                 <%= label_tag "assigner_ids_#{user.id}", class: "btn filter__button" do %>
                   <%= check_box_tag "assigner_ids[]", user.id, filter.assigners.include?(user), id: "assigner_ids_#{user.id}",
-                      hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %>
+                      hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignment_status" } %>
                   <span><%= user.name %></span>
                   <%= image_tag "close.svg", aria: { hidden: true }, size: 24 %>
                 <% end %>

app/views/bubbles/index.html.erb

@@ -1,4 +1,4 @@
-<% @page_title = @filter.plain_summary %>
+<% @page_title = @filter.summary %>
 
 <% turbo_refreshes_with method: :morph, scroll: :preserve %>
 <%= render "filters/broadcasts", filter: @filter %>
@@ -25,7 +25,7 @@
   </nav>
 <% end %>
 
-<section class="windshield flex-inline flex-wrap gap justify-center align-end" style="view-transition-name: windshield_<%= params[:filter_id] %>">
+<section class="windshield flex-inline flex-wrap gap justify-center align-end" style="view-transition-name: windshield_<%= @filter.id %>">
   <% if @bubbles.any? %>
     <%= render partial: "bubbles/bubble", collection: @bubbles.limit(10), cached: true %>
   <% else %>

app/views/bubbles/list/_bubble.html.erb

@@ -16,7 +16,7 @@
 
   <div class="flex align-center gap-half flex-item-no-shrink">
     <% bubble.assignees.each do |assignee| %>
-      <%= link_to bubbles_path(@filter.to_params.merge(assignee_ids: [ assignee.id ])), class: "btn avatar txt-x-small" do %>
+      <%= link_to bubbles_path(@filter.as_params.merge(assignee_ids: [ assignee.id ])), class: "btn avatar txt-x-small" do %>
         <%= avatar_image_tag assignee, loading: :lazy %>
       <% end %>
     <% end %>

app/views/filters/_filter.html.erb

@@ -1,6 +1,6 @@
 <% cache_if filter.cacheable?, filter do %>
   <li class="bucket flex flex-column txt-align-center max-width position-relative">
-    <%= link_to bubbles_path(**filter.to_params), class: "border border-radius margin-block-end-half windshield__container flex justify-center align-center position-relative" do %>
+    <%= link_to bubbles_path(**filter.as_params), class: "border border-radius margin-block-end-half windshield__container flex justify-center align-center position-relative" do %>
       <div class="windshield bucket__windshield flex flex-wrap gap justify-center align-end" style="view-transition-name: windshield_<%= filter.id %>">
         <% filter.bubbles.ordered_by_activity.limit(10).each do |bubble| %>
           <div class="bubble" style="--bubble-color: <%= bubble.color %>; <%= bubble_rotation(bubble) %> <%= bubble_size(bubble) %>">
@@ -11,7 +11,7 @@
     <% end %>
 
     <div class="flex align-center justify-center flex-column flex-wrap center gap-half position-relative full-width">
-      <%= link_to bubbles_path(**filter.to_params), class: "txt-ink" do %>
+      <%= link_to bubbles_path(**filter.as_params), class: "txt-ink" do %>
         <strong class="txt-large"><%= image_tag "filter.svg", aria: { hidden: true }, size: 30, class: "flex-inline", style: "vertical-align: bottom" %> <%= filter.summary %></strong>
       <% end %>
       <%= button_to filter_path(filter), method: :delete, class: "btn txt-small btn--negative bucket__button", data: { turbo_confirm: "Are you sure you want to delete this filter?" } do %>

test/controllers/bubbles_controller_test.rb

@@ -11,7 +11,7 @@ class BubblesControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "filtered index" do
-    get bubbles_url(filters(:jz_assignments).to_params.merge(term: "haggis"))
+    get bubbles_url(filters(:jz_assignments).as_params.merge(term: "haggis"))
     assert_response :success
   end
 

test/controllers/filters_controller_test.rb

@@ -9,16 +9,16 @@ class FiltersControllerTest < ActionDispatch::IntegrationTest
     assert_difference "users(:david).filters.count", +1 do
       post filters_url, params: {
         indexed_by: "popped",
-        assignments: "unassigned",
+        assignment_status: "unassigned",
         tag_ids: [ tags(:mobile).id ],
         assignee_ids: [ users(:jz).id ],
         bucket_ids: [ buckets(:writebook).id ] }
     end
-    assert_redirected_to bubbles_path(Filter.last.to_params)
+    assert_redirected_to bubbles_path(Filter.last.as_params)
 
     filter = Filter.last
     assert_predicate filter.indexed_by, :popped?
-    assert_predicate filter.assignments, :unassigned?
+    assert_predicate filter.assignment_status, :unassigned?
     assert_equal [ tags(:mobile) ], filter.tags
     assert_equal [ users(:jz) ], filter.assignees
     assert_equal [ buckets(:writebook) ], filter.buckets

test/models/filter_test.rb

@@ -1,16 +1,6 @@
 require "test_helper"
 
 class FilterTest < ActiveSupport::TestCase
-  test "remembering" do
-    assert_difference "users(:david).filters.count", +1 do
-      filter = users(:david).filters.remember(indexed_by: "most_boosted", tag_ids: [ tags(:mobile).id ])
-
-      assert_changes "filter.reload.updated_at" do
-        assert_equal filter, users(:david).filters.remember(indexed_by: "most_boosted", tag_ids: [ tags(:mobile).id ])
-      end
-    end
-  end
-
   test "bubbles" do
     Current.set session: sessions(:david) do
       @new_bucket = accounts("37s").buckets.create! name: "Inaccessible Bucket"
@@ -28,7 +18,7 @@ class FilterTest < ActiveSupport::TestCase
     filter = users(:david).filters.new assigner_ids: [ users(:david).id ], tag_ids: [ tags(:mobile).id ]
     assert_equal [ bubbles(:layout) ], filter.bubbles
 
-    filter = users(:david).filters.new assignments: "unassigned", bucket_ids: [ @new_bucket.id ]
+    filter = users(:david).filters.new assignment_status: "unassigned", bucket_ids: [ @new_bucket.id ]
     assert_equal [ @new_bubble ], filter.bubbles
 
     filter = users(:david).filters.new terms: [ "haggis" ]
@@ -48,37 +38,37 @@ class FilterTest < ActiveSupport::TestCase
     assert_empty users(:david).filters.new(bucket_ids: [ buckets(:writebook).id ]).bubbles
   end
 
-  test "turning into params" do
-    expected = { indexed_by: "most_discussed", tag_ids: [ tags(:mobile).id ], assignee_ids: [ users(:jz).id ], filter_id: filters(:jz_assignments).id }
-    assert_equal expected.stringify_keys, filters(:jz_assignments).to_params.to_h
+  test "remembering equivalent filters" do
+    assert_difference "Filter.count", +1 do
+      filter = users(:david).filters.remember(indexed_by: "most_active", 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")
+      end
+    end
   end
 
-  test "param sanitization" do
+  test "remembering equivalent filters for different users" do
+    assert_difference "Filter.count", +2 do
+      users(:david).filters.remember(assignment_status: "unassigned", tag_ids: [ tags(:mobile).id ])
+      users(:kevin).filters.remember(assignment_status: "unassigned", tag_ids: [ tags(:mobile).id ])
+    end
+  end
+
+  test "turning into params" do
     filter = users(:david).filters.new indexed_by: "most_active", tag_ids: "", assignee_ids: [ users(:jz).id ], bucket_ids: [ buckets(:writebook).id ]
     expected = { assignee_ids: [ users(:jz).id ], bucket_ids: [ buckets(:writebook).id ] }
     assert_equal expected, filter.as_params
   end
 
-  test "cacheable" do
+  test "cacheability" do
     assert_not filters(:jz_assignments).cacheable?
     assert users(:david).filters.create!(bucket_ids: [ buckets(:writebook).id ]).cacheable?
   end
 
-  test "default fields" do
-    assert_equal "most_active", users(:david).filters.new.indexed_by
-  end
-
-  test "indexed by" do
-    assert_predicate users(:david).filters.new(indexed_by: "most_discussed").indexed_by, :most_discussed?
-  end
-
-  test "assignments" do
-    assert_predicate users(:david).filters.new(assignments: "unassigned").assignments, :unassigned?
-  end
-
   test "terms" do
-    assert_empty users(:david).filters.new.terms
-    assert_includes users(:david).filters.new(terms: [ "haggis" ]).terms, "haggis"
+    assert_equal [], users(:david).filters.new.terms
+    assert_equal [ "haggis" ], users(:david).filters.new(terms: [ "haggis" ]).terms
   end
 
   test "resource removal" do
@@ -92,7 +82,7 @@ class FilterTest < ActiveSupport::TestCase
     assert_changes "filter.reload.updated_at" do
       tags(:mobile).destroy!
     end
-    assert_nil Filter.find(filter.id).as_params["tag_ids"] # can't reload because as_params is memoized
+    assert_nil filter.reload.as_params[:tag_ids]
 
     assert_changes "Filter.exists?(filter.id)" do
       buckets(:writebook).destroy!
@@ -112,23 +102,19 @@ class FilterTest < ActiveSupport::TestCase
     assert_equal "Most discussed, tagged #Mobile, and assigned to JZ in all projects", filters(:jz_assignments).summary
   end
 
-  test "plain summary" do
-    assert_equal "Most discussed, tagged #Mobile, and assigned to JZ in all projects", filters(:jz_assignments).plain_summary
-  end
-
   test "params without a key-value pair" do
     filter = users(:david).filters.new indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ]
 
     expected = { indexed_by: "most_discussed", assignee_ids: [ users(:kevin).id ] }
-    assert_equal expected.stringify_keys, filter.params_without(:assignee_ids, users(:jz).id).to_h
+    assert_equal expected, filter.as_params_without(:assignee_ids, users(:jz).id).to_h
 
     expected = { assignee_ids: [ users(:jz).id, users(:kevin).id ] }
-    assert_equal expected.stringify_keys, filter.params_without(:indexed_by, "most_discussed").to_h
+    assert_equal expected, filter.as_params_without(:indexed_by, "most_discussed").to_h
 
     expected = { indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ] }
-    assert_equal expected.stringify_keys, filter.params_without(:indexed_by, "most_active").to_h
+    assert_equal expected, filter.as_params_without(:indexed_by, "most_active").to_h
 
     expected = { indexed_by: "most_discussed", assignee_ids: [ users(:jz).id, users(:kevin).id ] }
-    assert_equal expected.stringify_keys, filter.params_without(:assignee_ids, users(:david).id).to_h
+    assert_equal expected, filter.as_params_without(:assignee_ids, users(:david).id).to_h
   end
 end