FIX: makes dashboard more resilient to errors (#6217)

This commit is an attempt to limit cases where the dashboard will generate a full exception page and also make it easier to track the error.

app/assets/javascripts/admin/components/admin-report.js.es6

@@ -110,7 +110,9 @@ export default Ember.Component.extend({
     unregisterTooltip($(".info[data-tooltip]"));
   },
 
-  showTimeoutError: Ember.computed.alias("model.timeout"),
+  showError: Ember.computed.or("showTimeoutError", "showExceptionError"),
+  showTimeoutError: Ember.computed.equal("model.error", "timeout"),
+  showExceptionError: Ember.computed.equal("model.error", "exception"),
 
   hasData: Ember.computed.notEmpty("model.data"),
 

app/assets/javascripts/admin/controllers/admin-dashboard-next-general.js.es6

@@ -66,7 +66,9 @@ export default Ember.Controller.extend(PeriodComputationMixin, {
           this.setProperties({
             dashboardFetchedAt: new Date(),
             model: adminDashboardNextModel,
-            reports: adminDashboardNextModel.reports.map(x => Report.create(x))
+            reports: Ember.makeArray(adminDashboardNextModel.reports).map(x =>
+              Report.create(x)
+            )
           });
         })
         .catch(e => {

app/assets/javascripts/admin/models/report.js.es6

@@ -12,7 +12,7 @@ import { renderAvatar } from "discourse/helpers/user-avatar";
 
 // Change this line each time report format change
 // and you want to ensure cache is reset
-export const SCHEMA_VERSION = 1;
+export const SCHEMA_VERSION = 2;
 
 const Report = Discourse.Model.extend({
   average: false,

app/assets/javascripts/admin/templates/components/admin-report.hbs

@@ -1,11 +1,5 @@
 {{#if isEnabled}}
   {{#conditional-loading-section isLoading=isLoading}}
-    {{#if showTimeoutError}}
-      <div class="alert alert-error">
-        {{i18n "admin.dashboard.timeout_error"}}
-      </div>
-    {{/if}}
-
     {{#if showHeader}}
       <div class="report-header">
         {{#if showTitle}}
@@ -66,17 +60,29 @@
     {{/if}}
 
     <div class="report-body">
-      {{#unless showTimeoutError}}
-      {{#if hasData}}
-        {{#if currentMode}}
-          {{component modeComponent model=model options=options}}
+      {{#unless showError}}
+        {{#if hasData}}
+          {{#if currentMode}}
+            {{component modeComponent model=model options=options}}
+          {{/if}}
+        {{else}}
+          <div class="alert alert-info no-data">
+            {{d-icon "pie-chart"}}
+            {{i18n 'admin.dashboard.reports.no_data'}}
+          </div>
         {{/if}}
       {{else}}
-        <div class="alert alert-info no-data-alert">
-          {{d-icon "pie-chart"}}
-          {{i18n 'admin.dashboard.reports.no_data'}}
-        </div>
-      {{/if}}
+        {{#if showTimeoutError}}
+          <div class="alert alert-error report-error timeout">
+            {{i18n "admin.dashboard.timeout_error"}}
+          </div>
+        {{/if}}
+
+        {{#if showExceptionError}}
+          <div class="alert alert-error report-error exception">
+            {{i18n "admin.dashboard.exception_error"}}
+          </div>
+        {{/if}}
       {{/unless}}
 
       {{#if showFilteringUI}}

app/assets/stylesheets/common/admin/admin_report.scss

@@ -1,12 +1,22 @@
 .admin-report {
-  .no-data-alert {
-    background: $secondary;
-    border: 1px solid $primary-low;
-    color: $primary-low-mid;
+  .report-error,
+  .no-data {
+    width: 100%;
     width: 100%;
     align-self: flex-start;
     text-align: center;
     padding: 3em;
+  }
+
+  .report-error {
+    color: $danger;
+    border: 1px solid $danger;
+  }
+
+  .no-data {
+    background: $secondary;
+    border: 1px solid $primary-low;
+    color: $primary-low-mid;
 
     .d-icon-pie-chart {
       color: currentColor;

app/models/admin_dashboard_next_data.rb

@@ -5,10 +5,6 @@ class AdminDashboardNextData
     @opts = opts
   end
 
-  def self.fetch_stats
-    self.class.new.as_json
-  end
-
   def self.fetch_stats
     new.as_json
   end

app/models/report.rb

@@ -3,14 +3,14 @@ require_dependency 'topic_subtype'
 class Report
   # Change this line each time report format change
   # and you want to ensure cache is reset
-  SCHEMA_VERSION = 1
+  SCHEMA_VERSION = 2
 
   attr_accessor :type, :data, :total, :prev30Days, :start_date,
                 :end_date, :category_id, :group_id, :labels, :async,
                 :prev_period, :facets, :limit, :processing, :average, :percent,
                 :higher_is_better, :icon, :modes, :category_filtering,
                 :group_filtering, :prev_data, :prev_start_date, :prev_end_date,
-                :dates_filtering, :timeout
+                :dates_filtering, :error
 
   def self.default_days
     30
@@ -113,7 +113,7 @@ class Report
       group_filtering: self.group_filtering,
       modes: self.modes,
     }.tap do |json|
-      json[:timeout] = self.timeout if self.timeout
+      json[:error] = self.error if self.error
       json[:total] = self.total if self.total
       json[:prev_period] = self.prev_period if self.prev_period
       json[:prev30Days] = self.prev30Days if self.prev30Days
@@ -160,15 +160,24 @@ class Report
   def self.find(type, opts = nil)
     clear_cache
 
-    report = _get(type, opts)
-    report_method = :"report_#{type}"
+    begin
+      report = _get(type, opts)
+      report_method = :"report_#{type}"
 
-    if respond_to?(report_method)
-      send(report_method, report)
-    elsif type =~ /_reqs$/
-      req_report(report, type.split(/_reqs$/)[0].to_sym)
-    else
-      return nil
+      if respond_to?(report_method)
+        send(report_method, report)
+      elsif type =~ /_reqs$/
+        req_report(report, type.split(/_reqs$/)[0].to_sym)
+      else
+        return nil
+      end
+    rescue Exception => e
+      report.error = :exception
+
+      # given reports can be added by plugins we don’t want dashboard failures
+      # on report computation, however we do want to log which report is provoking
+      # an error
+      Rails.logger.error("Error while computing report `#{report.type}`: #{e.message}")
     end
 
     report
@@ -596,7 +605,7 @@ class Report
 
     options = { end_date: report.end_date, start_date: report.start_date, limit: report.limit || 8 }
     result = nil
-    report.timeout = wrap_slow_query do
+    report.error = wrap_slow_query do
       result = IncomingLinksReport.find(:top_referred_topics, options)
       report.data = result.data
     end
@@ -622,7 +631,7 @@ class Report
 
     options = { end_date: report.end_date, start_date: report.start_date, limit: report.limit || 8 }
     result = nil
-    report.timeout = wrap_slow_query do
+    report.error = wrap_slow_query do
       result = IncomingLinksReport.find(:top_traffic_sources, options)
       report.data = result.data
     end

config/locales/client.en.yml

@@ -2792,6 +2792,7 @@ en:
         moderation_tab: "Moderation"
         disabled: Disabled
         timeout_error: Sorry, query is taking too long, please pick a shorter interval
+        exception_error: Sorry, an error occurred while executing the query
 
         reports:
           trend_title: "%{percent} change. Currently %{current}, was %{prev} in previous period."

spec/models/report_spec.rb

@@ -475,6 +475,8 @@ describe Report do
       let(:post) { Fabricate(:post) }
 
       before do
+        freeze_time
+
         PostAction.act(flagger, post, PostActionType.types[:spam], message: 'bad')
       end
 
@@ -507,6 +509,8 @@ describe Report do
       let(:post) { Fabricate(:post) }
 
       before do
+        freeze_time
+
         post.revise(editor, raw: 'updated body', edit_reason: 'not cool')
       end
 
@@ -674,4 +678,38 @@ describe Report do
       end
     end
   end
+
+  describe "exception report" do
+    before(:each) do
+      class Report
+        def self.report_exception_test(report)
+          report.data = x
+        end
+      end
+    end
+
+    it "returns a report with an exception error" do
+      report = Report.find("exception_test")
+      expect(report.error).to eq(:exception)
+    end
+  end
+
+  describe "timeout report" do
+    before(:each) do
+      freeze_time
+
+      class Report
+        def self.report_timeout_test(report)
+          report.error = wrap_slow_query(1) do
+            ActiveRecord::Base.connection.execute("SELECT pg_sleep(5)")
+          end
+        end
+      end
+    end
+
+    it "returns a report with a timeout error" do
+      report = Report.find("timeout_test")
+      expect(report.error).to eq(:timeout)
+    end
+  end
 end

test/javascripts/components/admin-report-test.js.es6

@@ -112,7 +112,7 @@ componentTest("timeout", {
   template: "{{admin-report dataSourceName='signups_timeout'}}",
 
   test(assert) {
-    assert.ok(exists(".alert-error"), "it displays a timeout error");
+    assert.ok(exists(".alert-error.timeout"), "it displays a timeout error");
   }
 });
 
@@ -120,6 +120,14 @@ componentTest("no data", {
   template: "{{admin-report dataSourceName='posts'}}",
 
   test(assert) {
-    assert.ok(exists(".no-data-alert"), "it displays a no data alert");
+    assert.ok(exists(".no-data"), "it displays a no data alert");
+  }
+});
+
+componentTest("exception", {
+  template: "{{admin-report dataSourceName='signups_exception'}}",
+
+  test(assert) {
+    assert.ok(exists(".alert-error.exception"), "it displays an error");
   }
 });

test/javascripts/fixtures/signups_exception.js.es6

@@ -0,0 +1,11 @@
+import signups from "fixtures/signups";
+
+const signupsExceptionKey = "/admin/reports/signups_exception";
+const signupsKey = "/admin/reports/signups";
+
+let fixture = {};
+
+fixture[signupsExceptionKey] = JSON.parse(JSON.stringify(signups[signupsKey]));
+fixture[signupsExceptionKey].report.error = "exception";
+
+export default fixture;

test/javascripts/fixtures/signups_timeout.js.es6

@@ -1,35 +1,11 @@
-export default {
-  "/admin/reports/signups_timeout": {
-    report: {
-      type: "signups",
-      title: "Signups",
-      xaxis: "Day",
-      yaxis: "Number of signups",
-      description: "New account registrations for this period",
-      data: null,
-      start_date: "2018-06-16T00:00:00Z",
-      end_date: "2018-07-16T23:59:59Z",
-      prev_data: null,
-      prev_start_date: "2018-05-17T00:00:00Z",
-      prev_end_date: "2018-06-17T00:00:00Z",
-      category_id: null,
-      group_id: null,
-      prev30Days: null,
-      dates_filtering: true,
-      report_key: "reports:signups_timeout::20180616:20180716::[:prev_period]:",
-      labels: [
-        { type: "date", properties: ["x"], title: "Day" },
-        { type: "number", properties: ["y"], title: "Count" }
-      ],
-      processing: false,
-      average: false,
-      percent: false,
-      higher_is_better: true,
-      category_filtering: false,
-      group_filtering: true,
-      modes: ["table", "chart"],
-      prev_period: 961,
-      timeout: true
-    }
-  }
-};
+import signups from "fixtures/signups";
+
+const signupsTimeoutKey = "/admin/reports/signups_timeout";
+const signupsKey = "/admin/reports/signups";
+
+let fixture = {};
+
+fixture[signupsTimeoutKey] = JSON.parse(JSON.stringify(signups[signupsKey]));
+fixture[signupsTimeoutKey].report.error = "timeout";
+
+export default fixture;