[GitHub] [superset] john-bodley commented on a diff in pull request #20552: feat(report): allow capturing dashboard reports in specific state

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

john-bodley commented on code in PR #20552:
URL: https://github.com/apache/superset/pull/20552#discussion_r928005461


##########
superset/migrations/versions/2022-07-11_11-26_ffa79af61a56_rename_report_schedule_extra_to_extra_.py:
##########
@@ -0,0 +1,53 @@
+# 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.
+"""rename_report_schedule_extra_to_extra_json
+
+So we can reuse the ExtraJSONMixin
+
+Revision ID: ffa79af61a56
+Revises: a39867932713
+Create Date: 2022-07-11 11:26:00.010714
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "ffa79af61a56"
+down_revision = "a39867932713"
+
+from alembic import op
+from sqlalchemy.types import Text
+
+
+def upgrade():
+    op.alter_column(
+        "report_schedule",
+        "extra",
+        new_column_name="extra_json",

Review Comment:
   Why the rename? Is this consistent with other tables. I've always felt that the `extra` column was always implicitly interpreted as being JSON encoded.



##########
superset/reports/commands/execute.py:
##########
@@ -198,49 +207,33 @@ def _get_screenshots(self) -> List[bytes]:
         :raises: ReportScheduleScreenshotFailedError
         """
         image_data = []
-        screenshots: List[BaseScreenshot] = []
+        url = self._get_url()
+        user = self._get_user()
         if self._report_schedule.chart:
-            url = self._get_url()
-            logger.info("Screenshotting chart at %s", url)
-            screenshots = [
-                ChartScreenshot(
-                    url,
-                    self._report_schedule.chart.digest,
-                    window_size=app.config["WEBDRIVER_WINDOW"]["slice"],
-                    thumb_size=app.config["WEBDRIVER_WINDOW"]["slice"],
-                )
-            ]
+            screenshot: Union[ChartScreenshot, DashboardScreenshot] = ChartScreenshot(
+                url,
+                self._report_schedule.chart.digest,
+                window_size=app.config["WEBDRIVER_WINDOW"]["slice"],
+                thumb_size=app.config["WEBDRIVER_WINDOW"]["slice"],
+            )
         else:
-            tabs: Optional[List[str]] = json.loads(self._report_schedule.extra).get(
-                "dashboard_tab_ids", None
+            screenshot = DashboardScreenshot(
+                url,
+                self._report_schedule.dashboard.digest,
+                window_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
+                thumb_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
             )
-            dashboard_base_url = self._get_url()
-            if tabs is None:
-                urls = [dashboard_base_url]
-            else:
-                urls = [f"{dashboard_base_url}#{tab_id}" for tab_id in tabs]
-            screenshots = [
-                DashboardScreenshot(
-                    url,
-                    self._report_schedule.dashboard.digest,
-                    window_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
-                    thumb_size=app.config["WEBDRIVER_WINDOW"]["dashboard"],
-                )
-                for url in urls
-            ]
-        user = self._get_user()
-        for screenshot in screenshots:
-            try:
-                image = screenshot.get_screenshot(user=user)
-            except SoftTimeLimitExceeded as ex:
-                logger.warning("A timeout occurred while taking a screenshot.")
-                raise ReportScheduleScreenshotTimeout() from ex
-            except Exception as ex:
-                raise ReportScheduleScreenshotFailedError(
-                    f"Failed taking a screenshot {str(ex)}"
-                ) from ex
-            if image is not None:
-                image_data.append(image)
+        try:
+            image = screenshot.get_screenshot(user=user)
+        except SoftTimeLimitExceeded as ex:
+            logger.warning("A timeout occurred while taking a screenshot.")
+            raise ReportScheduleScreenshotTimeout() from ex
+        except Exception as ex:
+            raise ReportScheduleScreenshotFailedError(
+                f"Failed taking a screenshot {str(ex)}"
+            ) from ex
+        if image is not None:

Review Comment:
   ```suggestion
           if not image:
   ```
   
   Could this be falsely for anything other than `None`?



##########
superset/reports/models.py:
##########
@@ -147,25 +147,27 @@ class ReportSchedule(Model, AuditMixinNullable):
     # (Alerts/Reports) Unlock a possible stalled working state
     working_timeout = Column(Integer, default=60 * 60 * 1)
 
-    # Store the selected dashboard tabs etc.
-    extra = Column(Text, default="{}")
-
     # (Reports) When generating a screenshot, bypass the cache?
     force_screenshot = Column(Boolean, default=False)
 
+    extra: ReportScheduleExtra  # type: ignore
+
     def __repr__(self) -> str:
         return str(self.name)
 
     @renders("crontab")
     def crontab_humanized(self) -> str:
         return get_description(self.crontab)
 
-    @validates("extra")
-    # pylint: disable=unused-argument,no-self-use
-    def validate_extra(self, key: str, value: Dict[Any, Any]) -> Optional[str]:
-        if value is not None:
-            return json.dumps(value)
-        return None
+    @validates("extra_json")

Review Comment:
   Any reason this isn't part of the ExtraJSONMixin mixin?



##########
superset/reports/commands/create.py:
##########
@@ -117,20 +122,28 @@ def validate(self) -> None:
             raise exception
 
     def _validate_report_extra(self, exceptions: List[ValidationError]) -> None:
-        extra = self._properties.get("extra")
+        extra: Optional[ReportScheduleExtra] = self._properties.get("extra")
         dashboard = self._properties.get("dashboard")
 
         if extra is None or dashboard is None:
             return
 
-        dashboard_tab_ids = extra.get("dashboard_tab_ids")
-        if dashboard_tab_ids is None:
+        dashboard_state = extra.get("dashboard")
+        if not dashboard_state:
             return
-        position_data = json.loads(dashboard.position_json)
-        invalid_tab_ids = [
-            tab_id for tab_id in dashboard_tab_ids if tab_id not in position_data
-        ]
+
+        position_data = json.loads(dashboard.position_json or "{}")
+        active_tabs = dashboard_state.get("activeTabs") or []
+        anchor = dashboard_state.get("anchor")
+        invalid_tab_ids = {
+            tab_id for tab_id in active_tabs if tab_id not in position_data
+        }

Review Comment:
   You could inline this as two set comprehensions via `{ ... } | { ... }`.



##########
superset/reports/commands/execute.py:
##########
@@ -640,11 +633,13 @@ class AsyncExecuteReportScheduleCommand(BaseCommand):
     - On Alerts uses related Command AlertCommand and sends configured notifications
     """
 
-    def __init__(self, task_id: str, model_id: int, scheduled_dttm: datetime):
+    def __init__(
+        self, task_id: Union[UUID, str], model_id: int, scheduled_dttm: datetime

Review Comment:
   Any reason we support both `str` and `UUID` and not just the later?



##########
superset/reports/models.py:
##########
@@ -147,25 +147,27 @@ class ReportSchedule(Model, AuditMixinNullable):
     # (Alerts/Reports) Unlock a possible stalled working state
     working_timeout = Column(Integer, default=60 * 60 * 1)
 
-    # Store the selected dashboard tabs etc.
-    extra = Column(Text, default="{}")

Review Comment:
   Oh. I see that the `ExtraJSONMixin` is the one which defines the column as `extra_json`.



##########
tests/integration_tests/reports/utils.py:
##########
@@ -83,3 +95,99 @@ def insert_report_schedule(
     db.session.add(report_schedule)
     db.session.commit()
     return report_schedule
+
+
+def create_report_notification(
+    email_target: Optional[str] = None,
+    slack_channel: Optional[str] = None,
+    chart: Optional[Slice] = None,
+    dashboard: Optional[Dashboard] = None,
+    database: Optional[Database] = None,
+    sql: Optional[str] = None,
+    report_type: Optional[ReportScheduleType] = None,
+    validator_type: Optional[str] = None,
+    validator_config_json: Optional[str] = None,
+    grace_period: Optional[int] = None,
+    report_format: Optional[ReportDataFormat] = None,
+    name: Optional[str] = None,
+    extra: Optional[Dict[str, Any]] = None,
+    force_screenshot: bool = False,
+) -> ReportSchedule:
+    report_type = report_type or ReportScheduleType.REPORT

Review Comment:
   I would likely inline `report_type` and `target` as they are single use variables.



##########
superset/reports/dao.py:
##########
@@ -155,7 +155,7 @@ def validate_update_uniqueness(
         return found_id is None or found_id == expect_id
 
     @classmethod
-    def create(cls, properties: Dict[str, Any], commit: bool = True) -> Model:
+    def create(cls, properties: Dict[str, Any], commit: bool = True) -> ReportSchedule:

Review Comment:
   Thanks for making the types more specific.



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