diff --git a/app/models/incoming_links_report.rb b/app/models/incoming_links_report.rb index 6786eb015c7..2d1563f6860 100644 --- a/app/models/incoming_links_report.rb +++ b/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 diff --git a/spec/fabricators/incoming_link_fabricator.rb b/spec/fabricators/incoming_link_fabricator.rb new file mode 100644 index 00000000000..82e5245ddb7 --- /dev/null +++ b/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 diff --git a/spec/models/incoming_links_report_spec.rb b/spec/models/incoming_links_report_spec.rb index 780d0fa3d99..a27001eb7f4 100644 --- a/spec/models/incoming_links_report_spec.rb +++ b/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])