FEATURE: Revive legacy pageview reports (#29308)

This commit brings back some reports hidden or changed
by the commit in 14b436923c if
the site setting `use_legacy_pageviews` is false.

* Unhide the old “Consolidated Pageviews” report and rename it
  to “Legacy Consolidated Pageviews”
* Add a legacy_page_view_total_reqs report called “Legacy Pageviews”,
  which calculates pageviews in the same way the old page_view_total_reqs
  report did.

This will allow admins to better compare old and new pageview
stats which are based on browser detection if they have switched
over to _not_ use legacy pageviews.

app/controllers/admin/reports_controller.rb

@@ -3,10 +3,9 @@
 class Admin::ReportsController < Admin::StaffController
   REPORTS_LIMIT = 50
 
-  HIDDEN_PAGEVIEW_REPORTS = ["site_traffic"]
+  HIDDEN_PAGEVIEW_REPORTS = %w[site_traffic page_view_legacy_total_reqs]
 
   HIDDEN_LEGACY_PAGEVIEW_REPORTS = %w[
-    consolidated_page_views
     consolidated_page_views_browser_detection
     page_view_anon_reqs
     page_view_logged_in_reqs
@@ -21,6 +20,10 @@ class Admin::ReportsController < Admin::StaffController
           .select { |r| r =~ /\Apage_view_/ && r !~ /mobile/ }
           .map { |r| r + "_reqs" }
 
+    if !SiteSetting.use_legacy_pageviews
+      page_view_req_report_methods << "page_view_legacy_total_reqs"
+    end
+
     reports_methods =
       page_view_req_report_methods +
         Report.singleton_methods.grep(/\Areport_(?!about|storage_stats)/)
@@ -38,13 +41,27 @@ class Admin::ReportsController < Admin::StaffController
             next reports_acc if HIDDEN_LEGACY_PAGEVIEW_REPORTS.include?(type)
           end
 
-          reports_acc << {
+          report_data = {
             type: type,
             title: I18n.t("reports.#{type}.title"),
             description: description.presence ? description : nil,
             description_link: description_link.presence ? description_link : nil,
           }
 
+          # HACK: We need to show a different label and description for this
+          # old report while people are still relying on it, that lets us
+          # point toward the new 'Site traffic' report as well. Not ideal,
+          # but apart from duplicating the report there's not a nicer way to do this.
+          if SiteSetting.use_legacy_pageviews
+            if type == "consolidated_page_views" ||
+                 type === "consolidated_page_views_browser_detection"
+              report_data[:title] = I18n.t("reports.#{type}.title_legacy")
+              report_data[:description] = I18n.t("reports.#{type}.description_legacy")
+            end
+          end
+
+          reports_acc << report_data
+
           reports_acc
         end
         .sort_by { |report| report[:title] }

app/models/concerns/reports/consolidated_page_views.rb

@@ -4,8 +4,10 @@ module Reports::ConsolidatedPageViews
   extend ActiveSupport::Concern
 
   class_methods do
-    # NOTE: This report is deprecated, once use_legacy_pageviews is
+    # NOTE: This report is superseded by "SiteTraffic". Eventually once
-    # always false or no longer needed we can delete this.
+    # use_legacy_pageviews is always false or no longer needed, and users
+    # no longer rely on the data in this old report in the transition period,
+    # we can delete this.
     def report_consolidated_page_views(report)
       filters = %w[page_view_logged_in page_view_anon page_view_crawler]
 

app/models/report.rb

@@ -305,30 +305,14 @@ class Report
   # they can be removed.
   def self.req_report(report, filter = nil)
     data =
-      if filter == :page_view_total
       # For this report we intentionally do not want to count mobile pageviews.
-        if SiteSetting.use_legacy_pageviews
+      if filter == :page_view_total
-          # We purposefully exclude "browser" pageviews. See
+        SiteSetting.use_legacy_pageviews ? legacy_page_view_requests : page_view_requests
-          # `ConsolidatedPageViewsBrowserDetection` for browser pageviews.
+        # This is a separate report because if people have switched over
-          ApplicationRequest.where(
+        # to _not_ use legacy pageviews, we want to show both a Pageviews
-            req_type: [
+        # and Legacy Pageviews report.
-              ApplicationRequest.req_types[:page_view_crawler],
+      elsif filter == :page_view_legacy_total_reqs
-              ApplicationRequest.req_types[:page_view_anon],
+        legacy_page_view_requests
-              ApplicationRequest.req_types[:page_view_logged_in],
-            ].flatten,
-          )
-        else
-          # We purposefully exclude "crawler" pageviews here and by
-          # only doing browser pageviews we are excluding "other" pageviews
-          # too. This is to reflect what is shown in the "Site traffic" report
-          # by default.
-          ApplicationRequest.where(
-            req_type: [
-              ApplicationRequest.req_types[:page_view_anon_browser],
-              ApplicationRequest.req_types[:page_view_logged_in_browser],
-            ].flatten,
-          )
-        end
       else
         ApplicationRequest.where(req_type: ApplicationRequest.req_types[filter])
       end
@@ -351,6 +335,31 @@ class Report
       )
   end
 
+  # We purposefully exclude "browser" pageviews. See
+  # `ConsolidatedPageViewsBrowserDetection` for browser pageviews.
+  def self.legacy_page_view_requests
+    ApplicationRequest.where(
+      req_type: [
+        ApplicationRequest.req_types[:page_view_crawler],
+        ApplicationRequest.req_types[:page_view_anon],
+        ApplicationRequest.req_types[:page_view_logged_in],
+      ].flatten,
+    )
+  end
+
+  # We purposefully exclude "crawler" pageviews here and by
+  # only doing browser pageviews we are excluding "other" pageviews
+  # too. This is to reflect what is shown in the "Site traffic" report
+  # by default.
+  def self.page_view_requests
+    ApplicationRequest.where(
+      req_type: [
+        ApplicationRequest.req_types[:page_view_anon_browser],
+        ApplicationRequest.req_types[:page_view_logged_in_browser],
+      ].flatten,
+    )
+  end
+
   def self.report_about(report, subject_class, report_method = :count_per_day)
     basic_report_about report, subject_class, report_method, report.start_date, report.end_date
     add_counts report, subject_class

config/locales/server.en.yml

@@ -1353,13 +1353,15 @@ en:
       yaxis: "Day"
       description: "Number of users who increased their Trust Level during this period."
     consolidated_page_views:
-      title: "Consolidated Pageviews"
+      title: "Legacy Consolidated Pageviews"
+      title_legacy: "Consolidated Pageviews"
       xaxis:
         page_view_crawler: "Crawlers"
         page_view_anon: "Anonymous users"
         page_view_logged_in: "Logged in users"
       yaxis: "Day"
-      description: "Pageviews for logged in users, anonymous users and crawlers."
+      description: "Legacy report showing pageviews for logged in users, anonymous users and crawlers. This has been superseded by the 'Site traffic' report."
+      description_legacy: "Pageviews for logged in users, anonymous users and crawlers."
       labels:
         post: Post
         editor: Editor
@@ -1374,13 +1376,15 @@ en:
       description: "API requests for regular API keys and user API keys."
     consolidated_page_views_browser_detection:
       title: "Consolidated Pageviews with Browser Detection (Experimental)"
+      title_legacy: "Consolidated Pageviews with Browser Detection (Experimental)"
       xaxis:
         page_view_anon_browser: "Anonymous Browser"
         page_view_logged_in_browser: "Logged In Browser"
         page_view_crawler: "Known Crawler"
         page_view_other: "Other pageviews"
       yaxis: "Day"
-      description: "Pageviews for logged in users, anonymous users, known crawlers and other. This experimental report ensures logged-in/anon requests are coming from real browsers before counting them. Historical data for this report is unavailable, for historical data see the ‘Consolidated Pageviews' report."
+      description: "Pageviews for logged in users, anonymous users, known crawlers and other. This experimental report ensures logged-in/anon requests are coming from real browsers before counting them. Historical data for this report is unavailable, for historical data see the 'Legacy Consolidated Pageviews' report."
+      description_legacy: "Pageviews for logged in users, anonymous users, known crawlers and other. This experimental report ensures logged-in/anon requests are coming from real browsers before counting them. Historical data for this report is unavailable, for historical data see the ‘Consolidated Pageviews' report."
     site_traffic:
       title: "Site traffic"
       xaxis:
@@ -1541,6 +1545,11 @@ en:
       xaxis: "Day"
       yaxis: "Total Pageviews"
       description: "Number of new pageviews from all visitors."
+    page_view_legacy_total_reqs:
+      title: "Legacy Pageviews"
+      xaxis: "Day"
+      yaxis: "Total Pageviews"
+      description: "Legacy report showing the number of new pageviews from all visitors."
     page_view_logged_in_mobile_reqs:
       title: "Logged In Pageviews"
       xaxis: "Day"

spec/requests/admin/reports_controller_spec.rb

@@ -246,7 +246,7 @@ RSpec.describe Admin::ReportsController do
         expect(response.status).to eq(200)
         expect(response.parsed_body["reports"].count).to eq(5)
         expect(response.parsed_body["reports"][0]["type"]).to eq("site_traffic")
-        expect(response.parsed_body["reports"][1]).to include("error" => "not_found", "data" => nil)
+        expect(response.parsed_body["reports"][1]["type"]).to eq("consolidated_page_views")
         expect(response.parsed_body["reports"][2]).to include("error" => "not_found", "data" => nil)
         expect(response.parsed_body["reports"][3]).to include("error" => "not_found", "data" => nil)
         expect(response.parsed_body["reports"][4]).to include("error" => "not_found", "data" => nil)
@@ -366,6 +366,11 @@ RSpec.describe Admin::ReportsController do
           expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
         end
       end
+
+      it "does not allow running the page_view_legacy_total_reqs report" do
+        get "/admin/reports/page_view_legacy_total_reqs.json"
+        expect(response.status).to eq(404)
+      end
     end
 
     context "when use_legacy_pageviews is false" do
@@ -381,6 +386,11 @@ RSpec.describe Admin::ReportsController do
           expect(response.parsed_body["errors"]).to include(I18n.t("not_found"))
         end
       end
+
+      it "does allow running the page_view_legacy_total_reqs report" do
+        get "/admin/reports/page_view_legacy_total_reqs.json"
+        expect(response.status).to eq(200)
+      end
     end
   end
 end