FIX: Admin panel referral stats not counting topics correctly

Due to what seems to be a bug in ActiveRecord, the distinct: true option
is not recognized on counts with string column names. This commit fixes
that by moving the DISTINCT into the count string.

For robustness, the integration spec for IncomingLinksReport was
rewritten to be an actual integration spec, running the actual interface
on actual fake data.

app/models/incoming_links_report.rb

@@ -46,7 +46,6 @@ class IncomingLinksReport
     @per_user_query ||= IncomingLink
         .where('incoming_links.created_at > ? AND incoming_links.user_id IS NOT NULL', 30.days.ago)
         .joins(:user)
-        .joins(:post)
         .group('users.username')
   end
 
@@ -55,7 +54,7 @@ class IncomingLinksReport
   end
 
   def self.topic_count_per_user
-    per_user.count('topic_id', distinct: true)
+    per_user.joins(:post).count("DISTINCT posts.topic_id")
   end
 
 
@@ -85,14 +84,13 @@ class IncomingLinksReport
   def self.per_domain(domains)
     IncomingLink
         .joins(:incoming_referer => :incoming_domain)
-        .joins(:post)
         .where('incoming_links.created_at > ? AND incoming_domains.name IN (?)', 30.days.ago, domains)
         .group('incoming_domains.name')
   end
 
   def self.topic_count_per_domain(domains)
     # COUNT(DISTINCT) is slow
-    per_domain(domains).count('topic_id', distinct: true)
+    per_domain(domains).joins(:post).count("DISTINCT posts.topic_id")
   end
 
 

spec/fabricators/incoming_link_fabricator.rb

@@ -0,0 +1,5 @@
+Fabricator(:incoming_link) do
+  user
+  post
+  ip_address { sequence(:ip_address) { |n| "123.#{(n*3)%255}.#{(n*2)%255}.#{n%255}" } }
+end

spec/models/incoming_links_report_spec.rb

@@ -5,40 +5,76 @@ describe IncomingLinksReport do
   describe 'integration' do
     it 'runs correctly' do
       p1 = create_post
+      p2 = create_post
 
-      IncomingLink.add(
+      p1.topic.save
-        referer: 'http://test.com',
+      p2.topic.save
-        host: 'http://boo.com',
-        topic_id: p1.topic.id,
-        ip_address: '10.0.0.2',
-        username: p1.user.username
-      )
 
+      7.times do |n|
+        IncomingLink.add(
+          referer: 'http://test.com',
+          host: 'http://discourse.example.com',
+          topic_id: p1.topic.id,
+          ip_address: "10.0.0.#{n}",
+          username: p1.user.username
+        )
+      end
+      3.times do |n|
+        IncomingLink.add(
+          referer: 'http://foo.com',
+          host: 'http://discourse.example.com',
+          topic_id: p2.topic.id,
+          ip_address: "10.0.0.#{n + 7}",
+          username: p2.user.username
+        )
+      end
+      2.times do |n|
+        IncomingLink.add(
+          referer: 'http://foo.com',
+          host: 'http://discourse.example.com',
+          topic_id: p2.topic.id,
+          ip_address: "10.0.0.#{n + 7 + 3}",
+          username: p1.user.username # ! user1 is the referer !
+        )
+      end
 
-      c = IncomingLinksReport.link_count_per_topic
+      r = IncomingLinksReport.find('top_referrers').as_json
-      c[p1.topic_id].should == 1
+      r[:data].should == [
+        {username: p1.user.username, num_clicks: 7 + 2, num_topics: 2},
+        {username: p2.user.username, num_clicks: 3, num_topics: 1}
+      ]
 
-      c = IncomingLinksReport.link_count_per_domain
+      r = IncomingLinksReport.find('top_traffic_sources').as_json
-      c["test.com"].should == 1
+      r[:data].should == [
+        {domain: 'test.com', num_clicks: 7, num_topics: 1},
+        {domain: 'foo.com', num_clicks: 3 + 2, num_topics: 1}
+      ]
 
-      c = IncomingLinksReport.topic_count_per_domain(['test.com', 'foo.com'])
+      r = IncomingLinksReport.find('top_referred_topics').as_json
-      c["test.com"].should == 1
+      r[:data].should == [
-
+        {topic_id: p1.topic.id, topic_title: p1.topic.title, topic_slug: p1.topic.slug, num_clicks: 7},
-      c = IncomingLinksReport.topic_count_per_user()
+        {topic_id: p2.topic.id, topic_title: p2.topic.title, topic_slug: p2.topic.slug, num_clicks: 2 + 3},
-      c[p1.username].should == 1
+      ]
     end
   end
 
   describe 'top_referrers' do
     subject(:top_referrers) { IncomingLinksReport.find('top_referrers').as_json }
 
-    def stub_empty_referrers_data
+    let(:amy) { Fabricate(:user, username: 'amy') }
-      IncomingLinksReport.stubs(:link_count_per_user).returns({})
+    let(:bob) { Fabricate(:user, username: 'bob') }
-      IncomingLinksReport.stubs(:topic_count_per_user).returns({})
+    let(:post1) { Fabricate(:post) }
+    let(:post2) { Fabricate(:post) }
+    let(:topic1) { post1.topic }
+    let(:topic2) { post2.topic }
+
+    def save_base_objects
+      amy.save; bob.save
+      post1.save; post2.save
+      topic1.save; topic2.save
     end
 
     it 'returns localized titles' do
-      stub_empty_referrers_data
       top_referrers[:title].should be_present
       top_referrers[:xaxis].should be_present
       top_referrers[:ytitles].should be_present
@@ -47,21 +83,30 @@ describe IncomingLinksReport do
     end
 
     it 'with no IncomingLink records, it returns correct data' do
-      stub_empty_referrers_data
+      IncomingLink.delete_all
       top_referrers[:data].size.should == 0
     end
 
     it 'with some IncomingLink records, it returns correct data' do
-      IncomingLinksReport.stubs(:link_count_per_user).returns({'luke' => 4, 'chewie' => 2})
+      save_base_objects
-      IncomingLinksReport.stubs(:topic_count_per_user).returns({'luke' => 2, 'chewie' => 1})
+
-      top_referrers[:data][0].should == {username: 'luke', num_clicks: 4, num_topics: 2}
+      2.times do
-      top_referrers[:data][1].should == {username: 'chewie', num_clicks: 2, num_topics: 1}
+        Fabricate(:incoming_link, user: amy, post: post1).save
+      end
+      Fabricate(:incoming_link, user: amy, post: post2).save
+      2.times do
+        Fabricate(:incoming_link, user: bob, post: post1).save
+      end
+
+      top_referrers[:data][0].should == {username: 'amy', num_clicks: 3, num_topics: 2}
+      top_referrers[:data][1].should == {username: 'bob', num_clicks: 2, num_topics: 1}
     end
   end
 
   describe 'top_traffic_sources' do
     subject(:top_traffic_sources) { IncomingLinksReport.find('top_traffic_sources').as_json }
 
+    # TODO: STOP THE STUBBING
     def stub_empty_traffic_source_data
       IncomingLinksReport.stubs(:link_count_per_domain).returns({})
       IncomingLinksReport.stubs(:topic_count_per_domain).returns({})
@@ -94,6 +139,7 @@ describe IncomingLinksReport do
   describe 'top_referred_topics' do
     subject(:top_referred_topics) { IncomingLinksReport.find('top_referred_topics').as_json }
 
+    # TODO: STOP THE STUBBING
     def stub_empty_referred_topics_data
       IncomingLinksReport.stubs(:link_count_per_topic).returns({})
     end
@@ -113,6 +159,7 @@ describe IncomingLinksReport do
 
     it 'with some IncomingLink records, it returns correct data' do
       topic1 = Fabricate.build(:topic, id: 123); topic2 = Fabricate.build(:topic, id: 234)
+      # TODO: OMG OMG THE STUBBING
       IncomingLinksReport.stubs(:link_count_per_topic).returns({topic1.id => 8, topic2.id => 3})
       Topic.stubs(:select).returns(Topic); Topic.stubs(:where).returns(Topic) # bypass some activerecord methods
       Topic.stubs(:all).returns([topic1, topic2])