Track pageviews for custom pages

app/controllers/page_views_controller.rb

@@ -15,7 +15,7 @@ class PageViewsController < ApplicationMetalController
   VISITOR_IMPRESSIONS_AGGREGATE_COUNTS_FOR_NUMBER_OF_VIEWS = 10
 
   def create
-    page_view_create_params = params.slice(:article_id, :referrer, :user_agent)
+    page_view_create_params = params.slice(:article_id, :page_id, :referrer, :user_agent)
     if session_current_user_id
       page_view_create_params[:user_id] = session_current_user_id
     else

app/javascript/packs/baseTracking.js

@@ -149,13 +149,14 @@ function checkUserLoggedIn() {
 function trackCustomImpressions() {
   setTimeout(()=> {
     const ArticleElement = document.getElementById('article-body') || document.getElementById('comment-article-indicator');
+    const PageElement = document.getElementById('page-body');
     const tokenMeta = document.querySelector("meta[name='csrf-token']")
     const isBot = /bot|google|baidu|bing|msn|duckduckbot|teoma|slurp|yandex/i.test(navigator.userAgent) // is crawler
     // eslint-disable-next-line no-unused-vars
     const windowBigEnough =  window.innerWidth > 1023
 
     // page view
-    if (ArticleElement && tokenMeta && !isBot) {
+    if ((ArticleElement || PageElement) && tokenMeta && !isBot) {
       // See https://github.com/forem/forem/blob/main/app/controllers/page_views_controller.rb
       //
       // If you change the 10, you should look at the PageViewsController as well.
@@ -164,10 +165,12 @@ function trackCustomImpressions() {
         return;
       }
       const dataBody = {
-        article_id: ArticleElement.dataset.articleId,
         referrer: document.referrer,
         user_agent: navigator.userAgent,
       };
+
+      if (ArticleElement) dataBody.article_id = ArticleElement.dataset.articleId;
+      if (PageElement) dataBody.page_id = PageElement.dataset.pageId;
       const csrfToken = tokenMeta.getAttribute('content');
       trackPageView(dataBody, csrfToken);
       let timeOnSiteCounter = 0;

app/models/page.rb

@@ -7,6 +7,7 @@ class Page < ApplicationRecord
   PRIVACY_SLUG = "privacy".freeze
 
   has_many :billboards, dependent: :nullify
+  has_many :page_views, dependent: :delete_all
 
   validates :title, presence: true
   validates :description, presence: true

app/models/page_view.rb

@@ -3,13 +3,22 @@
 #       destroy callbacks will be called on this object.
 class PageView < ApplicationRecord
   belongs_to :user, optional: true
-  belongs_to :article
+  belongs_to :article, optional: true
+  belongs_to :page, optional: true
 
   before_create :extract_domain_and_path
   after_create_commit :record_field_test_event
 
+  validate :article_or_page_present
+
   private
 
+  def article_or_page_present
+    return if article.present? ^ page.present?
+
+    errors.add(:base, "PageView must belong to either an Article or a Page, but not both")
+  end
+
   def extract_domain_and_path
     return unless referrer
 

app/services/page_view_rollup.rb

@@ -1,5 +1,5 @@
 class PageViewRollup
-  ATTRIBUTES_PRESERVED = %i[article_id created_at user_id].freeze
+  ATTRIBUTES_PRESERVED = %i[article_id page_id created_at user_id].freeze
   ATTRIBUTES_DESTROYED = %i[id domain path referrer updated_at user_agent counts_for_number_of_views
                             time_tracked_in_seconds].freeze
 
@@ -57,7 +57,7 @@ def rollup(date)
     (0..23).each do |hour|
       start_hour = fixed_date.change(hour: hour)
       end_hour = fixed_date.change(hour: hour + 1)
-      rows = relation.where(user_id: nil, created_at: start_hour...end_hour)
+      rows = relation.where(page_id: nil, user_id: nil, created_at: start_hour...end_hour)
       aggregate_into_groups(rows).each do |compacted_views|
         created << compact_records(start_hour, compacted_views)
       end

app/views/pages/show.en.html.erb

@@ -19,27 +19,29 @@
 <% end %>
 
 <main id="main-content">
-  <% if @page.template == "contained" %>
-    <div class="crayons-layout crayons-layout--limited-l">
-      <div class="crayons-card text-styles text-padding">
-        <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
+  <div id="page-body" data-page-id="<%= @page.id %>">
+    <% if @page.template == "contained" %>
+      <div class="crayons-layout crayons-layout--limited-l">
+        <div class="crayons-card text-styles text-padding">
+          <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
 
-        <%= @page.processed_html.html_safe %>
+          <%= @page.processed_html.html_safe %>
+        </div>
       </div>
-    </div>
-  <% elsif @page.template == "nav_bar_included" %>
-    <div class="crayons-layout crayons-layout--2-cols">
-      <div class="hidden m:block">
-        <%= render "articles/sidebar" %>
+    <% elsif @page.template == "nav_bar_included" %>
+      <div class="crayons-layout crayons-layout--2-cols">
+        <div class="hidden m:block">
+          <%= render "articles/sidebar" %>
+        </div>
+        <div class="crayons-card text-styles text-padding">
+          <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
+          <%= @page.processed_html.html_safe %>
+        </div>
       </div>
-      <div class="crayons-card text-styles text-padding">
-        <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
-        <%= @page.processed_html.html_safe %>
-      </div>
-    </div>
-  <% else %>
-    <%= @page.processed_html.html_safe %>
-  <% end %>
+    <% else %>
+      <%= @page.processed_html.html_safe %>
+    <% end %>
+  </div>
 </main>
 <div class="js-billboard-container pb-4 crayons-layout__comments-billboard" data-async-url="<%= billboard_path(page_id: @page.id, placement_area: :page_fixed_bottom) %>"></div>
 <%= javascript_include_tag "billboard", defer: true %>

app/views/pages/show.fr.html.erb

@@ -19,26 +19,28 @@
 <% end %>
 
 <main id="main-content">
-  <% if @page.template == "contained" %>
-    <div class="crayons-layout crayons-layout--limited-l">
-      <div class="crayons-card text-styles text-padding">
-        <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
+  <div id="page-body" data-page-id="<%= @page.id %>">
+    <% if @page.template == "contained" %>
+      <div class="crayons-layout crayons-layout--limited-l">
+        <div class="crayons-card text-styles text-padding">
+          <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
 
-        <%= @page.processed_html.html_safe %>
+          <%= @page.processed_html.html_safe %>
+        </div>
       </div>
-    </div>
-  <% elsif @page.template == "nav_bar_included" %>
-    <div class="crayons-layout crayons-layout--2-cols">
-      <div class="hidden m:block">
-        <%= render "articles/sidebar" %>
+    <% elsif @page.template == "nav_bar_included" %>
+      <div class="crayons-layout crayons-layout--2-cols">
+        <div class="hidden m:block">
+          <%= render "articles/sidebar" %>
+        </div>
+        <div class="crayons-card text-styles text-padding">
+          <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
+          <%= @page.processed_html.html_safe %>
+        </div>
       </div>
-      <div class="crayons-card text-styles text-padding">
-        <h1 class="fs-3xl s:fs-4xl l:fs-5xl fw-bold s:fw-heavy lh-tight mb-4 mt-0"><%= @page.title %></h1>
-        <%= @page.processed_html.html_safe %>
-      </div>
-    </div>
-  <% else %>
-    <%= @page.processed_html.html_safe %>
-  <% end %>
+    <% else %>
+      <%= @page.processed_html.html_safe %>
+    <% end %>
+  </div>
 </main>
 <div class="js-billboard-container pb-4 crayons-layout__comments-billboard" data-async-url="<%= billboard_path(page_id: @page.id, placement_area: :page_fixed_bottom) %>"></div>

app/workers/articles/update_page_views_worker.rb

@@ -9,27 +9,31 @@ class UpdatePageViewsWorker
 
     # @see Articles::PageViewUpdater
     def perform(create_params)
-      article = Article.find_by(id: create_params["article_id"])
-      return unless article&.published?
-      return if create_params[:user_id] && article.user_id == create_params[:user_id]
+      if create_params["article_id"]
+        article = Article.find_by(id: create_params["article_id"])
+        return unless article&.published?
+        return if create_params[:user_id] && article.user_id == create_params[:user_id]
 
-      PageView.create!(create_params)
+        PageView.create!(create_params)
 
-      updated_count = article.page_views.sum(:counts_for_number_of_views)
-      if updated_count > article.page_views_count
-        article.update_column(:page_views_count, updated_count)
-      end
+        updated_count = article.page_views.sum(:counts_for_number_of_views)
+        if updated_count > article.page_views_count
+          article.update_column(:page_views_count, updated_count)
+        end
 
-      # PageViewsController#create called the method update_organic_page_views
-      # at the end. The relationship between the two was 12.5% chance (rand(8))
-      # and 1% chance (rand(100)), or roughly 12x more likely for page view
-      # updates vs. organic page view updates. We kept a similar relationship
-      # between the two workers, this one here is schedule after 2 minutes,
-      # organic page view updates after 25 minutes.
-      Articles::UpdateOrganicPageViewsWorker.perform_at(
-        25.minutes.from_now,
-        article.id,
-      )
+        # PageViewsController#create called the method update_organic_page_views
+        # at the end. The relationship between the two was 12.5% chance (rand(8))
+        # and 1% chance (rand(100)), or roughly 12x more likely for page view
+        # updates vs. organic page view updates. We kept a similar relationship
+        # between the two workers, this one here is schedule after 2 minutes,
+        # organic page view updates after 25 minutes.
+        Articles::UpdateOrganicPageViewsWorker.perform_at(
+          25.minutes.from_now,
+          article.id,
+        )
+      elsif create_params["page_id"]
+        PageView.create!(create_params)
+      end
     end
   end
 end

db/migrate/20240422145748_add_page_to_page_views.rb

@@ -0,0 +1,7 @@
+class AddPageToPageViews < ActiveRecord::Migration[7.0]
+  disable_ddl_transaction!
+
+  def change
+    add_reference :page_views, :page, null: true, index: {algorithm: :concurrently}
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_04_19_201108) do
+ActiveRecord::Schema[7.0].define(version: 2024_04_22_145748) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "ltree"
@@ -777,6 +777,7 @@
     t.integer "counts_for_number_of_views", default: 1
     t.datetime "created_at", precision: nil, null: false
     t.string "domain"
+    t.bigint "page_id"
     t.string "path"
     t.string "referrer"
     t.integer "time_tracked_in_seconds", default: 15
@@ -785,6 +786,7 @@
     t.bigint "user_id"
     t.index ["article_id"], name: "index_page_views_on_article_id"
     t.index ["created_at"], name: "index_page_views_on_created_at"
+    t.index ["page_id"], name: "index_page_views_on_page_id"
     t.index ["user_id"], name: "index_page_views_on_user_id"
   end
 

spec/factories/page_views.rb

@@ -4,4 +4,10 @@
     article
     referrer { Faker::Internet.url }
   end
+
+  factory :page_page_view, class: "PageView" do
+    user
+    page
+    referrer { Faker::Internet.url }
+  end
 end

spec/models/page_view_spec.rb

@@ -2,9 +2,35 @@
 
 RSpec.describe PageView do
   let(:page_view) { create(:page_view, referrer: "http://example.com/page") }
+  let(:page_page_view) { create(:page_page_view, referrer: "http://example.com/page") }
 
-  it { is_expected.to belong_to(:user).optional }
-  it { is_expected.to belong_to(:article) }
+  describe "article validations" do
+    subject { page_view }
+
+    it { is_expected.to belong_to(:user).optional }
+    it { is_expected.to belong_to(:article).optional } # Add .optional here
+  end
+
+  describe "page validations" do
+    subject { page_page_view }
+
+    it { is_expected.to belong_to(:user).optional }
+    it { is_expected.to belong_to(:page).optional } # Add .optional here
+  end
+
+  describe "validations" do
+    # Add a new describe block for validations
+    context "when both article and page are present" do
+      let(:invalid_page_view) { build(:page_view, article: create(:article), page: create(:page)) }
+
+      it "is invalid" do
+        expect(invalid_page_view).not_to be_valid
+        expect(invalid_page_view.errors[:base]).to include(
+          "PageView must belong to either an Article or a Page, but not both",
+        )
+      end
+    end
+  end
 
   context "when callbacks are triggered before create" do
     describe "#domain" do

spec/requests/page_views_spec.rb

@@ -3,6 +3,7 @@
 RSpec.describe "PageViews" do
   let(:user) { create(:user, :trusted) }
   let(:article) { create(:article) }
+  let(:page) { create(:page) }
 
   def page_views_post(params)
     sidekiq_perform_enqueued_jobs do
@@ -27,6 +28,15 @@ def page_views_post(params)
         expect(PageView.last.counts_for_number_of_views).to eq(1)
       end
 
+      it "creates a new page view for pages", :aggregate_failures do
+        page_views_post(page_id: page.id)
+
+        expect(page.reload.page_views.size).to eq(1)
+        expect(page.reload.page_views.sum(:counts_for_number_of_views)).to eq(1)
+        expect(user.reload.page_views.size).to eq(1)
+        expect(PageView.last.counts_for_number_of_views).to eq(1)
+      end
+
       it "sends referrer" do
         page_views_post(article_id: article.id, referrer: "test")
 
@@ -78,6 +88,15 @@ def page_views_post(params)
         expect(PageView.last.counts_for_number_of_views).to eq(10)
       end
 
+      it "creates a new page view for pages", :aggregate_failures do
+        page_views_post(page_id: page.id)
+
+        expect(page.reload.page_views.size).to eq(1)
+        expect(page.reload.page_views.sum(:counts_for_number_of_views)).to eq(10)
+        expect(user.reload.page_views.size).to eq(0)
+        expect(PageView.last.counts_for_number_of_views).to eq(10)
+      end
+
       it "stores aggregate page views" do
         page_views_post(article_id: article.id)
         page_views_post(article_id: article.id)

spec/services/page_view_rollup_spec.rb

@@ -3,6 +3,8 @@
 RSpec.describe PageViewRollup, type: :service do
   let(:article1) { create(:article) }
   let(:article2) { create(:article) }
+  let(:page1) { create(:page) }
+  let(:page2) { create(:page) }
   let(:user1) { create(:user) }
   let(:user2) { create(:user) }
 
@@ -72,4 +74,15 @@ def days_ago_as_range(num)
     expect(PageView.pluck(:article_id,
                           :counts_for_number_of_views)).to contain_exactly([article1.id, 2], [article2.id, 2])
   end
+
+  it "does not compact views of the same page" do
+    create(:page_page_view, page: page1, user: nil, created_at: 2.days.ago)
+    create(:page_page_view, page: page1, user: nil, created_at: 2.days.ago)
+    create(:page_page_view, page: page2, user: nil, created_at: 2.days.ago)
+    create(:page_page_view, page: page2, user: nil, created_at: 2.days.ago)
+
+    expect do
+      described_class.rollup(2.days.ago)
+    end.not_to change(PageView, :count)
+  end
 end