FEATURE: Only count topic views for explicit/deferred tracked views (#27533)

Followup 2f2da72747

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/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();
       }

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() {

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()) {

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
 

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

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?
+    # These are set in the same place as the respective track view headers in the client.
-    has_auth_cookie ||= Auth::DefaultCurrentUserProvider.find_v1_auth_cookie(env).present?
+    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
+    # Auth cookie can be used to find the ID for logged in users, but API calls must look up the
-    # see `scripts/pageview.js` and `instance-initializers/page-tracking.js`
+    # current user based on env variables.
-    has_deferred_track_header = %w[1 true].include?(env["HTTP_DISCOURSE_DEFERRED_TRACK_VIEW"])
+    #
+    # 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
+      rescue StandardError => err
-        Discourse.warn_exception(e, message: "RequestTracker.get_data failed")
+        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
 

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

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(

spec/requests/topics_controller_spec.rb

@@ -2697,8 +2697,8 @@ RSpec.describe TopicsController do
       end
     end
 
-    it "records a view" do
+    it "does not record a topic view" do
-      expect do get "/t/#{topic.slug}/#{topic.id}.json" end.to change(TopicViewItem, :count).by(1)
+      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

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)
-      sign_in(other_user)
+    topic_page.visit_topic(topic)
-      topic_page.visit_topic(topic)
+    try_until_success { expect(TopicViewItem.count).to eq(2) }
-      page.refresh
+    page.refresh
-    }.to change(topic_map, :views_count).by 1
+    expect(topic_map.views_count).to eq(2)
 
     # likes count
     expect(topic_map).to have_no_likes

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
+  describe "pageviews" do
-    visit "/"
+    it "tracks an anonymous visit correctly" do
+      visit "/"
 
-    try_until_success do
+      try_until_success do
-      CachedCounting.flush
+        CachedCounting.flush
-      expect(ApplicationRequest.stats).to include(
+        expect(ApplicationRequest.stats).to include(
-        "page_view_anon_total" => 1,
+          "page_view_anon_total" => 1,
-        "page_view_anon_browser_total" => 1,
+          "page_view_anon_browser_total" => 1,
-        "page_view_logged_in_total" => 0,
+          "page_view_logged_in_total" => 0,
-        "page_view_crawler_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
+      visit "/"
-      CachedCounting.flush
+
-      expect(ApplicationRequest.stats).to include(
+      try_until_success do
-        "page_view_anon_total" => 2,
+        CachedCounting.flush
-        "page_view_anon_browser_total" => 2,
+        expect(ApplicationRequest.stats).to include(
-        "page_view_logged_in_total" => 0,
+          "page_view_anon_total" => 0,
-        "page_view_crawler_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
+  describe "topic views" do
-    # Can't change Selenium's user agent... so change site settings to make Discourse detect chrome as a crawler
+    fab!(:current_user) { Fabricate(:user) }
-    SiteSetting.crawler_user_agents += "|chrome"
+    fab!(:topic)
+    fab!(:post) { Fabricate(:post, topic: topic) }
 
-    visit "/"
+    context "when logged in" do
+      before { sign_in(current_user) }
 
-    try_until_success do
+      it "tracks user viewing a topic correctly with deferred tracking" do
-      CachedCounting.flush
+        visit topic.url
-      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
+        try_until_success do
-    sign_in Fabricate(:user)
+          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
+        find(".topic-list-item .raw-topic-link[data-topic-id='#{topic.id}']").click
-      CachedCounting.flush
+
-      expect(ApplicationRequest.stats).to include(
+        try_until_success do
-        "page_view_anon_total" => 0,
+          CachedCounting.flush
-        "page_view_anon_browser_total" => 0,
+          expect(TopicViewItem.exists?(topic_id: topic.id, user_id: current_user.id)).to eq(true)
-        "page_view_logged_in_total" => 1,
+          expect(
-        "page_view_crawler_total" => 0,
+            TopicViewStat.exists?(
-        "page_view_logged_in_browser_total" => 1,
+              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
+        try_until_success do
-      CachedCounting.flush
+          CachedCounting.flush
-      expect(ApplicationRequest.stats).to include(
+          expect(TopicViewItem.exists?(topic_id: topic.id, user_id: nil)).to eq(true)
-        "page_view_anon_total" => 0,
+          expect(
-        "page_view_anon_browser_total" => 0,
+            TopicViewStat.exists?(
-        "page_view_logged_in_total" => 2,
+              topic_id: topic.id,
-        "page_view_crawler_total" => 0,
+              viewed_at: Time.zone.now.to_date,
-        "page_view_logged_in_browser_total" => 2,
+              anonymous_views: 1,
-      )
+              logged_in_views: 0,
-    end
+            ),
-  end
+          ).to eq(true)
+        end
+      end
 
-  it "tracks normal error pages correctly" do
+      it "tracks an anonymous user viewing a topic correctly with explicit tracking" do
-    SiteSetting.bootstrap_error_pages = false
+        visit "/"
 
-    visit "/foobar"
+        find(".topic-list-item .raw-topic-link[data-topic-id='#{topic.id}']").click
 
-    try_until_success do
+        try_until_success do
-      CachedCounting.flush
+          CachedCounting.flush
-
+          expect(TopicViewItem.exists?(topic_id: topic.id, user_id: nil)).to eq(true)
-      # Does not count error as a pageview
+          expect(
-      expect(ApplicationRequest.stats).to include(
+            TopicViewStat.exists?(
-        "http_4xx_total" => 1,
+              topic_id: topic.id,
-        "page_view_anon_total" => 0,
+              viewed_at: Time.zone.now.to_date,
-        "page_view_anon_browser_total" => 0,
+              anonymous_views: 1,
-        "page_view_logged_in_total" => 0,
+              logged_in_views: 0,
-        "page_view_crawler_total" => 0,
+            ),
-      )
+          ).to eq(true)
-    end
+        end
-
+      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

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)
-      sign_in(other_user)
+    topic_page.visit_topic(topic)
-      topic_page.visit_topic(topic)
+    try_until_success { expect(TopicViewItem.count).to eq(2) }
-      page.refresh
+    page.refresh
-    }.to change(topic_map, :views_count).by 1
+    expect(topic_map.views_count).to eq(2)
 
     # likes count
     expect(topic_map).to have_no_likes