diff --git a/app/controllers/page_views_controller.rb b/app/controllers/page_views_controller.rb index 671cd99c62f1..e2b6cdb72b21 100644 --- a/app/controllers/page_views_controller.rb +++ b/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 diff --git a/app/javascript/packs/baseTracking.js b/app/javascript/packs/baseTracking.js index 2ce6abaaea0b..9776529b2551 100644 --- a/app/javascript/packs/baseTracking.js +++ b/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; diff --git a/app/models/page.rb b/app/models/page.rb index 47e5b9952a62..b528dd9de1aa 100644 --- a/app/models/page.rb +++ b/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 diff --git a/app/models/page_view.rb b/app/models/page_view.rb index 315fe1427b8e..961de3dbd622 100644 --- a/app/models/page_view.rb +++ b/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 diff --git a/app/services/page_view_rollup.rb b/app/services/page_view_rollup.rb index 21f3d7569360..3478a30790aa 100644 --- a/app/services/page_view_rollup.rb +++ b/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 diff --git a/app/views/pages/show.en.html.erb b/app/views/pages/show.en.html.erb index e5851924a7c4..bb34f51e3db2 100644 --- a/app/views/pages/show.en.html.erb +++ b/app/views/pages/show.en.html.erb @@ -19,27 +19,29 @@ <% end %>
- <% if @page.template == "contained" %> -
-
-

<%= @page.title %>

+
+ <% if @page.template == "contained" %> +
+
+

<%= @page.title %>

- <%= @page.processed_html.html_safe %> + <%= @page.processed_html.html_safe %> +
-
- <% elsif @page.template == "nav_bar_included" %> -
- - <% else %> - <%= @page.processed_html.html_safe %> - <% end %> + <% else %> + <%= @page.processed_html.html_safe %> + <% end %> +
<%= javascript_include_tag "billboard", defer: true %> diff --git a/app/views/pages/show.fr.html.erb b/app/views/pages/show.fr.html.erb index 958bbdf6f933..8d53b81fccb0 100644 --- a/app/views/pages/show.fr.html.erb +++ b/app/views/pages/show.fr.html.erb @@ -19,26 +19,28 @@ <% end %>
- <% if @page.template == "contained" %> -
-
-

<%= @page.title %>

+
+ <% if @page.template == "contained" %> +
+
+

<%= @page.title %>

- <%= @page.processed_html.html_safe %> + <%= @page.processed_html.html_safe %> +
-
- <% elsif @page.template == "nav_bar_included" %> -
- - <% else %> - <%= @page.processed_html.html_safe %> - <% end %> + <% else %> + <%= @page.processed_html.html_safe %> + <% end %> +
diff --git a/app/workers/articles/update_page_views_worker.rb b/app/workers/articles/update_page_views_worker.rb index 06ca7741144f..2984eb41c4d6 100644 --- a/app/workers/articles/update_page_views_worker.rb +++ b/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 diff --git a/db/migrate/20240422145748_add_page_to_page_views.rb b/db/migrate/20240422145748_add_page_to_page_views.rb new file mode 100644 index 000000000000..9ec47fa0677e --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 2ddce313dcdc..794e02b9d611 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/factories/page_views.rb b/spec/factories/page_views.rb index 24f144573634..975d50dac658 100644 --- a/spec/factories/page_views.rb +++ b/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 diff --git a/spec/models/page_view_spec.rb b/spec/models/page_view_spec.rb index b88788b1e829..e3f807dd4ac6 100644 --- a/spec/models/page_view_spec.rb +++ b/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 diff --git a/spec/requests/page_views_spec.rb b/spec/requests/page_views_spec.rb index b91bf82e240e..87be71546f8b 100644 --- a/spec/requests/page_views_spec.rb +++ b/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) diff --git a/spec/services/page_view_rollup_spec.rb b/spec/services/page_view_rollup_spec.rb index c95dd1606cbd..d5244ca50a15 100644 --- a/spec/services/page_view_rollup_spec.rb +++ b/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