[GitHub] [incubator-superset] Ofeknielsen commented on a change in pull request #11083: feat: custom favorite filter for dashboards, charts and saved queries

[GitBox <gi...@apache.org>]

Ofeknielsen commented on a change in pull request #11083:
URL: https://github.com/apache/incubator-superset/pull/11083#discussion_r497627702



##########
File path: tests/queries/saved_queries/api_tests.py
##########
@@ -326,17 +326,22 @@ def test_get_saved_query_favorite_filter(self):
         self.login(username="admin")
         uri = f"api/v1/saved_query/?q={prison.dumps(arguments)}"
         rv = self.client.get(uri)
-        self.assertEqual(rv.status_code, 200)
         data = json.loads(rv.data.decode("utf-8"))
-        self.assertEqual(data["count"], len(expected_models))
+        assert rv.status_code == 200

Review comment:
       why not use self.assert200(rv)?

##########
File path: tests/queries/saved_queries/api_tests.py
##########
@@ -326,17 +326,22 @@ def test_get_saved_query_favorite_filter(self):
         self.login(username="admin")
         uri = f"api/v1/saved_query/?q={prison.dumps(arguments)}"
         rv = self.client.get(uri)
-        self.assertEqual(rv.status_code, 200)
         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"]

Review comment:
       why did you change it to basic assert?

##########
File path: superset/views/base_api.py
##########
@@ -84,6 +91,31 @@ def __init__(self, field_name: str, filter_class: Type[BaseFilter]):
         self.filter_class = filter_class
 
 
+class BaseFavoriteFilter(BaseFilter):  # pylint: disable=too-few-public-methods
+    """
+    Base Custom filter for the GET list that filters all dashboards, slices
+    that a user has favored or not
+    """
+
+    name = _("Is favorite")
+    arg_name = ""

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

##########
File path: tests/dashboards/api_tests.py
##########
@@ -78,6 +82,32 @@ def insert_dashboard(
         db.session.commit()
         return dashboard
 
+    @pytest.fixture()
+    def create_dashboards(self):
+        with self.create_app().app_context():
+            dashboards = []
+            admin = self.get_user("admin")
+            for cx in range(DASHBOARDS_FIXTURE_COUNT - 1):
+                dashboards.append(
+                    self.insert_dashboard(f"title{cx}", f"slug{cx}", [admin.id])
+                )
+            fav_dashboards = []
+            for cx in range(round(DASHBOARDS_FIXTURE_COUNT / 2)):
+                fav_star = FavStar(
+                    user_id=admin.id, class_name="Dashboard", obj_id=dashboards[cx].id
+                )
+                db.session.add(fav_star)
+                db.session.commit()
+                fav_dashboards.append(fav_star)
+            yield dashboards
+
+            # rollback changes
+            for dashboard in dashboards:

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

##########
File path: tests/dashboards/api_tests.py
##########
@@ -268,12 +301,58 @@ def test_get_dashboards_custom_filter(self):
         data = json.loads(rv.data.decode("utf-8"))
         self.assertEqual(data["count"], 0)
 
-        # rollback changes
-        db.session.delete(dashboard1)
-        db.session.delete(dashboard2)
-        db.session.delete(dashboard3)
-        db.session.delete(dashboard4)
-        db.session.commit()
+    @pytest.mark.usefixtures("create_dashboards")
+    def test_get_dashboards_favorite_filter(self):
+        """
+        Dashboard API: Test get dashboards favorite filter
+        """
+        admin = self.get_user("admin")
+        users_favorite_query = db.session.query(FavStar.obj_id).filter(
+            and_(FavStar.user_id == admin.id, FavStar.class_name == "Dashboard")
+        )
+        expected_models = (
+            db.session.query(Dashboard)
+            .filter(and_(Dashboard.id.in_(users_favorite_query)))
+            .order_by(Dashboard.dashboard_title.asc())
+            .all()
+        )
+
+        arguments = {
+            "filters": [{"col": "id", "opr": "dashboard_is_fav", "value": True}],
+            "order_column": "dashboard_title",
+            "order_direction": "asc",
+            "keys": ["none"],
+            "columns": ["dashboard_title"],
+        }
+        self.login(username="admin")
+        uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}"
+        rv = self.client.get(uri)
+        assert rv.status_code == 200
+        data = json.loads(rv.data.decode("utf-8"))
+        assert len(expected_models) == data["count"]
+
+        for i, expected_model in enumerate(expected_models):
+            assert (
+                expected_model.dashboard_title == data["result"][i]["dashboard_title"]
+            )
+
+        # Test not favorite dashboards
+        expected_models = (
+            db.session.query(Dashboard)
+            .filter(and_(~Dashboard.id.in_(users_favorite_query)))
+            .order_by(Dashboard.dashboard_title.asc())
+            .all()
+        )
+        arguments["filters"][0]["value"] = False
+        uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}"
+        rv = self.client.get(uri)

Review comment:
       why not separate to another test case ? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org