From d1191b7f5fbe9e8ea1b645d5a61c79fa93693c0c Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 27 May 2024 15:25:32 +1000 Subject: [PATCH] FEATURE: topic_view_stats table with daily fidelity (#27197) This gives us daily fidelity of topic view stats New table stores a row per topic viewed per day tracking anonymous and logged on views We also have a new endpoint `/t/ID/views-stats.json` to get the statistics for the topic. --- .../topic_view_stats_controller.rb | 35 +++++++++ app/models/topic.rb | 3 +- app/models/topic_view_item.rb | 26 ++++--- app/models/topic_view_stat.rb | 40 ++++++++++ config/locales/server.en.yml | 3 + config/routes.rb | 3 + .../20240527015009_add_topic_view_stats.rb | 15 ++++ spec/models/topic_view_item_spec.rb | 27 +++++++ .../topic_view_stats_controller_spec.rb | 73 +++++++++++++++++++ 9 files changed, 214 insertions(+), 11 deletions(-) create mode 100644 app/controllers/topic_view_stats_controller.rb create mode 100644 app/models/topic_view_stat.rb create mode 100644 db/migrate/20240527015009_add_topic_view_stats.rb create mode 100644 spec/requests/topic_view_stats_controller_spec.rb diff --git a/app/controllers/topic_view_stats_controller.rb b/app/controllers/topic_view_stats_controller.rb new file mode 100644 index 00000000000..d84a79f57b1 --- /dev/null +++ b/app/controllers/topic_view_stats_controller.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class TopicViewStatsController < ApplicationController + MAX_STATS_PER_API_REQUEST = 300 + + def index + topic = Topic.find(params[:topic_id].to_i) + guardian.ensure_can_see!(topic) + + from = 30.days.ago.to_date + to = Date.today + + begin + from = params[:from].to_date if params[:from].present? + to = params[:to].to_date if params[:to].present? + rescue Date::Error + render_json_error(I18n.t("topic_view_stats.invalid_date"), status: 422) + return + end + + stats = + TopicViewStat + .where(topic_id: topic.id, viewed_at: from..to) + .order(viewed_at: :desc) + .limit(MAX_STATS_PER_API_REQUEST) + + rows = [] + + stats.each do |stat| + rows << { viewed_at: stat.viewed_at, views: stat.anonymous_views + stat.logged_in_views } + end + + render json: { topic_id: topic.id, stats: rows.reverse } + end +end diff --git a/app/models/topic.rb b/app/models/topic.rb index da50e58657b..4568d0c05c7 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -262,6 +262,7 @@ class Topic < ActiveRecord::Base has_many :group_archived_messages, dependent: :destroy has_many :user_archived_messages, dependent: :destroy + has_many :topic_view_stats, dependent: :destroy has_many :allowed_groups, through: :topic_allowed_groups, source: :group has_many :allowed_group_users, through: :allowed_groups, source: :users @@ -862,7 +863,7 @@ class Topic < ActiveRecord::Base FROM posts WHERE deleted_at IS NULL AND post_type <> 4 GROUP BY topic_id - ) + ) UPDATE topics SET highest_staff_post_number = X.highest_post_number, diff --git a/app/models/topic_view_item.rb b/app/models/topic_view_item.rb index 4472ed4b1d6..8640ca3c59e 100644 --- a/app/models/topic_view_item.rb +++ b/app/models/topic_view_item.rb @@ -24,13 +24,14 @@ class TopicViewItem < ActiveRecord::Base TopicViewItem.transaction do # this is called real frequently, working hard to avoid exceptions - sql = - "INSERT INTO topic_views (topic_id, ip_address, viewed_at, user_id) - SELECT :topic_id, :ip_address, :viewed_at, :user_id - WHERE NOT EXISTS ( - SELECT 1 FROM topic_views - /*where*/ - )" + sql = <<~SQL + INSERT INTO topic_views (topic_id, ip_address, viewed_at, user_id) + SELECT :topic_id, :ip_address, :viewed_at, :user_id + WHERE NOT EXISTS ( + SELECT 1 FROM topic_views + /*where*/ + ) + SQL builder = DB.build(sql) @@ -43,15 +44,20 @@ class TopicViewItem < ActiveRecord::Base result = builder.exec(topic_id: topic_id, ip_address: ip, viewed_at: at, user_id: user_id) - Topic.where(id: topic_id).update_all "views = views + 1" - if result > 0 if user_id UserStat.where(user_id: user_id).update_all "topics_entered = topics_entered + 1" end end - # Update the views count in the parent, if it exists. + Topic.where(id: topic_id).update_all "views = views + 1" + + TopicViewStat.add( + topic_id: topic_id, + date: at, + anonymous_views: user_id ? 0 : 1, + logged_in_views: user_id ? 1 : 0, + ) end end end diff --git a/app/models/topic_view_stat.rb b/app/models/topic_view_stat.rb new file mode 100644 index 00000000000..27581dfd0e5 --- /dev/null +++ b/app/models/topic_view_stat.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +class TopicViewStat < ActiveRecord::Base + belongs_to :topic + + def self.add(topic_id:, date:, anonymous_views:, logged_in_views:) + sql = <<~SQL + INSERT INTO topic_view_stats (topic_id, viewed_at, anonymous_views, logged_in_views) + VALUES (:topic_id, :viewed_at, :anon_views, :logged_in_views) + ON CONFLICT (topic_id, viewed_at) + DO UPDATE SET + anonymous_views = topic_view_stats.anonymous_views + :anon_views, + logged_in_views = topic_view_stats.logged_in_views + :logged_in_views + SQL + + DB.exec( + sql, + topic_id: topic_id, + viewed_at: date, + anon_views: anonymous_views, + logged_in_views: logged_in_views, + ) + end +end + +# == Schema Information +# +# Table name: topic_view_stats +# +# id :bigint not null, primary key +# topic_id :integer not null +# viewed_at :date not null +# anonymous_views :integer default(0), not null +# logged_in_views :integer default(0), not null +# +# Indexes +# +# index_topic_view_stats_on_topic_id_and_viewed_at (topic_id,viewed_at) UNIQUE +# index_topic_view_stats_on_viewed_at_and_topic_id (viewed_at,topic_id) +# diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c68cbe0fafb..6258c83180e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -836,6 +836,9 @@ en: admin_quick_start_title: "Admin Guide: Getting Started" + topic_view_stats: + invalid_date: "Date is in an invalid format, use YYYY-MM-DD" + category: topic_prefix: "About the %{category} category" replace_paragraph: "(Replace this first paragraph with a brief description of your new category. This guidance will appear in the category selection area, so try to keep it below 200 characters.)" diff --git a/config/routes.rb b/config/routes.rb index 02128934166..13e3909b573 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1583,6 +1583,9 @@ Discourse::Application.routes.draw do constraints: HomePageConstraint.new("#{filter}"), as: "list_#{filter}" end + + get "/t/:topic_id/view-stats.json" => "topic_view_stats#index" + # special case for categories root to: "categories#index", constraints: HomePageConstraint.new("categories"), diff --git a/db/migrate/20240527015009_add_topic_view_stats.rb b/db/migrate/20240527015009_add_topic_view_stats.rb new file mode 100644 index 00000000000..64919ba1de4 --- /dev/null +++ b/db/migrate/20240527015009_add_topic_view_stats.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddTopicViewStats < ActiveRecord::Migration[7.0] + def change + create_table :topic_view_stats do |t| + t.integer :topic_id, null: false + t.date :viewed_at, null: false + t.integer :anonymous_views, default: 0, null: false + t.integer :logged_in_views, default: 0, null: false + end + + add_index :topic_view_stats, %i[topic_id viewed_at], unique: true + add_index :topic_view_stats, %i[viewed_at topic_id] + end +end diff --git a/spec/models/topic_view_item_spec.rb b/spec/models/topic_view_item_spec.rb index d09ca09d203..293d0167a9f 100644 --- a/spec/models/topic_view_item_spec.rb +++ b/spec/models/topic_view_item_spec.rb @@ -6,6 +6,33 @@ RSpec.describe TopicViewItem do TopicViewItem.add(topic_id, ip, user_id, nil, skip_redis) end + it "correctly increase topic view stats" do + topic = Fabricate(:topic) + + freeze_time "2021-01-01 12:00" + + add(topic.id, "1.1.1.1", nil) + add(topic.id, "1.1.1.1", nil) + + stat = TopicViewStat.find_by(topic_id: topic.id, viewed_at: Date.today) + expect(stat.anonymous_views).to eq(2) + expect(stat.logged_in_views).to eq(0) + + add(topic.id, "1.1.1.1", topic.user.id) + stat.reload + + expect(stat.anonymous_views).to eq(2) + expect(stat.logged_in_views).to eq(1) + + freeze_time(1.day.from_now) + + add(topic.id, "1.1.1.1", nil) + stat = TopicViewStat.find_by(topic_id: topic.id, viewed_at: Date.today) + + expect(stat.anonymous_views).to eq(1) + expect(stat.logged_in_views).to eq(0) + end + it "raises nothing for dupes" do add(2, "1.1.1.1") add(2, "1.1.1.1", 1) diff --git a/spec/requests/topic_view_stats_controller_spec.rb b/spec/requests/topic_view_stats_controller_spec.rb new file mode 100644 index 00000000000..a517ce588de --- /dev/null +++ b/spec/requests/topic_view_stats_controller_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +describe TopicViewStatsController do + fab!(:topic) + + it "will error if accessed on require login sites" do + SiteSetting.login_required = true + get "/t/#{topic.id}/view-stats.json" + expect(response.status).to eq(403) + end + + it "will not allow access to private topics" do + topic.category.update!(read_restricted: true) + + get "/t/#{topic.id}/view-stats.json" + expect(response.status).to eq(403) + end + + it "will raise correct errors if any param is invalid" do + get "/t/999999999999999999999999999999990000009/view-stats.json" + expect(response.status).to eq(404) + end + + it "will return an error if from and to are not valid dates" do + get "/t/#{topic.id}/view-stats.json?from=abc&to=xxx" + + expect(response.status).to eq(422) + end + + it "will return view stats for public topics" do + freeze_time "2021-01-01 12:00" + + TopicViewStat.create!( + topic_id: topic.id, + viewed_at: Date.yesterday, + anonymous_views: 2, + logged_in_views: 3, + ) + + TopicViewStat.create!( + topic_id: topic.id, + viewed_at: Date.today, + anonymous_views: 1, + logged_in_views: 2, + ) + + get "/t/#{topic.id}/view-stats.json" + expect(response.status).to eq(200) + + expected = { + "topic_id" => topic.id, + "stats" => [ + { "viewed_at" => "2020-12-31", "views" => 5 }, + { "viewed_at" => "2021-01-01", "views" => 3 }, + ], + } + + expect(response.parsed_body).to eq(expected) + + get "/t/#{topic.id}/view-stats.json?from=2019-12-31&to=2019-12-31" + expect(response.parsed_body).to eq({ "topic_id" => topic.id, "stats" => [] }) + + get "/t/#{topic.id}/view-stats.json?from=2000-12-31&to=2020-12-31" + expected = { + "topic_id" => topic.id, + "stats" => [{ "viewed_at" => "2020-12-31", "views" => 5 }], + } + expect(response.parsed_body).to eq(expected) + + get "/t/#{topic.id}/view-stats.json?from=2020-12-31&to=2020-12-31" + expect(response.parsed_body).to eq(expected) + end +end