From 728e8a262c3bf32bde5e9ab296174ae1a8a429f3 Mon Sep 17 00:00:00 2001 From: riking Date: Mon, 24 Nov 2014 10:04:06 -0800 Subject: [PATCH] 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 | 6 +- spec/fabricators/incoming_link_fabricator.rb | 5 + spec/models/incoming_links_report_spec.rb | 97 +++++++++++++++----- 3 files changed, 79 insertions(+), 29 deletions(-) create mode 100644 spec/fabricators/incoming_link_fabricator.rb diff --git a/app/models/incoming_links_report.rb b/app/models/incoming_links_report.rb index 6786eb015..2d1563f68 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 000000000..82e5245dd --- /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 780d0fa3d..a27001eb7 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])