Skip to content

Commit

Permalink
FEATURE: Exposing a way to add a generic report filter (#6816)
Browse files Browse the repository at this point in the history
* 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](#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
  • Loading branch information
khalilovcmded committed Mar 15, 2019
1 parent d352baa commit f8480ed
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 17 deletions.
Expand Up @@ -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;
};
Expand Down
47 changes: 46 additions & 1 deletion app/assets/javascripts/admin/components/admin-report.js.es6
Expand Up @@ -56,6 +56,7 @@ export default Ember.Component.extend({
endDate: null,
category: null,
groupId: null,
filter: null,
showTrend: false,
showHeader: true,
showTitle: true,
Expand Down Expand Up @@ -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
});
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:";
Expand All @@ -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
Expand All @@ -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")
});
Expand Down Expand Up @@ -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");
}
Expand Down
@@ -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) {
Expand All @@ -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
};
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion app/assets/javascripts/admin/models/report.js.es6
Expand Up @@ -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);
Expand Down
12 changes: 12 additions & 0 deletions app/assets/javascripts/admin/templates/components/admin-report.hbs
Expand Up @@ -173,6 +173,18 @@
</div>
{{/if}}

{{#each filterOptions as |filterOption|}}
<div class="control">
<div class="input">
{{combo-box content=filterOption.choices
filterable=true
allowAny=true
value=filterOption.selected
onSelect=(action "filter" filterOption.id)}}
</div>
</div>
{{/each}}

{{#if showExport}}
<div class="control">
<div class="input">
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/admin/reports_controller.rb
Expand Up @@ -113,11 +113,17 @@ def parse_params(report_params)
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
}
Expand Down
42 changes: 32 additions & 10 deletions app/models/report.rb
Expand Up @@ -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
Expand All @@ -29,6 +29,8 @@ def initialize(type)
@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)
Expand All @@ -43,6 +45,7 @@ def self.cache_key(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,
Expand Down Expand Up @@ -73,9 +76,15 @@ def prev_end_date
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),
Expand All @@ -90,6 +99,7 @@ def as_json(options = nil)
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),
Expand All @@ -113,6 +123,7 @@ def as_json(options = nil)
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
Expand Down Expand Up @@ -141,6 +152,7 @@ def self._get(type, opts = nil)
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
Expand Down Expand Up @@ -1484,7 +1496,14 @@ def self.report_storage_stats(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,
Expand Down Expand Up @@ -1529,14 +1548,18 @@ def self.report_top_uploads(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
Expand All @@ -1545,7 +1568,6 @@ def self.report_top_uploads(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
Expand Down
1 change: 1 addition & 0 deletions config/locales/client.en.yml
Expand Up @@ -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
Expand Down

0 comments on commit f8480ed

Please sign in to comment.