From bd4e8422fe46f5eff63fdc073f480ec948b0e1f8 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 22 Oct 2024 10:06:22 +1000 Subject: [PATCH] FEATURE: Revive legacy pageview reports (#29308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit brings back some reports hidden or changed by the commit in 14b436923c5b582cea454b69441a28e16f2f191e 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 | 23 +++++++- .../reports/consolidated_page_views.rb | 6 +- app/models/report.rb | 55 +++++++++++-------- config/locales/server.en.yml | 15 ++++- .../requests/admin/reports_controller_spec.rb | 12 +++- 5 files changed, 79 insertions(+), 32 deletions(-) diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index ea3c0a6cd5b..c22e2697739 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/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] } diff --git a/app/models/concerns/reports/consolidated_page_views.rb b/app/models/concerns/reports/consolidated_page_views.rb index d73f764c93a..4d48f18415b 100644 --- a/app/models/concerns/reports/consolidated_page_views.rb +++ b/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 - # always false or no longer needed we can delete this. + # NOTE: This report is superseded by "SiteTraffic". Eventually once + # 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] diff --git a/app/models/report.rb b/app/models/report.rb index 03765b52b98..37770371bd3 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -305,30 +305,14 @@ class Report # they can be removed. def self.req_report(report, filter = nil) data = + # For this report we intentionally do not want to count mobile pageviews. if filter == :page_view_total - # For this report we intentionally do not want to count mobile pageviews. - if SiteSetting.use_legacy_pageviews - # We purposefully exclude "browser" pageviews. See - # `ConsolidatedPageViewsBrowserDetection` for browser pageviews. - ApplicationRequest.where( - req_type: [ - ApplicationRequest.req_types[:page_view_crawler], - ApplicationRequest.req_types[:page_view_anon], - 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 + SiteSetting.use_legacy_pageviews ? legacy_page_view_requests : page_view_requests + # This is a separate report because if people have switched over + # to _not_ use legacy pageviews, we want to show both a Pageviews + # and Legacy Pageviews report. + elsif filter == :page_view_legacy_total_reqs + legacy_page_view_requests 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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 561be644488..4d883b97abe 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/spec/requests/admin/reports_controller_spec.rb b/spec/requests/admin/reports_controller_spec.rb index 0e2f53f38d5..3a6b1a57f16 100644 --- a/spec/requests/admin/reports_controller_spec.rb +++ b/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