feat: custom favorite filter for dashboards, charts and saved queries #11083

dpgaspar · 2020-09-27T20:49:44Z

SUMMARY

Add a new custom filter to fetch a user's favorite and not favorite dashboards, slices or saved queries

Rison/JSON arguments:

{
    "filters": [
        {"col": "id", "opr": "dashboard_is_fav", "value": true}
    ]
}

{
    "filters": [
        {"col": "id", "opr": "chart_is_fav", "value": true}
    ]
}

{
    "filters": [
        {"col": "id", "opr": "saved_query_is_fav", "value": true}
    ]
}

ADDITIONAL INFORMATION

codecov-commenter · 2020-09-27T21:18:23Z

Codecov Report

Merging #11083 into master will increase coverage by 4.17%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master   #11083      +/-   ##
==========================================
+ Coverage   61.64%   65.81%   +4.17%     
==========================================
  Files         815      815              
  Lines       38498    38512      +14     
  Branches     3620     3620              
==========================================
+ Hits        23733    25348    +1615     
+ Misses      14579    13054    -1525     
+ Partials      186      110      -76

Flag	Coverage Δ
#cypress	`56.39% <ø> (?)`
#javascript	`62.05% <ø> (ø)`
#python	`61.36% <85.71%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/dashboards/filters.py	`92.50% <81.81%> (-4.06%)`	⬇️
superset/dashboards/api.py	`80.86% <100.00%> (ø)`
superset/db_engine_specs/mysql.py	`79.59% <0.00%> (-12.25%)`	⬇️
...et-frontend/src/SqlLab/components/LimitControl.tsx	`84.78% <0.00%> (-1.89%)`	⬇️
superset/models/core.py	`87.56% <0.00%> (-0.83%)`	⬇️
superset/views/core.py	`73.92% <0.00%> (-0.25%)`	⬇️
...ontend/src/components/ListView/TableCollection.tsx	`100.00% <0.00%> (ø)`
...-frontend/src/datasource/ChangeDatasourceModal.tsx	`75.00% <0.00%> (+0.45%)`	⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js	`60.25% <0.00%> (+0.64%)`	⬆️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx	`52.12% <0.00%> (+1.21%)`	⬆️
... and 168 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f1e4e4...59733d3. Read the comment docs.

willbarrett

One comment on API consistency, but other than that looks good!

superset/charts/filters.py

willbarrett · 2020-09-28T21:38:45Z

superset/views/base_api.py

+    """
+
+    name = _("Is favorite")
+    arg_name = ""


We could even leverage this default and leave it blank in the subclasses :) (re: favorited)

True, nice catch!

I would create a BaseOuterJoinFilter with "where" and "on" arguments and then extend it

That's interesting can you add more detail or an example?

willbarrett · 2020-09-28T21:40:18Z

Added @rusackas as his team will be consuming these API changes as well.

rusackas · 2020-09-28T21:41:33Z

And on that note, a ping to @pkdotson

Ofeknielsen · 2020-09-30T16:03:56Z

tests/queries/saved_queries/api_tests.py

        data = json.loads(rv.data.decode("utf-8"))
-        self.assertEqual(data["count"], len(expected_models))
+        assert rv.status_code == 200


why not use self.assert200(rv)?

Because it's used nowhere on the test codebase, not written in stone but with pytest now, we are slowly refactoring to use assert

Ofeknielsen · 2020-09-30T16:04:21Z

tests/queries/saved_queries/api_tests.py

        data = json.loads(rv.data.decode("utf-8"))
-        self.assertEqual(data["count"], len(expected_models))
+        assert rv.status_code == 200
+        assert len(expected_models) == data["count"]


why did you change it to basic assert?

not written in stone but with pytest now, we are slowly refactoring to use assert

Ofeknielsen · 2020-09-30T16:32:33Z

tests/dashboards/api_tests.py

+            yield dashboards
+
+            # rollback changes
+            for dashboard in dashboards:


is that rollback will also remove from dashboard_user table and dashboard_slices ?

Good point. But, yes it cleans up after dashboard_user and no dashboard_slices are added so nothing to do here

Ofeknielsen · 2020-09-30T16:38:29Z

tests/dashboards/api_tests.py

+        )
+        arguments["filters"][0]["value"] = False
+        uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}"
+        rv = self.client.get(uri)


why not separate to another test case ?

ok, done that

Ofeknielsen · 2020-09-30T16:53:56Z

superset/views/base_api.py

+    """
+
+    name = _("Is favorite")
+    arg_name = ""


I would create a BaseOuterJoinFilter with "where" and "on" arguments and then extend it

…apache#11083) * feat: custom favorite filter for dashboards * lint and sort * add favored for charts * fix tests and lint * more tests and saved query filter * fix tests * fix tests * lint * lint and fix conflict * remove unnecessary prop * separate tests

feat: custom favorite filter for dashboards

af0ecd1

superset-github-bot bot added the preset-io label Sep 27, 2020

pull-request-size bot added the size/L label Sep 27, 2020

Merge branch 'master' into feat/api-favorites

9f06eb2

dpgaspar requested review from nytai and willbarrett September 27, 2020 20:53

lint and sort

59733d3

add favored for charts

0f75be8

dpgaspar changed the title ~~feat: custom favorite filter for dashboards~~ feat: custom favorite filter for dashboards, charts and saved queries Sep 27, 2020

dpgaspar added 7 commits September 27, 2020 23:52

fix tests and lint

30ef91c

more tests and saved query filter

8eb19e9

fix tests

aede712

fix tests

91cd438

lint

3b7c4d0

Merge branch 'master' into feat/api-favorites

3355761

lint and fix conflict

832266c

willbarrett requested changes Sep 28, 2020

View reviewed changes

willbarrett requested a review from rusackas September 28, 2020 21:40

remove unnecessary prop

f5a84e5

dpgaspar requested a review from willbarrett September 29, 2020 08:30

willbarrett approved these changes Sep 29, 2020

View reviewed changes

Ofeknielsen reviewed Sep 30, 2020

View reviewed changes

separate tests

2d38a86

pkdotson approved these changes Oct 1, 2020

View reviewed changes

willbarrett merged commit 4c85d33 into apache:master Oct 1, 2020

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 12, 2024

mistercrunch added the 🚢 0.38.0 label Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom favorite filter for dashboards, charts and saved queries #11083

feat: custom favorite filter for dashboards, charts and saved queries #11083

dpgaspar commented Sep 27, 2020 •

edited by rusackas

Loading

codecov-commenter commented Sep 27, 2020 •

edited

Loading

willbarrett left a comment

willbarrett Sep 28, 2020

dpgaspar Sep 29, 2020

Ofeknielsen Sep 30, 2020

dpgaspar Oct 1, 2020

willbarrett commented Sep 28, 2020

rusackas commented Sep 28, 2020

Ofeknielsen Sep 30, 2020

dpgaspar Oct 1, 2020 •

edited

Loading

Ofeknielsen Sep 30, 2020

dpgaspar Oct 1, 2020

Ofeknielsen Sep 30, 2020

dpgaspar Oct 1, 2020 •

edited

Loading

Ofeknielsen Sep 30, 2020

dpgaspar Oct 1, 2020

Ofeknielsen Sep 30, 2020

feat: custom favorite filter for dashboards, charts and saved queries #11083

feat: custom favorite filter for dashboards, charts and saved queries #11083

Conversation

dpgaspar commented Sep 27, 2020 • edited by rusackas Loading

SUMMARY

ADDITIONAL INFORMATION

codecov-commenter commented Sep 27, 2020 • edited Loading

Codecov Report

willbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbarrett commented Sep 28, 2020

rusackas commented Sep 28, 2020

Choose a reason for hiding this comment

dpgaspar Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar commented Sep 27, 2020 •

edited by rusackas

Loading

codecov-commenter commented Sep 27, 2020 •

edited

Loading

dpgaspar Oct 1, 2020 •

edited

Loading

dpgaspar Oct 1, 2020 •

edited

Loading