From f2706d0f74965957ff997ca3bf54905d0efc2d06 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Tue, 5 Nov 2024 13:04:05 -0600 Subject: [PATCH 01/23] Unnest bubble filters --- app/controllers/bubbles_controller.rb | 24 +++----- app/controllers/buckets/views_controller.rb | 29 --------- app/controllers/buckets_controller.rb | 7 ++- app/controllers/filters_controller.rb | 30 +++++++++ app/helpers/filters_helper.rb | 61 +++++-------------- .../controllers/back_navigation_controller.js | 13 ++++ .../controllers/query_merger_controller.js | 9 +++ app/models/bubble.rb | 24 ++------ app/models/bubble/searchable.rb | 8 +-- app/models/bucket.rb | 2 +- app/models/bucket/bubble_filter.rb | 34 ----------- app/models/bucket/filterable.rb | 5 -- app/models/bucket/view.rb | 27 -------- app/models/bucket/view/assignees.rb | 12 ---- app/models/bucket/view/order_by.rb | 15 ----- app/models/bucket/view/status.rb | 10 --- app/models/bucket/view/summarized.rb | 24 -------- app/models/bucket/view/tags.rb | 12 ---- app/models/bucket/views.rb | 7 --- app/models/filter.rb | 53 ++++++++++++++++ app/models/filter/assignments.rb | 9 +++ app/models/filter/buckets.rb | 11 ++++ app/models/filter/indexes.rb | 7 +++ app/models/filter/summarized.rb | 36 +++++++++++ app/models/filter/tags.rb | 11 ++++ app/models/pop.rb | 2 +- app/models/user.rb | 7 ++- app/models/user/viewer.rb | 7 --- app/views/bubbles/_filters.html.erb | 26 +++----- app/views/bubbles/_tags.html.erb | 2 +- ...signees.html.erb => _assignments.html.erb} | 14 +++-- app/views/bubbles/filters/_bookmark.html.erb | 16 +++++ .../filters/_bucket_view_form.html.erb | 19 ------ app/views/bubbles/filters/_buckets.html.erb | 15 +++++ .../{_bucket.html.erb => _index.html.erb} | 8 +-- app/views/bubbles/filters/_main.html.erb | 17 ------ app/views/bubbles/filters/_tags.html.erb | 12 ++-- app/views/bubbles/index.html.erb | 26 +++++--- app/views/bubbles/list/_bubble.html.erb | 6 +- app/views/bubbles/show.html.erb | 4 +- app/views/buckets/_bucket.html.erb | 41 ++++++------- app/views/buckets/_view.html.erb | 29 --------- app/views/buckets/edit.html.erb | 4 +- app/views/buckets/index.html.erb | 13 ++-- app/views/filters/_filter.html.erb | 26 ++++++++ config/routes.rb | 7 +-- ...05181312_rename_bucket_views_to_filters.rb | 14 +++++ db/schema.rb | 20 +++--- test/fixtures/filters.yml | 3 + 49 files changed, 381 insertions(+), 437 deletions(-) delete mode 100644 app/controllers/buckets/views_controller.rb create mode 100644 app/controllers/filters_controller.rb create mode 100644 app/javascript/controllers/back_navigation_controller.js create mode 100644 app/javascript/controllers/query_merger_controller.js delete mode 100644 app/models/bucket/bubble_filter.rb delete mode 100644 app/models/bucket/filterable.rb delete mode 100644 app/models/bucket/view.rb delete mode 100644 app/models/bucket/view/assignees.rb delete mode 100644 app/models/bucket/view/order_by.rb delete mode 100644 app/models/bucket/view/status.rb delete mode 100644 app/models/bucket/view/summarized.rb delete mode 100644 app/models/bucket/view/tags.rb delete mode 100644 app/models/bucket/views.rb create mode 100644 app/models/filter.rb create mode 100644 app/models/filter/assignments.rb create mode 100644 app/models/filter/buckets.rb create mode 100644 app/models/filter/indexes.rb create mode 100644 app/models/filter/summarized.rb create mode 100644 app/models/filter/tags.rb delete mode 100644 app/models/user/viewer.rb rename app/views/bubbles/filters/{_assignees.html.erb => _assignments.html.erb} (52%) create mode 100644 app/views/bubbles/filters/_bookmark.html.erb delete mode 100644 app/views/bubbles/filters/_bucket_view_form.html.erb create mode 100644 app/views/bubbles/filters/_buckets.html.erb rename app/views/bubbles/filters/{_bucket.html.erb => _index.html.erb} (63%) delete mode 100644 app/views/bubbles/filters/_main.html.erb delete mode 100644 app/views/buckets/_view.html.erb create mode 100644 app/views/filters/_filter.html.erb create mode 100644 db/migrate/20241105181312_rename_bucket_views_to_filters.rb create mode 100644 test/fixtures/filters.yml diff --git a/app/controllers/bubbles_controller.rb b/app/controllers/bubbles_controller.rb index 741e61c91..6d9306e7a 100644 --- a/app/controllers/bubbles_controller.rb +++ b/app/controllers/bubbles_controller.rb @@ -1,12 +1,14 @@ class BubblesController < ApplicationController include BucketScoped + skip_before_action :set_bucket, only: :index + + before_action :set_filter, only: :index before_action :set_bubble, only: %i[ show edit update ] - before_action :clear_assignees, only: :index - before_action :set_view, :set_filter, only: :index def index @bubbles = @filter.bubbles + @bubbles = @bubbles.mentioning(params[:term]) if params[:term].present? end def new @@ -31,6 +33,10 @@ class BubblesController < ApplicationController end private + def set_filter + @filter = Current.user.filters.build params: params.permit(*Filter::KNOWN_PARAMS) + end + def set_bubble @bubble = @bucket.bubbles.find params[:id] end @@ -38,18 +44,4 @@ class BubblesController < ApplicationController def bubble_params params.expect(bubble: [ :title, :color, :due_on, :image, tag_ids: [] ]) end - - def clear_assignees - params[:assignee_ids] = nil if helpers.unassigned_filter_activated? - end - - def set_view - @view = @bucket.views.find_by(creator: Current.user, id: params[:view_id]) if params[:view_id] - @view ||= @bucket.views.find_by(creator: Current.user, filters: helpers.bubble_filter_params.to_h) - params[:view_id] = @view&.id - end - - def set_filter - @filter = @bucket.bubble_filter_from helpers.view_filter_params - end end diff --git a/app/controllers/buckets/views_controller.rb b/app/controllers/buckets/views_controller.rb deleted file mode 100644 index 51484d818..000000000 --- a/app/controllers/buckets/views_controller.rb +++ /dev/null @@ -1,29 +0,0 @@ -class Buckets::ViewsController < ApplicationController - include BucketScoped - - before_action :set_view, only: %i[ update destroy ] - - def create - @view = @bucket.views.create! filters: filter_params - redirect_to bucket_bubbles_path(@bucket, **filter_params.merge(view_id: @view.id)), notice: "✓" - end - - def update - @view.update! filters: filter_params - redirect_to bucket_bubbles_path(@bucket, **filter_params.merge(view_id: @view.id)), notice: "✓" - end - - def destroy - @view.destroy - redirect_to buckets_path - end - - private - def set_view - @view = @bucket.views.find_by creator: Current.user, id: params[:id] - end - - def filter_params - helpers.bubble_filter_params.to_h - end -end diff --git a/app/controllers/buckets_controller.rb b/app/controllers/buckets_controller.rb index c12843fb1..8320b0735 100644 --- a/app/controllers/buckets_controller.rb +++ b/app/controllers/buckets_controller.rb @@ -2,7 +2,8 @@ class BucketsController < ApplicationController before_action :set_bucket, except: %i[ index new create ] def index - @buckets = (Current.user.buckets.all + Current.user.bucket_views.all).sort_by(&:updated_at).reverse! + @filters = Current.user.filters.all + @buckets = Current.user.buckets.all end def new @@ -11,7 +12,7 @@ class BucketsController < ApplicationController def create @bucket = Current.account.buckets.create! bucket_params - redirect_to bucket_bubbles_path(@bucket) + redirect_to bubbles_path(bucket_ids: @bucket) end def edit @@ -23,7 +24,7 @@ class BucketsController < ApplicationController @bucket.update! bucket_params @bucket.accesses.revise granted: grantees, revoked: revokees - redirect_to bucket_bubbles_path(@bucket) + redirect_to bubbles_path(bucket_ids: @bucket) end def destroy diff --git a/app/controllers/filters_controller.rb b/app/controllers/filters_controller.rb new file mode 100644 index 000000000..7683ce0c5 --- /dev/null +++ b/app/controllers/filters_controller.rb @@ -0,0 +1,30 @@ +class FiltersController < ApplicationController + before_action :set_filter, only: :destroy + + def create + @filter = Current.user.filters.create_or_find_by!(params: filter_params).tap(&:touch) + redirect_to bubbles_path(@filter.to_params) + end + + def destroy + @filter.destroy! + redirect_after_destroy + end + + private + def set_filter + @filter = Current.user.filters.find params[:id] + end + + def filter_params + params.permit(*Filter::KNOWN_PARAMS).compact_blank + end + + def redirect_after_destroy + if request.referer == root_url + redirect_to root_path + else + redirect_to bubbles_path(@filter.to_params) + end + end +end diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index 15919f416..e4e02db13 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -1,61 +1,32 @@ module FiltersHelper - def main_filter_text - (Bucket::View::ORDERS[params[:order_by]] || Bucket::View::STATUSES[params[:status]] || Bubble.default_order_by).humanize + def bubble_filters_heading(filter, &) + tag.h1 class: "txt-large flex align-center gap-half", + style: token_list("margin-inline-end: calc(var(--btn-size) / -2);": filter.savable?), & end - def tag_filter_text(filter) - if filter.tags - filter.tags.map(&:hashtag).to_choice_sentence + def buckets_filter_text(filter) + if filter.buckets.any? + filter.buckets.pluck(:name).to_choice_sentence else - "any tag" + "all projects" end end - def assignee_filter_text(filter) - if filter.assignees + def assignments_filter_text(filter) + if filter.assignees.present? "assigned to #{filter.assignees.pluck(:name).to_choice_sentence}" + elsif filter.assignments.unassigned? + "assigned to no one" else "assigned to anyone" end end - # `#bubble_filter_params` is memoized to avoid spam in logs about unpermitted params - def bubble_filter_params - @bubble_filter_params ||= params.permit :order_by, :status, assignee_ids: [], tag_ids: [] - end - - # `#view_filter_params` is memoized to avoid spam in logs about unpermitted params - def view_filter_params - @view_filter_params ||= bubble_filter_params.merge params.permit(:term, :view_id) - end - - def unassigned_filter_activated? - params[:status] == "unassigned" - end - - def default_filters? - bubble_filter_params.values.all?(&:blank?) || bubble_filter_params.to_h == Bucket::View.default_filters - end - - def bubble_filter_form_tag(path, method:, id: nil) - form_tag path, method: method, id: id do - yield if block_given? - - if params[:order_by].present? - concat hidden_field_tag(:order_by, params[:order_by]) - end - - if params[:status].present? - concat hidden_field_tag(:status, params[:status]) - end - - Array(params[:assignee_ids]).each do |assignee_id| - concat hidden_field_tag("assignee_ids[]", assignee_id, id: nil) - end - - Array(params[:tag_ids]).each do |tag_id| - concat hidden_field_tag("tag_ids[]", tag_id, id: nil) - end + def tags_filter_text(filter) + if filter.tags.present? + filter.tags.map(&:hashtag).to_choice_sentence + else + "any tag" end end end diff --git a/app/javascript/controllers/back_navigation_controller.js b/app/javascript/controllers/back_navigation_controller.js new file mode 100644 index 000000000..8fad83419 --- /dev/null +++ b/app/javascript/controllers/back_navigation_controller.js @@ -0,0 +1,13 @@ +import { Controller } from "@hotwired/stimulus" + +export default class extends Controller { + static values = { fallbackDestination: String } + + connect() { + if (history.state.turbo?.restorationIndex > 0) { + this.element.href = "javascript:history.back()" + } else { + this.element.href = this.fallbackDestinationValue + } + } +} diff --git a/app/javascript/controllers/query_merger_controller.js b/app/javascript/controllers/query_merger_controller.js new file mode 100644 index 000000000..2fd17d65d --- /dev/null +++ b/app/javascript/controllers/query_merger_controller.js @@ -0,0 +1,9 @@ +import { Controller } from "@hotwired/stimulus" + +export default class extends Controller { + merge({ params: { key, value } }) { + const url = new URL(window.location.href) + url.searchParams.set(key, value) + Turbo.visit(url) + } +} diff --git a/app/models/bubble.rb b/app/models/bubble.rb index 76e225fb4..a52cb6bb1 100644 --- a/app/models/bubble.rb +++ b/app/models/bubble.rb @@ -1,7 +1,7 @@ class Bubble < ApplicationRecord include Assignable, Boostable, Colored, Eventable, Messages, Poppable, Searchable, Staged, Taggable - belongs_to :bucket + belongs_to :bucket, touch: true belongs_to :creator, class_name: "User", default: -> { Current.user } has_one_attached :image, dependent: :purge_later @@ -10,6 +10,7 @@ class Bubble < ApplicationRecord scope :reverse_chronologically, -> { order created_at: :desc, id: :desc } scope :chronologically, -> { order created_at: :asc, id: :asc } + scope :in_bucket, ->(bucket) { where bucket: bucket } # FIXME: Compute activity and comment count at write time so it's easier to query for. scope :left_joins_comments, -> do @@ -22,29 +23,14 @@ class Bubble < ApplicationRecord left_joins_comments.select("bubbles.*, COUNT(comments.id) AS comment_count").group(:id).order("comment_count DESC") end - # FIXME: `status` implies an enum. Consider a name change. - scope :with_status, ->(status) do - status = status.presence_in %w[ popped active unassigned ] - public_send(status) if status - end - - scope :ordered_by, ->(order) do - case order + scope :indexed_by, ->(index) do + case index when "most_active" then ordered_by_activity when "most_discussed" then ordered_by_comments when "most_boosted" then ordered_by_boosts when "newest" then reverse_chronologically when "oldest" then chronologically - end - end - - class << self - def default_order_by - "most_active" - end - - def default_status - "active" + when "popped" then popped end end diff --git a/app/models/bubble/searchable.rb b/app/models/bubble/searchable.rb index 5cd3128d4..8132ae136 100644 --- a/app/models/bubble/searchable.rb +++ b/app/models/bubble/searchable.rb @@ -7,11 +7,9 @@ module Bubble::Searchable searchable_by :title, using: :bubbles_search_index scope :mentioning, ->(query) do - if query = query.presence - bubbles = search(query).select(:id).to_sql - comments = Comment.search(query).select(:id).to_sql - left_joins(:messages).where("bubbles.id in (#{bubbles}) or messages.messageable_id in (#{comments})").distinct - end + bubbles = Bubble.search(query).select(:id).to_sql + comments = Comment.search(query).select(:id).to_sql + left_joins(:messages).where("bubbles.id in (#{bubbles}) or messages.messageable_id in (#{comments})").distinct end end end diff --git a/app/models/bucket.rb b/app/models/bucket.rb index 728cae12d..f2476b761 100644 --- a/app/models/bucket.rb +++ b/app/models/bucket.rb @@ -1,5 +1,5 @@ class Bucket < ApplicationRecord - include Accessible, Filterable, Views + include Accessible belongs_to :account belongs_to :creator, class_name: "User", default: -> { Current.user } diff --git a/app/models/bucket/bubble_filter.rb b/app/models/bucket/bubble_filter.rb deleted file mode 100644 index ca44a9852..000000000 --- a/app/models/bucket/bubble_filter.rb +++ /dev/null @@ -1,34 +0,0 @@ -class Bucket::BubbleFilter - def initialize(bucket, params = {}) - @bucket = bucket - @status = params["status"] - @order_by = params["order_by"] - @term = params["term"] - @tag_ids = params["tag_ids"] - @assignee_ids = params["assignee_ids"] - end - - def bubbles - @bubbles ||= begin - result = bucket.bubbles - result = result.ordered_by(order_by || Bubble.default_order_by) - result = result.with_status(status || Bubble.default_status) - result = result.tagged_with(tags) if tags - result = result.assigned_to(assignees) if assignees - result = result.mentioning(term) if term - result - end - end - - def tags - @tags ||= account.tags.where(id: tag_ids) if tag_ids - end - - def assignees - @assignees ||= account.users.where(id: assignee_ids) if assignee_ids - end - - private - attr_reader :bucket, :status, :order_by, :term, :tag_ids, :assignee_ids - delegate :account, to: :bucket, private: true -end diff --git a/app/models/bucket/filterable.rb b/app/models/bucket/filterable.rb deleted file mode 100644 index c5f665aac..000000000 --- a/app/models/bucket/filterable.rb +++ /dev/null @@ -1,5 +0,0 @@ -module Bucket::Filterable - def bubble_filter_from(params = {}) - Bucket::BubbleFilter.new self, params - end -end diff --git a/app/models/bucket/view.rb b/app/models/bucket/view.rb deleted file mode 100644 index 01f29d3a1..000000000 --- a/app/models/bucket/view.rb +++ /dev/null @@ -1,27 +0,0 @@ -class Bucket::View < ApplicationRecord - include Assignees, OrderBy, Status, Summarized, Tags - - belongs_to :creator, class_name: "User", default: -> { Current.user } - belongs_to :bucket - - has_one :account, through: :creator - - validate :must_not_be_the_default_view - - class << self - def default_filters - { "order_by" => "most_active" } - end - end - - def bubbles - @bubbles ||= bucket.bubble_filter_from(filters).bubbles - end - - private - def must_not_be_the_default_view - if filters.values.all?(&:blank?) || filters == self.class.default_filters - errors.add :base, "must be different than the default view" - end - end -end diff --git a/app/models/bucket/view/assignees.rb b/app/models/bucket/view/assignees.rb deleted file mode 100644 index 4cb5128f1..000000000 --- a/app/models/bucket/view/assignees.rb +++ /dev/null @@ -1,12 +0,0 @@ -module Bucket::View::Assignees - extend ActiveSupport::Concern - - included do - store_accessor :filters, :assignee_ids - end - - private - def assignees - @assignees ||= account.users.where id: assignee_ids - end -end diff --git a/app/models/bucket/view/order_by.rb b/app/models/bucket/view/order_by.rb deleted file mode 100644 index 49d87a29e..000000000 --- a/app/models/bucket/view/order_by.rb +++ /dev/null @@ -1,15 +0,0 @@ -module Bucket::View::OrderBy - extend ActiveSupport::Concern - - included do - store_accessor :filters, :order_by - end - - private - ORDERS = { - "most_active" => "most active", - "most_discussed" => "most discussed", - "most_boosted" => "most boosted", - "newest" => "newest", - "oldest" => "oldest" } -end diff --git a/app/models/bucket/view/status.rb b/app/models/bucket/view/status.rb deleted file mode 100644 index 64d95f04a..000000000 --- a/app/models/bucket/view/status.rb +++ /dev/null @@ -1,10 +0,0 @@ -module Bucket::View::Status - extend ActiveSupport::Concern - - included do - store_accessor :filters, :status - end - - private - STATUSES = %w[ unassigned popped ].index_by(&:itself) -end diff --git a/app/models/bucket/view/summarized.rb b/app/models/bucket/view/summarized.rb deleted file mode 100644 index 46cdf70e3..000000000 --- a/app/models/bucket/view/summarized.rb +++ /dev/null @@ -1,24 +0,0 @@ -module Bucket::View::Summarized - def summary - [ order_by_summary, status_summary, tag_summary, assignee_summary ].compact.to_sentence.upcase_first - end - - private - delegate :to_sentence, to: "ApplicationController.helpers", private: true - - def order_by_summary - Bucket::View::ORDERS.fetch order_by, nil - end - - def status_summary - Bucket::View::STATUSES.fetch status, nil - end - - def assignee_summary - "assigned to #{assignees.pluck(:name).to_choice_sentence}" if assignees.any? - end - - def tag_summary - "tagged #{tags.map(&:hashtag).to_choice_sentence}" if tags.any? - end -end diff --git a/app/models/bucket/view/tags.rb b/app/models/bucket/view/tags.rb deleted file mode 100644 index 0b92d65ff..000000000 --- a/app/models/bucket/view/tags.rb +++ /dev/null @@ -1,12 +0,0 @@ -module Bucket::View::Tags - extend ActiveSupport::Concern - - included do - store_accessor :filters, :tag_ids - end - - private - def tags - @tags ||= account.tags.where id: tag_ids - end -end diff --git a/app/models/bucket/views.rb b/app/models/bucket/views.rb deleted file mode 100644 index ab9cba2c8..000000000 --- a/app/models/bucket/views.rb +++ /dev/null @@ -1,7 +0,0 @@ -module Bucket::Views - extend ActiveSupport::Concern - - included do - has_many :views, dependent: :delete_all - end -end diff --git a/app/models/filter.rb b/app/models/filter.rb new file mode 100644 index 000000000..b20a47d03 --- /dev/null +++ b/app/models/filter.rb @@ -0,0 +1,53 @@ +class Filter < ApplicationRecord + include Assignments, Buckets, Indexes, Summarized, Tags + + KNOWN_PARAMS = %i[ indexed_by bucket_ids assignments tag_ids ] + + belongs_to :creator, class_name: "User", default: -> { Current.user } + has_one :account, through: :creator + + class << self + def default_params + { "indexed_by" => "most_active" } + end + end + + def bubbles + @bubbles ||= begin + result = creator.accessible_bubbles.indexed_by(indexed_by) + result = result.active unless indexed_by.popped? + result = result.in_bucket(buckets) if buckets.present? + result = result.tagged_with(tags) if tags.present? + result = result.unassigned if assignments.unassigned? + result = result.assigned_to(assignees) if assignees.present? + result + end + end + + def to_params + ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| + params[:filter_id] = id if persisted? + end + end + + def savable? + !bucket_default? + end + + def cacheable? + buckets.exists? + end + + def cache_key + ActiveSupport::Cache.expand_cache_key buckets.cache_key_with_version, super + end + + private + def bucket_default? + non_default_params.keys == %w[ bucket_ids ] && buckets.one? + end + + def non_default_params + params.reject { |k, v| self.class.default_params[k] == v } + end +end diff --git a/app/models/filter/assignments.rb b/app/models/filter/assignments.rb new file mode 100644 index 000000000..43bcb90ad --- /dev/null +++ b/app/models/filter/assignments.rb @@ -0,0 +1,9 @@ +module Filter::Assignments + def assignments + params["assignments"].to_s.inquiry + end + + def assignees + @assignees ||= account.users.where id: assignments.split(",") + end +end diff --git a/app/models/filter/buckets.rb b/app/models/filter/buckets.rb new file mode 100644 index 000000000..e444c8d1a --- /dev/null +++ b/app/models/filter/buckets.rb @@ -0,0 +1,11 @@ +module Filter::Buckets + extend ActiveSupport::Concern + + included do + store_accessor :params, :bucket_ids + end + + def buckets + @buckets ||= account.buckets.where id: bucket_ids.to_s.split(",") + end +end diff --git a/app/models/filter/indexes.rb b/app/models/filter/indexes.rb new file mode 100644 index 000000000..746b8dec1 --- /dev/null +++ b/app/models/filter/indexes.rb @@ -0,0 +1,7 @@ +module Filter::Indexes + INDEXES = %w[ most_active most_discussed most_boosted newest oldest popped ] + + def indexed_by + (params["indexed_by"] || self.class.default_params["indexed_by"]).inquiry + end +end diff --git a/app/models/filter/summarized.rb b/app/models/filter/summarized.rb new file mode 100644 index 000000000..a1286da23 --- /dev/null +++ b/app/models/filter/summarized.rb @@ -0,0 +1,36 @@ +module Filter::Summarized + def summary + [ index_summary, tag_summary, assignee_summary ].compact.to_sentence + " #{bucket_summary}" + end + + def plain_summary + summary.remove(/<\/?mark>/) + end + + private + def index_summary + "#{indexed_by.humanize}" + end + + def tag_summary + if tags.exists? + "tagged #{tags.map(&:hashtag).to_choice_sentence}" + end + end + + def assignee_summary + if assignees.exists? + "assigned to #{assignees.pluck(:name).to_choice_sentence}" + elsif assignments.unassigned? + "assigned to no one" + end + end + + def bucket_summary + if buckets.exists? + "in #{buckets.pluck(:name).to_choice_sentence}" + else + "in all projects" + end + end +end diff --git a/app/models/filter/tags.rb b/app/models/filter/tags.rb new file mode 100644 index 000000000..4b457b461 --- /dev/null +++ b/app/models/filter/tags.rb @@ -0,0 +1,11 @@ +module Filter::Tags + extend ActiveSupport::Concern + + included do + store_accessor :params, :tag_ids + end + + def tags + @tags ||= account.tags.where id: tag_ids.to_s.split(",") + end +end diff --git a/app/models/pop.rb b/app/models/pop.rb index 059336c67..45ccb2411 100644 --- a/app/models/pop.rb +++ b/app/models/pop.rb @@ -1,4 +1,4 @@ class Pop < ApplicationRecord - belongs_to :bubble + belongs_to :bubble, touch: true belongs_to :user, optional: true end diff --git a/app/models/user.rb b/app/models/user.rb index d214b1120..d452c1756 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,4 @@ class User < ApplicationRecord - include Viewer - belongs_to :account has_many :sessions, dependent: :destroy @@ -8,7 +6,10 @@ class User < ApplicationRecord has_many :accesses, dependent: :destroy has_many :buckets, through: :accesses - has_many :bubbles, through: :buckets + has_many :accessible_bubbles, through: :buckets, source: :bubbles + + has_many :filters, foreign_key: :creator_id, inverse_of: :creator, dependent: :destroy + has_many :pops, dependent: :nullify has_many :assignments, foreign_key: :assignee_id, dependent: :destroy diff --git a/app/models/user/viewer.rb b/app/models/user/viewer.rb deleted file mode 100644 index a3d59585e..000000000 --- a/app/models/user/viewer.rb +++ /dev/null @@ -1,7 +0,0 @@ -module User::Viewer - extend ActiveSupport::Concern - - included do - has_many :bucket_views, class_name: "Bucket::View", foreign_key: :creator_id, dependent: :destroy - end -end diff --git a/app/views/bubbles/_filters.html.erb b/app/views/bubbles/_filters.html.erb index ce16c39b9..0903cdda3 100644 --- a/app/views/bubbles/_filters.html.erb +++ b/app/views/bubbles/_filters.html.erb @@ -1,28 +1,16 @@
-

- <%= render "bubbles/filters/main", bucket: bucket %> + <%= bubble_filters_heading filter do %> + <%= render "bubbles/filters/index", filter: filter %> in - <%= render "bubbles/filters/bucket", bucket: bucket %> + <%= render "bubbles/filters/buckets", filter: filter %> - <% if false %> - <%= button_tag type: :submit, class: "btn txt-small borderless" do %> - <%= image_tag "bookmark.svg", aria: { hidden: true }, size: 24 %> - Save this filter - <% end %> - <% else %> - <%= button_tag type: :submit, class: "btn txt-small borderless" do %> - <%= image_tag "bookmark-outline.svg", aria: { hidden: true }, size: 24 %> - Save this filter - <% end %> + <%= render "bubbles/filters/bookmark", filter: filter if filter.savable? %> <% end %> -

- <%= filter.tags ? "Tagged" : "with" %> - <%= render "bubbles/filters/tags", bucket: bucket, filter: filter %> + <%= filter.tags.present? ? "Tagged" : "with" %> + <%= render "bubbles/filters/tags", filter: filter %> - <% unless unassigned_filter_activated? %> - and <%= render "bubbles/filters/assignees", bucket: bucket, filter: filter %> - <% end %> + and <%= render "bubbles/filters/assignments", filter: filter %>

diff --git a/app/views/bubbles/_tags.html.erb b/app/views/bubbles/_tags.html.erb index 833a5f5e4..afd4a69e1 100644 --- a/app/views/bubbles/_tags.html.erb +++ b/app/views/bubbles/_tags.html.erb @@ -1,5 +1,5 @@ <% bubble.tags.each do |tag| %> - <%= link_to "##{tag.title}", bucket_bubbles_path(bubble.bucket, tag_id: tag), class: "bubble__bubble bubble__meta bubble__tag" %> + <%= link_to tag.hashtag, bubbles_path(bucket_id: bubble.bucket, tag_ids: tag), class: "bubble__bubble bubble__meta bubble__tag" %> <% end %> <% if bubble.tags.size < 3 %> diff --git a/app/views/bubbles/filters/_assignees.html.erb b/app/views/bubbles/filters/_assignments.html.erb similarity index 52% rename from app/views/bubbles/filters/_assignees.html.erb rename to app/views/bubbles/filters/_assignments.html.erb index f5bad92ea..f3a4df950 100644 --- a/app/views/bubbles/filters/_assignees.html.erb +++ b/app/views/bubbles/filters/_assignments.html.erb @@ -1,18 +1,20 @@
- + - <% bucket.users.active.order(:name).each do |user| %> -
  • <%= link_to user.name, bucket_bubbles_path(bucket, view_filter_params.merge(assignee_ids: [ user.id ])) %>
    • +
  • <%= link_to "No one", bubbles_path(filter.to_params.merge(assignments: :unassigned)), class: "filter__button" %>
    • + + <% Current.account.users.active.order(:name).each do |user| %> +
  • <%= link_to user.name, bubbles_path(filter.to_params.merge(assignments: user.id)) %>
    • <% end %>     - <% if filter.assignees %> - <%= link_to bucket_bubbles_path(bucket, view_filter_params.without(:assignee_ids)), class: "btn", style: "font-size: 0.4em;" do %> + <% if filter.assignments.present? %> + <%= link_to bubbles_path(filter.to_params.without(:assignments)), class: "btn", style: "font-size: 0.4em;" do %> <%= image_tag "remove.svg", aria: { hidden: true }, size: 24 %> Clear <% end %> diff --git a/app/views/bubbles/filters/_bookmark.html.erb b/app/views/bubbles/filters/_bookmark.html.erb new file mode 100644 index 000000000..c38cf4e63 --- /dev/null +++ b/app/views/bubbles/filters/_bookmark.html.erb @@ -0,0 +1,16 @@ +<% if params[:filter_id] %> + <%= button_to filter_path(params[:filter_id]), method: :delete, class: "btn txt-small borderless" do %> + <%= image_tag "bookmark.svg", aria: { hidden: true }, size: 24 %> + Delete this filter + <% end %> +<% else %> + <%= form_with url: filters_path, method: :post do %> + <% Filter::KNOWN_PARAMS.each do |param| %> + <%= hidden_field_tag param, params[param] if params[param].present? %> + <% end %> + <%= button_tag type: :submit, class: "btn txt-small borderless" do %> + <%= image_tag "bookmark-outline.svg", aria: { hidden: true }, size: 24 %> + Save this filter + <% end %> + <% end %> +<% end %> diff --git a/app/views/bubbles/filters/_bucket_view_form.html.erb b/app/views/bubbles/filters/_bucket_view_form.html.erb deleted file mode 100644 index 3d2afceff..000000000 --- a/app/views/bubbles/filters/_bucket_view_form.html.erb +++ /dev/null @@ -1,19 +0,0 @@ -<% view ||= Bucket::View.new %> - -<%= bubble_filter_form_tag bucket_views_path(bucket), method: :post, id: dom_id(view, :new_form) %> - -<% if view.persisted? %> - <%= bubble_filter_form_tag bucket_view_path(bucket, view), method: :put, id: dom_id(view, :edit_form) %> -<% end %> - -<%= button_tag type: :submit, form: dom_id(view, :new_form), class: "btn", style: "font-size: 0.4em;" do %> - <%= image_tag "add.svg", aria: { hidden: true }, size: 24 %> - New -<% end %> - -<% if view.persisted? %> - <%= button_tag type: :submit, form: dom_id(view, :edit_form), class: "btn", style: "font-size: 0.4em;" do %> - <%= image_tag "pencil.svg", aria: { hidden: true }, size: 24 %> - Update - <% end %> -<% end %> diff --git a/app/views/bubbles/filters/_buckets.html.erb b/app/views/bubbles/filters/_buckets.html.erb new file mode 100644 index 000000000..33130a767 --- /dev/null +++ b/app/views/bubbles/filters/_buckets.html.erb @@ -0,0 +1,15 @@ +
    + + + + +
  • <%= link_to "all projects", bubbles_path(filter.to_params.merge(bucket_ids: nil)), class: "filter__button" %>
    • + + <% Current.user.buckets.each do |bucket| %> +
  • <%= link_to bucket.name, bubbles_path(filter.to_params.merge(bucket_ids: bucket.id)), class: "filter__button" %>
    • + <% end %> +     +     +
    diff --git a/app/views/bubbles/filters/_bucket.html.erb b/app/views/bubbles/filters/_index.html.erb similarity index 63% rename from app/views/bubbles/filters/_bucket.html.erb rename to app/views/bubbles/filters/_index.html.erb index 376516df9..ff83a8640 100644 --- a/app/views/bubbles/filters/_bucket.html.erb +++ b/app/views/bubbles/filters/_index.html.erb @@ -1,12 +1,12 @@
    - + - <% Current.user.buckets.order(:name).each do |bucket| %> -
  • <%= link_to bucket.name, bucket_bubbles_path(bucket, view_filter_params), class: "filter__button" %>
    • + <% Filter::INDEXES.each do |index| %> +
  • <%= link_to index.humanize, bubbles_path(filter.to_params.merge(indexed_by: index)), class: "filter__button" %>
    • <% end %>     diff --git a/app/views/bubbles/filters/_main.html.erb b/app/views/bubbles/filters/_main.html.erb deleted file mode 100644 index ebb0adbd6..000000000 --- a/app/views/bubbles/filters/_main.html.erb +++ /dev/null @@ -1,17 +0,0 @@ -
    - - - - - <% Bucket::View::ORDERS.each do |key, value| %> -
  • <%= link_to value.humanize, bucket_bubbles_path(bucket, view_filter_params.merge(order_by: key, status: nil)), class: "filter__button" %>
    • - <% end %> - - <% Bucket::View::STATUSES.each do |key, value| %> -
  • <%= link_to value.humanize, bucket_bubbles_path(bucket, view_filter_params.merge(order_by: nil, status: key)), class: "filter__button" %>
    • - <% end %> -     -     -
    diff --git a/app/views/bubbles/filters/_tags.html.erb b/app/views/bubbles/filters/_tags.html.erb index 7fa28e9a6..e520a761c 100644 --- a/app/views/bubbles/filters/_tags.html.erb +++ b/app/views/bubbles/filters/_tags.html.erb @@ -1,18 +1,18 @@
    - + - <% bucket.tags.order(:title).each do |tag| %> -
  • <%= link_to tag.title, bucket_bubbles_path(bucket, view_filter_params.merge(tag_ids: [ tag.id ])) %>
    • + <% Current.account.tags.order(:title).each do |tag| %> +
  • <%= link_to tag.title, bubbles_path(filter.to_params.merge(tag_ids: tag.id)) %>
    • <% end %>     - <% if filter.tags %> - <%= link_to bucket_bubbles_path(bucket, view_filter_params.without(:tag_ids)), class: "btn", style: "font-size: 0.4em;" do %> + <% if filter.tags.present? %> + <%= link_to bubbles_path(filter.to_params.without(:tag_ids)), class: "btn", style: "font-size: 0.4em;" do %> <%= image_tag "remove.svg", aria: { hidden: true }, size: 24 %> Clear <% end %> diff --git a/app/views/bubbles/index.html.erb b/app/views/bubbles/index.html.erb index c6ae7517e..406a1960d 100644 --- a/app/views/bubbles/index.html.erb +++ b/app/views/bubbles/index.html.erb @@ -1,24 +1,30 @@ -<% @page_title = @bucket.name %> +<% @page_title = @filter.plain_summary %> <% content_for :header do %> <% end %> -
    +
    <% if @bubbles.any? %> <%= render partial: "bubbles/bubble", collection: @bubbles.limit(10), cached: true %> <% else %> @@ -28,12 +34,12 @@
    - <%= bubble_filter_form_tag bucket_bubbles_path(@bucket), method: :get do %> + <%= form_tag bubbles_path(@filter.to_params), method: :get do %> <%= search_field_tag :term, params[:term], class: "input center flex-inline", placeholder: "Type to filter…" %> <% end %>
      - <%= render partial: "bubbles/list/bubble", collection: @bubbles %> + <%= render partial: "bubbles/list/bubble", collection: @bubbles, cached: true %>
    diff --git a/app/views/bubbles/list/_bubble.html.erb b/app/views/bubbles/list/_bubble.html.erb index 2862f5777..1d822e861 100644 --- a/app/views/bubbles/list/_bubble.html.erb +++ b/app/views/bubbles/list/_bubble.html.erb @@ -20,8 +20,10 @@ <% bubble.tags.each do |tag| %> - <%# FIXME: This is not going to work, this partial must be cacheable, so it can't mutate a changeable view filter live. Need a JS solution. %> - <%= link_to "##{tag.title}", bucket_bubbles_path(bubble.bucket, view_filter_params.merge(tag_ids: tag.id)), style: "color: #{bubble.color}" %> + <%= button_tag tag.hashtag, type: :button, + class: "btn btn--plain", style: "color: #{bubble.color}", data: { + controller: "query-merger", action: "query-merger#merge", + query_merger_key_param: "tag_ids", query_merger_value_param: tag.id } %> <% end %> <%= tag.time bubble.created_at, class: "txt-nowrap flex-item-justify-end" do %> diff --git a/app/views/bubbles/show.html.erb b/app/views/bubbles/show.html.erb index 92c2b14cd..9d941ee9e 100644 --- a/app/views/bubbles/show.html.erb +++ b/app/views/bubbles/show.html.erb @@ -2,9 +2,9 @@ <% content_for :header do %>
    diff --git a/app/views/bubbles/filters/_tags.html.erb b/app/views/bubbles/filters/_tags.html.erb index e520a761c..7972e7e2f 100644 --- a/app/views/bubbles/filters/_tags.html.erb +++ b/app/views/bubbles/filters/_tags.html.erb @@ -6,7 +6,7 @@ <% Current.account.tags.order(:title).each do |tag| %> -
  • <%= link_to tag.title, bubbles_path(filter.to_params.merge(tag_ids: tag.id)) %>
    • +
  • <%= link_to tag.title, bubbles_path(filter.to_params.merge(tag_ids: [ tag.id ])) %>
    • <% end %>     diff --git a/app/views/buckets/_bucket.html.erb b/app/views/buckets/_bucket.html.erb index 4ed607d43..fc270bc8d 100644 --- a/app/views/buckets/_bucket.html.erb +++ b/app/views/buckets/_bucket.html.erb @@ -1,6 +1,6 @@ <% cache bucket do %>
  • - <%= link_to bubbles_path(bucket_ids: bucket), class: "windshield__container flex justify-center align-center position-relative" do %> + <%= link_to bubbles_path(bucket_ids: [ bucket ]), class: "windshield__container flex justify-center align-center position-relative" do %>
    <% bucket.bubbles.ordered_by_activity.limit(10).each do |bubble| %>
    @@ -14,7 +14,7 @@ <% end %>
    - <%= link_to bubbles_path(bucket_ids: bucket), class: "txt-ink flex flex-column" do %> + <%= link_to bubbles_path(bucket_ids: [ bucket ]), class: "txt-ink flex flex-column" do %> In <%= bucket.name %> <% end %>
    diff --git a/db/migrate/20241105224305_create_filter_join_tables.rb b/db/migrate/20241105224305_create_filter_join_tables.rb new file mode 100644 index 000000000..31a0bc697 --- /dev/null +++ b/db/migrate/20241105224305_create_filter_join_tables.rb @@ -0,0 +1,18 @@ +class CreateFilterJoinTables < ActiveRecord::Migration[8.0] + def change + create_join_table :filters, :tags do |t| + t.index :filter_id + t.index :tag_id + end + + create_join_table :filters, :buckets do |t| + t.index :filter_id + t.index :bucket_id + end + + create_join_table :filters, :assignees do |t| + t.index :filter_id + t.index :assignee_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 7bad74467..f77d399a4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2024_11_05_181312) do +ActiveRecord::Schema[8.0].define(version: 2024_11_05_224305) do create_table "accesses", force: :cascade do |t| t.integer "bucket_id", null: false t.integer "user_id", null: false @@ -58,6 +58,13 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_05_181312) do t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end + create_table "assignees_filters", id: false, force: :cascade do |t| + t.integer "filter_id", null: false + t.integer "assignee_id", null: false + t.index ["assignee_id"], name: "index_assignees_filters_on_assignee_id" + t.index ["filter_id"], name: "index_assignees_filters_on_filter_id" + end + create_table "assignments", force: :cascade do |t| t.integer "assignee_id", null: false t.integer "bubble_id", null: false @@ -92,6 +99,13 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_05_181312) do t.index ["creator_id"], name: "index_buckets_on_creator_id" end + create_table "buckets_filters", id: false, force: :cascade do |t| + t.integer "filter_id", null: false + t.integer "bucket_id", null: false + t.index ["bucket_id"], name: "index_buckets_filters_on_bucket_id" + t.index ["filter_id"], name: "index_buckets_filters_on_filter_id" + end + create_table "comments", force: :cascade do |t| t.text "body", null: false t.integer "creator_id", null: false @@ -123,6 +137,13 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_05_181312) do t.index ["creator_id", "params"], name: "index_filters_on_creator_id_and_params", unique: true end + create_table "filters_tags", id: false, force: :cascade do |t| + t.integer "filter_id", null: false + t.integer "tag_id", null: false + t.index ["filter_id"], name: "index_filters_tags_on_filter_id" + t.index ["tag_id"], name: "index_filters_tags_on_tag_id" + end + create_table "messages", force: :cascade do |t| t.integer "bubble_id", null: false t.string "messageable_type", null: false diff --git a/test/controllers/buckets_controller_test.rb b/test/controllers/buckets_controller_test.rb index 7bbce07c5..306d745e3 100644 --- a/test/controllers/buckets_controller_test.rb +++ b/test/controllers/buckets_controller_test.rb @@ -21,7 +21,7 @@ class BucketsControllerTest < ActionDispatch::IntegrationTest end bucket = Bucket.last - assert_redirected_to bubbles_path(bucket_ids: bucket.id) + assert_redirected_to bubbles_path(bucket_ids: [ bucket ]) assert_includes bucket.users, users(:kevin) assert_equal "Remodel Punch List", bucket.name end @@ -34,7 +34,7 @@ class BucketsControllerTest < ActionDispatch::IntegrationTest test "update" do patch bucket_url(buckets(:writebook)), params: { bucket: { name: "Writebook bugs" }, user_ids: users(:david, :jz).pluck(:id) } - assert_redirected_to bubbles_path(bucket_ids: buckets(:writebook)) + assert_redirected_to bubbles_path(bucket_ids: [ buckets(:writebook) ]) assert_equal "Writebook bugs", buckets(:writebook).reload.name assert_equal users(:david, :jz), buckets(:writebook).users end diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index 5fc0d61fc..0835844a6 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -1,3 +1,5 @@ jz_assignments: creator: david - params: <%= { indexed_by: :most_discussed, assignments: ActiveRecord::FixtureSet.identify(:jz).to_s, tag_ids: ActiveRecord::FixtureSet.identify(:mobile).to_s }.to_json %> + params: <%= { indexed_by: :most_discussed }.to_json %> + tags: mobile + assignees: jz From 12ce07639d064f8d6568e786460269452742b0e3 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 17:49:14 -0600 Subject: [PATCH 05/23] Add tests --- app/controllers/bubbles_controller.rb | 4 -- app/controllers/filters_controller.rb | 10 ++- app/helpers/filters_helper.rb | 5 -- app/models/concerns/filterable.rb | 4 +- app/models/filter.rb | 72 ++------------------ app/models/filter/params.rb | 51 ++++++++++++++ app/models/filter/resources.rb | 24 +++++++ app/views/bubbles/_filters.html.erb | 6 +- test/controllers/bubbles_controller_test.rb | 51 +++++++++++++- test/controllers/filters_controller_test.rb | 34 +++++++++ test/fixtures/files/.keep | 0 test/fixtures/files/moon.jpg | Bin 0 -> 12794 bytes test/fixtures/filters.yml | 2 +- test/models/bubble_test.rb | 35 +++++++++- test/models/filter_test.rb | 51 ++++++++++++++ 15 files changed, 258 insertions(+), 91 deletions(-) create mode 100644 app/models/filter/params.rb create mode 100644 app/models/filter/resources.rb create mode 100644 test/controllers/filters_controller_test.rb delete mode 100644 test/fixtures/files/.keep create mode 100644 test/fixtures/files/moon.jpg create mode 100644 test/models/filter_test.rb diff --git a/app/controllers/bubbles_controller.rb b/app/controllers/bubbles_controller.rb index 1b9591008..7765be897 100644 --- a/app/controllers/bubbles_controller.rb +++ b/app/controllers/bubbles_controller.rb @@ -11,10 +11,6 @@ class BubblesController < ApplicationController @bubbles = @bubbles.mentioning(params[:term]) if params[:term].present? end - def new - @bubble = @bucket.bubbles.build - end - def create @bubble = @bucket.bubbles.create! redirect_to @bubble diff --git a/app/controllers/filters_controller.rb b/app/controllers/filters_controller.rb index cc5ee531d..9d9964e52 100644 --- a/app/controllers/filters_controller.rb +++ b/app/controllers/filters_controller.rb @@ -1,8 +1,8 @@ class FiltersController < ApplicationController - before_action :set_filter, only: :destroy + before_action :set_filter, :remember_params, only: :destroy def create - @filter = Current.user.filters.create_or_find_by_params!(filter_params).tap(&:touch) + @filter = Current.user.filters.idempotent_create!(filter_params).tap(&:touch) redirect_to bubbles_path(@filter.to_params) end @@ -16,6 +16,10 @@ class FiltersController < ApplicationController @filter = Current.user.filters.find params[:id] end + def remember_params + @filter_params = @filter.to_params + end + def filter_params params.permit(*Filter::KNOWN_PARAMS).compact_blank end @@ -24,7 +28,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_params) end end end diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index 677e43fc0..05caac3d0 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -1,9 +1,4 @@ module FiltersHelper - def bubble_filters_heading(filter, &) - tag.h1 class: "txt-large flex align-center gap-half", - style: token_list("margin-inline-end: calc(var(--btn-size) / -2);": filter.savable?), & - end - def buckets_filter_text(filter) if filter.buckets.present? filter.buckets.map(&:name).to_choice_sentence diff --git a/app/models/concerns/filterable.rb b/app/models/concerns/filterable.rb index 81656b42c..b3cb07c46 100644 --- a/app/models/concerns/filterable.rb +++ b/app/models/concerns/filterable.rb @@ -10,8 +10,6 @@ module Filterable private def remove_from_filters - filters.each do |filter| - filter.resource_removed kind: self.class.name.downcase, id: id - end + filters.each { |filter| filter.resource_removed self } end end diff --git a/app/models/filter.rb b/app/models/filter.rb index 9c012911a..7ff26f981 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -1,34 +1,17 @@ class Filter < ApplicationRecord - include Summarized - - KNOWN_PARAMS = [ :indexed_by, :assignments, bucket_ids: [], assignee_ids: [], tag_ids: [] ] - INDEXES = %w[ most_active most_discussed most_boosted newest oldest popped ] + include Params, Resources, Summarized belongs_to :creator, class_name: "User", default: -> { Current.user } has_one :account, through: :creator - has_and_belongs_to_many :tags - has_and_belongs_to_many :buckets - has_and_belongs_to_many :assignees, class_name: "User", join_table: "assignees_filters", association_foreign_key: "assignee_id" - - before_validation :denormalize_resource_ids - before_validation :remove_default_params - - store_accessor :params, :indexed_by - store_accessor :params, :assignments - class << self - def default_params - { "indexed_by" => "most_active" } - end - - def create_or_find_by_params!(params) - filter = new params + def idempotent_create!(attrs) + filter = new(attrs) filter.save! filter rescue ActiveRecord::RecordNotUnique - find_by! params: filter.params + find_by! params: filter.params # possible thanks to denormalized params end end @@ -37,33 +20,13 @@ class Filter < ApplicationRecord result = creator.accessible_bubbles.indexed_by(indexed_by) result = result.active unless indexed_by.popped? result = result.unassigned if assignments.unassigned? + result = result.assigned_to(assignees.ids) if assignees.present? result = result.in_bucket(buckets.ids) if buckets.present? result = result.tagged_with(tags.ids) if tags.present? - result = result.assigned_to(assignees.ids) if assignees.present? result end end - def to_params - params.merge(tag_ids: tags.ids, assignee_ids: assignees.ids, bucket_ids: buckets.ids).then do |params| - ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| - params[:filter_id] = id if persisted? - end - end - end - - def indexed_by - (params["indexed_by"] || self.class.default_params["indexed_by"]).inquiry - end - - def assignments - params["assignments"].to_s.inquiry - end - - def savable? - !bucket_default? - end - def cacheable? buckets.exists? end @@ -71,29 +34,4 @@ class Filter < ApplicationRecord def cache_key ActiveSupport::Cache.expand_cache_key buckets.cache_key_with_version, super end - - def resource_removed(kind:, id:) - params["#{kind}_ids"] = Array(params["#{kind}_ids"]).without(id).presence - non_default_params.blank? ? destroy : touch - end - - private - def remove_default_params - self[:params] = non_default_params.compact_blank - end - - def non_default_params - params.reject { |k, v| self.class.default_params[k] == v } - end - - # `denormalize_resource_ids` stores ids in the params column to enforce uniqueness - def denormalize_resource_ids - params[:bucket_ids] = buckets.ids.presence - params[:tag_ids] = tags.ids.presence - params[:assignee_ids] = assignees.ids.presence - end - - def bucket_default? - non_default_params.keys == %w[ bucket_ids ] && buckets.one? - end end diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb new file mode 100644 index 000000000..df2354ebd --- /dev/null +++ b/app/models/filter/params.rb @@ -0,0 +1,51 @@ +module Filter::Params + extend ActiveSupport::Concern + + KNOWN_PARAMS = [ :indexed_by, :assignments, bucket_ids: [], assignee_ids: [], tag_ids: [] ] + INDEXES = %w[ most_active most_discussed most_boosted newest oldest popped ] + + class_methods do + def default_params + { "indexed_by" => "most_active" } + end + end + + included do + before_validation :sanitize_params + end + + def to_params + params.merge(tag_ids: tags.ids.presence, assignee_ids: assignees.ids.presence, bucket_ids: buckets.ids.presence).then do |params| + ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| + params[:filter_id] = id if persisted? + end + end + end + + def assignments=(value) + params["assignments"] = value + end + + def assignments + params["assignments"].to_s.inquiry + end + + def indexed_by=(value) + params["indexed_by"] = value + end + + def indexed_by + (params["indexed_by"] || self.class.default_params["indexed_by"]).inquiry + end + + private + def sanitize_params + denormalize_resource_ids + self.params = non_default_params + params.compact_blank! + end + + def non_default_params + params.reject { |k, v| self.class.default_params[k] == v } + end +end diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb new file mode 100644 index 000000000..6f04ded0a --- /dev/null +++ b/app/models/filter/resources.rb @@ -0,0 +1,24 @@ +module Filter::Resources + extend ActiveSupport::Concern + + included do + has_and_belongs_to_many :tags + has_and_belongs_to_many :buckets + has_and_belongs_to_many :assignees, class_name: "User", join_table: "assignees_filters", association_foreign_key: "assignee_id" + end + + def resource_removed(resource) + kind = resource.class.model_name.plural + send("#{kind}=", send(kind).without(resource)) + sanitize_params + params.blank? ? destroy! : save! + end + + private + # `denormalize_resource_ids` stores ids in the params column to enforce uniqueness with a db constraint. + def denormalize_resource_ids + params["bucket_ids"] = buckets.ids + params["tag_ids"] = tags.ids + params["assignee_ids"] = assignees.ids + end +end diff --git a/app/views/bubbles/_filters.html.erb b/app/views/bubbles/_filters.html.erb index 0903cdda3..2efc451c8 100644 --- a/app/views/bubbles/_filters.html.erb +++ b/app/views/bubbles/_filters.html.erb @@ -1,11 +1,11 @@
    - <%= bubble_filters_heading filter do %> +

    <%= render "bubbles/filters/index", filter: filter %> in <%= render "bubbles/filters/buckets", filter: filter %> - <%= render "bubbles/filters/bookmark", filter: filter if filter.savable? %> - <% end %> + <%= render "bubbles/filters/bookmark", filter: filter %> +

    <%= filter.tags.present? ? "Tagged" : "with" %> diff --git a/test/controllers/bubbles_controller_test.rb b/test/controllers/bubbles_controller_test.rb index cc5bc5526..d440a1dec 100644 --- a/test/controllers/bubbles_controller_test.rb +++ b/test/controllers/bubbles_controller_test.rb @@ -1,7 +1,52 @@ require "test_helper" class BubblesControllerTest < ActionDispatch::IntegrationTest - # test "the truth" do - # assert true - # end + setup do + sign_in_as :kevin + end + + test "index" do + get bubbles_url + assert_response :success + end + + test "filtered index" do + get bubbles_url(filters(:jz_assignments).to_params.merge(term: "haggis")) + assert_response :success + end + + test "create" do + assert_difference "Bubble.count", 1 do + post bucket_bubbles_url(buckets(:writebook)) + end + assert_redirected_to bucket_bubble_url(buckets(:writebook), Bubble.last) + end + + test "show" do + get bucket_bubble_url(buckets(:writebook), bubbles(:logo)) + assert_response :success + end + + test "edit" do + get edit_bucket_bubble_url(buckets(:writebook), bubbles(:logo)) + assert_response :success + end + + test "update" do + patch bucket_bubble_url(buckets(:writebook), bubbles(:logo)), params: { + bubble: { + title: "Logo needs to change", + color: "#000000", + due_on: 1.week.from_now, + image: fixture_file_upload("moon.jpg", "image/jpeg"), + tag_ids: [ tags(:mobile).id ] } } + assert_redirected_to bucket_bubble_url(buckets(:writebook), bubbles(:logo)) + + bubble = bubbles(:logo).reload + assert_equal "Logo needs to change", bubble.title + assert_equal "#000000", bubble.color + assert_equal 1.week.from_now.to_date, bubble.due_on + assert_equal "moon.jpg", bubble.image.filename.to_s + assert_equal [ tags(:mobile) ], bubble.tags + end end diff --git a/test/controllers/filters_controller_test.rb b/test/controllers/filters_controller_test.rb new file mode 100644 index 000000000..332b11d5e --- /dev/null +++ b/test/controllers/filters_controller_test.rb @@ -0,0 +1,34 @@ +require "test_helper" + +class FiltersControllerTest < ActionDispatch::IntegrationTest + setup do + sign_in_as :david + end + + test "create" do + assert_difference "Filter.count", +1 do + post filters_url, params: { + indexed_by: "popped", + assignments: "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) + + filter = Filter.last + assert_predicate filter.indexed_by, :popped? + assert_predicate filter.assignments, :unassigned? + assert_equal [ tags(:mobile) ], filter.tags + assert_equal [ users(:jz) ], filter.assignees + assert_equal [ buckets(:writebook) ], filter.buckets + end + + test "destroy" do + params = filters(:jz_assignments).to_params + assert_difference "Filter.count", -1 do + delete filter_url(filters(:jz_assignments)) + end + assert_redirected_to bubbles_path(params) + end +end diff --git a/test/fixtures/files/.keep b/test/fixtures/files/.keep deleted file mode 100644 index e69de29bb..000000000 diff --git a/test/fixtures/files/moon.jpg b/test/fixtures/files/moon.jpg new file mode 100644 index 0000000000000000000000000000000000000000..2ef418957e99b71c25aabd42213096acad4efb46 GIT binary patch literal 12794 zcmbVydtA)x_wQ$Bnr^0|JyGeZ(oCeL1|@02beX7`DavS*L>EG$R4&uaW_G$xAxX_N zDde(EWRpvDwTqGv+DPfLV^fLT&U!wR@AvmQ=bv+q{Tds6=J`C&TJQB<>sjmlE`Z!+t!)mSC#N$|}laRTUK#H8oXr3RQzLX%a9cJaOeWLV($U$$Zs8nzrX5;@pr)oqnMBdm(9pFrH88dNfByC3 zCN@QtC?pC=gc%rd3V}3*@Z%0<4ErP#@L$-(F9MOIL{?T&Ra2h?4^&RZhy)Ues6--@ zm6YJ=J@9i(X$o1}l;NuU3vZpuj3}yIVpg%L+2RYgboj0Rn%f6&Pf}B#I!#wk-(sdE zjc&!9J8!;&3J0o*ZAl6)XJ%RteUJY}gnY79J73V`oh4uDJN*l+^vd9XKe; z&JpM49X@h2Um`7$m7XjsKlR5&dF7?cRagG3xqYX$uD+qMsqI1g!;VLfpLD)_)&07s z_f22_!21uwBcmV3K7AfX=OSPv#j)Ub#lil!b4`JB5tWokN-F4F1Y#_FNK=%^rVM3m zSDwncs9$E-C8|;vXBA(#rDkT&|5qn?d#n0XbLLBn_vmQ&$^P#Smh}I1vi}_Hzvt@2 zC?opcSxFI5B%tYv`E7hml9w%1cjC7N3)N}0RE)=l zk9&s*qe;@Z1a~HnMJA@Va|xa4G%5a53T+5I^7G>!j}|2p)j6;rPeiN5s5~}C70Y2| z#rrTxx|~alk>tUe()X~4nBUqd)XvM!6Kakm<%yK=FQ6~&kV%9DHkC_YTU)>~9Qpy8 zOp>YQ-j?Wkia?XV_lMZZLUr#Exfql4c^>Q7hOiBJ5;_4*0-x?`y&|lYO{L%uqc6M? zIrmNGu?_pMW^2oCHfC)><=Vc(-(g4tD@=S6*;G|HaGNfEuFhmn)+F?ddjxHWMW%uc zsC+K5)f2vN81B?|pWrH$Szy+1N>y5kB=L86)&yBLT!RXav5lLA>TBA?@JDw6nL)O; zur|@o7P+Vwsnd$?6Fj{5dG}MyD@Fu8LT|C3w2W3&3GuO0itL->^(Z#SO$#iKj8;7@z-*R-_~= zkrffM4~sbHL2S(kJc)Y|FmRv1wm|34!_7$Hb%Ep18_~pvrSuRDCd5 zPNj{&JB$#n)@*HIqE3?)1^)*PKT*+%Tz~i+hoh`;u6z7XFO!g`wDZu<0aiqQt7{2? zZ8YKw7NKKeIgh30g)d@(%?WqWr-xzE3OG!uh@;pN2VSRypu|BA7m%qKA0bCBt}dG# z1Ga)?obD4_(FyRs{e)MnL{y}$@b(a^MntCrfcW=`JTFi1Z7eLc0KS9KWVIG(gNZ6K zSdv1MR^mukb%FybFvX0ORw@wAd)$hk;Xzca`i@0b&Zx_655FpDMmi4p9 z7S@JkSP8&eD+1yKZ4!=-8y>mb!Wx#$M}D3YhLybYf&&3kNR@O609!ySBEwN8cM3Ry zBJJb}3-e=S{jhawT}k3WKq-RE5DQ{CrxHB!4i?}w<6EaJlB1pQ*hF=jv=jI5Pb4XD zy>9^x`-=Pv&)BNF=+jCF`83H9WRy%bw=h`|iOIv)V`GMW(ju^wPYuM|R1vKoT-6D1 zqK6?mp@-oxMTVk>(-ZES;N}-Gka=aLCPh59$uE`G)ZZ$&$<>Dc!Hiqj2!Ilg-yt?w z9=!>X(s(P|rcF4x^`~uB;+=Tz%>S|u3h1I%bTIb_1;oIciuh54A|A-#bQ-~_4OT&5 z<4+FG&J(Q%JAjQi0<09k$2QRhB2+*H1)3XJ5)qR+9grsx34$yjcrox^Uy&=D8jH?J z7xEAZA^0Iz4taXH$3rZJJMne#z$t2L;}X0&5YIyR2Y7Fuh z{#nOX2G}7+-Kj7b9_{i{~S1Tv{YrNT%I z0Sw_9S0WKkg>$07MZf`CfnX>FcL-P(hW)fta4ZeXFl-3+g9+@}Kx3AbR4yZ&CdovL zP4b*Tm?Eb2;5E*$)r@#pZAkGmIK92%A&Qm&*#UhN2AkMDpSO(s&eO{?pGB1)?*~lF zY61U}j2bu$6R|1!0He0?Kb91cr4_(=7;gkWVJE)xW*}F^bNGJb2*2ELC-hHZ{bDkh z2KfvUSgV3Bj)%e7HSj=&bJR>27c2yragWddoWQzHysl(GaT#t>5w=LIhz$544x|sb zLzHCFYKR=5dYgFgDo#nT-(^IROD?drM5GKHi-3SARgzOloGJppBGW_mM84$`zcXP$ zc%T%5zdQ;tHasriu&8{VpNS;VwJ0c@9yS4c@De5+{IAKu^9ZeJQLv?rc=Y@enjzrusu{B5)Mq;o86K!x0tPk%1;G$#0PP0V< zjDsFsz+(dw{fs16ndC@PPW2qR(|eQ+tLa#$fOd+Y2zPLW8Yip-xZ5TI`~b_K%nGaT z1dHbjI2f&2u`y&3{Nn{CrSe=M8rtzALH359OmG*9E=XBC$h)3s4`L%Z`j#B_3`rZY zA^vneOX~qlz2=G(3WxNtZsPUoyZd;K)4Dk)2@5j?7^G6^k)O}>u7F--od;}Z( z&1tQ;4NQNF|A}K@2Z#=+5U_YW0^9|R3B2-HDiQco($8GPk%0SJC}x1zG)MSt2+Xfg z9Rlw8c_^aDsTB`=VEZ5xN|dYVMoYJk6LsEKIxXdP3&(&j-!2zjODv7#_c7HpX=| zE`2QCXX<-IO0t`g(|_p@yD1|#Eo5i+poib1_GwdN=3{3%rVgEWw^SpzYnRLIvE*OE zSH39hJEqmk;B@yaimSCqu=s)1Jl77Y>*(!$_+?Ab;P{L0m5l=~l+lFXx)u)`zvz_6 zjEv&%`;XT)wHW2-M7*u*t&7%pc8YpoQUxUxt*2-my0;w~Z zZJKIf6XD&iic0WxKzNE!{Gb0Hk))Tv`&DFyWq5FLur=*`KGZvSWTR@k)*AD7eR)D{ zz{ujy4?{V36SdxLG1y)H-SRPib69Pqyzs-0q%O-me+NId#lep{{e^~~{5TbLH|C40 z8Y)xwcYhz=VeInNUy9mtB%>?PLwW3Lg4R0U>MveV^5N0)&EZpbd`YmL z`sknW4dRve$E+N;ZjV1vS5(!pxH09|`u>~c+HO@_s(5v*vKtp4ZgXReZn=@FKcf41 zlr+;*W4It=9sgGJ?2EQos*!ZOo!4pAvpqI6E&ser3>KK$Uu(^}bk;94d6e1rc7@Mh z2VQ)f8!-Rv@D20#a=%R`Ee1j3iQX>+JEI#MdKhff?I&bc>NrIR|3bm5!5N5G=^ zH(J*gWwRx=%jw%!e9rv+{FHaQ9k(s~ zYkAn4u~=1yMfLZ;teENL?$nvyUcy@j%odDNWa2?!rkgYhTq0huKz_mat*9Eb zfLcXaM5j2lqF251^7J4=MMz9Ug_Th|g#lYhB-8YY)ce2cy>z_hbLv3L>x;i>MbV6R z#A|)b*xHh#xBgb4dewW2TZNJD!&{BJI4X1Y{dxWSx6{vNpQ#x6;xIJ(rAOqez!@g} z?+V{^<#p6JIXP5Rm`#3q=a@FLPda`gzMyz{vFmhqOn(V|ZD8Gb>#DT3>$C3pe%Cp^ zX{%+SUJ3Vgw{FWO{)XwvFTKXQWlN~D%|rKUZ8j1YcF$z-u9#NuDbH!xpv*dU@Jng& z(WW}JFTal+7#um%cW&O*vmft84gN!(mAGTxFVTleKFUH(Fvie5&dI19e*bj(=r(+2 zjWv6{p{Kp&W!?NP58O~f`P84DT&bz1(Zb+8p&ow{o#_-R^W5>f_7)1OpLJl;jI8~= z1-jeqA7rg~u9RY%X%x|}Hoe%az-Ze7F1hUaW6kg4uXUf5BKIB{ElUxlHECVEpc{p`)cj2=OBC;j($$G_9246Yh>PxU?CCC^uC%2~|qo#*7V z|BVi%oIJvRvc`O?K{Dm6gUic*B6`1jIM&V!_}Fx9_V2e;3^F3JGmPHvOEJG+I)C0( z*@Nd*>l@QfyxiKC@MtD`kMZ1!>gQ5dE43+)Hq~{TK5Dz>Q`@C?_;zQOAEje^c2MT{ zz8zmg^2V^k&VoPif;W*ToaVb(4`6E zxA_Wtg>(AjBU?_FJ$>Bi7E&0iDssyCHmhI5c(MH7mfyFQbmj&p@3|vA^M}#N{>KUa z8vo|pe$~>lc5>>-vHD)Je0qtG(`( zbX^s`7N!2JXLN0gifmTuelI7wWlZa{@s$Y?o6KL)o|+YJj^eFl9yB=G@C@TC+rQnU zb9DT}_U%3uOE#GAI6e1zbJq2>mfH-0Ywk6bt?*XUwd}mo_pI)vI5RraowxeZn9*)d z_hpr?--U;_%eP%xmFtw4zKQWh%Vc`GQ_pG6=iJD^KekMd7WF1i_9QWF-Tt`S<8mZh zXDIcw&5pKGHS_eNb4BJ(39kw*_uM|m+#lBKzd7|@+sX7^uMkt}*@o#?PsGE4uNjOY_*R=0j+=Dd8ypSNOC>h+F&;?Xnh4g zskG_%0L>kj!(Rh)ST{fa_&OA zomw|X??p^DUNrD$WL-{K^M)N)Hyk{_NA(}~3xj@}I`*1{NxpEF^O@hiUDmq%Nn$x} z+`Fmqo{mQzRg6k!8mjMr(k!fjyQI~&+Q}?QW~w9`?nh)IFdHf#QN0f}B(^bqq`IFr zSq}dOJh=iB$4ZjXCA*Xec!Kg6YAysUo;&A%OP=U<0c2SM4Hydys#0kMX1K}MwN2JB zdewQs9R0Y(pX*oo@^Cx5tCQ^;-_DvlbnLGu7cLZPETL0{&bH38 zVw$Gt{8?t!{!04Wvd_Guo{q0S)&?HW{u?{%P@0)mvEt}`lVIiT?Db_K6lL#C%b%|q zI(pM8UfZE;eZXsMvL>-w`g&& zLe;cL-NkVO{^dG5ia#7LcHBJT+~3g@bnwdJYV4ip;JYay8>~DHM(e^4tj(XXOsMSe zT(mouaX!eK@l@w<^d9p+BGt=^KFo48a$HA|CJ(W19``VR9o12AM_BY?@&Z50Zv~0{ z1-|-`Z}KeDG^5!ycLqGnqvWIIOYayS{p_Qwv)rJn!NKfC<@{!DlYQMjt2t9=TE;!i zbR2MxNOR-|IGBt)ZMJip=~^k)d@=54Z9jwv$%(zonRi-m z<Zuj zd-=AI`Hu8t>hzGS*4w&X6xj5UcTqiledx~LHml#!^m|LP_a7N0w&uIWY4-;CbqDo6 z4Bd1QoUyQ*`b@TBS7Z5s$5&4%6+R4bIp(PJf+Wi0ZDHuAz16EXf3W*6gHy>_+^s4L zLks05b6A#Pqa#I1nR9N7jo-eK_^-f}>UIr8w6{=Jt2TcBw_7Cp@-VXLZ~CHwS3>6A z2R*B=D0rNL&T=W&MxMv$wHsG$z>5n!SX0dxx@4P<%edlucZgf zyD)2xxwWQUO}p+1lMW_(&`=jnT9c|8Gd`yT$XOy;_5vJZE``_EjZY>#5GTtsjn zot#_$sikMl-KMP4IIr@xb_>ba4&VOOj}9CCda?4>Q#~?k^W^Tt(b2Q>@2p<`;cKIT zT=tl__eKAq>WkyPtCJu1E{C)%x6^EgybP_VW(%S^T+h}4GZOLkB;XBtMr8cH2c3~N z#SMWF&xfl-67Z;!L}Zm>*jFNBQWlBDye^?S12vIR3YKI;BO3a+0$^CYL{#)-GH{-> zuLrb_!DDOM)$QE{tne_~-w!4Dq+apAa>(b~$gKY59w(37(K=RSIk$b>!)K%} z#+F1E^U*?c^NPuF{TJvctv zXQh)+cl^P2S*dXtdh2TVfb#+zm_U0-*UXMfo6tAtM&Om5yN4GPNk>>pYg zJgc{Cwgwh<=4s8_hOvrC4&CC^7*at&f$yS5Y9`I+nsZ!pz@9z^g*q0wrJ1%K z_|3vECwO~W^?J=>}n|7!u}sOR!q z#H*b)woT{ELbr8){XVyKU_R$qzT>Ln-+l8M{6<>hvpx?8R{S*-#fZ6g-qPW;h}xCH z#=5q{_ga}G91 z8=5`Pb=I(qpks0|5UnTlMu0-)VpRc$9uMPs?Gbd84>2bh{dsbTTu7GxHLET6XjUonr}m@6LQ^zIldd2Qd$T@`Drb& z`?o~#OO~5cQXfY;@;Xm1+-~~e@m?eG&GV{R^Mm&8ZqFOI^Cl~!qj-Pk;=REvyzpi)n06# zHnaBO?VGX72+#liI&#XpW@XK5Y@62dhtr#=ljl>?ON+y=td$Xh7r$nhUN#bLgN9*E zkm}0W&#%<^ys$d{SM32tS$%23L$}PFmYZ&_mgm~CRx&rz)Se$IzZ)fFtmt%IFpCza z^Z99-wu@1&z-fPeq-gMh+MHnn8t)OY_)5a>S4iY2-9V)SFWsHJX_K_?#+<*?6}aV} zrx{IGW+grz=|1^LCnLJt?blp`wFqLVENU`>25=>XVCuLp<@XJFV;JlpI9Pl zXg`}3P4Oob?EcUo{cTI4YdyYlLyyhpwNl#9!Axa15=`jBOo&t=4gg}&5-HkVz?FMrL|6UXz?Ny>P>lQ zr$`wt=FqyaX-gMl{8$)5l4pjY@mXFbRIxBP494|Pp_(AyHQ)NHp=>MoZC|KI#2Ul+8mICGBK`vVK8=soG|`Xb}r=k|uO z<9E-i`;9J44(JgzL_L0ZrC>O@HuRa}@O<^g{2BLSb~d;$bT|9`Li#YA({|w5P}j5` zQnnSpr74g;<>M9?E3AU{;bmw1sK%;$PIQx3XCrKW-(7yMxjE#TkY;(Zf;?m2{N&8I zvC^Z#q@?JPQ+11@p=OXj-n?Fhg+E@nJZASJt7G2YR_uJD*Lli{^tM@cdOEz8msQ=Q zHkrren&JHxtXm()ie_(Gdbd&O{359=VELKb2CrXC$?s)9IdnS{UYpJjb+AcS`@VDfv#%%f4Av)ilpSl(5xJ#!TqE2dxpX9!to2HXG5HYJ zx%%^~^1Is-Z_Novyyctt0~9)tih~VH9fodH{-)wE1ald zjb(99BRM5m8P`5JMz_E3Up4%!-mvBM=2Jpqxk*UJhB|3f9(z|(Vd>o0-!1c{y#>dg zy=i|)%k0f6j*GwdOWKPq7|kqw!I#l9e|S5Wdxes8jS9t`@-rLgfzUjMSv547HQS-) zLv`Q8zyV!U&`b%T5k9V{d#KBA1L{OQV*FC%2CX47^>}!MkOQL~0_uZThcjR<31c?M z>d-Gz+;`AY_`n#=7v*B;Mz{UMw@n0~Q8%|(8WAarhzbPa#Uw!JLNf{=f5|?_ry>2D&v`2#v-E z&Im4C+xhtMs>doTwofY$94$A!@_CzGLigN*2m5O<{(syPC!=uK`@VCD! zmSx}Gc4l{5U{GY8n)AQTR>$kSm0d@8bu^-i%QcO{XhEhoSNUf5*;!M1>LwY)l;{1x zN^7DE_mn?&laHm${8kd0wqoXu*R)_QAHlto&(+s9xkn#etXXL1shX@&xTjs z&%Q|sHAzeyi1ynMHPwN@>Ne=T*wr5ps&s=Ny+Z%R@S~`ABH0v2^I!A6H2r!(+0D%8 znET~wUffe9%{OzW&+D$oVwg=OX$60~dN%gLMF6fK(5h<#RM@nkZohXsj0?cg381RL zfx(!-%*5mcpB2De*pMy^&8A5WdPPb$9Jq{3%~6b-PZ1c+(7nN7fsZo0*cK!_O-mHz zBI?21hZmria0zKVn9;B-V6G`7czFWx!L=GN77;ebT-f7TU%IO-p>@d;?>bpwY3q&I z?l)v>zdu;CI(Pf)jH8K@k`mqr_Bc2^c3pey;zrq>R=+*VE3bbZyEtv@O2=}Ik1sU; znbIGyC>+DcwA||O?HQeBJ1bRet~^h7Q(G9~ySKG>zIoXXENq|Pjy=P9x$;J{Y>zr+ zACBXaPP2^(nP#_c(;W56_LtVrGPNoR%6$W+N|W7%!SS{&%+E^)rk&kF+NO7~-ck3| ztH>*_Ull#C(+s!U6JFwV%)*}WZrL24-BF^AVUjz~)1N($_8gUnf+x4KVPuI{(X=6G zf5LPZ^e!WkBWi5?5{FN3@j<5wKJtN1Da5J>{WD|)E*+W<*5DWuDeym84oG8l-r@jS>S zKOd8euK|KABLeT6z^omv6u1t+Jpu-;_%ISCWw0bi3AJv(nlQ<2rs{K)p$lzcO17Rr zA3`7+8_SRP0X>Rg8VhzzZ%1khn2&hEOici@MBEwEDv{A`I2<%~SB&woes>Pc&|uVv zdk`W9t(OlIG%-cs6OZPMirp&yFYQVnL4$NS5ZvgYojJm_l|arm4FmDZIuvZKU}y?G z3K`J7lBx=qC$4P?=rYtff(2Ytn6Vu1)2%K;GjtdOdB8AuVjp-*9x7T09{w1*g`w5k z72Cmm_cL~pNC@^M2!c84=tSEKN1m0CgPR=2)-ffrL4XWZ{#!n#g)3T^;1Y;RG;U&> z{FKxHU=GVL%Lr%45OA|Yy$KkDBdSLLm{7aW2(T&)`qM{_gCvK(TtxmH?6@B7$&lx;s;t0E(g)a z7;a&M@GgF9A#F0)mP>}42o!EZu;m)0l0fi59w0*Z5R3+Pjg>%|c+gz{f|gLKSk7fI zygDHfPWO@CtW4{Oj};2w{`S3%Rp^Hc;89+KcLv`<_d11th~giTAC zz`#7jR5<_R#X&O>X~yKB)&g?TMBIV4a}#Mp;I3qkU?HvM!Z--+A1p?JmErqn5D(-c zgujjZ!kg$A^sa#OBzd6W5u?t%hP7!b90Wv6VA=nm7T<>i^bi1O(BO!`w)oV0uOA># z8%$-ZaJxEu1#Q2+8^sKsXcX!z98xvOu)78N3KStEAz{$FWkA5FNKuTfAqWinK}f-K zMcNY(4d5We zY(@7?K!pLKBDCHsn3xmj38*g!N!phXhNl7e!Gwl}9wDUT0vH(R!gqo9(6@jIF@Y-w zF0oMYs-5NQfx!QA{GlD}m&o6t|$d6VLP0eLRgQ zuqNhDMx&QncniP?1UoaM*z`fx9VQ|T`~%_^+K99oG(dDnM8G861^01iBl!^-qTH7P`g#WD`EZyghDYK$urQgy&8q zi4(xNOQ>>nY}qi(6Z-ItP239ePz(6b16Z3~0=20lFV3`>VmvNOuOuqv`UI5E6(0uq~GF z1gQuF4NOz0koBQh0F$$yAv4%uPiMFQJho;d^6sb&sqF4*UJE4a2v)0^(RXcwexcDBccjN9RZ&% zt6&fTvWmFS@VACTELtsTV0A@j1Q7qrfq|oqxD{2Mh69glqm6#$| z_XyCZAp>GSbP%ah8J<2tUWKmgoQWGe;2&NH&V<1cJ`Q+*G6f8zb|9pK!l+2y5%2Re zK)jRz;1qf3?TBhTS<2zzP@<6Gg@rV%8SJ2<2uh?2l7m-p5o1m@6oe?;Kr`43 zTP{U~7l%ejXzc=H62~Bx5`p%(wkU-um=_FcSSGDxpaBN@D#rT++foMMWgDN=pr3jqYk!&NEBTp;O@98rnD^SXQot{y;a096$LTxlHT0VpA$to#}dULeU(-%e8u z=tey|44y)zAp{{VDu4q`Lh(teP*i;i88YNiFfC9nf;6%Nh#*lU(KSGqga&;WaCkpj z24shb!XYhorGxMV3Fx?Cs4((HWsHypyo*^5%kXj#`~XKZXj4!xmms2nbQJd=O3v=x zY_3|D81V;{*A4`NC%_457+!>f7ZsMm9f_Bf{m7GIJ|c09?gZGzpx$&}h^YN%E=Hb* zs!cHvXYyl^)@&yb0q#}&WfL7DBn&bHE&>052a*Tz*ezk8lss8nT@Gm+xCI+>2V|jA U;0Qbnkb{f}i$MZ+`SIj`0mv=64*&oF literal 0 HcmV?d00001 diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index 0835844a6..7dfaa219f 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -1,5 +1,5 @@ jz_assignments: creator: david - params: <%= { indexed_by: :most_discussed }.to_json %> + params: <%= { indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json %> tags: mobile assignees: jz diff --git a/test/models/bubble_test.rb b/test/models/bubble_test.rb index b2ab21c1b..11e50d926 100644 --- a/test/models/bubble_test.rb +++ b/test/models/bubble_test.rb @@ -34,11 +34,42 @@ class BubbleTest < ActiveSupport::TestCase test "ordering by activity" do bubbles(:layout).update! boost_count: 1_000 - assert_equal bubbles(:layout, :logo, :shipping, :text), Bubble.ordered_by_activity.to_a + assert_equal bubbles(:layout, :logo, :shipping, :text), Bubble.ordered_by_activity end test "ordering by comments" do - assert_equal bubbles(:logo, :layout, :shipping, :text), Bubble.ordered_by_comments.to_a + assert_equal bubbles(:logo, :layout, :shipping, :text), Bubble.ordered_by_comments + end + + test "ordering by boosts" do + bubbles(:layout).update! boost_count: 1_000 + assert_equal bubbles(:layout, :logo, :shipping, :text), Bubble.ordered_by_boosts + end + + test "popped" do + assert_equal [ bubbles(:shipping) ], Bubble.popped + end + + test "active" do + assert_equal bubbles(:logo, :layout, :text), Bubble.active + end + + test "unassigned" do + assert_equal bubbles(:shipping, :text), Bubble.unassigned + end + + test "assigned to" do + assert_equal bubbles(:logo, :layout), Bubble.assigned_to(users(:jz)) + end + + test "in bucket" do + new_bucket = accounts("37s").buckets.create! name: "New Bucket", creator: users(:david) + assert_equal bubbles(:logo, :shipping, :layout, :text), Bubble.in_bucket(buckets(:writebook)) + assert_empty Bubble.in_bucket(new_bucket) + end + + test "tagged with" do + assert_equal bubbles(:layout, :text), Bubble.tagged_with(tags(:mobile)) end test "mentioning" do diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb new file mode 100644 index 000000000..f66466235 --- /dev/null +++ b/test/models/filter_test.rb @@ -0,0 +1,51 @@ +require "test_helper" + +class FilterTest < ActiveSupport::TestCase + test "idempotent creation" do + assert_difference "Filter.count", +1 do + filter = users(:david).filters.idempotent_create!(indexed_by: "most_boosted") + assert_equal filter, users(:david).filters.idempotent_create!(indexed_by: "most_boosted") + end + end + + test "bubbles" do + Current.set session: sessions(:david) do + @inaccessible_bucket = accounts("37s").buckets.create! name: "Inaccessible Bucket" + @inaccessible_bubble = @inaccessible_bucket.bubbles.create! + end + + assert_not_includes users(:kevin).filters.new.bubbles, @inaccessible_bubble + 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 + end + + test "cacheable" do + assert_not filters(:jz_assignments).cacheable? + assert users(:david).filters.create!(bucket_ids: [ buckets(:writebook).id ]).cacheable? + end + + test "resource removal" do + filter = users(:david).filters.create! tag_ids: [ tags(:mobile).id ], bucket_ids: [ buckets(:writebook).id ] + assert_includes filter.tags, tags(:mobile) + assert_includes filter.buckets, buckets(:writebook) + + assert_changes "filter.reload.updated_at" do + tags(:mobile).destroy! + end + + assert_changes "Filter.exists?(filter.id)" do + buckets(:writebook).destroy! + end + end + + test "summary" do + 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 +end From e6cf8d3571cfb849fd146ab44f8c89be88ee2b80 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 17:52:31 -0600 Subject: [PATCH 06/23] Delegate default_params to class --- app/models/filter/params.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index df2354ebd..eb15c26a2 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -35,10 +35,12 @@ module Filter::Params end def indexed_by - (params["indexed_by"] || self.class.default_params["indexed_by"]).inquiry + (params["indexed_by"] || default_params["indexed_by"]).inquiry end private + delegate :default_params, to: :class, private: true + def sanitize_params denormalize_resource_ids self.params = non_default_params @@ -46,6 +48,6 @@ module Filter::Params end def non_default_params - params.reject { |k, v| self.class.default_params[k] == v } + params.reject { |k, v| default_params[k] == v } end end From a853f75ef94a7031bdff400d3108c92fef63ca75 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 17:54:43 -0600 Subject: [PATCH 07/23] No need to set resources in `to_params` anymore They're denormalized now --- app/models/filter/params.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index eb15c26a2..cd4855b08 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -15,10 +15,8 @@ module Filter::Params end def to_params - params.merge(tag_ids: tags.ids.presence, assignee_ids: assignees.ids.presence, bucket_ids: buckets.ids.presence).then do |params| - ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| - params[:filter_id] = id if persisted? - end + ActionController::Parameters.new(params).permit(*KNOWN_PARAMS).tap do |params| + params[:filter_id] = id if persisted? end end From be0973705cb324b8be212a51cd39445382143562 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 17:56:46 -0600 Subject: [PATCH 08/23] Unnecessary newline --- app/models/filter.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/filter.rb b/app/models/filter.rb index 7ff26f981..b93965ac2 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -2,7 +2,6 @@ class Filter < ApplicationRecord include Params, Resources, Summarized belongs_to :creator, class_name: "User", default: -> { Current.user } - has_one :account, through: :creator class << self From 49f3894bcb49f12115963ec9b5ebd7a34e276bc5 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:01:40 -0600 Subject: [PATCH 09/23] Update denormalization comment --- app/models/filter/resources.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb index 6f04ded0a..7708b33e8 100644 --- a/app/models/filter/resources.rb +++ b/app/models/filter/resources.rb @@ -15,7 +15,9 @@ module Filter::Resources end private - # `denormalize_resource_ids` stores ids in the params column to enforce uniqueness with a db constraint. + # `denormalize_resource_ids` stores resource ids in the params column to + # 1) enforce uniqueness via db constraints + # 2) easily turn all filter params into a query string def denormalize_resource_ids params["bucket_ids"] = buckets.ids params["tag_ids"] = tags.ids From 32618debf627fe7e546c239c3daf1bb7c0868f6f Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:05:15 -0600 Subject: [PATCH 10/23] Pull out `strip_default_params` --- app/models/filter/params.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index cd4855b08..1340e57f5 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -41,11 +41,11 @@ module Filter::Params def sanitize_params denormalize_resource_ids - self.params = non_default_params + strip_default_params params.compact_blank! end - def non_default_params - params.reject { |k, v| default_params[k] == v } + def strip_default_params + self.params = params.reject { |k, v| default_params[k] == v } end end From 9efe94a2774ead7c65ca1d13017c2ae6d35d79cd Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:09:55 -0600 Subject: [PATCH 11/23] idempotent_create -> persist --- app/controllers/filters_controller.rb | 2 +- app/models/filter.rb | 4 ++-- test/models/filter_test.rb | 9 ++++++--- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/controllers/filters_controller.rb b/app/controllers/filters_controller.rb index 9d9964e52..ba3c2d94a 100644 --- a/app/controllers/filters_controller.rb +++ b/app/controllers/filters_controller.rb @@ -2,7 +2,7 @@ class FiltersController < ApplicationController before_action :set_filter, :remember_params, only: :destroy def create - @filter = Current.user.filters.idempotent_create!(filter_params).tap(&:touch) + @filter = Current.user.filters.persist! filter_params redirect_to bubbles_path(@filter.to_params) end diff --git a/app/models/filter.rb b/app/models/filter.rb index b93965ac2..63d0555ab 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -5,12 +5,12 @@ class Filter < ApplicationRecord has_one :account, through: :creator class << self - def idempotent_create!(attrs) + def persist!(attrs) filter = new(attrs) filter.save! filter rescue ActiveRecord::RecordNotUnique - find_by! params: filter.params # possible thanks to denormalized params + find_by!(params: filter.params).tap(&:touch) # possible thanks to denormalized params end end diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index f66466235..ec29c99ae 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -1,10 +1,13 @@ require "test_helper" class FilterTest < ActiveSupport::TestCase - test "idempotent creation" do + test "persistence" do assert_difference "Filter.count", +1 do - filter = users(:david).filters.idempotent_create!(indexed_by: "most_boosted") - assert_equal filter, users(:david).filters.idempotent_create!(indexed_by: "most_boosted") + filter = users(:david).filters.persist!(indexed_by: "most_boosted") + + assert_changes "filter.reload.updated_at" do + assert_equal filter, users(:david).filters.persist!(indexed_by: "most_boosted") + end end end From f7495ac1fa838cfb4551f41083e361b4258f2b1e Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:13:05 -0600 Subject: [PATCH 12/23] Improve denormalized filters note --- app/models/filter.rb | 2 +- app/models/filter/resources.rb | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/filter.rb b/app/models/filter.rb index 63d0555ab..4dbd12eed 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -10,7 +10,7 @@ class Filter < ApplicationRecord filter.save! filter rescue ActiveRecord::RecordNotUnique - find_by!(params: filter.params).tap(&:touch) # possible thanks to denormalized params + find_by!(params: filter.params).tap(&:touch) end end diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb index 7708b33e8..399c9d271 100644 --- a/app/models/filter/resources.rb +++ b/app/models/filter/resources.rb @@ -16,8 +16,9 @@ module Filter::Resources private # `denormalize_resource_ids` stores resource ids in the params column to - # 1) enforce uniqueness via db constraints - # 2) easily turn all filter params into a query string + # 1) Enforce uniqueness via db constraints + # 2) Look up identical filters by a single column + # 3) Easily turn all filter params into a query string def denormalize_resource_ids params["bucket_ids"] = buckets.ids params["tag_ids"] = tags.ids From e73f44cde9d7652000aeff2001fa59110b11f229 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:16:18 -0600 Subject: [PATCH 13/23] Unnecessary parens --- app/models/filter/resources.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb index 399c9d271..f3e364580 100644 --- a/app/models/filter/resources.rb +++ b/app/models/filter/resources.rb @@ -9,7 +9,7 @@ module Filter::Resources def resource_removed(resource) kind = resource.class.model_name.plural - send("#{kind}=", send(kind).without(resource)) + send "#{kind}=", send(kind).without(resource) sanitize_params params.blank? ? destroy! : save! end From f53ee0ddac77efcac3db02ff3fd3b43c1cbcfca2 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:19:19 -0600 Subject: [PATCH 14/23] Sanitize params after initialization --- app/models/filter/params.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index 1340e57f5..160c30775 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -11,6 +11,7 @@ module Filter::Params end included do + after_initialize :sanitize_params before_validation :sanitize_params end From ec04ccef4d8a885cf46165d7e2d3c8cdef65f8ed Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Wed, 6 Nov 2024 18:22:10 -0600 Subject: [PATCH 15/23] Test param sanitization --- test/models/filter_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index ec29c99ae..0b3e8ba46 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -25,6 +25,12 @@ class FilterTest < ActiveSupport::TestCase assert_equal expected.stringify_keys, filters(:jz_assignments).to_params.to_h end + test "param sanitization" 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.stringify_keys, filter.params + end + test "cacheable" do assert_not filters(:jz_assignments).cacheable? assert users(:david).filters.create!(bucket_ids: [ buckets(:writebook).id ]).cacheable? From 4c02a46867b7470d23a562043f8ac3cc9de65021 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 11:32:21 -0600 Subject: [PATCH 16/23] Comment on the query merger controller --- app/javascript/controllers/query_merger_controller.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/javascript/controllers/query_merger_controller.js b/app/javascript/controllers/query_merger_controller.js index 2fd17d65d..3681578e3 100644 --- a/app/javascript/controllers/query_merger_controller.js +++ b/app/javascript/controllers/query_merger_controller.js @@ -1,5 +1,6 @@ import { Controller } from "@hotwired/stimulus" +// FIXME: Can we do this without a controller? https://github.com/basecamp/fizzy/pull/130#discussion_r1833094616 export default class extends Controller { merge({ params: { key, value } }) { const url = new URL(window.location.href) From 8f17152108019caa70795b444a4dbeb18c86e1d8 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 11:32:44 -0600 Subject: [PATCH 17/23] Put includes on the same line --- app/models/bucket.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/bucket.rb b/app/models/bucket.rb index 3f986e0fb..ad8c0ce53 100644 --- a/app/models/bucket.rb +++ b/app/models/bucket.rb @@ -1,6 +1,5 @@ class Bucket < ApplicationRecord - include Filterable - include Accessible + include Accessible, Filterable belongs_to :account belongs_to :creator, class_name: "User", default: -> { Current.user } From 201a543a9b60311400b6512b9dbfe2a0b9e0f88b Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 11:34:56 -0600 Subject: [PATCH 18/23] Assert params after resource removal --- test/models/filter_test.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 0b3e8ba46..00ac7c9f6 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -38,12 +38,16 @@ class FilterTest < ActiveSupport::TestCase test "resource removal" do filter = users(:david).filters.create! tag_ids: [ tags(:mobile).id ], bucket_ids: [ buckets(:writebook).id ] + + assert_includes filter.params["tag_ids"], tags(:mobile).id assert_includes filter.tags, tags(:mobile) + assert_includes filter.params["bucket_ids"], buckets(:writebook).id assert_includes filter.buckets, buckets(:writebook) assert_changes "filter.reload.updated_at" do tags(:mobile).destroy! end + assert_nil filter.reload.params["tag_ids"] assert_changes "Filter.exists?(filter.id)" do buckets(:writebook).destroy! From fb08ff394d3b8dffcdf04f4c6246cd58b7c946a4 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 11:35:26 -0600 Subject: [PATCH 19/23] exists? -> any? --- app/models/filter/summarized.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/filter/summarized.rb b/app/models/filter/summarized.rb index a1286da23..67f194e81 100644 --- a/app/models/filter/summarized.rb +++ b/app/models/filter/summarized.rb @@ -13,13 +13,13 @@ module Filter::Summarized end def tag_summary - if tags.exists? + if tags.any? "tagged #{tags.map(&:hashtag).to_choice_sentence}" end end def assignee_summary - if assignees.exists? + if assignees.any? "assigned to #{assignees.pluck(:name).to_choice_sentence}" elsif assignments.unassigned? "assigned to no one" @@ -27,7 +27,7 @@ module Filter::Summarized end def bucket_summary - if buckets.exists? + if buckets.any? "in #{buckets.pluck(:name).to_choice_sentence}" else "in all projects" From 77a9b267d8fccd9bba2251437b7f4cf67063401d Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 12:44:26 -0600 Subject: [PATCH 20/23] Split out `fields` and `params` for filters --- app/models/filter.rb | 2 +- app/models/filter/fields.rb | 30 ++++++++++++ app/models/filter/params.rb | 48 +++++++------------ app/models/filter/resources.rb | 14 +----- ...05181312_rename_bucket_views_to_filters.rb | 4 +- db/schema.rb | 1 + test/fixtures/filters.yml | 3 +- 7 files changed, 54 insertions(+), 48 deletions(-) create mode 100644 app/models/filter/fields.rb diff --git a/app/models/filter.rb b/app/models/filter.rb index 4dbd12eed..58edae90f 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -1,5 +1,5 @@ class Filter < ApplicationRecord - include Params, Resources, Summarized + include Fields, Params, Resources, Summarized belongs_to :creator, class_name: "User", default: -> { Current.user } has_one :account, through: :creator diff --git a/app/models/filter/fields.rb b/app/models/filter/fields.rb new file mode 100644 index 000000000..2db92f0e3 --- /dev/null +++ b/app/models/filter/fields.rb @@ -0,0 +1,30 @@ +module Filter::Fields + extend ActiveSupport::Concern + + INDEXES = %w[ most_active most_discussed most_boosted newest oldest popped ] + + class_methods do + def default_fields + { "indexed_by" => "most_active" } + end + end + + def assignments=(value) + fields["assignments"] = value + end + + def assignments + fields["assignments"].to_s.inquiry + end + + def indexed_by=(value) + fields["indexed_by"] = value + end + + def indexed_by + (fields["indexed_by"] || default_fields["indexed_by"]).inquiry + end + + private + delegate :default_fields, to: :class, private: true +end diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index 160c30775..45d28c4b6 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -2,17 +2,9 @@ module Filter::Params extend ActiveSupport::Concern KNOWN_PARAMS = [ :indexed_by, :assignments, bucket_ids: [], assignee_ids: [], tag_ids: [] ] - INDEXES = %w[ most_active most_discussed most_boosted newest oldest popped ] - - class_methods do - def default_params - { "indexed_by" => "most_active" } - end - end included do - after_initialize :sanitize_params - before_validation :sanitize_params + after_initialize :derive_params end def to_params @@ -21,32 +13,24 @@ module Filter::Params end end - def assignments=(value) - params["assignments"] = value - end - - def assignments - params["assignments"].to_s.inquiry - end - - def indexed_by=(value) - params["indexed_by"] = value - end - - def indexed_by - (params["indexed_by"] || default_params["indexed_by"]).inquiry - end - private - delegate :default_params, to: :class, private: true - - def sanitize_params - denormalize_resource_ids - strip_default_params + # `derive_params` stores a denormalized version of the filter in `params` to + # 1) Enforce uniqueness via db constraints + # 2) Look up identical filters by a single column + # 3) Easily turn all filter params into a query string + def derive_params + derive_params_from_resource_ids + derive_params_from_fields params.compact_blank! end - def strip_default_params - self.params = params.reject { |k, v| default_params[k] == v } + def derive_params_from_resource_ids + params["tag_ids"] = tags.ids + params["bucket_ids"] = buckets.ids + params["assignee_ids"] = assignees.ids + end + + def derive_params_from_fields + self.params.merge! fields.reject { |k, v| default_fields[k] == v } end end diff --git a/app/models/filter/resources.rb b/app/models/filter/resources.rb index f3e364580..5215087f6 100644 --- a/app/models/filter/resources.rb +++ b/app/models/filter/resources.rb @@ -10,18 +10,6 @@ module Filter::Resources def resource_removed(resource) kind = resource.class.model_name.plural send "#{kind}=", send(kind).without(resource) - sanitize_params - params.blank? ? destroy! : save! + derived_params.blank? ? destroy! : save! end - - private - # `denormalize_resource_ids` stores resource ids in the params column to - # 1) Enforce uniqueness via db constraints - # 2) Look up identical filters by a single column - # 3) Easily turn all filter params into a query string - def denormalize_resource_ids - params["bucket_ids"] = buckets.ids - params["tag_ids"] = tags.ids - params["assignee_ids"] = assignees.ids - end end diff --git a/db/migrate/20241105181312_rename_bucket_views_to_filters.rb b/db/migrate/20241105181312_rename_bucket_views_to_filters.rb index a7ab1f4ce..6e02f2020 100644 --- a/db/migrate/20241105181312_rename_bucket_views_to_filters.rb +++ b/db/migrate/20241105181312_rename_bucket_views_to_filters.rb @@ -5,10 +5,12 @@ class RenameBucketViewsToFilters < ActiveRecord::Migration[8.0] remove_index :filters, %i[ bucket_id creator_id filters ], unique: true remove_index :filters, :creator_id - remove_column :filters, :bucket_id + remove_reference :filters, :bucket rename_column :filters, :filters, :params + add_column :filters, :fields, :jsonb, null: false, default: {} + add_index :filters, %i[ creator_id params ], unique: true end end diff --git a/db/schema.rb b/db/schema.rb index f77d399a4..bbb0daf2c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -134,6 +134,7 @@ ActiveRecord::Schema[8.0].define(version: 2024_11_05_224305) do t.json "params", default: {}, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.json "fields", default: {}, null: false t.index ["creator_id", "params"], name: "index_filters_on_creator_id_and_params", unique: true end diff --git a/test/fixtures/filters.yml b/test/fixtures/filters.yml index 7dfaa219f..b80de3a69 100644 --- a/test/fixtures/filters.yml +++ b/test/fixtures/filters.yml @@ -1,5 +1,6 @@ jz_assignments: creator: david - params: <%= { indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json %> tags: mobile assignees: jz + fields: <%= { indexed_by: :most_discussed }.to_json %> + params: <%= { indexed_by: :most_discussed, tag_ids: [ ActiveRecord::FixtureSet.identify(:mobile) ], assignee_ids: [ ActiveRecord::FixtureSet.identify(:jz) ] }.to_json %> From aca9a69ce1f8fe0032bc8f0b177ad784dc8adccc Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 12:48:27 -0600 Subject: [PATCH 21/23] Add derived_params alias --- app/models/filter/params.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/filter/params.rb b/app/models/filter/params.rb index 45d28c4b6..2726e153a 100644 --- a/app/models/filter/params.rb +++ b/app/models/filter/params.rb @@ -23,6 +23,7 @@ module Filter::Params derive_params_from_fields params.compact_blank! end + alias_method :derived_params, :derive_params def derive_params_from_resource_ids params["tag_ids"] = tags.ids From 5cc4aa063401616f25cb14c8cddb56d425b6b44a Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 16:09:12 -0600 Subject: [PATCH 22/23] Simplify filter destruction --- app/controllers/filters_controller.rb | 8 ++------ test/controllers/filters_controller_test.rb | 3 +-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/app/controllers/filters_controller.rb b/app/controllers/filters_controller.rb index ba3c2d94a..34a020c4f 100644 --- a/app/controllers/filters_controller.rb +++ b/app/controllers/filters_controller.rb @@ -1,5 +1,5 @@ class FiltersController < ApplicationController - before_action :set_filter, :remember_params, only: :destroy + before_action :set_filter, only: :destroy def create @filter = Current.user.filters.persist! filter_params @@ -16,10 +16,6 @@ class FiltersController < ApplicationController @filter = Current.user.filters.find params[:id] end - def remember_params - @filter_params = @filter.to_params - end - def filter_params params.permit(*Filter::KNOWN_PARAMS).compact_blank end @@ -28,7 +24,7 @@ class FiltersController < ApplicationController if request.referer == root_url redirect_to root_path else - redirect_to bubbles_path(@filter_params) + redirect_to bubbles_path(@filter.to_params) end end end diff --git a/test/controllers/filters_controller_test.rb b/test/controllers/filters_controller_test.rb index 332b11d5e..7d3c59817 100644 --- a/test/controllers/filters_controller_test.rb +++ b/test/controllers/filters_controller_test.rb @@ -25,10 +25,9 @@ class FiltersControllerTest < ActionDispatch::IntegrationTest end test "destroy" do - params = filters(:jz_assignments).to_params assert_difference "Filter.count", -1 do delete filter_url(filters(:jz_assignments)) end - assert_redirected_to bubbles_path(params) + assert_redirected_to bubbles_path(filters(:jz_assignments).params) end end From da37fa56d86a822c6c99cf181d1a654eeb724646 Mon Sep 17 00:00:00 2001 From: Jose Farias Date: Thu, 7 Nov 2024 16:13:00 -0600 Subject: [PATCH 23/23] Add some more tests for bubble filters --- test/models/filter_test.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 00ac7c9f6..5d5c4d261 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -13,11 +13,20 @@ class FilterTest < ActiveSupport::TestCase test "bubbles" do Current.set session: sessions(:david) do - @inaccessible_bucket = accounts("37s").buckets.create! name: "Inaccessible Bucket" - @inaccessible_bubble = @inaccessible_bucket.bubbles.create! + @new_bucket = accounts("37s").buckets.create! name: "Inaccessible Bucket" + @new_bubble = @new_bucket.bubbles.create! end - assert_not_includes users(:kevin).filters.new.bubbles, @inaccessible_bubble + assert_not_includes users(:kevin).filters.new.bubbles, @new_bubble + + filter = users(:david).filters.new indexed_by: "most_discussed", assignee_ids: [ users(:jz).id ], tag_ids: [ tags(:mobile).id ] + assert_equal [ bubbles(:layout) ], filter.bubbles + + filter = users(:david).filters.new assignments: "unassigned", bucket_ids: [ @new_bucket.id ] + assert_equal [ @new_bubble ], filter.bubbles + + filter = users(:david).filters.new indexed_by: "popped" + assert_equal [ bubbles(:shipping) ], filter.bubbles end test "turning into params" do