You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2023/01/19 19:24:09 UTC
[GitHub] [superset] jfrag1 opened a new pull request, #22789: chore: Migrate /superset/recent_activity// to /api/v1/
jfrag1 opened a new pull request, #22789:
URL: https://github.com/apache/superset/pull/22789
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Introduces a new endpoint to replace the deprecated one:
* `/api/v1/log/recent_activity/<user_id>/`
It is pretty much the same as the old one, except that it uses rison, has better test coverage, and openapi yaml/schemas.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1084830867
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
With `ENABLE_BROAD_ACTIVITY_ACCESS` enabled, users can view other users' profiles, which will fetch the those users' recent activity. I don't think we can make this change without breaking this behavior, which seems to me like something that would have to wait for 3.0 (correct me if I'm wrong).
I can think of one alternative: we make this new endpoint only get the current logged in user activity, and have the frontend for the profile page call the deprecated endpoint until it's removed in 3.0. Then in 3.0 the ability to view other users' profiles would be removed, and we can switch that page to use the new endpoint.
However, I think that only makes sense if `ENABLE_BROAD_ACTIVITY_ACCESS` will be removed in 3.0, what do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity/<user_id>/ to /api/v1/
URL: https://github.com/apache/superset/pull/22789
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1084830867
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
With `ENABLE_BROAD_ACTIVITY_ACCESS` enabled, users can view other users' profiles, which will fetch those users' recent activity. I don't think we can make this change without breaking this behavior, which seems to me like something that would have to wait for 3.0 (correct me if I'm wrong).
I can think of one alternative: we make this new endpoint only get the current logged in user activity, and have the frontend for the profile page call the deprecated endpoint until it's removed in 3.0. Then in 3.0 the ability to view other users' profiles would be removed, and we can switch that page to use the new endpoint.
However, I think that only makes sense if `ENABLE_BROAD_ACTIVITY_ACCESS` will be removed in 3.0, what do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity/<user_id>/ to /api/v1/
URL: https://github.com/apache/superset/pull/22789
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1082243101
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @rison(get_recent_activity_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".recent_activity",
+ log_to_statsd=False,
+ )
+ def recent_activity(self, user_id: int, **kwargs: Any) -> FlaskResponse:
+ """Get recent activity data for a user
+ ---
+ get:
+ description: Get recent activity data for a user
Review Comment:
nit: `summary` looks better on the docs, I know almost all the other endpoints are using description but, let's change it. We can use description also
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1084830867
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
With `ENABLE_BROAD_ACTIVITY_ACCESS` enabled, users can view other users' profiles, which will fetch those users' recent activity. I don't think we can make this change without breaking this behavior, which seems to me like something that would have to wait for 3.0 (correct me if I'm wrong).
I can think of one alternative: we make this new endpoint only get the current logged in user activity, and have the frontend for the profile page call the deprecated endpoint until it's removed in 3.0. Then in 3.0 the ability to view other users' profiles would be removed, and we can switch that page to use the new endpoint.
However, I think that only makes sense if `ENABLE_BROAD_ACTIVITY_ACCESS` will for certain be removed in 3.0, what do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] dpgaspar merged pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar merged PR #22789:
URL: https://github.com/apache/superset/pull/22789
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1082245627
##########
superset/views/log/dao.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from datetime import datetime, timedelta
+from typing import Any, Dict, List
+
+import humanize
+from sqlalchemy import and_, or_
+from sqlalchemy.sql import functions as func
+
+from superset import db
+from superset.dao.base import BaseDAO
+from superset.models.core import Log
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.utils.dates import datetime_to_epoch
+
+
+class LogDAO(BaseDAO):
+ model_cls = Log
+
+ @staticmethod
+ def get_recent_activity(
+ user_id: int, limit: int, actions: List[str], distinct: bool
+ ) -> List[Dict[str, Any]]:
+ has_subject_title = or_(
+ and_(
+ Dashboard.dashboard_title is not None,
+ Dashboard.dashboard_title != "",
+ ),
+ and_(Slice.slice_name is not None, Slice.slice_name != ""),
+ )
+
+ if distinct:
+ one_year_ago = datetime.today() - timedelta(days=365)
+ subqry = (
+ db.session.query(
+ Log.dashboard_id,
+ Log.slice_id,
+ Log.action,
+ func.max(Log.dttm).label("dttm"),
+ )
+ .group_by(Log.dashboard_id, Log.slice_id, Log.action)
+ .filter(
+ and_(
+ Log.action.in_(actions),
+ Log.user_id == user_id,
+ # limit to one year of data to improve performance
+ Log.dttm > one_year_ago,
+ or_(Log.dashboard_id.isnot(None), Log.slice_id.isnot(None)),
+ )
+ )
+ .subquery()
+ )
+ qry = (
+ db.session.query(
+ subqry,
+ Dashboard.slug.label("dashboard_slug"),
+ Dashboard.dashboard_title,
+ Slice.slice_name,
+ )
+ .outerjoin(Dashboard, Dashboard.id == subqry.c.dashboard_id)
+ .outerjoin(
+ Slice,
+ Slice.id == subqry.c.slice_id,
+ )
+ .filter(has_subject_title)
+ .order_by(subqry.c.dttm.desc())
+ .limit(limit)
+ )
+ else:
+ qry = (
+ db.session.query(
+ Log.dttm,
+ Log.action,
+ Log.dashboard_id,
+ Log.slice_id,
+ Dashboard.slug.label("dashboard_slug"),
+ Dashboard.dashboard_title,
+ Slice.slice_name,
+ )
+ .outerjoin(Dashboard, Dashboard.id == Log.dashboard_id)
+ .outerjoin(Slice, Slice.id == Log.slice_id)
+ .filter(has_subject_title)
+ .filter(Log.action.in_(actions), Log.user_id == user_id)
+ .order_by(Log.dttm.desc())
+ .limit(limit)
+ )
+
+ payload = []
+ for log in qry.all():
+ item_url = None
+ item_title = None
+ item_type = None
+ if log.dashboard_id:
+ item_type = "dashboard"
+ item_url = Dashboard(id=log.dashboard_id, slug=log.dashboard_slug).url
Review Comment:
nit: What about making this a static method on dashboard or making an helper method. Basically avoid instantiating a class on each list row here
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @rison(get_recent_activity_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".recent_activity",
+ log_to_statsd=False,
+ )
+ def recent_activity(self, user_id: int, **kwargs: Any) -> FlaskResponse:
+ """Get recent activity data for a user
+ ---
+ get:
+ description: Get recent activity data for a user
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: user_id
+ description: The id of the user
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_recent_activity_schema'
+ responses:
+ 200:
+ description: A List of recent activity objects
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RecentActivityResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ error_obj = self.get_user_activity_access_error(user_id)
+ if error_obj:
+ return error_obj
+
+ limit = kwargs["rison"].get("limit", 100)
Review Comment:
let's set a down and upper limits to `limit`, use the json schema for it. Better yet let's support proper pagination use `page` and `page_size` like on the other endpoints
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @rison(get_recent_activity_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".recent_activity",
+ log_to_statsd=False,
+ )
+ def recent_activity(self, user_id: int, **kwargs: Any) -> FlaskResponse:
+ """Get recent activity data for a user
+ ---
+ get:
+ description: Get recent activity data for a user
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: user_id
+ description: The id of the user
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_recent_activity_schema'
+ responses:
+ 200:
+ description: A List of recent activity objects
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RecentActivityResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/404'
Review Comment:
`$ref: '#/components/responses/403'`
##########
superset/views/log/dao.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from datetime import datetime, timedelta
+from typing import Any, Dict, List
+
+import humanize
+from sqlalchemy import and_, or_
+from sqlalchemy.sql import functions as func
+
+from superset import db
+from superset.dao.base import BaseDAO
+from superset.models.core import Log
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.utils.dates import datetime_to_epoch
+
+
+class LogDAO(BaseDAO):
+ model_cls = Log
+
+ @staticmethod
+ def get_recent_activity(
+ user_id: int, limit: int, actions: List[str], distinct: bool
+ ) -> List[Dict[str, Any]]:
+ has_subject_title = or_(
+ and_(
+ Dashboard.dashboard_title is not None,
+ Dashboard.dashboard_title != "",
+ ),
+ and_(Slice.slice_name is not None, Slice.slice_name != ""),
+ )
+
+ if distinct:
+ one_year_ago = datetime.today() - timedelta(days=365)
+ subqry = (
+ db.session.query(
+ Log.dashboard_id,
+ Log.slice_id,
+ Log.action,
+ func.max(Log.dttm).label("dttm"),
+ )
+ .group_by(Log.dashboard_id, Log.slice_id, Log.action)
+ .filter(
+ and_(
+ Log.action.in_(actions),
+ Log.user_id == user_id,
+ # limit to one year of data to improve performance
+ Log.dttm > one_year_ago,
+ or_(Log.dashboard_id.isnot(None), Log.slice_id.isnot(None)),
+ )
+ )
+ .subquery()
+ )
+ qry = (
+ db.session.query(
+ subqry,
+ Dashboard.slug.label("dashboard_slug"),
+ Dashboard.dashboard_title,
+ Slice.slice_name,
+ )
+ .outerjoin(Dashboard, Dashboard.id == subqry.c.dashboard_id)
+ .outerjoin(
+ Slice,
+ Slice.id == subqry.c.slice_id,
+ )
+ .filter(has_subject_title)
+ .order_by(subqry.c.dttm.desc())
+ .limit(limit)
+ )
+ else:
+ qry = (
+ db.session.query(
+ Log.dttm,
+ Log.action,
+ Log.dashboard_id,
+ Log.slice_id,
+ Dashboard.slug.label("dashboard_slug"),
+ Dashboard.dashboard_title,
+ Slice.slice_name,
+ )
+ .outerjoin(Dashboard, Dashboard.id == Log.dashboard_id)
+ .outerjoin(Slice, Slice.id == Log.slice_id)
+ .filter(has_subject_title)
+ .filter(Log.action.in_(actions), Log.user_id == user_id)
+ .order_by(Log.dttm.desc())
+ .limit(limit)
+ )
+
+ payload = []
+ for log in qry.all():
+ item_url = None
+ item_title = None
+ item_type = None
+ if log.dashboard_id:
+ item_type = "dashboard"
+ item_url = Dashboard(id=log.dashboard_id, slug=log.dashboard_slug).url
+ item_title = log.dashboard_title
+ elif log.slice_id:
+ slc = Slice(id=log.slice_id, slice_name=log.slice_name)
Review Comment:
this will actually create a new Slice model on SQLAlchemy state history, let's avoid creating new model instances
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
I vote for just removing the user_id we always get the current logged in user, what do you think about just updating /recent_activity to user_activity or similar logic?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity/<user_id>/ to /api/v1/
URL: https://github.com/apache/superset/pull/22789
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #22789: chore: Migrate /superset/recent_activity/<user_id>/ to /api/v1/
URL: https://github.com/apache/superset/pull/22789
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1086390647
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
oh true, I remember that one!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1084853793
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @rison(get_recent_activity_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".recent_activity",
+ log_to_statsd=False,
+ )
+ def recent_activity(self, user_id: int, **kwargs: Any) -> FlaskResponse:
+ """Get recent activity data for a user
+ ---
+ get:
+ description: Get recent activity data for a user
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: user_id
+ description: The id of the user
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/get_recent_activity_schema'
+ responses:
+ 200:
+ description: A List of recent activity objects
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/RecentActivityResponseSchema"
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ error_obj = self.get_user_activity_access_error(user_id)
+ if error_obj:
+ return error_obj
+
+ limit = kwargs["rison"].get("limit", 100)
Review Comment:
Added pagination args
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] codecov[bot] commented on pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22789:
URL: https://github.com/apache/superset/pull/22789#issuecomment-1397498394
# [Codecov](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22789](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b504ce) into [master](https://codecov.io/gh/apache/superset/commit/5026da50e1c525d48c526bdf3aec7fb52829b5b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5026da5) will **decrease** coverage by `11.45%`.
> The diff coverage is `64.40%`.
> :exclamation: Current head 2b504ce differs from pull request most recent head 5552e7d. Consider uploading reports for the commit 5552e7d to get more accurate results
```diff
@@ Coverage Diff @@
## master #22789 +/- ##
===========================================
- Coverage 67.27% 55.83% -11.45%
===========================================
Files 1875 1879 +4
Lines 71820 72062 +242
Branches 7870 7893 +23
===========================================
- Hits 48319 40235 -8084
- Misses 21473 29799 +8326
Partials 2028 2028
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.53% <55.96%> (+0.03%)` | :arrow_up: |
| python | `57.98% <62.38%> (-23.92%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.58% <57.79%> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <ø> (ø)` | |
| [...uperset-frontend/src/SqlLab/SqlLabGlobalStyles.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9TcWxMYWJHbG9iYWxTdHlsZXMudHN4) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/types.ts](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi90eXBlcy50cw==) | `57.14% <ø> (ø)` | |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `91.61% <ø> (ø)` | |
| [superset-frontend/src/components/Select/styles.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L3N0eWxlcy50c3g=) | `80.76% <ø> (ø)` | |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `65.51% <ø> (ø)` | |
| [...Filters/FiltersConfigModal/FilterConfigurePane.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlckNvbmZpZ3VyZVBhbmUudHN4) | `100.00% <ø> (ø)` | |
| [...rontend/src/dashboard/containers/DashboardPage.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZFBhZ2UudHN4) | `7.14% <ø> (ø)` | |
| [...ols/DndColumnSelectControl/ColumnSelectPopover.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EbmRDb2x1bW5TZWxlY3RDb250cm9sL0NvbHVtblNlbGVjdFBvcG92ZXIudHN4) | `3.37% <0.00%> (+0.03%)` | :arrow_up: |
| [.../FilterControl/AdhocFilterPopoverTrigger/index.tsx](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaWx0ZXJDb250cm9sL0FkaG9jRmlsdGVyUG9wb3ZlclRyaWdnZXIvaW5kZXgudHN4) | `83.33% <ø> (ø)` | |
| ... and [375 more](https://codecov.io/gh/apache/superset/pull/22789?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1085005500
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
Is there any way for a normal user to view other user's profile? without tempering with the URL?
Since the frontend always calls the old and new endpoint with the current user login `ENABLE_BROAD_ACTIVITY_ACCESS` will not affect the frontend in anyway.
Would not be a breaking change IMO, since the old endpoint is still there and still supports `ENABLE_BROAD_ACTIVITY_ACCESS` up until 3.0, this functionality is only leveraged by calling the endpoint directly.
Yet, let's keep `ENABLE_BROAD_ACTIVITY_ACCESS` for now, since this config key is tied to other functionality also, we prob should tackle this in a more integrated way.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
[GitHub] [superset] jfrag1 commented on a diff in pull request #22789: chore: Migrate /superset/recent_activity// to /api/v1/
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #22789:
URL: https://github.com/apache/superset/pull/22789#discussion_r1085807940
##########
superset/views/log/api.py:
##########
@@ -44,13 +56,82 @@ class LogRestApi(LogMixin, BaseSupersetModelRestApi):
"referrer",
]
show_columns = list_columns
+ apispec_parameter_schemas = {
+ "get_recent_activity_schema": get_recent_activity_schema,
+ }
+ openapi_spec_component_schemas = (
+ RecentActivityResponseSchema,
+ RecentActivitySchema,
+ )
@staticmethod
def is_enabled() -> bool:
return app.config["FAB_ADD_SECURITY_VIEWS"] and app.config["SUPERSET_LOG_VIEW"]
- @before_request
+ @before_request(only=["get_list", "get", "post"])
def ensure_enabled(self) -> None:
if not self.is_enabled():
return self.response_404()
return None
+
+ def get_user_activity_access_error(self, user_id: int) -> Optional[FlaskResponse]:
+ try:
+ security_manager.raise_for_user_activity_access(user_id)
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+ return None
+
+ @expose("/recent_activity/<int:user_id>/", methods=["GET"])
Review Comment:
There is a way to view another user's profile without modifying the URL, on the dashboard and chart listviews the "Modified By" column will display a link to the user's profile who last modified the entity when `ENABLE_BROAD_ACTIVITY_ACCESS` is enabled:
Then the frontend of the profile page will request that user's activity
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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