diff --git a/app/assets/javascripts/admin/components/admin-report.js.es6 b/app/assets/javascripts/admin/components/admin-report.js.es6 index 95d831af821..eb84df91ce4 100644 --- a/app/assets/javascripts/admin/components/admin-report.js.es6 +++ b/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"), diff --git a/app/assets/javascripts/admin/controllers/admin-dashboard-next-general.js.es6 b/app/assets/javascripts/admin/controllers/admin-dashboard-next-general.js.es6 index b8b74739677..ddc8c319d0c 100644 --- a/app/assets/javascripts/admin/controllers/admin-dashboard-next-general.js.es6 +++ b/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 => { diff --git a/app/assets/javascripts/admin/models/report.js.es6 b/app/assets/javascripts/admin/models/report.js.es6 index aceed0eef30..2098699eca8 100644 --- a/app/assets/javascripts/admin/models/report.js.es6 +++ b/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, diff --git a/app/assets/javascripts/admin/templates/components/admin-report.hbs b/app/assets/javascripts/admin/templates/components/admin-report.hbs index 6208c7796a0..b8a7c41144f 100644 --- a/app/assets/javascripts/admin/templates/components/admin-report.hbs +++ b/app/assets/javascripts/admin/templates/components/admin-report.hbs @@ -1,11 +1,5 @@ {{#if isEnabled}} {{#conditional-loading-section isLoading=isLoading}} - {{#if showTimeoutError}} -
- {{i18n "admin.dashboard.timeout_error"}} -
- {{/if}} - {{#if showHeader}}
{{#if showTitle}} @@ -66,17 +60,29 @@ {{/if}}
- {{#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}} +
+ {{d-icon "pie-chart"}} + {{i18n 'admin.dashboard.reports.no_data'}} +
{{/if}} {{else}} -
- {{d-icon "pie-chart"}} - {{i18n 'admin.dashboard.reports.no_data'}} -
- {{/if}} + {{#if showTimeoutError}} +
+ {{i18n "admin.dashboard.timeout_error"}} +
+ {{/if}} + + {{#if showExceptionError}} +
+ {{i18n "admin.dashboard.exception_error"}} +
+ {{/if}} {{/unless}} {{#if showFilteringUI}} diff --git a/app/assets/stylesheets/common/admin/admin_report.scss b/app/assets/stylesheets/common/admin/admin_report.scss index 5aaaa39b40c..492b6febca6 100644 --- a/app/assets/stylesheets/common/admin/admin_report.scss +++ b/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; diff --git a/app/models/admin_dashboard_next_data.rb b/app/models/admin_dashboard_next_data.rb index b384ec4a514..fbc76359e42 100644 --- a/app/models/admin_dashboard_next_data.rb +++ b/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 diff --git a/app/models/report.rb b/app/models/report.rb index add82795edc..5d94723c524 100644 --- a/app/models/report.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 532495a2a62..e90a58a7ec7 100644 --- a/config/locales/client.en.yml +++ b/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." diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index 6c1a3b0d3dc..0c2edd963b3 100644 --- a/spec/models/report_spec.rb +++ b/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 diff --git a/test/javascripts/components/admin-report-test.js.es6 b/test/javascripts/components/admin-report-test.js.es6 index 5a531cfafc5..59e94c564a5 100644 --- a/test/javascripts/components/admin-report-test.js.es6 +++ b/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"); } }); diff --git a/test/javascripts/fixtures/signups_exception.js.es6 b/test/javascripts/fixtures/signups_exception.js.es6 new file mode 100644 index 00000000000..9924484a8fc --- /dev/null +++ b/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; diff --git a/test/javascripts/fixtures/signups_timeout.js.es6 b/test/javascripts/fixtures/signups_timeout.js.es6 index 0c92485b111..9af1e2b8a89 100644 --- a/test/javascripts/fixtures/signups_timeout.js.es6 +++ b/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;