Merge pull request #2999 from riking/fix_top_referrers_topic_count

FIX: Topic referrals by user were wrong

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(
-        referer: 'http://test.com',
-        host: 'http://boo.com',
-        topic_id: p1.topic.id,
-        ip_address: '10.0.0.2',
-        username: p1.user.username
-      )
+      p1.topic.save
+      p2.topic.save
 
+      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
-      c[p1.topic_id].should == 1
+      r = IncomingLinksReport.find('top_referrers').as_json
+      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
-      c["test.com"].should == 1
+      r = IncomingLinksReport.find('top_traffic_sources').as_json
+      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'])
-      c["test.com"].should == 1
-
-      c = IncomingLinksReport.topic_count_per_user()
-      c[p1.username].should == 1
+      r = IncomingLinksReport.find('top_referred_topics').as_json
+      r[:data].should == [
+        {topic_id: p1.topic.id, topic_title: p1.topic.title, topic_slug: p1.topic.slug, num_clicks: 7},
+        {topic_id: p2.topic.id, topic_title: p2.topic.title, topic_slug: p2.topic.slug, num_clicks: 2 + 3},
+      ]
     end
   end
 
   describe 'top_referrers' do
     subject(:top_referrers) { IncomingLinksReport.find('top_referrers').as_json }
 
-    def stub_empty_referrers_data
-      IncomingLinksReport.stubs(:link_count_per_user).returns({})
-      IncomingLinksReport.stubs(:topic_count_per_user).returns({})
+    let(:amy) { Fabricate(:user, username: 'amy') }
+    let(:bob) { Fabricate(:user, username: 'bob') }
+    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})
-      IncomingLinksReport.stubs(:topic_count_per_user).returns({'luke' => 2, 'chewie' => 1})
-      top_referrers[:data][0].should == {username: 'luke', num_clicks: 4, num_topics: 2}
-      top_referrers[:data][1].should == {username: 'chewie', num_clicks: 2, num_topics: 1}
+      save_base_objects
+
+      2.times do
+        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])