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

@@ -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

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

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)
 

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

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 }