Split led_count from bookmarks_count (and add sorting by bookmarks_count) (#591)

maebeale · web-flow · commit 6dba3946ef0d · 2025-12-27T17:09:48.000-05:00
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
@@ -40,7 +40,6 @@ def personal
   def create
     @bookmark = current_user.bookmarks.find_or_create_by(bookmark_params)
     @bookmarkable = @bookmark.bookmarkable
-    @bookmarkable.update(led_count: @bookmarkable.led_count + 1) if @bookmarkable.has_attribute?(:led_count)
     respond_to do |format|
       format.html {
         redirect_to authenticated_root_path, notice: "#{@bookmark.bookmarkable_type} added to your bookmarks."
@@ -63,7 +62,6 @@ def destroy
     if @bookmark
       @bookmark.destroy
       @bookmarkable = @bookmark.bookmarkable
-      @bookmarkable.update(led_count: @bookmarkable.led_count - 1) if @bookmarkable.has_attribute?(:led_count)
       respond_to do |format|
         format.html {
           redirect_to authenticated_root_path, notice: "Bookmark has been deleted."
diff --git a/app/models/resource.rb b/app/models/resource.rb
@@ -103,11 +103,6 @@ def custom_label_list
     "#{self.title} (#{self.kind.upcase})" unless self.kind.nil?
   end
 
-  # Methods
-  def led_count
-    0
-  end
-
   def name
     title || id
   end
diff --git a/app/models/workshop.rb b/app/models/workshop.rb
@@ -87,6 +87,11 @@ class Workshop < ApplicationRecord
     ) #{sort_order == "asc" ? "ASC" : "DESC"}
     SQL
   }
+  scope :with_bookmarks_count, -> {
+    left_joins(:bookmarks)
+      .select("workshops.*, COUNT(bookmarks.id) AS bookmarks_count")
+      .group("workshops.id")
+  }
 
   # Search Cop
   include SearchCop
diff --git a/app/services/workshop_search_service.rb b/app/services/workshop_search_service.rb
@@ -5,8 +5,13 @@ class WorkshopSearchService
 	def initialize(params = {}, super_user: false)
 		@params = params
 		@super_user = super_user
-		@workshops = Workshop.all
 		@sort = default_sort
+		@workshops =
+			if @sort == "popularity"
+				Workshop.with_bookmarks_count
+			else
+				Workshop.all
+			end
 	end
 
 	# Main entry point
@@ -196,45 +201,55 @@ def normalize_published_param
 
 	def order_by_params
 		case sort
-		when 'created'
-			# order by year/month desc, then created_at desc, then title asc
+		when "created"
 			@workshops = @workshops.order(
 				Arel.sql(<<~SQL.squish)
-          CASE WHEN year IS NOT NULL AND month IS NOT NULL THEN 1 ELSE 2 END ASC,
-          year DESC,
-          month DESC,
-          created_at DESC,
-          title ASC
-        SQL
+        CASE WHEN year IS NOT NULL AND month IS NOT NULL THEN 1 ELSE 2 END ASC,
+        year DESC,
+        month DESC,
+        created_at DESC,
+        title ASC
+      SQL
 			)
-		when 'led'
+		when "led"
 			@workshops = @workshops.order(led_count: :desc, title: :asc)
-		when 'title'
+		when "popularity"
+			@workshops = @workshops.order(
+				Arel.sql("bookmarks_count DESC, title ASC")
+			)
+		when "title"
 			@workshops = @workshops.order(title: :asc)
-		when 'keywords'
-			# already ordered in filter_by_query
+		when "keywords"
+			# already ordered
 		else
 			@workshops = @workshops.order(created_at: :asc, title: :asc)
 		end
 	end
 
+
 	# --- Handle distinct + order by FIELD(id, ...) for complex joins ---
 	def resolve_ids_order
-		return if sort == 'keywords' # ordering is already handled if this is the case
-
-		# Determine sort columns for select
-		sort_columns = case sort
-									 when 'created' then [:id, :created_at, :year, :month, :title]
-									 when 'led'     then [:id, :led_count, :title]
-									 when 'title'   then [:id, :title]
-									 else [:id, :title]
-									 end
-
-    workshop_ids = @workshops
-      .select(*sort_columns)
-      .order(sort_columns)
-      .map(&:id)
-		@workshops = Workshop.where(id: workshop_ids)
-												 .order(Arel.sql("FIELD(id, #{workshop_ids.join(',')})"))
+		return if sort == "keywords"
+
+		sort_columns =
+			case sort
+			when "created"    then [:id, :created_at, :year, :month, :title]
+			when "led"        then [:id, :led_count, :title]
+			when "popularity" then [:id, :bookmarks_count, :title]
+			when "title"      then [:id, :title]
+			else [:id, :title]
+			end
+
+		workshop_ids =
+			@workshops
+				.select(*sort_columns)
+				.order(sort_columns)
+				.map(&:id)
+
+		@workshops =
+			Workshop
+				.where(id: workshop_ids)
+				.order(Arel.sql("FIELD(id, #{workshop_ids.join(',')})"))
 	end
+
 end
diff --git a/app/views/bookmarks/_flex.html.erb b/app/views/bookmarks/_flex.html.erb
@@ -147,7 +147,7 @@
       </div>
 
       <div class="text-sm text-gray-600">
-        Bookmarked:
+        Led:
         <%= workshop.led_count %>
         <%= "time".pluralize(workshop.led_count) %>
       </div>
diff --git a/app/views/bookmarks/tally.html.erb b/app/views/bookmarks/tally.html.erb
@@ -45,7 +45,7 @@
     <table class="w-full border border-gray-200 bg-gray-300 rounded-lg shadow-sm">
       <thead>
         <tr>
-          <th colspan="2" class="bg-gray-400 px-4 py-2 text-white">Workshop Led Counts</th>
+          <th colspan="2" class="bg-gray-400 px-4 py-2 text-white">Workshops led (via logs)</th>
         </tr>
         <tr class="bg-gray-400">
           <th class="px-4 py-2 text-left">Count</th>
diff --git a/app/views/bookmarks/update.turbo_stream.erb b/app/views/bookmarks/update.turbo_stream.erb
@@ -1,16 +1,21 @@
 <%= turbo_stream.replace dom_id(@bookmarkable, :bookmark_icon), partial: "editable_bookmark_icon", locals: {resource: @bookmarkable} %>
 <%= turbo_stream.replace dom_id(@bookmarkable, :bookmark_button), partial: "editable_bookmark_button", locals: {resource: @bookmarkable} %>
 
-<% if @bookmarkable.has_attribute?(:led_count)  %>
-  <%= turbo_stream.update dom_id(@bookmarkable, :bookmark_count) do  %>
-       Bookmarked:
-       <%= @bookmarkable.led_count %>
-       <%= "time".pluralize(@bookmarkable.led_count) %>
-  <% end  %>
-  <%= turbo_stream.update dom_id(@bookmarkable, :bookmark_count_facilitator) do  %>
-      (Bookmarked by
-      <%= pluralize(@bookmarkable.led_count, "facilitator") %>)
-  <% end  %>
+<%= turbo_stream.update dom_id(@bookmarkable, :led_count) do  %>
+  Led:
+  <%= @bookmarkable.led_count %>
+  <%= "time".pluralize(@bookmarkable.led_count) %>)
+<% end  %>
+
+<%= turbo_stream.update dom_id(@bookmarkable, :bookmark_count) do  %>
+  Bookmarked:
+  <%= @bookmarkable.bookmarks_count %>
+  <%= "time".pluralize(@bookmarkable.bookmarks_count) %>
+<% end  %>
+
+<%= turbo_stream.update dom_id(@bookmarkable, :bookmark_count_facilitator) do  %>
+  (Bookmarked by
+  <%= pluralize(@bookmarkable.bookmarks_count, "facilitator") %>,
 <% end  %>
 
 <%= turbo_stream.replace "flash_now", partial: "shared/flash_messages" %>
diff --git a/app/views/workshops/_index_row.html.erb b/app/views/workshops/_index_row.html.erb
@@ -65,7 +65,13 @@
         <!-- Bookmarked count -->
         <%= tag.div id: dom_id(workshop, :bookmark_count),
                     class: "text-sm text-gray-600 leading-tight" do %>
-          Bookmarked: <%= workshop.led_count %> <%= "time".pluralize(workshop.led_count) %>
+          Bookmarked: <%= workshop.bookmarks_count %> <%= "time".pluralize(workshop.bookmarks_count.to_i) %>
+        <% end %>
+
+        <!-- Led count -->
+        <%= tag.div id: dom_id(workshop, :led_count),
+                    class: "text-sm text-gray-600 leading-tight" do %>
+          Led: <%= workshop.led_count %> <%= "time".pluralize(workshop.led_count.to_i) %>
         <% end %>
 
         <!-- Age ranges -->
diff --git a/app/views/workshops/_show_actions_row.html.erb b/app/views/workshops/_show_actions_row.html.erb
@@ -5,7 +5,11 @@
     <%= render "bookmarks/editable_bookmark_button", resource: workshop.object %>
     <%= tag.span id: dom_id(workshop, :bookmark_count_facilitator), class: "text-gray-600 text-sm" do %>
       (Bookmarked by
-      <%= pluralize(workshop.led_count, "facilitator") %>)
+      <%= pluralize(workshop.bookmarks.count, "facilitator") %>,
+    <% end %>
+    <%= tag.span id: dom_id(workshop, :led_count), class: "text-gray-600 text-sm" do %>
+      Led:
+      <%= pluralize(workshop.led_count, "time") %>)
     <% end %>
   </div>
 
diff --git a/app/views/workshops/_sort_by_options.html.erb b/app/views/workshops/_sort_by_options.html.erb
@@ -18,6 +18,14 @@
   <label class="flex items-center gap-1 cursor-pointer">
     <%= radio_button_tag :sort, "led",
                          @sort == "led" %>
+    <span class="text-sm">Most led</span>
+  </label>
+</div>
+
+<div class="flex items-center gap-1">
+  <label class="flex items-center gap-1 cursor-pointer">
+    <%= radio_button_tag :sort, "popularity",
+                         @sort == "popularity" %>
     <span class="text-sm">Most bookmarked</span>
   </label>
 </div>
diff --git a/spec/services/workshop_search_service_spec.rb b/spec/services/workshop_search_service_spec.rb
@@ -12,6 +12,11 @@
 		create(:workshop, title: "Keyword Match", year: 2025, month: 1, created_at: 2.days.ago, led_count: 1)
 	end
 
+	let!(:bookmark_1) { create(:bookmark, bookmarkable: workshop_1) }
+	let!(:bookmark_2) { create(:bookmark, bookmarkable: workshop_1) }
+
+	let!(:bookmark_3) { create(:bookmark, bookmarkable: workshop_2) }
+
 	describe "#call" do
 		context "sorting by created" do
 			it "orders by year/month desc, then created_at desc, then title asc" do
@@ -49,6 +54,19 @@
 			end
 		end
 
+		context "sorting by bookmarks (popularity)" do
+			it "orders by bookmarks_count desc, then title asc" do
+				service = WorkshopSearchService.new({ sort: 'bookmarks' }).call
+				workshops = service.workshops
+
+				expect(workshops.map { |w| [w.bookmarks_count.to_i, w.title] }).to eq([
+																																								[2, "A Workshop"],
+																																								[1, "B Workshop"],
+																																								[0, "Keyword Match"]
+																																							])
+			end
+		end
+
 		# context "sorting by keywords" do # turned sorting by keywords off
 		# 	it "returns workshops matching the query" do
 		# 		# Stub filter_by_query to return a deterministic set
