New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Exposing a way to add a generic report filter #6816

Open
wants to merge 30 commits into
base: master
from

Conversation

5 participants
@khalilovcmded
Copy link
Contributor

khalilovcmded commented Dec 27, 2018

Why do we need this change?

Part of the work discussed here, and implemented a first spike here, 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 report, it can look like this on the backend:

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:

{
  "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

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

I would also say that a lot of code can be simplified / removed if we are able to jump across to the latest Ember version. (yet, this is hard work)

khalilovcmd and others added some commits Dec 24, 2018

FEATURE: Add `Top Uploads` report
## Why do we need this feature?

This is part of: https://meta.discourse.org/t/gain-understanding-of-file-uploads-usage/104994

I am introducing a new report: `Top Uploads` to take a first step in solving the problem of: `an Admin wants to gain understanding of their forum file uploads usage`.

## How does this work?

1. This is building on top of the `report.rb` class by providing a new method: `report_top_uploads` which in turn will be automatically detected as a new report by the `Admin::ReportsController#index` ...

2. The report will show the largest (by filesize) N (default: 250) uploads, and it will show by which user is the upload created.
Update app/assets/javascripts/admin/models/report.js.es6
Makes sense!

Co-Authored-By: khalilovcmded <45508821+khalilovcmded@users.noreply.github.com>
Update app/assets/javascripts/admin/components/admin-report-table.js.es6
Co-Authored-By: khalilovcmded <45508821+khalilovcmded@users.noreply.github.com>
Update app/assets/javascripts/admin/components/admin-report-table.js.es6
Co-Authored-By: khalilovcmded <45508821+khalilovcmded@users.noreply.github.com>
@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Dec 27, 2018

Thanks for contributing this pull request! Could you please sign our CLA so we can review it? http://www.discourse.org/cla

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Dec 27, 2018

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/gain-understanding-of-file-uploads-usage/104994/3

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 28, 2018

Do we need custom_filtering? Checking if there are custom_filter_options should be enough no ?

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 28, 2018

"I would also say that a lot of code can be simplified / removed if we are able to jump across to the latest Ember version. (yet, this is hard work)"

This will happen very soon. Inspect ember meta version. ;)

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 28, 2018

What about making filter something like this:

report.filter_options = [
  { 
     id: "file-extension", 
     default: "png",
     choices: ["jpeg", "gif", "png"],
     allowAny: true
  },
    { 
     id: "size", 
     default: "any",
     choices: ["small", "medium", "large"],
     allowAny: true
  },
]
{{#each report.filter_options as |filterOption|}}
  {{combo-box content=filterOption.choices onSelect=(action "filter" filterOption.id)}}
{{/each]

And support this client side:
xxxxx?filter=[size=small&file-extension=gif]

It's all pseudo code, but I hope you get the idea.

@khalilovcmded

This comment has been minimized.

Copy link
Contributor Author

khalilovcmded commented Dec 28, 2018

@jjaffeux

Do we need custom_filtering? Checking if there are custom_filter_options should be enough no ?

And support this client side:
xxxxx?filter=[size=small&file-extension=gif]
It's all pseudo code, but I hope you get the idea.

This sounds like a much better idea, it has been circulating in my head but I was unsure if I should go for it before I get some feedback on a simpler approach. It feels like a good approach, at least for simple, option based filters 😃

I will have to check how this plays out with QueryParams but I believe it should be alright.

This will happen very soon. Inspect ember meta version. ;)

I saw the PR, monumental effort! 😻If you need help, I would be happy to.

@khalilovcmded

This comment has been minimized.

Copy link
Contributor Author

khalilovcmded commented Dec 28, 2018

I took a first stab at the suggested custom multi-filtering, I also tried it with the needed changes on the backend.

How does it look like?

admin_-_the_electro-nerds

admin_-_the_electro-nerds

What's an example of a Report configuration?

def self.report_top_uploads(report)

    if report.filter.present?
      filters = report.filter.delete_prefix("[").delete_suffix("]").split("&").map do |param|
        param_pair = param.split("=")
        { id: param_pair[0], value: param_pair[1] }
      end

      extension_filter = filters.find { |pair| pair[:id] == "file-extension" }
      if extension_filter.present? && extension_filter[:value] != "any"
        extension_filter_sql =  "AND up.extension = '#{extension_filter[:value]}'"
      end

      size_filter = filters.find { |pair| pair[:id] == "size" }
    end

    report.modes = [:table]
    report.custom_filtering = true
    report.custom_filter_options = [
      {
        id: "file-extension",
        selected: extension_filter.present? ? extension_filter.fetch(:value, "any") : "any",
        choices: ["jpeg", "gif", "png"],
        allowAny: true
      },
      {
        id: "size",
        selected: size_filter.present? ? size_filter.fetch(:value, "any") : "any",
        choices: ["small", "medium", "large"],
        allowAny: true
      },
    ]
 
   ......
   ......
   ......
  end

khalilovcmd and others added some commits Dec 28, 2018

~Tarek Khalil
~Tarek Khalil

~Tarek Khalil added some commits Dec 28, 2018

~Tarek Khalil added some commits Dec 28, 2018

~Tarek Khalil
~Tarek Khalil
~Tarek Khalil
~Tarek Khalil
~Tarek Khalil
@@ -1434,8 +1439,27 @@ def self.report_storage_stats(report)
end

def self.report_top_uploads(report)
report.modes = [:table]
if report.filter.present?

This comment has been minimized.

@jjaffeux

jjaffeux Dec 28, 2018

Contributor

We need to find a better solution here. We can’t have to add so much code for each report to handle filters.

I suggest you look at what we do for categories.

We might also have to rewrite sql queries using the builder: https://github.com/discourse/mini_sql

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor

We need to find a better solution here. We can’t have to add so much code for each report to handle filters.

That's a good point, it seems that this can be easily extracted out into a method that can be used by any report.

We might also have to rewrite sql queries using the builder: https://github.com/discourse/mini_sql

Awesome, I will look into that 👍

ORDER BY up.filesize DESC
LIMIT #{report.limit || 250}
SQL

DB.query(sql).each do |row|
extension_filter = report.filter_values["file-extension"]

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor

@jjaffeux is this what you had in mind? (the extraction method is here)

I believe any new report that uses custom filters will need to add a single line per filter, I feel it is alright, but happy to hear your feedback 😃

builder.where("up.extension = :extension", extension: extension_filter) if extension_filter.present?

Also, I used the SQL Builder below 👇

builder = DB.build(sql)
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?

This comment has been minimized.

@jjaffeux

jjaffeux Dec 29, 2018

Contributor

do we need this .present? we should build filter_values, to make sure report.filter_values["xxx"] returns nil of not present to avoid this everywhere

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor

report.filter_values["xxx"] returns nil of not present to avoid this everywhere

So this is already the case, filter_values will return a nil if the key doesn't exist. Yet, the .present? here is a conditional for allowing the builder to specify the where statement itself.

If we don't have .present? then even if report.filter_values["xxx"] returns nil, we are still going to have a where condition that we don't need, which will result in wrong results.

id: "file-extension",
selected: report.filter_values.fetch("file-extension", "any"),
choices: ["jpeg", "gif", "png"],
allowAny: true

This comment has been minimized.

@jjaffeux

jjaffeux Dec 29, 2018

Contributor

I wonder three things here :p

  • do we need a allowAll ? as in "any file extension"
  • should allowAny means you can enter your own value? Maybe we need a better name ?
  • for this specific case, could choices use the allowed extensions site setting?

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor
  1. Yes, I think we do. That being said, I should use: SiteSetting.authorized_extensions for choices value
  2. The assumption now is that it has a default value that is fetched from client.en.yml, the default value is: any ... I would say this is okay.
  3. Yes, totally 😃

This comment has been minimized.

@jjaffeux

jjaffeux Dec 29, 2018

Contributor

For me "any" would mean I could type: "something" and it would use it in the where query.

So basically I don’t think this report should support "any". "all" or a choice in the list should be what we need. "any" should also not be a selectable entry in the list but we should only implement combo-box filtering handling.

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor

So basically I don’t think this report should support "any". "all" or a choice in the list should be what we need. "any" should also not be a selectable entry in the list but we should only implement combo-box filtering handling.

I think I get you. Perhaps I am using the incorrect wording here. What I meant is basically: "all" ... And yes, I agree this is what makes sense to have. From an Admin perspective, they would either select "all" or any other choice.

we should only implement combo-box filtering handling.

I believe this can be done by specifying: filterable=true in the Ember {{combo-box component. Something like this:

screen shot 2018-12-29 at 15 25 42

Is that what you meant?

@@ -29,6 +29,8 @@ def initialize(type)
@modes = [:table, :chart]
@prev_data = nil
@dates_filtering = true
@filter_options = nil
@filter = nil

This comment has been minimized.

@jjaffeux

jjaffeux Dec 29, 2018

Contributor

maybe it should be plural ?

This comment has been minimized.

@khalilovcmded

khalilovcmded Dec 29, 2018

Author Contributor

I think it would be okay to make it plural, though, the value of filter is a string, and not an array / collection / enumerable. From a correctness point of view, it is a singular value.

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 29, 2018

Generally speaking I think we need better naming here, the difference between "filter", "filter_values", "filter_options" is not clear without reading the code in details. maybe some of it should be private too.

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 29, 2018

Also we will need at least one acceptance test to check clicking on col head is actually setting the filter. And I see no trace of direction handling, filesize ASC/DESC ?

~Tarek Khalil
@khalilovcmded

This comment has been minimized.

Copy link
Contributor Author

khalilovcmded commented Dec 29, 2018

Generally speaking I think we need better naming here, the difference between "filter", "filter_values", "filter_options" is not clear without reading the code in details. maybe some of it should be private too.

Perhaps yes, naming is hard 😸 I am thinking that the naming problem here is because we want to make sure we mentally connect all these three variables for an Engineer reading the code. I personally find it mentally connectable once an Engineer inspects how filter is used in the reports.rb. Do you have a better naming on your mind?

Also we will need at least one acceptance test to check clicking on col head is actually setting the filter.

Can you elaborate more on this? which acceptance test? which col head?

And I see no trace of direction handling, filesize ASC/DESC ?

I am not sure I understand direction handling, the sorting is client side only, and in a case a filter is applied, the sorting behaviour isn't available. This is how it works 👇

gif_reports

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 29, 2018

Can you elaborate more on this? which acceptance test? which col head?

Sorry I meant when selecting filter

@jjaffeux

This comment has been minimized.

Copy link
Contributor

jjaffeux commented Dec 29, 2018

I am not sure I understand direction handling, the sorting is client side only, and in a case a filter is applied, the sorting behaviour isn't available. This is how it works 👇

I feel like filter should be handling filesize sorting server side for example. Indeed no point in sorting extension here.

Adding a filter on author could be cool too. But let's keep this for another PR.

@khalilovcmded

This comment has been minimized.

Copy link
Contributor Author

khalilovcmded commented Dec 29, 2018

Sorry I meant when selecting filter

When selecting a filtering, there is nothing that changes in the table or the columns of the table. The only thing that change is the query params which is backed by the filter property, perhaps that's what you meant by adding the test for that. Is that so?

~Tarek Khalil
Update admin-report.hbs
Allow search based filtering in the `custom filters` combo-box

@jjaffeux jjaffeux added the 2.3 label Jan 18, 2019

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Jan 24, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/toggle-to-hide-crawlers-from-consolidated-pageviews/107419/2

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Feb 13, 2019

@jjaffeux is this ready for merging? can you go ahead and merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment