From 527f02e99fee782f53e206f739fb4f12d63b6d2a Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 3 Jul 2024 10:38:49 +1000 Subject: [PATCH] FEATURE: Only count topic views for explicit/deferred tracked views (#27533) Followup 2f2da7274732cba30d03b6c5c3a4194652cb6783 This commit moves topic view tracking from happening every time a Topic is requested, which is susceptible to inflating numbers of views from web crawlers, to our request tracker middleware. In this new location, topic views are only tracked when the following headers are sent: * HTTP_DISCOURSE_TRACK_VIEW - This is sent on every page navigation when clicking around the ember app. We count these as browser page views because we know it comes from the AJAX call in our app. The topic ID is extracted from HTTP_DISCOURSE_TRACK_VIEW_TOPIC_ID * HTTP_DISCOURSE_DEFERRED_TRACK_VIEW - Sent when MessageBus initializes after first loading the page to count the initial page load view. The topic ID is extracted from HTTP_DISCOURSE_DEFERRED_TRACK_VIEW. This will bring topic views more in line with the change we made to page views in the referenced commit and result in more realistic topic view counts. --- .../app/instance-initializers/message-bus.js | 18 +- .../instance-initializers/page-tracking.js | 13 +- .../javascripts/discourse/app/lib/ajax.js | 11 +- app/controllers/topics_controller.rb | 24 +- config/initializers/200-first_middlewares.rb | 3 +- lib/middleware/request_tracker.rb | 91 +++- spec/integrity/middleware_order_spec.rb | 49 +++ spec/lib/middleware/request_tracker_spec.rb | 198 +++++++++ spec/requests/topics_controller_spec.rb | 5 +- spec/system/private_message_map_spec.rb | 10 +- spec/system/request_tracker_spec.rb | 392 +++++++++++------- spec/system/topic_map_spec.rb | 10 +- 12 files changed, 641 insertions(+), 183 deletions(-) create mode 100644 spec/integrity/middleware_order_spec.rb diff --git a/app/assets/javascripts/discourse/app/instance-initializers/message-bus.js b/app/assets/javascripts/discourse/app/instance-initializers/message-bus.js index 819db4c3387..a6bb040dd21 100644 --- a/app/assets/javascripts/discourse/app/instance-initializers/message-bus.js +++ b/app/assets/javascripts/discourse/app/instance-initializers/message-bus.js @@ -8,6 +8,7 @@ import getURL from "discourse-common/lib/get-url"; const LONG_POLL_AFTER_UNSEEN_TIME = 1200000; // 20 minutes let _sendDeferredPageview = false; +let _deferredViewTopicId = null; export function sendDeferredPageview() { _sendDeferredPageview = true; @@ -30,7 +31,14 @@ function mbAjax(messageBus, opts) { if (_sendDeferredPageview) { opts.headers["Discourse-Deferred-Track-View"] = "true"; + + if (_deferredViewTopicId) { + opts.headers["Discourse-Deferred-Track-View-Topic-Id"] = + _deferredViewTopicId; + } + _sendDeferredPageview = false; + _deferredViewTopicId = null; } const oldComplete = opts.complete; @@ -53,7 +61,8 @@ export default { const messageBus = owner.lookup("service:message-bus"), user = owner.lookup("service:current-user"), - siteSettings = owner.lookup("service:site-settings"); + siteSettings = owner.lookup("service:site-settings"), + router = owner.lookup("service:router"); messageBus.alwaysLongPoll = !isProduction(); messageBus.shouldLongPollCallback = () => @@ -86,6 +95,13 @@ export default { // pass in a position const interval = setInterval(() => { if (document.readyState === "complete") { + if ( + router.currentRouteName === "topic.fromParams" || + router.currentRouteName === "topic.fromParamsNear" + ) { + _deferredViewTopicId = router.currentRoute.parent.params.id; + } + clearInterval(interval); messageBus.start(); } diff --git a/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js b/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js index e3f226cb72e..e9aaa2659a8 100644 --- a/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js +++ b/app/assets/javascripts/discourse/app/instance-initializers/page-tracking.js @@ -1,4 +1,8 @@ -import { resetAjax, trackNextAjaxAsPageview } from "discourse/lib/ajax"; +import { + resetAjax, + trackNextAjaxAsPageview, + trackNextAjaxAsTopicView, +} from "discourse/lib/ajax"; import { googleTagManagerPageChanged, resetPageTracking, @@ -86,6 +90,13 @@ export default { } trackNextAjaxAsPageview(); + + if ( + transition.to.name === "topic.fromParamsNear" || + transition.to.name === "topic.fromParams" + ) { + trackNextAjaxAsTopicView(transition.to.parent.params.id); + } }, teardown() { diff --git a/app/assets/javascripts/discourse/app/lib/ajax.js b/app/assets/javascripts/discourse/app/lib/ajax.js index fb46b69e738..28581f42110 100644 --- a/app/assets/javascripts/discourse/app/lib/ajax.js +++ b/app/assets/javascripts/discourse/app/lib/ajax.js @@ -9,6 +9,7 @@ import { isTesting } from "discourse-common/config/environment"; import getURL from "discourse-common/lib/get-url"; let _trackView = false; +let _topicId = null; let _transientHeader = null; let _logoffCallback; @@ -16,6 +17,10 @@ export function setTransientHeader(key, value) { _transientHeader = { key, value }; } +export function trackNextAjaxAsTopicView(topicId) { + _topicId = topicId; +} + export function trackNextAjaxAsPageview() { _trackView = true; } @@ -92,8 +97,12 @@ export function ajax() { if (_trackView && (!args.type || args.type === "GET")) { _trackView = false; - // DON'T CHANGE: rack is prepending "HTTP_" in the header's name args.headers["Discourse-Track-View"] = "true"; + + if (_topicId) { + args.headers["Discourse-Track-View-Topic-Id"] = _topicId; + } + _topicId = null; } if (userPresent()) { diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 0a8c9fd199b..641fa05a563 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -1266,7 +1266,6 @@ class TopicsController < ApplicationController topic_id = @topic_view.topic.id ip = request.remote_ip user_id = (current_user.id if current_user) - track_visit = should_track_visit_to_topic? if !request.format.json? hash = { @@ -1283,13 +1282,32 @@ class TopicsController < ApplicationController TopicsController.defer_add_incoming_link(hash) end - TopicsController.defer_track_visit(topic_id, ip, user_id, track_visit) + TopicsController.defer_track_visit_v2(topic_id, user_id) if should_track_visit_to_topic? end + # TODO (martin) Remove this once discourse-docs is updated. def self.defer_track_visit(topic_id, ip, user_id, track_visit) + self.defer_track_visit_v2(topic_id, user_id) if track_visit + self.defer_topic_view(topic_id, ip, user_id) + end + def self.defer_track_visit_v2(topic_id, user_id) Scheduler::Defer.later "Track Visit" do + TopicUser.track_visit!(topic_id, user_id) + end + end + + def self.defer_topic_view(topic_id, ip, user_id = nil) + Scheduler::Defer.later "Topic View" do + topic = Topic.find_by(id: topic_id) + return if topic.blank? + + # We need to make sure that we aren't allowing recording + # random topic views against topics the user cannot see. + user = User.find_by(id: user_id) if user_id.present? + return if user_id.present? && user.blank? + return if !Guardian.new(user).can_see_topic?(topic) + TopicViewItem.add(topic_id, ip, user_id) - TopicUser.track_visit!(topic_id, user_id) if track_visit end end diff --git a/config/initializers/200-first_middlewares.rb b/config/initializers/200-first_middlewares.rb index 55c2ac05102..60073484b7e 100644 --- a/config/initializers/200-first_middlewares.rb +++ b/config/initializers/200-first_middlewares.rb @@ -13,7 +13,8 @@ Rails.configuration.middleware.unshift(MessageBus::Rack::Middleware) # page view (we serve all assets out of thin in development) if Rails.env != "development" || ENV["TRACK_REQUESTS"] require "middleware/request_tracker" - Rails.configuration.middleware.unshift Middleware::RequestTracker + Rails.configuration.middleware.unshift(Middleware::RequestTracker) + Rails.configuration.middleware.move_before(Middleware::RequestTracker, ActionDispatch::RemoteIp) MethodProfiler.ensure_discourse_instrumentation! if GlobalSetting.enable_performance_http_headers end diff --git a/lib/middleware/request_tracker.rb b/lib/middleware/request_tracker.rb index e217dcd2f20..20ad515f98b 100644 --- a/lib/middleware/request_tracker.rb +++ b/lib/middleware/request_tracker.rb @@ -75,18 +75,32 @@ class Middleware::RequestTracker elsif data[:has_auth_cookie] ApplicationRequest.increment!(:page_view_logged_in) ApplicationRequest.increment!(:page_view_logged_in_mobile) if data[:is_mobile] + if data[:explicit_track_view] # Must be a browser if it had this header from our ajax implementation ApplicationRequest.increment!(:page_view_logged_in_browser) ApplicationRequest.increment!(:page_view_logged_in_browser_mobile) if data[:is_mobile] + + if data[:topic_id].present? && data[:current_user_id].present? + TopicsController.defer_topic_view( + data[:topic_id], + data[:request_remote_ip], + data[:current_user_id], + ) + end end elsif !SiteSetting.login_required ApplicationRequest.increment!(:page_view_anon) ApplicationRequest.increment!(:page_view_anon_mobile) if data[:is_mobile] + if data[:explicit_track_view] # Must be a browser if it had this header from our ajax implementation ApplicationRequest.increment!(:page_view_anon_browser) ApplicationRequest.increment!(:page_view_anon_browser_mobile) if data[:is_mobile] + + if data[:topic_id].present? + TopicsController.defer_topic_view(data[:topic_id], data[:request_remote_ip]) + end end end end @@ -97,9 +111,21 @@ class Middleware::RequestTracker if data[:has_auth_cookie] ApplicationRequest.increment!(:page_view_logged_in_browser) ApplicationRequest.increment!(:page_view_logged_in_browser_mobile) if data[:is_mobile] + + if data[:topic_id].present? && data[:current_user_id].present? + TopicsController.defer_topic_view( + data[:topic_id], + data[:request_remote_ip], + data[:current_user_id], + ) + end elsif !SiteSetting.login_required ApplicationRequest.increment!(:page_view_anon_browser) ApplicationRequest.increment!(:page_view_anon_browser_mobile) if data[:is_mobile] + + if data[:topic_id].present? + TopicsController.defer_topic_view(data[:topic_id], data[:request_remote_ip]) + end end end @@ -129,7 +155,15 @@ class Middleware::RequestTracker request ||= Rack::Request.new(env) helper = Middleware::AnonymousCache::Helper.new(env, request) - # Value of the discourse-track-view request header + # Since ActionDispatch::RemoteIp middleware is run before this middleware, + # we have access to the normalised remote IP based on ActionDispatch::RemoteIp::GetIp + # + # NOTE: Locally with MessageBus requests, the remote IP ends up as ::1 because + # of the X-Forwarded-For header set...somewhere, whereas all other requests + # end up as 127.0.0.1. + request_remote_ip = env["action_dispatch.remote_ip"].to_s + + # Value of the discourse-track-view request header, set in `lib/ajax.js` env_track_view = env["HTTP_DISCOURSE_TRACK_VIEW"] # Was the discourse-track-view request header set to true? Likely @@ -141,10 +175,25 @@ class Middleware::RequestTracker status == 200 && !%w[0 false].include?(env_track_view) && request.get? && !request.xhr? && headers["Content-Type"] =~ %r{text/html} + # This header is sent on a follow-up request after a real browser loads up a page + # see `scripts/pageview.js` and `instance-initializers/page-tracking.js` + deferred_track_view = %w[1 true].include?(env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"]) + + # This is treated separately from deferred tracking in #log_request, this is + # why deferred_track_view is not counted here. track_view = !!(explicit_track_view || implicit_track_view) - has_auth_cookie = Auth::DefaultCurrentUserProvider.find_v0_auth_cookie(request).present? - has_auth_cookie ||= Auth::DefaultCurrentUserProvider.find_v1_auth_cookie(env).present? + # These are set in the same place as the respective track view headers in the client. + topic_id = + if deferred_track_view + env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW_TOPIC_ID"] + elsif explicit_track_view || implicit_track_view + env["HTTP_DISCOURSE_TRACK_VIEW_TOPIC_ID"] + end + + auth_cookie = Auth::DefaultCurrentUserProvider.find_v0_auth_cookie(request) + auth_cookie ||= Auth::DefaultCurrentUserProvider.find_v1_auth_cookie(env) + has_auth_cookie = auth_cookie.present? is_api ||= !!env[Auth::DefaultCurrentUserProvider::API_KEY_ENV] is_user_api ||= !!env[Auth::DefaultCurrentUserProvider::USER_API_KEY_ENV] @@ -152,14 +201,31 @@ class Middleware::RequestTracker is_message_bus = request.path.start_with?("#{Discourse.base_path}/message-bus/") is_topic_timings = request.path.start_with?("#{Discourse.base_path}/topics/timings") - # This header is sent on a follow-up request after a real browser loads up a page - # see `scripts/pageview.js` and `instance-initializers/page-tracking.js` - has_deferred_track_header = %w[1 true].include?(env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"]) + # Auth cookie can be used to find the ID for logged in users, but API calls must look up the + # current user based on env variables. + # + # We only care about this for topic views, other pageviews it's enough to know if the user is + # logged in or not, and we have separate pageview tracking for API views. + current_user_id = + if topic_id.present? + begin + (auth_cookie&.[](:user_id) || CurrentUser.lookup_from_env(env)&.id) + rescue Discourse::InvalidAccess => err + # This error is raised when the API key is invalid, no need to stop the show. + Discourse.warn_exception( + err, + message: "RequestTracker.get_data failed with an invalid API key error", + ) + nil + end + end h = { status: status, is_crawler: helper.is_crawler?, has_auth_cookie: has_auth_cookie, + current_user_id: current_user_id, + topic_id: topic_id, is_api: is_api, is_user_api: is_user_api, is_background: is_message_bus || is_topic_timings, @@ -168,7 +234,8 @@ class Middleware::RequestTracker timing: timing, queue_seconds: env["REQUEST_QUEUE_SECONDS"], explicit_track_view: explicit_track_view, - deferred_track: has_deferred_track_header, + deferred_track: deferred_track_view, + request_remote_ip: request_remote_ip, } if h[:is_background] @@ -199,12 +266,16 @@ class Middleware::RequestTracker end def log_request_info(env, result, info, request = nil) - # we got to skip this on error ... its just logging + # We've got to skip this on error ... its just logging data = begin self.class.get_data(env, result, info, request) - rescue StandardError => e - Discourse.warn_exception(e, message: "RequestTracker.get_data failed") + rescue StandardError => err + Discourse.warn_exception(err, message: "RequestTracker.get_data failed") + + # This is super hard to find if in testing, we should still raise in this case. + raise err if Rails.env.test? + nil end diff --git a/spec/integrity/middleware_order_spec.rb b/spec/integrity/middleware_order_spec.rb new file mode 100644 index 00000000000..f18f422ab38 --- /dev/null +++ b/spec/integrity/middleware_order_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +RSpec.describe "Middleware order" do + it "order of middleware in the application is correct" do + middlewares = Rails.configuration.middleware.map { |middleware| "#{middleware.inspect}" } + expect(middlewares).to eq( + %w[ + BlockRequestsMiddleware + TestMultisiteMiddleware + ActionDispatch::RemoteIp + Middleware::RequestTracker + MessageBus::Rack::Middleware + ActionDispatch::HostAuthorization + Rack::Sendfile + ActionDispatch::Static + ActionDispatch::Executor + Rack::MethodOverride + Middleware::EnforceHostname + ActionDispatch::RequestId + SilenceLogger + ActionDispatch::ShowExceptions + ActionDispatch::DebugExceptions + ActionDispatch::Callbacks + ActionDispatch::Cookies + ActionDispatch::Session::DiscourseCookieStore + Discourse::Cors + ActionDispatch::Flash + RspecErrorTracker + Middleware::CspScriptNonceInjector + Middleware::AnonymousCache + ContentSecurityPolicy::Middleware + ActionDispatch::PermissionsPolicy::Middleware + Rack::Head + Rack::ConditionalGet + Rack::TempfileReaper + Middleware::OmniauthBypassMiddleware + ], + ) + end + + it "ensures that ActionDispatch::RemoteIp comes before Middleware::RequestTracker" do + remote_ip_found = false + request_tracker_found = false + Rails.configuration.middleware.each do |middleware| + remote_ip_found = true if middleware.inspect == "ActionDispatch::RemoteIp" + expect(remote_ip_found).to eq(true) if middleware.inspect == "Middleware::RequestTracker" + end + end +end diff --git a/spec/lib/middleware/request_tracker_spec.rb b/spec/lib/middleware/request_tracker_spec.rb index 5fd4e37357d..c01916fef77 100644 --- a/spec/lib/middleware/request_tracker_spec.rb +++ b/spec/lib/middleware/request_tracker_spec.rb @@ -243,6 +243,204 @@ RSpec.describe Middleware::RequestTracker do expect(ApplicationRequest.page_view_anon.first.count).to eq(1) end + describe "topic views" do + fab!(:topic) + fab!(:post) { Fabricate(:post, topic: topic) } + fab!(:user) { Fabricate(:user, active: true) } + + let!(:auth_cookie) do + token = UserAuthToken.generate!(user_id: user.id) + create_auth_cookie( + token: token.unhashed_auth_token, + user_id: user.id, + trust_level: user.trust_level, + issued_at: 5.minutes.ago, + ) + end + + use_redis_snapshotting + + def log_topic_view(authenticated: false, deferred: false) + headers = { "action_dispatch.remote_ip" => "127.0.0.1" } + + headers["HTTP_COOKIE"] = "_t=#{auth_cookie};" if authenticated + + if deferred + headers["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"] = "1" + headers["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW_TOPIC_ID"] = topic.id + path = "/message-bus/abcde/poll" + else + headers["HTTP_DISCOURSE_TRACK_VIEW"] = "1" + headers["HTTP_DISCOURSE_TRACK_VIEW_TOPIC_ID"] = topic.id + path = URI.parse(topic.url).path + end + + data = + Middleware::RequestTracker.get_data( + env(path: path, **headers), + ["200", { "Content-Type" => "text/html" }], + 0.1, + ) + Middleware::RequestTracker.log_request(data) + data + end + + it "logs deferred topic views correctly for logged in users" do + data = log_topic_view(authenticated: true, deferred: true) + + expect(data[:topic_id]).to eq(topic.id) + expect(data[:request_remote_ip]).to eq("127.0.0.1") + expect(data[:current_user_id]).to eq(user.id) + CachedCounting.flush + + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: user.id, ip_address: nil)).to eq( + true, + ) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 0, + logged_in_views: 1, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(true) + end + + it "does not log deferred topic views for topics the user cannot access" do + topic.update!(category: Fabricate(:private_category, group: Fabricate(:group))) + log_topic_view(authenticated: true, deferred: true) + CachedCounting.flush + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: user.id, ip_address: nil)).to eq( + false, + ) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 0, + logged_in_views: 1, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(false) + end + + it "logs deferred topic views correctly for anonymous" do + data = log_topic_view(authenticated: false, deferred: true) + + expect(data[:topic_id]).to eq(topic.id) + expect(data[:request_remote_ip]).to eq("127.0.0.1") + expect(data[:current_user_id]).to eq(nil) + CachedCounting.flush + + expect( + TopicViewItem.exists?(topic_id: topic.id, user_id: nil, ip_address: "127.0.0.1"), + ).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 1, + logged_in_views: 0, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(true) + end + + it "does not log deferred topic views for topics the anonymous user cannot access" do + topic.update!(category: Fabricate(:private_category, group: Fabricate(:group))) + log_topic_view(authenticated: false, deferred: true) + CachedCounting.flush + + expect( + TopicViewItem.exists?(topic_id: topic.id, user_id: nil, ip_address: "127.0.0.1"), + ).to eq(false) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 1, + logged_in_views: 0, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(false) + end + + it "logs explicit topic views correctly for logged in users" do + data = log_topic_view(authenticated: true, deferred: false) + + expect(data[:topic_id]).to eq(topic.id) + expect(data[:request_remote_ip]).to eq("127.0.0.1") + expect(data[:current_user_id]).to eq(user.id) + CachedCounting.flush + + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: user.id, ip_address: nil)).to eq( + true, + ) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 0, + logged_in_views: 1, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(true) + end + + it "does not log explicit topic views for topics the user cannot access" do + topic.update!(category: Fabricate(:private_category, group: Fabricate(:group))) + log_topic_view(authenticated: true, deferred: false) + CachedCounting.flush + + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: user.id, ip_address: nil)).to eq( + false, + ) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 0, + logged_in_views: 1, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(false) + end + + it "logs explicit topic views correctly for anonymous" do + data = log_topic_view(authenticated: false, deferred: false) + + expect(data[:topic_id]).to eq(topic.id) + expect(data[:request_remote_ip]).to eq("127.0.0.1") + expect(data[:current_user_id]).to eq(nil) + CachedCounting.flush + + expect( + TopicViewItem.exists?(topic_id: topic.id, user_id: nil, ip_address: "127.0.0.1"), + ).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 1, + logged_in_views: 0, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(true) + end + + it "does not log explicit topic views for topics the anonymous user cannot access" do + topic.update!(category: Fabricate(:private_category, group: Fabricate(:group))) + log_topic_view(authenticated: false, deferred: false) + CachedCounting.flush + + expect( + TopicViewItem.exists?(topic_id: topic.id, user_id: nil, ip_address: "127.0.0.1"), + ).to eq(false) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + anonymous_views: 1, + logged_in_views: 0, + viewed_at: Time.zone.now.to_date, + ), + ).to eq(false) + end + end + context "when ignoring anonymous page views" do let(:anon_data) do Middleware::RequestTracker.get_data( diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 95ee3fa3df6..a60f1633f39 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2697,8 +2697,8 @@ RSpec.describe TopicsController do end end - it "records a view" do - expect do get "/t/#{topic.slug}/#{topic.id}.json" end.to change(TopicViewItem, :count).by(1) + it "does not record a topic view" do + expect { get "/t/#{topic.slug}/#{topic.id}.json" }.not_to change(TopicViewItem, :count) end it "records a view to invalid post_number" do @@ -3018,7 +3018,6 @@ RSpec.describe TopicsController do expect(response.status).to eq(200) topic.reload - expect(topic.views).to eq(1) end it "returns 403 for an invalid key" do diff --git a/spec/system/private_message_map_spec.rb b/spec/system/private_message_map_spec.rb index 4567e7198da..8bb79c911a4 100644 --- a/spec/system/private_message_map_spec.rb +++ b/spec/system/private_message_map_spec.rb @@ -75,11 +75,11 @@ describe "Topic Map - Private Message", type: :system do expect(topic_map.expanded_map_avatars_details.length).to eq 4 # views count - expect { - sign_in(other_user) - topic_page.visit_topic(topic) - page.refresh - }.to change(topic_map, :views_count).by 1 + sign_in(other_user) + topic_page.visit_topic(topic) + try_until_success { expect(TopicViewItem.count).to eq(2) } + page.refresh + expect(topic_map.views_count).to eq(2) # likes count expect(topic_map).to have_no_likes diff --git a/spec/system/request_tracker_spec.rb b/spec/system/request_tracker_spec.rb index 43df17b7296..b334d7c0003 100644 --- a/spec/system/request_tracker_spec.rb +++ b/spec/system/request_tracker_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -describe "request tracking", type: :system do +describe "Request tracking", type: :system do before do ApplicationRequest.enable CachedCounting.reset @@ -13,176 +13,262 @@ describe "request tracking", type: :system do CachedCounting.disable end - it "tracks an anonymous visit correctly" do - visit "/" + describe "pageviews" do + it "tracks an anonymous visit correctly" do + visit "/" - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 1, - "page_view_anon_browser_total" => 1, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 1, + "page_view_anon_browser_total" => 1, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + + find(".nav-item_categories a").click + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 2, + "page_view_anon_browser_total" => 2, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end end - find(".nav-item_categories a").click + it "tracks a crawler visit correctly" do + # Can't change Selenium's user agent... so change site settings to make Discourse detect chrome as a crawler + SiteSetting.crawler_user_agents += "|chrome" - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 2, - "page_view_anon_browser_total" => 2, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) + visit "/" + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 0, + "page_view_anon_browser_total" => 0, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 1, + ) + end + end + + it "tracks a logged-in session correctly" do + sign_in Fabricate(:user) + + visit "/" + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 0, + "page_view_anon_browser_total" => 0, + "page_view_logged_in_total" => 1, + "page_view_crawler_total" => 0, + "page_view_logged_in_browser_total" => 1, + ) + end + + find(".nav-item_categories a").click + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 0, + "page_view_anon_browser_total" => 0, + "page_view_logged_in_total" => 2, + "page_view_crawler_total" => 0, + "page_view_logged_in_browser_total" => 2, + ) + end + end + + it "tracks normal error pages correctly" do + SiteSetting.bootstrap_error_pages = false + + visit "/foobar" + + try_until_success do + CachedCounting.flush + + # Does not count error as a pageview + expect(ApplicationRequest.stats).to include( + "http_4xx_total" => 1, + "page_view_anon_total" => 0, + "page_view_anon_browser_total" => 0, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + + find("#site-logo").click + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "http_4xx_total" => 1, + "page_view_anon_total" => 1, + "page_view_anon_browser_total" => 1, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + end + + it "tracks non-ember pages correctly" do + visit "/safe-mode" + + try_until_success do + CachedCounting.flush + + # Does not count error as a pageview + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 1, + "page_view_anon_browser_total" => 1, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + end + + it "tracks bootstrapped error pages correctly" do + SiteSetting.bootstrap_error_pages = true + + visit "/foobar" + + try_until_success do + CachedCounting.flush + + # Does not count error as a pageview + expect(ApplicationRequest.stats).to include( + "http_4xx_total" => 1, + "page_view_anon_total" => 0, + "page_view_anon_browser_total" => 0, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + + find("#site-logo").click + + try_until_success do + CachedCounting.flush + expect(ApplicationRequest.stats).to include( + "http_4xx_total" => 1, + "page_view_anon_total" => 1, + "page_view_anon_browser_total" => 1, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end + end + + it "tracks published pages correctly" do + SiteSetting.enable_page_publishing = true + page = + Fabricate(:published_page, public: true, slug: "some-page", topic: Fabricate(:post).topic) + + visit "/pub/some-page" + + try_until_success do + CachedCounting.flush + + # Does not count error as a pageview + expect(ApplicationRequest.stats).to include( + "page_view_anon_total" => 1, + "page_view_anon_browser_total" => 1, + "page_view_logged_in_total" => 0, + "page_view_crawler_total" => 0, + ) + end end end - it "tracks a crawler visit correctly" do - # Can't change Selenium's user agent... so change site settings to make Discourse detect chrome as a crawler - SiteSetting.crawler_user_agents += "|chrome" + describe "topic views" do + fab!(:current_user) { Fabricate(:user) } + fab!(:topic) + fab!(:post) { Fabricate(:post, topic: topic) } - visit "/" + context "when logged in" do + before { sign_in(current_user) } - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 0, - "page_view_anon_browser_total" => 0, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 1, - ) - end - end + it "tracks user viewing a topic correctly with deferred tracking" do + visit topic.url - it "tracks a logged-in session correctly" do - sign_in Fabricate(:user) + try_until_success do + CachedCounting.flush + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: current_user.id)).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + viewed_at: Time.zone.now.to_date, + anonymous_views: 0, + logged_in_views: 1, + ), + ).to eq(true) + end + end - visit "/" + it "tracks user viewing a topic correctly with explicit tracking" do + visit "/" - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 0, - "page_view_anon_browser_total" => 0, - "page_view_logged_in_total" => 1, - "page_view_crawler_total" => 0, - "page_view_logged_in_browser_total" => 1, - ) + find(".topic-list-item .raw-topic-link[data-topic-id='#{topic.id}']").click + + try_until_success do + CachedCounting.flush + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: current_user.id)).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + viewed_at: Time.zone.now.to_date, + anonymous_views: 0, + logged_in_views: 1, + ), + ).to eq(true) + end + end end - find(".nav-item_categories a").click + context "when anonymous" do + it "tracks an anonymous user viewing a topic correctly with deferred tracking" do + visit topic.url - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 0, - "page_view_anon_browser_total" => 0, - "page_view_logged_in_total" => 2, - "page_view_crawler_total" => 0, - "page_view_logged_in_browser_total" => 2, - ) - end - end + try_until_success do + CachedCounting.flush + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: nil)).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + viewed_at: Time.zone.now.to_date, + anonymous_views: 1, + logged_in_views: 0, + ), + ).to eq(true) + end + end - it "tracks normal error pages correctly" do - SiteSetting.bootstrap_error_pages = false + it "tracks an anonymous user viewing a topic correctly with explicit tracking" do + visit "/" - visit "/foobar" + find(".topic-list-item .raw-topic-link[data-topic-id='#{topic.id}']").click - try_until_success do - CachedCounting.flush - - # Does not count error as a pageview - expect(ApplicationRequest.stats).to include( - "http_4xx_total" => 1, - "page_view_anon_total" => 0, - "page_view_anon_browser_total" => 0, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) - end - - find("#site-logo").click - - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "http_4xx_total" => 1, - "page_view_anon_total" => 1, - "page_view_anon_browser_total" => 1, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) - end - end - - it "tracks non-ember pages correctly" do - visit "/safe-mode" - - try_until_success do - CachedCounting.flush - - # Does not count error as a pageview - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 1, - "page_view_anon_browser_total" => 1, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) - end - end - - it "tracks bootstrapped error pages correctly" do - SiteSetting.bootstrap_error_pages = true - - visit "/foobar" - - try_until_success do - CachedCounting.flush - - # Does not count error as a pageview - expect(ApplicationRequest.stats).to include( - "http_4xx_total" => 1, - "page_view_anon_total" => 0, - "page_view_anon_browser_total" => 0, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) - end - - find("#site-logo").click - - try_until_success do - CachedCounting.flush - expect(ApplicationRequest.stats).to include( - "http_4xx_total" => 1, - "page_view_anon_total" => 1, - "page_view_anon_browser_total" => 1, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) - end - end - - it "tracks published pages correctly" do - SiteSetting.enable_page_publishing = true - page = - Fabricate(:published_page, public: true, slug: "some-page", topic: Fabricate(:post).topic) - - visit "/pub/some-page" - - try_until_success do - CachedCounting.flush - - # Does not count error as a pageview - expect(ApplicationRequest.stats).to include( - "page_view_anon_total" => 1, - "page_view_anon_browser_total" => 1, - "page_view_logged_in_total" => 0, - "page_view_crawler_total" => 0, - ) + try_until_success do + CachedCounting.flush + expect(TopicViewItem.exists?(topic_id: topic.id, user_id: nil)).to eq(true) + expect( + TopicViewStat.exists?( + topic_id: topic.id, + viewed_at: Time.zone.now.to_date, + anonymous_views: 1, + logged_in_views: 0, + ), + ).to eq(true) + end + end end end end diff --git a/spec/system/topic_map_spec.rb b/spec/system/topic_map_spec.rb index 574fdbdfea7..f3cc70e9f49 100644 --- a/spec/system/topic_map_spec.rb +++ b/spec/system/topic_map_spec.rb @@ -63,11 +63,11 @@ describe "Topic Map", type: :system do expect(topic_map.expanded_map_avatars_details.length).to eq 4 # views count - expect { - sign_in(other_user) - topic_page.visit_topic(topic) - page.refresh - }.to change(topic_map, :views_count).by 1 + sign_in(other_user) + topic_page.visit_topic(topic) + try_until_success { expect(TopicViewItem.count).to eq(2) } + page.refresh + expect(topic_map.views_count).to eq(2) # likes count expect(topic_map).to have_no_likes