From f8480ed911bb41ada4fc1f09d3b7cbbeb3a0833e Mon Sep 17 00:00:00 2001 From: Tarek Khalil <45508821+khalilovcmded@users.noreply.github.com> Date: Fri, 15 Mar 2019 12:15:38 +0000 Subject: [PATCH] FEATURE: Exposing a way to add a generic report filter (#6816) * FEATURE: Exposing a way to add a generic report filter ## Why do we need this change? Part of the work discussed [here](https://meta.discourse.org/t/gain-understanding-of-file-uploads-usage/104994), and implemented a first spike [here](https://github.com/discourse/discourse/pull/6809), I am trying to expose a single generic filter selector per report. ## How does this work? We basically expose a simple, single generic filter that is computed and displayed based on backend values passed into the report. This would be a simple contract between the frontend and the backend. **Backend changes:** we simply need to return a list of dropdown / select options, and enable the report's newly introduced `custom_filtering` property. For example, for our [Top Uploads](https://github.com/discourse/discourse/pull/6809/files#diff-3f97cbb8726f3310e0b0c386dbe89e22R1423) report, it can look like this on the backend: ```ruby report.custom_filtering = true report.custom_filter_options = [{ id: "any", name: "Any" }, { id: "jpg", name: "JPEG" } ] ``` In our javascript report HTTP call, it will look like: ```js { "custom_filtering": true, "custom_filter_options": [ { "id": "any", "name": "Any" }, { "id": "jpg", "name": "JPG" } ] } ``` **Frontend changes:** We introduced a generic `filter` param and a `combo-box` which hooks up into the existing framework for fetching a report. This works alright, with the limitation of being a single custom filter per report. If we wanted to add, for an instance a `filesize filter`, this will not work for us. _I went through with this approach because it is hard to predict and build abstractions for requirements or problems we don't have yet, or might not have._ ## How does it look like? ![a1ktg1odde](https://user-images.githubusercontent.com/45508821/50485875-f17edb80-09ee-11e9-92dd-1454ab041fbb.gif) ## More on the bigger picture The major concern here I have is the solution I introduced might serve the `think small` version of the reporting work, but I don't think it serves the `think big`, I will try to shed some light into why. Within the current design, It is hard to maintain QueryParams for dynamically generated params (based on the idea of introducing more than one custom filter per report). To allow ourselves to have more than one generic filter, we will need to: a. Use the Route's model to retrieve the report's payload (we are now dependent on changes of the QueryParams via computed properties) b. After retrieving the payload, we can use the `setupController` to define our dynamic QueryParams based on the custom filters definitions we received from the backend c. Load a custom filter specific Ember component based on the definitions we received from the backend --- .../components/admin-report-table.js.es6 | 4 +- .../admin/components/admin-report.js.es6 | 47 ++++++++++++++++++- .../controllers/admin-reports-show.js.es6 | 8 ++-- .../javascripts/admin/models/report.js.es6 | 8 +++- .../templates/components/admin-report.hbs | 12 +++++ app/controllers/admin/reports_controller.rb | 6 +++ app/models/report.rb | 42 +++++++++++++---- config/locales/client.en.yml | 1 + 8 files changed, 111 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/admin/components/admin-report-table.js.es6 b/app/assets/javascripts/admin/components/admin-report-table.js.es6 index 7f75af90273..28a7e3d84d3 100644 --- a/app/assets/javascripts/admin/components/admin-report-table.js.es6 +++ b/app/assets/javascripts/admin/components/admin-report-table.js.es6 @@ -79,8 +79,8 @@ export default Ember.Component.extend({ if (sortLabel) { const compare = (label, direction) => { return (a, b) => { - let aValue = label.compute(a).value; - let bValue = label.compute(b).value; + const aValue = label.compute(a, { useSortProperty: true }).value; + const bValue = label.compute(b, { useSortProperty: true }).value; const result = aValue < bValue ? -1 : aValue > bValue ? 1 : 0; return result * direction; }; diff --git a/app/assets/javascripts/admin/components/admin-report.js.es6 b/app/assets/javascripts/admin/components/admin-report.js.es6 index 541d77eb681..d1c186effec 100644 --- a/app/assets/javascripts/admin/components/admin-report.js.es6 +++ b/app/assets/javascripts/admin/components/admin-report.js.es6 @@ -56,6 +56,7 @@ export default Ember.Component.extend({ endDate: null, category: null, groupId: null, + filter: null, showTrend: false, showHeader: true, showTitle: true, @@ -85,6 +86,7 @@ export default Ember.Component.extend({ this.setProperties({ category: Category.findById(state.categoryId), groupId: state.groupId, + filter: state.filter, startDate: state.startDate, endDate: state.endDate }); @@ -174,6 +176,18 @@ export default Ember.Component.extend({ return `admin-report-${currentMode}`; }, + @computed("model.filter_options") + filterOptions(options) { + if (options) { + return options.map(option => { + if (option.allowAny) { + option.choices.unshift(I18n.t("admin.dashboard.report_filter_any")); + } + return option; + }); + } + }, + @computed("startDate") normalizedStartDate(startDate) { return startDate && typeof startDate.isValid === "function" @@ -202,10 +216,11 @@ export default Ember.Component.extend({ "dataSourceName", "categoryId", "groupId", + "filter", "normalizedStartDate", "normalizedEndDate" ) - reportKey(dataSourceName, categoryId, groupId, startDate, endDate) { + reportKey(dataSourceName, categoryId, groupId, filter, startDate, endDate) { if (!dataSourceName || !startDate || !endDate) return null; let reportKey = "reports:"; @@ -215,6 +230,7 @@ export default Ember.Component.extend({ startDate.replace(/-/g, ""), endDate.replace(/-/g, ""), groupId, + filter, "[:prev_period]", this.get("reportOptions.table.limit"), SCHEMA_VERSION @@ -227,10 +243,35 @@ export default Ember.Component.extend({ }, actions: { + filter(filterOptionId, value) { + let params = []; + let paramPairs = {}; + let newParams = []; + + if (this.get("filter")) { + const filter = this.get("filter").slice(1, -1); + params = filter.split("&") || []; + params.map(p => { + const pair = p.split("="); + paramPairs[pair[0]] = pair[1]; + }); + } + + paramPairs[filterOptionId] = value; + Object.keys(paramPairs).forEach(key => { + if (paramPairs[key] !== I18n.t("admin.dashboard.report_filter_any")) { + newParams.push(`${key}=${paramPairs[key]}`); + } + }); + + this.set("filter", `[${newParams.join("&")}]`); + }, + refreshReport() { this.attrs.onRefresh({ categoryId: this.get("categoryId"), groupId: this.get("groupId"), + filter: this.get("filter"), startDate: this.get("startDate"), endDate: this.get("endDate") }); @@ -366,6 +407,10 @@ export default Ember.Component.extend({ payload.data.category_id = this.get("categoryId"); } + if (this.get("filter") && this.get("filter") !== "all") { + payload.data.filter = this.get("filter"); + } + if (this.get("reportOptions.table.limit")) { payload.data.limit = this.get("reportOptions.table.limit"); } diff --git a/app/assets/javascripts/admin/controllers/admin-reports-show.js.es6 b/app/assets/javascripts/admin/controllers/admin-reports-show.js.es6 index e04b16f6a75..8c773181ed9 100644 --- a/app/assets/javascripts/admin/controllers/admin-reports-show.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-reports-show.js.es6 @@ -1,7 +1,7 @@ import computed from "ember-addons/ember-computed-decorators"; export default Ember.Controller.extend({ - queryParams: ["start_date", "end_date", "category_id", "group_id"], + queryParams: ["start_date", "end_date", "category_id", "group_id", "filter"], @computed("model.type") reportOptions(type) { @@ -14,11 +14,12 @@ export default Ember.Controller.extend({ return options; }, - @computed("category_id", "group_id", "start_date", "end_date") - filters(categoryId, groupId, startDate, endDate) { + @computed("category_id", "group_id", "start_date", "end_date", "filter") + filters(categoryId, groupId, startDate, endDate, filter) { return { categoryId, groupId, + filter, startDate, endDate }; @@ -28,6 +29,7 @@ export default Ember.Controller.extend({ onParamsChange(params) { this.setProperties({ start_date: params.startDate, + filter: params.filter, category_id: params.categoryId, group_id: params.groupId, end_date: params.endDate diff --git a/app/assets/javascripts/admin/models/report.js.es6 b/app/assets/javascripts/admin/models/report.js.es6 index 53e9d90a207..aff881dd2d9 100644 --- a/app/assets/javascripts/admin/models/report.js.es6 +++ b/app/assets/javascripts/admin/models/report.js.es6 @@ -264,7 +264,13 @@ const Report = Discourse.Model.extend({ mainProperty, type, compute: (row, opts = {}) => { - const value = row[mainProperty]; + let value = null; + + if (opts.useSortProperty) { + value = row[label.sort_property || mainProperty]; + } else { + value = row[mainProperty]; + } if (type === "user") return this._userLabel(label.properties, row); if (type === "post") return this._postLabel(label.properties, row); diff --git a/app/assets/javascripts/admin/templates/components/admin-report.hbs b/app/assets/javascripts/admin/templates/components/admin-report.hbs index 14191e90234..c06e8b11aba 100644 --- a/app/assets/javascripts/admin/templates/components/admin-report.hbs +++ b/app/assets/javascripts/admin/templates/components/admin-report.hbs @@ -173,6 +173,18 @@ {{/if}} + {{#each filterOptions as |filterOption|}} +
+
+ {{combo-box content=filterOption.choices + filterable=true + allowAny=true + value=filterOption.selected + onSelect=(action "filter" filterOption.id)}} +
+
+ {{/each}} + {{#if showExport}}
diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb index 1ec347f25c0..fa5a6e374df 100644 --- a/app/controllers/admin/reports_controller.rb +++ b/app/controllers/admin/reports_controller.rb @@ -113,11 +113,17 @@ class Admin::ReportsController < Admin::AdminController limit = report_params[:limit].to_i end + filter = nil + if report_params.has_key?(:filter) + filter = report_params[:filter] + end + { start_date: start_date, end_date: end_date, category_id: category_id, group_id: group_id, + filter: filter, facets: facets, limit: limit } diff --git a/app/models/report.rb b/app/models/report.rb index e0410effa5b..c217844d4d8 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -6,11 +6,11 @@ class Report SCHEMA_VERSION = 3 attr_accessor :type, :data, :total, :prev30Days, :start_date, - :end_date, :category_id, :group_id, :labels, :async, - :prev_period, :facets, :limit, :processing, :average, :percent, + :end_date, :category_id, :group_id, :filter, + :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, :error, :primary_color, :secondary_color + :dates_filtering, :error, :primary_color, :secondary_color, :filter_options def self.default_days 30 @@ -29,6 +29,8 @@ class Report @modes = [:table, :chart] @prev_data = nil @dates_filtering = true + @filter_options = nil + @filter = nil tertiary = ColorScheme.hex_for_name('tertiary') || '0088cc' @primary_color = rgba_color(tertiary) @@ -43,6 +45,7 @@ class Report report.start_date.to_date.strftime("%Y%m%d"), report.end_date.to_date.strftime("%Y%m%d"), report.group_id, + report.filter, report.facets, report.limit, SCHEMA_VERSION, @@ -73,9 +76,15 @@ class Report self.start_date end + def filter_values + if self.filter.present? + return self.filter.delete_prefix("[").delete_suffix("]").split("&").map { |param| param.split("=") }.to_h + end + {} + end + def as_json(options = nil) description = I18n.t("reports.#{type}.description", default: "") - { type: type, title: I18n.t("reports.#{type}.title", default: nil), @@ -90,6 +99,7 @@ class Report prev_end_date: prev_end_date&.iso8601, category_id: category_id, group_id: group_id, + filter: self.filter, prev30Days: self.prev30Days, dates_filtering: self.dates_filtering, report_key: Report.cache_key(self), @@ -113,6 +123,7 @@ class Report higher_is_better: self.higher_is_better, category_filtering: self.category_filtering, group_filtering: self.group_filtering, + filter_options: self.filter_options, modes: self.modes, }.tap do |json| json[:icon] = self.icon if self.icon @@ -141,6 +152,7 @@ class Report report.end_date = opts[:end_date] if opts[:end_date] report.category_id = opts[:category_id] if opts[:category_id] report.group_id = opts[:group_id] if opts[:group_id] + report.filter = opts[:filter] if opts[:filter] report.facets = opts[:facets] || [:total, :prev30Days] report.limit = opts[:limit] if opts[:limit] report.processing = false @@ -1484,7 +1496,14 @@ class Report def self.report_top_uploads(report) report.modes = [:table] - + report.filter_options = [ + { + id: "file-extension", + selected: report.filter_values.fetch("file-extension", "any"), + choices: (SiteSetting.authorized_extensions.split("|") + report.filter_values.values).uniq, + allowAny: true + } + ] report.labels = [ { type: :link, @@ -1529,14 +1548,18 @@ class Report FROM uploads up JOIN users u ON u.id = up.user_id - WHERE up.created_at >= '#{report.start_date}' - AND up.created_at <= '#{report.end_date}' - AND up.id > #{Upload::SEEDED_ID_THRESHOLD} + /*where*/ ORDER BY up.filesize DESC LIMIT #{report.limit || 250} SQL - DB.query(sql).each do |row| + extension_filter = report.filter_values["file-extension"] + builder = DB.build(sql) + builder.where("up.id > :seeded_id_threshold", seeded_id_threshold: Upload::SEEDED_ID_THRESHOLD) + builder.where("up.created_at >= :start_date", start_date: report.start_date) + builder.where("up.created_at < :end_date", end_date: report.end_date) + builder.where("up.extension = :extension", extension: extension_filter) if extension_filter.present? + builder.query.each do |row| data = {} data[:author_id] = row.user_id data[:author_username] = row.username @@ -1545,7 +1568,6 @@ class Report data[:extension] = row.extension data[:file_url] = Discourse.store.cdn_url(row.url) data[:file_name] = row.original_filename.truncate(25) - report.data << data end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 154e6e19051..d7c57975b36 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2950,6 +2950,7 @@ en: moderation_tab: "Moderation" security_tab: "Security" reports_tab: "Reports" + report_filter_any: "any" 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