Merge pull request #2261 from ujh/better-pens-leader-board

Pens leader board using the data from Pens::Model

app/models/leader_board.rb

@@ -13,7 +13,7 @@ class LeaderBoard
   WORKERS = TYPES.map { |t| "refresh_leader_board/#{t}".camelize.constantize }
 
   def self.refresh!(type)
-    self.send(type, force: true)
+    send(type, force: true)
   end
 
   def self.users_by_description_edits(force: false)
@@ -27,15 +27,13 @@ def self.top_users_by_description_edits
   def self.pens_by_popularity(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#pens_by_popularity", force: force) do
-        CollectedPen
-          .group("lower(concat(brand, model))")
-          .where(archived_on: nil)
-          .select(
-            "initcap(min(brand)) as brand, initcap(min(model)) as model, count(id) as count"
-          )
-          .having("count(distinct user_id) > 1")
-          .order("count desc")
+      .fetch("LeaderBoard#pens_by_popularity", force:) do
+        Pens::Model
+          .includes(:collected_pens)
+          .reject { |model| model.collected_pens.size.zero? }
+          .map do |model|
+            { name: model.name, count: model.collected_pens.size }
+          end
       end
   end
 
@@ -46,7 +44,7 @@ def self.top_pens_by_popularity
   def self.ink_review_submissions(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#ink_review_submissions", force: force) do
+      .fetch("LeaderBoard#ink_review_submissions", force:) do
         relation =
           User
             .where("users.id <> 1")
@@ -65,7 +63,7 @@ def self.top_ink_review_submissions
   def self.usage_records(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#usage_records", force: force) do
+      .fetch("LeaderBoard#usage_records", force:) do
         relation =
           User
             .joins(currently_inkeds: :usage_records)
@@ -83,7 +81,7 @@ def self.top_usage_records
   def self.currently_inked(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#currently_inked", force: force) do
+      .fetch("LeaderBoard#currently_inked", force:) do
         relation =
           User
             .joins(:currently_inkeds)
@@ -101,7 +99,7 @@ def self.top_currently_inked
   def self.inks_by_popularity(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#inks_by_popularity", force: force) do
+      .fetch("LeaderBoard#inks_by_popularity", force:) do
         MacroCluster
           .joins(micro_clusters: :collected_inks)
           .includes(:brand_cluster)
@@ -117,7 +115,7 @@ def self.top_inks_by_popularity
   end
 
   def self.inks(force: false)
-    Rails.cache.fetch("LeaderBoard#inks", force: force) { extract(build) }
+    Rails.cache.fetch("LeaderBoard#inks", force:) { extract(build) }
   end
 
   def self.top_inks
@@ -127,7 +125,7 @@ def self.top_inks
   def self.bottles(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#bottles", force: force) do
+      .fetch("LeaderBoard#bottles", force:) do
         extract(build.where(collected_inks: { kind: "bottle" }))
       end
   end
@@ -139,7 +137,7 @@ def self.top_bottles
   def self.samples(force: false)
     Rails
       .cache
-      .fetch("LeaderBoard#samples", force: force) do
+      .fetch("LeaderBoard#samples", force:) do
         extract(build.where(collected_inks: { kind: "sample" }))
       end
   end

app/models/leader_board_row/pen_models.rb

@@ -0,0 +1,2 @@
+class LeaderBoardRow::PenModels < LeaderBoardRow
+end

app/models/pens/model.rb

@@ -4,21 +4,27 @@ class Pens::Model < ApplicationRecord
            class_name: "Pens::ModelMicroCluster",
            dependent: :nullify
   has_many :model_variants, through: :model_micro_clusters
+  has_many :micro_clusters, through: :model_variants
+  has_many :collected_pens, through: :micro_clusters
 
   paginates_per 100
 
-  scope :ordered, lambda { order(:brand, :model) }
+  scope :ordered, -> { order(:brand, :model) }
 
   def self.search(query)
     return self if query.blank?
 
     query = query.split(/\s+/).join("%")
     joins(model_micro_clusters: :model_variants).where(
       <<~SQL,
-      CONCAT(pens_model_variants.brand, pens_model_variants.model)
-      ILIKE ?
-    SQL
+        CONCAT(pens_model_variants.brand, pens_model_variants.model)
+        ILIKE ?
+      SQL
       "%#{query}%"
     ).group("pens_models.id")
   end
+
+  def name
+    "#{brand} #{model}"
+  end
 end

app/views/pages/leaderboards.html.slim

@@ -79,10 +79,10 @@ div.row.mb-3
           = " (#{macro_cluster.ci_count})"
     p= link_to "Show all", page_path("inks_by_popularity")
 
-  div class="col-sm-12 fpc-leaderboards__board"
+  div class="col-sm-12 col-md-6 fpc-leaderboards__board"
     h2 class="h4" Popular pens
     p.text-muted Ranked by number of people who own this
     ol
       - LeaderBoard.top_pens_by_popularity.each do |pen|
-        li= "#{pen.brand} #{pen.model} (#{pen.count})"
+        li= "#{pen[:name]} (#{pen[:count]})"
     p= link_to "Show all", page_path("pens_by_popularity")

app/views/pages/pens_by_popularity.html.slim

@@ -9,4 +9,4 @@
 
 ol class="fpc-leaderboard"
   - LeaderBoard.pens_by_popularity.each do |pen|
-    li="#{pen.brand} #{pen.model} (#{pen.count})"
+    li="#{pen[:name]} (#{pen[:count]})"

spec/models/leader_board_spec.rb

@@ -1,61 +1,6 @@
 require "rails_helper"
 
 describe LeaderBoard do
-  describe "#pens_by_popularity" do
-    it "orders the pens by popularity" do
-      user1 = create(:user)
-      create(:collected_pen, user: user1, brand: "Brand1", model: "Model1")
-      create(:collected_pen, user: user1, brand: "Brand1", model: "Model2")
-      user2 = create(:user)
-      create(:collected_pen, user: user2, brand: "Brand1", model: "Model1")
-      create(:collected_pen, user: user2, brand: "Brand1", model: "Model2")
-      user3 = create(:user)
-      create(:collected_pen, user: user3, brand: "Brand1", model: "Model1")
-
-      expect(
-        described_class.pens_by_popularity.map do |pen|
-          [pen.brand, pen.model, pen.count]
-        end
-      ).to eq([["Brand1", "Model1", 3], ["Brand1", "Model2", 2]])
-    end
-
-    it "does not count pens that only appear once" do
-      user = create(:user)
-      create(:collected_pen, user: user, brand: "Brand1", model: "Model1")
-
-      expect(described_class.pens_by_popularity).to be_empty
-    end
-
-    it "does not count pens that only one user has (but multiple)" do
-      user = create(:user)
-      create(:collected_pen, user: user, brand: "Brand1", model: "Model1")
-      create(:collected_pen, user: user, brand: "Brand1", model: "Model1")
-
-      expect(described_class.pens_by_popularity).to be_empty
-    end
-
-    it "does not include archived entries" do
-      user1 = create(:user)
-      create(
-        :collected_pen,
-        user: user1,
-        brand: "Brand1",
-        model: "Model1",
-        archived_on: Date.today
-      )
-      user2 = create(:user)
-      create(
-        :collected_pen,
-        user: user2,
-        brand: "Brand1",
-        model: "Model1",
-        archived_on: Date.today
-      )
-
-      expect(described_class.pens_by_popularity).to be_empty
-    end
-  end
-
   describe "#top_pens_by_popularity" do
     it "returns the first 10 entries" do
       allow(described_class).to receive(:pens_by_popularity).and_return(
@@ -162,7 +107,7 @@
 
     it "returns the user name" do
       user = create(:user, name: "the name")
-      create(:collected_ink, user: user)
+      create(:collected_ink, user:)
       expect(described_class.inks.first).to include(public_name: "the name")
     end
 
@@ -173,7 +118,7 @@
 
     it "does not include users with only private inks" do
       user = create(:user)
-      create(:collected_ink, user: user, private: true)
+      create(:collected_ink, user:, private: true)
       expect(described_class.inks).to be_empty
     end
 
@@ -191,7 +136,7 @@
 
     it "returns the patreon status" do
       user = create(:user, patron: true)
-      create(:collected_ink, user: user)
+      create(:collected_ink, user:)
       expect(described_class.inks.first).to include(patron: true)
     end
   end