From 5ec47d99dc56105a7d457c3702ad69fa64e465f4 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Tue, 18 Jan 2022 11:23:18 -0500 Subject: [PATCH] Ensuring we don't track views of author or unpublished (#16143) * Ensuring we don't track views of author or unpublished Prior to this commit, I was surprised to learn that we: 1) Tracked an author's view of their article. 2) Tracked views of an unpublished article. This came up from a Forem creator asking if they could reset the view counter. Or trigger the reset on publication. I think a general business logic policy of don't track views for the author and don't track views for unpublished articles is a reasonable default. Were we to pursue the clear views on publication, we'd need to consider something that went from unpublished -> published -> unpublished -> published. Without a more explicit state machine, triggering a busineiss logic behavior seems a bit unexpected. In other words, I wrote an article. There are 20 views when I realize that I need to unpublished it. I make the changes in the unpublished state, and re-publish. I'd assume that those 20 views would still be "recorded" and counted towards my article's view counts. * Adjusting condition structure Prior to this commit, the `if` clause was rather far to the right. This helps make the if clause more pronounced. --- app/controllers/page_views_controller.rb | 7 +--- app/services/articles/page_view_updater.rb | 33 +++++++++++++++++++ .../articles/update_page_views_worker.rb | 5 +-- .../articles/page_view_updater_spec.rb | 33 +++++++++++++++++++ .../articles/update_page_views_worker_spec.rb | 20 +++++++++++ 5 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 app/services/articles/page_view_updater.rb create mode 100644 spec/services/articles/page_view_updater_spec.rb diff --git a/app/controllers/page_views_controller.rb b/app/controllers/page_views_controller.rb index 02ed085da..671cd99c6 100644 --- a/app/controllers/page_views_controller.rb +++ b/app/controllers/page_views_controller.rb @@ -32,12 +32,7 @@ class PageViewsController < ApplicationMetalController def update if session_current_user_id - page_view = PageView.order(created_at: :desc) - .find_or_create_by(article_id: params[:id], user_id: session_current_user_id) - - unless page_view.new_record? - page_view.update_column(:time_tracked_in_seconds, page_view.time_tracked_in_seconds + 15) - end + Articles::PageViewUpdater.call(article_id: params[:id], user_id: session_current_user_id) end head :ok diff --git a/app/services/articles/page_view_updater.rb b/app/services/articles/page_view_updater.rb new file mode 100644 index 000000000..f5a8d69df --- /dev/null +++ b/app/services/articles/page_view_updater.rb @@ -0,0 +1,33 @@ +module Articles + # This module is responsible for updating a specific page view for a given article and user. + # + # @see Articles::UpdatePageViewsWorker for the sibling that's responsible for recording page + # views. + module PageViewUpdater + # @param article_id [Integer] + # @param user_id [Integer] + # + # @return [TrueClass] if we updated or created a PageView + # @return [FalseClass] if we did not update a PageView + # + # @note Regardless of return status, consider the business logic successful unless we raise an + # exception. The return value is present for easing testing. The `find_or_create_by` + # adds a complication in the testing logic + # (e.g., `expect { Articles::PageViewUpdater.call }.not_to change(PageView, :count) ` + def self.call(article_id:, user_id:) + # Don't record views to unpublished articles. + return false if Article.unpublished.exists?(id: article_id) + # Don't record author's own views. + return false if Article.published.exists?(id: article_id, user_id: user_id) + + page_view = PageView.order(created_at: :desc) + .find_or_create_by(article_id: article_id, user_id: user_id) + + return true if page_view.new_record? + + page_view.update_column(:time_tracked_in_seconds, page_view.time_tracked_in_seconds + 15) + + true + end + end +end diff --git a/app/workers/articles/update_page_views_worker.rb b/app/workers/articles/update_page_views_worker.rb index 6f5d0f5ac..098bb8ea8 100644 --- a/app/workers/articles/update_page_views_worker.rb +++ b/app/workers/articles/update_page_views_worker.rb @@ -7,11 +7,12 @@ module Articles on_conflict: :replace, retry: false - # @todo Should we ever record a PageView for an article in draft status? (e.g., a preview) - # @todo Should we ever record a PageView for the author of the article? + # @see Articles::PageViewUpdater def perform(create_params) article = Article.find_by(id: create_params["article_id"]) return unless article + return unless article.published? + return if create_params[:user_id] && article.user_id == create_params[:user_id] PageView.create!(create_params) diff --git a/spec/services/articles/page_view_updater_spec.rb b/spec/services/articles/page_view_updater_spec.rb new file mode 100644 index 000000000..a223dabb7 --- /dev/null +++ b/spec/services/articles/page_view_updater_spec.rb @@ -0,0 +1,33 @@ +require "rails_helper" + +RSpec.describe Articles::PageViewUpdater do + describe "#call" do + subject(:method_call) { described_class.call(article_id: article.id, user_id: user.id) } + + let(:user) { create(:user) } + + context "when article published and written by another user" do + let(:article) { create(:article, user: create(:user)) } + + it "updates a user's page view" do + expect { method_call }.to change(PageView, :count) + end + end + + context "when article is unpublished" do + let(:article) { create(:article, published: false, published_at: nil) } + + it "skips updating" do + expect { method_call }.not_to change(PageView, :count) + end + end + + context "when article written by given user" do + let(:article) { create(:article, user: user) } + + it "skips updating" do + expect { method_call }.not_to change(PageView, :count) + end + end + end +end diff --git a/spec/workers/articles/update_page_views_worker_spec.rb b/spec/workers/articles/update_page_views_worker_spec.rb index 6cde17c4a..45f40e3e2 100644 --- a/spec/workers/articles/update_page_views_worker_spec.rb +++ b/spec/workers/articles/update_page_views_worker_spec.rb @@ -3,6 +3,26 @@ require "rails_helper" RSpec.describe Articles::UpdatePageViewsWorker, type: :worker do let(:worker) { described_class.new } + context "when the article is unpublished" do + subject(:perform) { worker.perform(article_id: article.id) } + + let(:article) { create(:article, published: false, published_at: nil) } + + it "does not create a page view" do + expect { perform }.not_to change(PageView, :count) + end + end + + context "when the article is published and written by the given user" do + subject(:perform) { worker.perform(article_id: article.id, user_id: article.user_id) } + + let(:article) { create(:article) } + + it "does not create a page view" do + expect { perform }.not_to change(PageView, :count) + end + end + context "when the article id is invalid" do let(:article_id) { :no_article_with_this_id }