From 58cfa47693c46c3d369c1d1e5b0aba0c9758e6bf Mon Sep 17 00:00:00 2001
From: Vitor Avila <vitor.avila@preset.io>
Date: Mon, 22 Apr 2024 14:57:45 -0300
Subject: [PATCH 1/5] feat(reports): Set a minimum interval for each report's
 execution

---
 docs/docs/configuration/alerts-reports.mdx    |   9 +
 superset/commands/report/base.py              |  45 +++-
 superset/commands/report/create.py            |  54 +++--
 superset/commands/report/exceptions.py        |  19 ++
 superset/commands/report/update.py            |  69 +++---
 superset/config.py                            |   4 +
 tests/integration_tests/reports/api_tests.py  | 208 ++++++++++++++++++
 tests/unit_tests/commands/report/base_test.py | 191 ++++++++++++++++
 8 files changed, 551 insertions(+), 48 deletions(-)
 create mode 100644 tests/unit_tests/commands/report/base_test.py

diff --git a/docs/docs/configuration/alerts-reports.mdx b/docs/docs/configuration/alerts-reports.mdx
index 925a9ebdf940c..9e1cf2446d791 100644
--- a/docs/docs/configuration/alerts-reports.mdx
+++ b/docs/docs/configuration/alerts-reports.mdx
@@ -195,6 +195,15 @@ Please refer to `ExecutorType` in the codebase for other executor types.
   its default value of `http://0.0.0.0:8080/`.
 
 
+It's also possible to specify a minimum interval (in minutes) between each report's execution through the config file:
+``` python
+# Set a minimal interval threshold between executions (for each Alert/Report)
+# Value should be the amount of minutes (integer). Min: 1, Max: 60
+ALERT_MINIMAL_INTERVAL_MINUTES = 10 # Change it as desired
+REPORT_MINIMAL_INTERVAL_MINUTES = 10 # Change it as desired
+```
+
+
 ### Custom Dockerfile
 
 If you're running the dev version of a released Superset image, like `apache/superset:3.1.0-dev`, you should be set with the above.
diff --git a/superset/commands/report/base.py b/superset/commands/report/base.py
index 3b2f280816a4d..bd0d0b614bb40 100644
--- a/superset/commands/report/base.py
+++ b/superset/commands/report/base.py
@@ -17,6 +17,7 @@
 import logging
 from typing import Any
 
+from flask import current_app
 from marshmallow import ValidationError
 
 from superset.commands.base import BaseCommand
@@ -26,11 +27,12 @@
     DashboardNotFoundValidationError,
     DashboardNotSavedValidationError,
     ReportScheduleEitherChartOrDashboardError,
+    ReportScheduleFrequencyNotAllowed,
     ReportScheduleOnlyChartOrDashboardError,
 )
 from superset.daos.chart import ChartDAO
 from superset.daos.dashboard import DashboardDAO
-from superset.reports.models import ReportCreationMethod
+from superset.reports.models import ReportCreationMethod, ReportScheduleType
 
 logger = logging.getLogger(__name__)
 
@@ -76,3 +78,44 @@ def validate_chart_dashboard(
             self._properties["dashboard"] = dashboard
         elif not update:
             exceptions.append(ReportScheduleEitherChartOrDashboardError())
+
+    def validate_report_frequency(
+        self,
+        cron_schedule: str,
+        report_type: str,
+    ) -> None:
+        """
+        Validates if the report frequency doesn't exceed a configured limit. The minimal
+        interval configuration (via UI or API) when creating/modifying reports is every
+        minute, so logic is only validated in case ``minimal_interval`` (set in config) is > 1.
+
+        :param cron_schedule: The cron schedule configured.
+        :param report_type: The report type (Alert/Report).
+        """
+        config_key = (
+            "ALERT_MINIMAL_INTERVAL_MINUTES"
+            if report_type == ReportScheduleType.ALERT
+            else "REPORT_MINIMAL_INTERVAL_MINUTES"
+        )
+        minimal_interval = current_app.config.get(config_key)
+        if not minimal_interval or minimal_interval < 2:
+            return
+
+        minutes_interval = cron_schedule.split(" ")[0]
+        # Check if cron is set to every minute (*)
+        # or if it has an every-minute block (1-5)
+        if "-" in minutes_interval or minutes_interval == "*":
+            raise ReportScheduleFrequencyNotAllowed(report_type, minimal_interval)
+
+        # Calculate minimal interval (in minutes)
+        minutes_list = minutes_interval.split(",")
+        if len(minutes_list) > 1:
+            scheduled_minutes = [int(min) for min in minutes_list]
+            scheduled_minutes.sort()
+            differences = [
+                scheduled_minutes[i] - scheduled_minutes[i - 1]
+                for i in range(1, len(scheduled_minutes))
+            ]
+
+            if min(differences) < minimal_interval:
+                raise ReportScheduleFrequencyNotAllowed(report_type, minimal_interval)
diff --git a/superset/commands/report/create.py b/superset/commands/report/create.py
index aa9bfefc6e963..e73da467d12dc 100644
--- a/superset/commands/report/create.py
+++ b/superset/commands/report/create.py
@@ -30,7 +30,6 @@
     ReportScheduleCreationMethodUniquenessValidationError,
     ReportScheduleInvalidError,
     ReportScheduleNameUniquenessValidationError,
-    ReportScheduleRequiredTypeValidationError,
 )
 from superset.daos.database import DatabaseDAO
 from superset.daos.exceptions import DAOCreateFailedError
@@ -58,38 +57,53 @@ def run(self) -> ReportSchedule:
             raise ReportScheduleCreateFailedError() from ex
 
     def validate(self) -> None:
-        exceptions: list[ValidationError] = []
-        owner_ids: Optional[list[int]] = self._properties.get("owners")
-        name = self._properties.get("name", "")
-        report_type = self._properties.get("type")
-        creation_method = self._properties.get("creation_method")
+        """
+        Validates the properties of a report schedule configuration, including uniqueness
+        of name and type, relations based on the report type, frequency, etc. Populates
+        a list of `ValidationErrors` to be returned in the API response if any.
+
+        Fields were loaded according to the `ReportSchedulePostSchema` schema.
+        """
+        # Required fields
+        cron_schedule = self._properties["crontab"]
+        name = self._properties["name"]
+        report_type = self._properties["type"]
+
+        # Optional fields
         chart_id = self._properties.get("chart")
+        creation_method = self._properties.get("creation_method")
         dashboard_id = self._properties.get("dashboard")
+        owner_ids: Optional[list[int]] = self._properties.get("owners")
 
-        # Validate type is required
-        if not report_type:
-            exceptions.append(ReportScheduleRequiredTypeValidationError())
+        exceptions: list[ValidationError] = []
 
         # Validate name type uniqueness
-        if report_type and not ReportScheduleDAO.validate_update_uniqueness(
-            name, report_type
-        ):
+        if not ReportScheduleDAO.validate_update_uniqueness(name, report_type):
             exceptions.append(
                 ReportScheduleNameUniquenessValidationError(
                     report_type=report_type, name=name
                 )
             )
 
-        # validate relation by report type
+        # Validate if DB exists (for alerts)
         if report_type == ReportScheduleType.ALERT:
-            database_id = self._properties.get("database")
-            if not database_id:
-                exceptions.append(ReportScheduleAlertRequiredDatabaseValidationError())
-            else:
-                database = DatabaseDAO.find_by_id(database_id)
-                if not database:
+            try:
+                database_id = self._properties["database"]
+                if database := DatabaseDAO.find_by_id(database_id):
+                    self._properties["database"] = database
+                else:
                     exceptions.append(DatabaseNotFoundValidationError())
-                self._properties["database"] = database
+            except KeyError:
+                exceptions.append(ReportScheduleAlertRequiredDatabaseValidationError())
+
+        # validate report frequency
+        try:
+            self.validate_report_frequency(
+                cron_schedule,
+                report_type,
+            )
+        except ValidationError as exc:
+            exceptions.append(exc)
 
         # Validate chart or dashboard relations
         self.validate_chart_dashboard(exceptions)
diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py
index db929c63a2980..f219022d1c54c 100644
--- a/superset/commands/report/exceptions.py
+++ b/superset/commands/report/exceptions.py
@@ -93,6 +93,25 @@ def __init__(self) -> None:
         )
 
 
+class ReportScheduleFrequencyNotAllowed(ValidationError):
+    """
+    Marshmallow validation error for report schedule configured to run more
+    frequently than allowed
+    """
+
+    def __init__(self, report_type: str, minimum_interval: str) -> None:
+        super().__init__(
+            _(
+                "%(report_type)s schedule frequency exceeding limit."
+                " Please configure a schedule with a minimal interval of"
+                " %(minimum_interval)s minutes per execution.",
+                report_type=report_type,
+                minimum_interval=minimum_interval,
+            ),
+            field_name="crontab",
+        )
+
+
 class ChartNotSavedValidationError(ValidationError):
     """
     Marshmallow validation error for charts that haven't been saved yet
diff --git a/superset/commands/report/update.py b/superset/commands/report/update.py
index cb63ec5011daf..f5ff0ca1583f5 100644
--- a/superset/commands/report/update.py
+++ b/superset/commands/report/update.py
@@ -59,17 +59,29 @@ def run(self) -> Model:
         return report_schedule
 
     def validate(self) -> None:
-        exceptions: list[ValidationError] = []
-        owner_ids: Optional[list[int]] = self._properties.get("owners")
-        report_type = self._properties.get("type", ReportScheduleType.ALERT)
-
-        name = self._properties.get("name", "")
+        """
+        Validates the properties of a report schedule configuration, including uniqueness
+        of name and type, relations based on the report type, frequency, etc. Populates
+        a list of `ValidationErrors` to be returned in the API response if any.
+
+        Fields were loaded according to the `ReportSchedulePutSchema` schema.
+        """
+        # Load existing report schedule config
         self._model = ReportScheduleDAO.find_by_id(self._model_id)
-
-        # Does the report exist?
         if not self._model:
             raise ReportScheduleNotFoundError()
 
+        # Required fields for validation
+        cron_schedule = self._properties.get("crontab", self._model.crontab)
+        name = self._properties.get("name", self._model.name)
+        report_type = self._properties.get("type", self._model.type)
+
+        # Optional fields
+        database_id = self._properties.get("database")
+        owner_ids: Optional[list[int]] = self._properties.get("owners")
+
+        exceptions: list[ValidationError] = []
+
         # Change the state to not triggered when the user deactivates
         # A report that is currently in a working state. This prevents
         # an alert/report from being kept in a working state if activated back
@@ -80,28 +92,31 @@ def validate(self) -> None:
         ):
             self._properties["last_state"] = ReportState.NOOP
 
-        # validate relation by report type
-        if not report_type:
-            report_type = self._model.type
-
-        # Validate name type uniqueness
-        if not ReportScheduleDAO.validate_update_uniqueness(
-            name, report_type, expect_id=self._model_id
-        ):
-            exceptions.append(
-                ReportScheduleNameUniquenessValidationError(
-                    report_type=report_type, name=name
+        # Validate name/type uniqueness if either is changing
+        if name != self._model.name or report_type != self._model.type:
+            if not ReportScheduleDAO.validate_update_uniqueness(
+                name, report_type, expect_id=self._model_id
+            ):
+                exceptions.append(
+                    ReportScheduleNameUniquenessValidationError(
+                        report_type=report_type, name=name
+                    )
                 )
-            )
 
-        if report_type == ReportScheduleType.ALERT:
-            database_id = self._properties.get("database")
-            # If database_id was sent let's validate it exists
-            if database_id:
-                database = DatabaseDAO.find_by_id(database_id)
-                if not database:
-                    exceptions.append(DatabaseNotFoundValidationError())
-                self._properties["database"] = database
+        # Validate if DB exists (for alerts)
+        if report_type == ReportScheduleType.ALERT and database_id:
+            if not (database := DatabaseDAO.find_by_id(database_id)):
+                exceptions.append(DatabaseNotFoundValidationError())
+            self._properties["database"] = database
+
+        # validate report frequency
+        try:
+            self.validate_report_frequency(
+                cron_schedule,
+                report_type,
+            )
+        except ValidationError as exc:
+            exceptions.append(exc)
 
         # Validate chart or dashboard relations
         self.validate_chart_dashboard(exceptions, update=True)
diff --git a/superset/config.py b/superset/config.py
index 9388edbe84fe0..2c4d4c079d855 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1325,6 +1325,10 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 # Custom width for screenshots
 ALERT_REPORTS_MIN_CUSTOM_SCREENSHOT_WIDTH = 600
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
+# Set a minimal interval threshold between executions (for each Alert/Report)
+# Value should be the amount of minutes (integer). Min: 1, Max: 60
+ALERT_MINIMAL_INTERVAL_MINUTES = None
+REPORT_MINIMAL_INTERVAL_MINUTES = None
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "
diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py
index 1e1d91f77f827..a10819e642a82 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -18,6 +18,7 @@
 """Unit tests for Superset"""
 
 from datetime import datetime
+from unittest.mock import patch
 import json
 
 import pytz
@@ -1259,6 +1260,213 @@ def test_create_report_schedule_invalid_creation_method(self):
         }
         assert rv.status_code == 400
 
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    def test_create_report_schedule_valid_schedule(self):
+        """
+        ReportSchedule API: Test create report schedule when a minimum
+        interval is set in config.
+        """
+        self.login(ADMIN_USERNAME)
+
+        chart = db.session.query(Slice).first()
+        example_db = get_example_database()
+        report_schedule_data = {
+            "type": ReportScheduleType.ALERT,
+            "name": "Alert with a valid frequency",
+            "description": "description",
+            "creation_method": "alerts_reports",
+            "crontab": "5,10 9 * * *",
+            "recipients": [
+                {
+                    "type": ReportRecipientType.EMAIL,
+                    "recipient_config_json": {"target": "target@superset.org"},
+                },
+                {
+                    "type": ReportRecipientType.SLACK,
+                    "recipient_config_json": {"target": "channel"},
+                },
+            ],
+            "grace_period": 14400,
+            "working_timeout": 3600,
+            "chart": chart.id,
+            "database": example_db.id,
+        }
+        with patch.dict(
+            "superset.commands.report.base.current_app.config",
+            {
+                "ALERT_MINIMAL_INTERVAL_MINUTES": 2,
+                "REPORT_MINIMAL_INTERVAL_MINUTES": 5,
+            },
+        ):
+            uri = "api/v1/report/"
+            rv = self.post_assert_metric(uri, report_schedule_data, "post")
+            assert rv.status_code == 201
+            report_schedule_data["type"] = ReportScheduleType.REPORT
+            report_schedule_data["name"] = "Report with a valid frequency"
+            del report_schedule_data["database"]
+            rv = self.post_assert_metric(uri, report_schedule_data, "post")
+            assert rv.status_code == 201
+
+    @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+    def test_create_report_schedule_invalid_schedule(self):
+        """
+        ReportSchedule API: Test create report schedule when a minimum
+        interval is set in config and the scheduled frequency exceeds it.
+        """
+        self.login(ADMIN_USERNAME)
+
+        chart = db.session.query(Slice).first()
+        example_db = get_example_database()
+        report_schedule_data = {
+            "type": ReportScheduleType.ALERT,
+            "name": "Invalid Frequency",
+            "description": "description",
+            "creation_method": "alerts_reports",
+            "crontab": "5,10 9 * * *",
+            "recipients": [
+                {
+                    "type": ReportRecipientType.EMAIL,
+                    "recipient_config_json": {"target": "target@superset.org"},
+                },
+                {
+                    "type": ReportRecipientType.SLACK,
+                    "recipient_config_json": {"target": "channel"},
+                },
+            ],
+            "grace_period": 14400,
+            "working_timeout": 3600,
+            "chart": chart.id,
+            "database": example_db.id,
+        }
+        with patch.dict(
+            "superset.commands.report.base.current_app.config",
+            {
+                "ALERT_MINIMAL_INTERVAL_MINUTES": 6,
+                "REPORT_MINIMAL_INTERVAL_MINUTES": 8,
+            },
+        ):
+            uri = "api/v1/report/"
+            rv = self.post_assert_metric(uri, report_schedule_data, "post")
+            response = json.loads(rv.data.decode("utf-8"))
+            assert response == {
+                "message": {
+                    "crontab": (
+                        "Alert schedule frequency exceeding limit. "
+                        "Please configure a schedule with a minimal interval of 6 minutes per execution."
+                    )
+                }
+            }
+            assert rv.status_code == 422
+            report_schedule_data["type"] = ReportScheduleType.REPORT
+            del report_schedule_data["database"]
+            rv = self.post_assert_metric(uri, report_schedule_data, "post")
+            response = json.loads(rv.data.decode("utf-8"))
+            assert response == {
+                "message": {
+                    "crontab": (
+                        "Report schedule frequency exceeding limit. "
+                        "Please configure a schedule with a minimal interval of 8 minutes per execution."
+                    )
+                }
+            }
+            assert rv.status_code == 422
+
+    @pytest.mark.usefixtures("create_report_schedules")
+    def test_update_report_schedule_valid_schedule(self) -> None:
+        """
+        ReportSchedule API: Test update report schedule when a minimum
+        interval is set in config.
+        """
+        self.login(ADMIN_USERNAME)
+        report_schedule = (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.name == "name2")
+            .one_or_none()
+        )
+        assert report_schedule.type == ReportScheduleType.ALERT
+        previous_cron = report_schedule.crontab
+        update_payload = {
+            "crontab": "5,10 * * * *",
+        }
+        with patch.dict(
+            "superset.commands.report.base.current_app.config",
+            {
+                "ALERT_MINIMAL_INTERVAL_MINUTES": 5,
+                "REPORT_MINIMAL_INTERVAL_MINUTES": 3,
+            },
+        ):
+            # Test alert minimum interval
+            uri = f"api/v1/report/{report_schedule.id}"
+            rv = self.put_assert_metric(uri, update_payload, "put")
+            assert rv.status_code == 200
+
+            # Test report minimum interval
+            update_payload["crontab"] = "5,8 * * * *"
+            update_payload["type"] = ReportScheduleType.REPORT
+            uri = f"api/v1/report/{report_schedule.id}"
+            rv = self.put_assert_metric(uri, update_payload, "put")
+            assert rv.status_code == 200
+
+            # Undo changes
+            update_payload["crontab"] = previous_cron
+            update_payload["type"] = ReportScheduleType.ALERT
+            uri = f"api/v1/report/{report_schedule.id}"
+            rv = self.put_assert_metric(uri, update_payload, "put")
+            assert rv.status_code == 200
+
+    @pytest.mark.usefixtures("create_report_schedules")
+    def test_update_report_schedule_invalid_schedule(self) -> None:
+        """
+        ReportSchedule API: Test update report schedule when a minimum
+        interval is set in config and the scheduled frequency exceeds it.
+        """
+        self.login(ADMIN_USERNAME)
+        report_schedule = (
+            db.session.query(ReportSchedule)
+            .filter(ReportSchedule.name == "name2")
+            .one_or_none()
+        )
+        assert report_schedule.type == ReportScheduleType.ALERT
+        update_payload = {
+            "crontab": "5,10 * * * *",
+        }
+        with patch.dict(
+            "superset.commands.report.base.current_app.config",
+            {
+                "ALERT_MINIMAL_INTERVAL_MINUTES": 6,
+                "REPORT_MINIMAL_INTERVAL_MINUTES": 4,
+            },
+        ):
+            # Exceed alert minimum interval
+            uri = f"api/v1/report/{report_schedule.id}"
+            rv = self.put_assert_metric(uri, update_payload, "put")
+            assert rv.status_code == 422
+            response = json.loads(rv.data.decode("utf-8"))
+            assert response == {
+                "message": {
+                    "crontab": (
+                        "Alert schedule frequency exceeding limit. "
+                        "Please configure a schedule with a minimal interval of 6 minutes per execution."
+                    )
+                }
+            }
+
+            # Exceed report minimum interval
+            update_payload["crontab"] = "5,8 * * * *"
+            update_payload["type"] = ReportScheduleType.REPORT
+            uri = f"api/v1/report/{report_schedule.id}"
+            rv = self.put_assert_metric(uri, update_payload, "put")
+            assert rv.status_code == 422
+            response = json.loads(rv.data.decode("utf-8"))
+            assert response == {
+                "message": {
+                    "crontab": (
+                        "Report schedule frequency exceeding limit. "
+                        "Please configure a schedule with a minimal interval of 4 minutes per execution."
+                    )
+                }
+            }
+
     @pytest.mark.usefixtures("create_report_schedules")
     def test_update_report_schedule(self):
         """
diff --git a/tests/unit_tests/commands/report/base_test.py b/tests/unit_tests/commands/report/base_test.py
new file mode 100644
index 0000000000000..b39d23a427ff8
--- /dev/null
+++ b/tests/unit_tests/commands/report/base_test.py
@@ -0,0 +1,191 @@
+# 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 functools import wraps
+from typing import Any, Callable
+from unittest.mock import patch
+
+import pytest
+
+from superset.commands.report.base import BaseReportScheduleCommand
+from superset.commands.report.exceptions import ReportScheduleFrequencyNotAllowed
+from superset.reports.models import ReportScheduleType
+
+REPORT_TYPES = {
+    ReportScheduleType.ALERT,
+    ReportScheduleType.REPORT,
+}
+
+TEST_SCHEDULES_EVERY_MINUTE = {
+    "* * * * *",
+    "1-5 * * * *",
+    "10-20 * * * *",
+    "0,45,10-20 * * * *",
+    "23,45,50,51 * * * *",
+    "10,20,30,40-45 * * * *",
+}
+
+TEST_SCHEDULES_SINGLE_MINUTES = {
+    "1,5,8,10,12 * * * *",
+    "10 1 * * *",
+    "27,2 1-5 * * *",
+}
+
+TEST_SCHEDULES = TEST_SCHEDULES_EVERY_MINUTE.union(TEST_SCHEDULES_SINGLE_MINUTES)
+
+
+def app_custom_config(
+    alert_minimal: int | None = None,
+    report_minimal: int | None = None,
+) -> Callable[[Callable[..., Any]], Callable[..., Any]]:
+    """
+    Decorator to mock the current_app.config values dynamically for each test.
+
+    :param alert_minimal: Minimum interval in minutes for alerts. Defaults to None.
+    :param report_minimal: Minimum interval in minutes for reports. Defaults to None.
+
+    :returns: A decorator that wraps a function.
+    """
+
+    def decorator(func: Callable[..., Any]) -> Callable[..., Any]:
+        @wraps(func)
+        def wrapper(*args: Any, **kwargs: Any) -> Any:
+            with patch(
+                "superset.commands.report.base.current_app.config"
+            ) as mock_config:
+                mock_config.get.side_effect = lambda key: {
+                    "ALERT_MINIMAL_INTERVAL_MINUTES": alert_minimal,
+                    "REPORT_MINIMAL_INTERVAL_MINUTES": report_minimal,
+                }.get(key)
+                return func(*args, **kwargs)
+
+        return wrapper
+
+    return decorator
+
+
+@app_custom_config()
+def test_validate_report_frequency() -> None:
+    """
+    Test the ``validate_report_frequency`` method when there's
+    no minimal frequency configured.
+    """
+    for report_type in REPORT_TYPES:
+        for schedule in TEST_SCHEDULES:
+            BaseReportScheduleCommand().validate_report_frequency(
+                schedule,
+                report_type,
+            )
+
+
+@app_custom_config(alert_minimal=4, report_minimal=5)
+def test_validate_report_frequency_minimal_set() -> None:
+    """
+    Test the ``validate_report_frequency`` method when there's
+    minimal frequencies configured.
+    """
+
+    BaseReportScheduleCommand().validate_report_frequency(
+        "1,5 * * * *",
+        ReportScheduleType.ALERT,
+    )
+    BaseReportScheduleCommand().validate_report_frequency(
+        "6,11 * * * *",
+        ReportScheduleType.REPORT,
+    )
+
+
+@app_custom_config(alert_minimal=2, report_minimal=5)
+def test_validate_report_frequency_invalid_schedule() -> None:
+    """
+    Test the ``validate_report_frequency`` method when the configured
+    schedule exceeds the limit.
+    """
+    with pytest.raises(ReportScheduleFrequencyNotAllowed):
+        BaseReportScheduleCommand().validate_report_frequency(
+            "1,2 * * * *",
+            ReportScheduleType.ALERT,
+        )
+
+    with pytest.raises(ReportScheduleFrequencyNotAllowed):
+        BaseReportScheduleCommand().validate_report_frequency(
+            "1,5 * * * *",
+            ReportScheduleType.REPORT,
+        )
+
+
+@app_custom_config(alert_minimal=10)
+def test_validate_report_frequency_alert_only() -> None:
+    """
+    Test the ``validate_report_frequency`` method when there's
+    only a configuration for alerts and user is creating report.
+    """
+    for schedule in TEST_SCHEDULES:
+        BaseReportScheduleCommand().validate_report_frequency(
+            schedule,
+            ReportScheduleType.REPORT,
+        )
+
+
+@app_custom_config(report_minimal=10)
+def test_validate_report_frequency_report_only() -> None:
+    """
+    Test the ``validate_report_frequency`` method when there's
+    only a configuration for reports and user is creating alert.
+    """
+    for schedule in TEST_SCHEDULES:
+        BaseReportScheduleCommand().validate_report_frequency(
+            schedule,
+            ReportScheduleType.ALERT,
+        )
+
+
+@app_custom_config(alert_minimal=1, report_minimal=1)
+def test_validate_report_frequency_accepts_every_minute_with_one() -> None:
+    """
+    Test the ``validate_report_frequency`` method when configuration
+    is set to `1`. Validates the usage of `-` and `*` in the cron.
+    """
+    for report_type in REPORT_TYPES:
+        for schedule in TEST_SCHEDULES:
+            BaseReportScheduleCommand().validate_report_frequency(
+                schedule,
+                report_type,
+            )
+
+
+@app_custom_config(alert_minimal=2, report_minimal=2)
+def test_validate_report_frequency_accepts_every_minute_with_two() -> None:
+    """
+    Test the ``validate_report_frequency`` method when configuration
+    is set to `2`. Validates the usage of `-` and `*` in the cron.
+    """
+    for report_type in REPORT_TYPES:
+        # Should fail for schedules with `-` and `*`
+        for schedule in TEST_SCHEDULES_EVERY_MINUTE:
+            with pytest.raises(ReportScheduleFrequencyNotAllowed):
+                BaseReportScheduleCommand().validate_report_frequency(
+                    schedule,
+                    report_type,
+                )
+
+        # Should work for schedules with single with bigger intervals
+        for schedule in TEST_SCHEDULES_SINGLE_MINUTES:
+            BaseReportScheduleCommand().validate_report_frequency(
+                schedule,
+                report_type,
+            )

From 8152441a43de3cfbeadfc7a3a5def959c36df666 Mon Sep 17 00:00:00 2001
From: Vitor Avila <vitor.avila@preset.io>
Date: Mon, 22 Apr 2024 15:33:59 -0300
Subject: [PATCH 2/5] feat(reports): Set a minimum interval for each report's
 execution

---
 docs/docs/configuration/alerts-reports.mdx    |  7 +++--
 superset/commands/report/base.py              | 21 +++++++------
 superset/commands/report/exceptions.py        |  2 +-
 superset/config.py                            |  6 ++--
 tests/integration_tests/reports/api_tests.py  | 24 +++++++--------
 tests/unit_tests/commands/report/base_test.py | 30 +++++++++----------
 6 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/docs/docs/configuration/alerts-reports.mdx b/docs/docs/configuration/alerts-reports.mdx
index 9e1cf2446d791..26698387e8314 100644
--- a/docs/docs/configuration/alerts-reports.mdx
+++ b/docs/docs/configuration/alerts-reports.mdx
@@ -196,11 +196,12 @@ Please refer to `ExecutorType` in the codebase for other executor types.
 
 
 It's also possible to specify a minimum interval (in minutes) between each report's execution through the config file:
+
 ``` python
-# Set a minimal interval threshold between executions (for each Alert/Report)
+# Set a minimum interval threshold between executions (for each Alert/Report)
 # Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMAL_INTERVAL_MINUTES = 10 # Change it as desired
-REPORT_MINIMAL_INTERVAL_MINUTES = 10 # Change it as desired
+ALERT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
+REPORT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
 ```
 
 
diff --git a/superset/commands/report/base.py b/superset/commands/report/base.py
index bd0d0b614bb40..e7e7c08c3b14f 100644
--- a/superset/commands/report/base.py
+++ b/superset/commands/report/base.py
@@ -85,29 +85,28 @@ def validate_report_frequency(
         report_type: str,
     ) -> None:
         """
-        Validates if the report frequency doesn't exceed a configured limit. The minimal
-        interval configuration (via UI or API) when creating/modifying reports is every
-        minute, so logic is only validated in case ``minimal_interval`` (set in config) is > 1.
+        Validates if the report scheduled frequency doesn't exceed a limit
+        configured in `config.py`.
 
         :param cron_schedule: The cron schedule configured.
         :param report_type: The report type (Alert/Report).
         """
         config_key = (
-            "ALERT_MINIMAL_INTERVAL_MINUTES"
+            "ALERT_MINIMUM_INTERVAL_MINUTES"
             if report_type == ReportScheduleType.ALERT
-            else "REPORT_MINIMAL_INTERVAL_MINUTES"
+            else "REPORT_MINIMUM_INTERVAL_MINUTES"
         )
-        minimal_interval = current_app.config.get(config_key)
-        if not minimal_interval or minimal_interval < 2:
+        minimum_interval = current_app.config.get(config_key)
+        if not minimum_interval or minimum_interval < 2:
             return
 
         minutes_interval = cron_schedule.split(" ")[0]
         # Check if cron is set to every minute (*)
         # or if it has an every-minute block (1-5)
         if "-" in minutes_interval or minutes_interval == "*":
-            raise ReportScheduleFrequencyNotAllowed(report_type, minimal_interval)
+            raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
 
-        # Calculate minimal interval (in minutes)
+        # Calculate minimum interval (in minutes)
         minutes_list = minutes_interval.split(",")
         if len(minutes_list) > 1:
             scheduled_minutes = [int(min) for min in minutes_list]
@@ -117,5 +116,5 @@ def validate_report_frequency(
                 for i in range(1, len(scheduled_minutes))
             ]
 
-            if min(differences) < minimal_interval:
-                raise ReportScheduleFrequencyNotAllowed(report_type, minimal_interval)
+            if min(differences) < minimum_interval:
+                raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py
index f219022d1c54c..472b6a97c83f9 100644
--- a/superset/commands/report/exceptions.py
+++ b/superset/commands/report/exceptions.py
@@ -103,7 +103,7 @@ def __init__(self, report_type: str, minimum_interval: str) -> None:
         super().__init__(
             _(
                 "%(report_type)s schedule frequency exceeding limit."
-                " Please configure a schedule with a minimal interval of"
+                " Please configure a schedule with a minimum interval of"
                 " %(minimum_interval)s minutes per execution.",
                 report_type=report_type,
                 minimum_interval=minimum_interval,
diff --git a/superset/config.py b/superset/config.py
index 2c4d4c079d855..22ac9f2e5c7d0 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1325,10 +1325,10 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 # Custom width for screenshots
 ALERT_REPORTS_MIN_CUSTOM_SCREENSHOT_WIDTH = 600
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
-# Set a minimal interval threshold between executions (for each Alert/Report)
+# Set a minimum interval threshold between executions (for each Alert/Report)
 # Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMAL_INTERVAL_MINUTES = None
-REPORT_MINIMAL_INTERVAL_MINUTES = None
+ALERT_MINIMUM_INTERVAL_MINUTES = None
+REPORT_MINIMUM_INTERVAL_MINUTES = None
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "
diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py
index a10819e642a82..f6de07b32cbd5 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -1294,8 +1294,8 @@ def test_create_report_schedule_valid_schedule(self):
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMAL_INTERVAL_MINUTES": 2,
-                "REPORT_MINIMAL_INTERVAL_MINUTES": 5,
+                "ALERT_MINIMUM_INTERVAL_MINUTES": 2,
+                "REPORT_MINIMUM_INTERVAL_MINUTES": 5,
             },
         ):
             uri = "api/v1/report/"
@@ -1341,8 +1341,8 @@ def test_create_report_schedule_invalid_schedule(self):
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMAL_INTERVAL_MINUTES": 6,
-                "REPORT_MINIMAL_INTERVAL_MINUTES": 8,
+                "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
+                "REPORT_MINIMUM_INTERVAL_MINUTES": 8,
             },
         ):
             uri = "api/v1/report/"
@@ -1352,7 +1352,7 @@ def test_create_report_schedule_invalid_schedule(self):
                 "message": {
                     "crontab": (
                         "Alert schedule frequency exceeding limit. "
-                        "Please configure a schedule with a minimal interval of 6 minutes per execution."
+                        "Please configure a schedule with a minimum interval of 6 minutes per execution."
                     )
                 }
             }
@@ -1365,7 +1365,7 @@ def test_create_report_schedule_invalid_schedule(self):
                 "message": {
                     "crontab": (
                         "Report schedule frequency exceeding limit. "
-                        "Please configure a schedule with a minimal interval of 8 minutes per execution."
+                        "Please configure a schedule with a minimum interval of 8 minutes per execution."
                     )
                 }
             }
@@ -1391,8 +1391,8 @@ def test_update_report_schedule_valid_schedule(self) -> None:
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMAL_INTERVAL_MINUTES": 5,
-                "REPORT_MINIMAL_INTERVAL_MINUTES": 3,
+                "ALERT_MINIMUM_INTERVAL_MINUTES": 5,
+                "REPORT_MINIMUM_INTERVAL_MINUTES": 3,
             },
         ):
             # Test alert minimum interval
@@ -1433,8 +1433,8 @@ def test_update_report_schedule_invalid_schedule(self) -> None:
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMAL_INTERVAL_MINUTES": 6,
-                "REPORT_MINIMAL_INTERVAL_MINUTES": 4,
+                "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
+                "REPORT_MINIMUM_INTERVAL_MINUTES": 4,
             },
         ):
             # Exceed alert minimum interval
@@ -1446,7 +1446,7 @@ def test_update_report_schedule_invalid_schedule(self) -> None:
                 "message": {
                     "crontab": (
                         "Alert schedule frequency exceeding limit. "
-                        "Please configure a schedule with a minimal interval of 6 minutes per execution."
+                        "Please configure a schedule with a minimum interval of 6 minutes per execution."
                     )
                 }
             }
@@ -1462,7 +1462,7 @@ def test_update_report_schedule_invalid_schedule(self) -> None:
                 "message": {
                     "crontab": (
                         "Report schedule frequency exceeding limit. "
-                        "Please configure a schedule with a minimal interval of 4 minutes per execution."
+                        "Please configure a schedule with a minimum interval of 4 minutes per execution."
                     )
                 }
             }
diff --git a/tests/unit_tests/commands/report/base_test.py b/tests/unit_tests/commands/report/base_test.py
index b39d23a427ff8..9dba37d1eb8b5 100644
--- a/tests/unit_tests/commands/report/base_test.py
+++ b/tests/unit_tests/commands/report/base_test.py
@@ -49,14 +49,14 @@
 
 
 def app_custom_config(
-    alert_minimal: int | None = None,
-    report_minimal: int | None = None,
+    alert_minimum_interval: int | None = None,
+    report_minimum_interval: int | None = None,
 ) -> Callable[[Callable[..., Any]], Callable[..., Any]]:
     """
     Decorator to mock the current_app.config values dynamically for each test.
 
-    :param alert_minimal: Minimum interval in minutes for alerts. Defaults to None.
-    :param report_minimal: Minimum interval in minutes for reports. Defaults to None.
+    :param alert_minimum_interval: Minimum interval in minutes for alerts. Defaults to None.
+    :param report_minimum_interval: Minimum interval in minutes for reports. Defaults to None.
 
     :returns: A decorator that wraps a function.
     """
@@ -68,8 +68,8 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
                 "superset.commands.report.base.current_app.config"
             ) as mock_config:
                 mock_config.get.side_effect = lambda key: {
-                    "ALERT_MINIMAL_INTERVAL_MINUTES": alert_minimal,
-                    "REPORT_MINIMAL_INTERVAL_MINUTES": report_minimal,
+                    "ALERT_MINIMUM_INTERVAL_MINUTES": alert_minimum_interval,
+                    "REPORT_MINIMUM_INTERVAL_MINUTES": report_minimum_interval,
                 }.get(key)
                 return func(*args, **kwargs)
 
@@ -82,7 +82,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
 def test_validate_report_frequency() -> None:
     """
     Test the ``validate_report_frequency`` method when there's
-    no minimal frequency configured.
+    no minimum frequency configured.
     """
     for report_type in REPORT_TYPES:
         for schedule in TEST_SCHEDULES:
@@ -92,11 +92,11 @@ def test_validate_report_frequency() -> None:
             )
 
 
-@app_custom_config(alert_minimal=4, report_minimal=5)
-def test_validate_report_frequency_minimal_set() -> None:
+@app_custom_config(alert_minimum_interval=4, report_minimum_interval=5)
+def test_validate_report_frequency_minimum_set() -> None:
     """
     Test the ``validate_report_frequency`` method when there's
-    minimal frequencies configured.
+    minimum frequencies configured.
     """
 
     BaseReportScheduleCommand().validate_report_frequency(
@@ -109,7 +109,7 @@ def test_validate_report_frequency_minimal_set() -> None:
     )
 
 
-@app_custom_config(alert_minimal=2, report_minimal=5)
+@app_custom_config(alert_minimum_interval=2, report_minimum_interval=5)
 def test_validate_report_frequency_invalid_schedule() -> None:
     """
     Test the ``validate_report_frequency`` method when the configured
@@ -128,7 +128,7 @@ def test_validate_report_frequency_invalid_schedule() -> None:
         )
 
 
-@app_custom_config(alert_minimal=10)
+@app_custom_config(alert_minimum_interval=10)
 def test_validate_report_frequency_alert_only() -> None:
     """
     Test the ``validate_report_frequency`` method when there's
@@ -141,7 +141,7 @@ def test_validate_report_frequency_alert_only() -> None:
         )
 
 
-@app_custom_config(report_minimal=10)
+@app_custom_config(report_minimum_interval=10)
 def test_validate_report_frequency_report_only() -> None:
     """
     Test the ``validate_report_frequency`` method when there's
@@ -154,7 +154,7 @@ def test_validate_report_frequency_report_only() -> None:
         )
 
 
-@app_custom_config(alert_minimal=1, report_minimal=1)
+@app_custom_config(alert_minimum_interval=1, report_minimum_interval=1)
 def test_validate_report_frequency_accepts_every_minute_with_one() -> None:
     """
     Test the ``validate_report_frequency`` method when configuration
@@ -168,7 +168,7 @@ def test_validate_report_frequency_accepts_every_minute_with_one() -> None:
             )
 
 
-@app_custom_config(alert_minimal=2, report_minimal=2)
+@app_custom_config(alert_minimum_interval=2, report_minimum_interval=2)
 def test_validate_report_frequency_accepts_every_minute_with_two() -> None:
     """
     Test the ``validate_report_frequency`` method when configuration

From c8b30f36d5986d93f902ebcd51e5534ce85178f3 Mon Sep 17 00:00:00 2001
From: Vitor Avila <vitor.avila@preset.io>
Date: Fri, 3 May 2024 18:21:35 -0300
Subject: [PATCH 3/5] addressing PR feedback

---
 docs/docs/configuration/alerts-reports.mdx    |   8 +-
 superset/commands/report/base.py              |  47 +++---
 superset/commands/report/exceptions.py        |  15 +-
 superset/config.py                            |   6 +-
 tests/integration_tests/reports/api_tests.py  |  19 ++-
 tests/unit_tests/commands/report/base_test.py | 143 +++++++++++-------
 6 files changed, 142 insertions(+), 96 deletions(-)

diff --git a/docs/docs/configuration/alerts-reports.mdx b/docs/docs/configuration/alerts-reports.mdx
index 26698387e8314..878236798df55 100644
--- a/docs/docs/configuration/alerts-reports.mdx
+++ b/docs/docs/configuration/alerts-reports.mdx
@@ -195,13 +195,13 @@ Please refer to `ExecutorType` in the codebase for other executor types.
   its default value of `http://0.0.0.0:8080/`.
 
 
-It's also possible to specify a minimum interval (in minutes) between each report's execution through the config file:
+It's also possible to specify a minimum interval between each report's execution through the config file:
 
 ``` python
 # Set a minimum interval threshold between executions (for each Alert/Report)
-# Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
-REPORT_MINIMUM_INTERVAL_MINUTES = 10 # Change it as desired
+# Value should be an integer
+ALERT_MINIMUM_INTERVAL = int(timedelta(minutes=10).total_seconds())
+REPORT_MINIMUM_INTERVAL = int(timedelta(minutes=5).total_seconds())
 ```
 
 
diff --git a/superset/commands/report/base.py b/superset/commands/report/base.py
index e7e7c08c3b14f..3086023f031eb 100644
--- a/superset/commands/report/base.py
+++ b/superset/commands/report/base.py
@@ -17,6 +17,7 @@
 import logging
 from typing import Any
 
+from croniter import croniter
 from flask import current_app
 from marshmallow import ValidationError
 
@@ -92,29 +93,31 @@ def validate_report_frequency(
         :param report_type: The report type (Alert/Report).
         """
         config_key = (
-            "ALERT_MINIMUM_INTERVAL_MINUTES"
+            "ALERT_MINIMUM_INTERVAL"
             if report_type == ReportScheduleType.ALERT
-            else "REPORT_MINIMUM_INTERVAL_MINUTES"
+            else "REPORT_MINIMUM_INTERVAL"
         )
-        minimum_interval = current_app.config.get(config_key)
-        if not minimum_interval or minimum_interval < 2:
+        minimum_interval = current_app.config.get(config_key, 0)
+
+        if not isinstance(minimum_interval, int):
+            logger.error(
+                "Invalid value for %s: %s", config_key, minimum_interval, exc_info=True
+            )
+            return
+
+        # Since configuration is in minutes, we only need to validate
+        # in case `minimum_interval` is <= 120 (2min)
+        if minimum_interval < 120:
             return
 
-        minutes_interval = cron_schedule.split(" ")[0]
-        # Check if cron is set to every minute (*)
-        # or if it has an every-minute block (1-5)
-        if "-" in minutes_interval or minutes_interval == "*":
-            raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
-
-        # Calculate minimum interval (in minutes)
-        minutes_list = minutes_interval.split(",")
-        if len(minutes_list) > 1:
-            scheduled_minutes = [int(min) for min in minutes_list]
-            scheduled_minutes.sort()
-            differences = [
-                scheduled_minutes[i] - scheduled_minutes[i - 1]
-                for i in range(1, len(scheduled_minutes))
-            ]
-
-            if min(differences) < minimum_interval:
-                raise ReportScheduleFrequencyNotAllowed(report_type, minimum_interval)
+        iterations = 60 if minimum_interval <= 3660 else 24
+        schedule = croniter(cron_schedule)
+        current_exec = next(schedule)
+
+        for _ in range(iterations):
+            next_exec = next(schedule)
+            diff, current_exec = next_exec - current_exec, next_exec
+            if int(diff) < minimum_interval:
+                raise ReportScheduleFrequencyNotAllowed(
+                    report_type=report_type, minimum_interval=minimum_interval
+                )
diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py
index 472b6a97c83f9..2a3a17ae78fbe 100644
--- a/superset/commands/report/exceptions.py
+++ b/superset/commands/report/exceptions.py
@@ -99,14 +99,23 @@ class ReportScheduleFrequencyNotAllowed(ValidationError):
     frequently than allowed
     """
 
-    def __init__(self, report_type: str, minimum_interval: str) -> None:
+    def __init__(
+        self,
+        report_type: str = "Report",
+        minimum_interval: int = 120,
+    ) -> None:
+        interval_in_minutes = (
+            minimum_interval / 60
+            if not minimum_interval % 60
+            else minimum_interval // 60 + 1
+        )
         super().__init__(
             _(
                 "%(report_type)s schedule frequency exceeding limit."
                 " Please configure a schedule with a minimum interval of"
-                " %(minimum_interval)s minutes per execution.",
+                " %(minimum_interval)d minutes per execution.",
                 report_type=report_type,
-                minimum_interval=minimum_interval,
+                minimum_interval=interval_in_minutes,
             ),
             field_name="crontab",
         )
diff --git a/superset/config.py b/superset/config.py
index 22ac9f2e5c7d0..4d16e31ca506b 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1326,9 +1326,9 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 ALERT_REPORTS_MIN_CUSTOM_SCREENSHOT_WIDTH = 600
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
 # Set a minimum interval threshold between executions (for each Alert/Report)
-# Value should be the amount of minutes (integer). Min: 1, Max: 60
-ALERT_MINIMUM_INTERVAL_MINUTES = None
-REPORT_MINIMUM_INTERVAL_MINUTES = None
+# Value should be an integer i.e. int(timedelta(minutes=5).total_seconds())
+ALERT_MINIMUM_INTERVAL = None
+REPORT_MINIMUM_INTERVAL = None
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "
diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py
index f6de07b32cbd5..d107cd360403c 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -16,8 +16,7 @@
 # under the License.
 # isort:skip_file
 """Unit tests for Superset"""
-
-from datetime import datetime
+from datetime import datetime, timedelta
 from unittest.mock import patch
 import json
 
@@ -1294,8 +1293,8 @@ def test_create_report_schedule_valid_schedule(self):
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMUM_INTERVAL_MINUTES": 2,
-                "REPORT_MINIMUM_INTERVAL_MINUTES": 5,
+                "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=2).total_seconds()),
+                "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=5).total_seconds()),
             },
         ):
             uri = "api/v1/report/"
@@ -1341,8 +1340,8 @@ def test_create_report_schedule_invalid_schedule(self):
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
-                "REPORT_MINIMUM_INTERVAL_MINUTES": 8,
+                "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=6).total_seconds()),
+                "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=8).total_seconds()),
             },
         ):
             uri = "api/v1/report/"
@@ -1391,8 +1390,8 @@ def test_update_report_schedule_valid_schedule(self) -> None:
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMUM_INTERVAL_MINUTES": 5,
-                "REPORT_MINIMUM_INTERVAL_MINUTES": 3,
+                "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=5).total_seconds()),
+                "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=3).total_seconds()),
             },
         ):
             # Test alert minimum interval
@@ -1433,8 +1432,8 @@ def test_update_report_schedule_invalid_schedule(self) -> None:
         with patch.dict(
             "superset.commands.report.base.current_app.config",
             {
-                "ALERT_MINIMUM_INTERVAL_MINUTES": 6,
-                "REPORT_MINIMUM_INTERVAL_MINUTES": 4,
+                "ALERT_MINIMUM_INTERVAL": int(timedelta(minutes=6).total_seconds()),
+                "REPORT_MINIMUM_INTERVAL": int(timedelta(minutes=4).total_seconds()),
             },
         ):
             # Exceed alert minimum interval
diff --git a/tests/unit_tests/commands/report/base_test.py b/tests/unit_tests/commands/report/base_test.py
index 9dba37d1eb8b5..5aa0bd647caaf 100644
--- a/tests/unit_tests/commands/report/base_test.py
+++ b/tests/unit_tests/commands/report/base_test.py
@@ -15,6 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
+from datetime import timedelta
 from functools import wraps
 from typing import Any, Callable
 from unittest.mock import patch
@@ -55,8 +56,8 @@ def app_custom_config(
     """
     Decorator to mock the current_app.config values dynamically for each test.
 
-    :param alert_minimum_interval: Minimum interval in minutes for alerts. Defaults to None.
-    :param report_minimum_interval: Minimum interval in minutes for reports. Defaults to None.
+    :param alert_minimum_interval: Minimum interval. Defaults to None.
+    :param report_minimum_interval: Minimum interval. Defaults to None.
 
     :returns: A decorator that wraps a function.
     """
@@ -67,10 +68,10 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
             with patch(
                 "superset.commands.report.base.current_app.config"
             ) as mock_config:
-                mock_config.get.side_effect = lambda key: {
-                    "ALERT_MINIMUM_INTERVAL_MINUTES": alert_minimum_interval,
-                    "REPORT_MINIMUM_INTERVAL_MINUTES": report_minimum_interval,
-                }.get(key)
+                mock_config.get.side_effect = lambda key, default=0: {
+                    "ALERT_MINIMUM_INTERVAL": alert_minimum_interval,
+                    "REPORT_MINIMUM_INTERVAL": report_minimum_interval,
+                }.get(key, default)
                 return func(*args, **kwargs)
 
         return wrapper
@@ -78,21 +79,24 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
     return decorator
 
 
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
 @app_custom_config()
-def test_validate_report_frequency() -> None:
+def test_validate_report_frequency(report_type: str, schedule: str) -> None:
     """
     Test the ``validate_report_frequency`` method when there's
     no minimum frequency configured.
     """
-    for report_type in REPORT_TYPES:
-        for schedule in TEST_SCHEDULES:
-            BaseReportScheduleCommand().validate_report_frequency(
-                schedule,
-                report_type,
-            )
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        report_type,
+    )
 
 
-@app_custom_config(alert_minimum_interval=4, report_minimum_interval=5)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=4).total_seconds()),
+    report_minimum_interval=int(timedelta(minutes=5).total_seconds()),
+)
 def test_validate_report_frequency_minimum_set() -> None:
     """
     Test the ``validate_report_frequency`` method when there's
@@ -109,7 +113,10 @@ def test_validate_report_frequency_minimum_set() -> None:
     )
 
 
-@app_custom_config(alert_minimum_interval=2, report_minimum_interval=5)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+    report_minimum_interval=int(timedelta(minutes=5).total_seconds()),
+)
 def test_validate_report_frequency_invalid_schedule() -> None:
     """
     Test the ``validate_report_frequency`` method when the configured
@@ -128,64 +135,92 @@ def test_validate_report_frequency_invalid_schedule() -> None:
         )
 
 
-@app_custom_config(alert_minimum_interval=10)
-def test_validate_report_frequency_alert_only() -> None:
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=10).total_seconds()),
+)
+def test_validate_report_frequency_alert_only(schedule: str) -> None:
     """
     Test the ``validate_report_frequency`` method when there's
     only a configuration for alerts and user is creating report.
     """
-    for schedule in TEST_SCHEDULES:
-        BaseReportScheduleCommand().validate_report_frequency(
-            schedule,
-            ReportScheduleType.REPORT,
-        )
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        ReportScheduleType.REPORT,
+    )
 
 
-@app_custom_config(report_minimum_interval=10)
-def test_validate_report_frequency_report_only() -> None:
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+    report_minimum_interval=int(timedelta(minutes=10).total_seconds()),
+)
+def test_validate_report_frequency_report_only(schedule: str) -> None:
     """
     Test the ``validate_report_frequency`` method when there's
     only a configuration for reports and user is creating alert.
     """
-    for schedule in TEST_SCHEDULES:
-        BaseReportScheduleCommand().validate_report_frequency(
-            schedule,
-            ReportScheduleType.ALERT,
-        )
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        ReportScheduleType.ALERT,
+    )
 
 
-@app_custom_config(alert_minimum_interval=1, report_minimum_interval=1)
-def test_validate_report_frequency_accepts_every_minute_with_one() -> None:
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=1).total_seconds()),
+    report_minimum_interval=int(timedelta(minutes=1).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_one(
+    report_type: str, schedule: str
+) -> None:
     """
     Test the ``validate_report_frequency`` method when configuration
     is set to `1`. Validates the usage of `-` and `*` in the cron.
     """
-    for report_type in REPORT_TYPES:
-        for schedule in TEST_SCHEDULES:
-            BaseReportScheduleCommand().validate_report_frequency(
-                schedule,
-                report_type,
-            )
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        report_type,
+    )
+
+
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES_SINGLE_MINUTES)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+    report_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_two(
+    report_type: str,
+    schedule: str,
+) -> None:
+    """
+    Test the ``validate_report_frequency`` method when configuration
+    is set to `2`.
+    """
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        report_type,
+    )
 
 
-@app_custom_config(alert_minimum_interval=2, report_minimum_interval=2)
-def test_validate_report_frequency_accepts_every_minute_with_two() -> None:
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES_EVERY_MINUTE)
+@app_custom_config(
+    alert_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+    report_minimum_interval=int(timedelta(minutes=2).total_seconds()),
+)
+def test_validate_report_frequency_accepts_every_minute_with_two_raises(
+    report_type: str,
+    schedule: str,
+) -> None:
     """
     Test the ``validate_report_frequency`` method when configuration
     is set to `2`. Validates the usage of `-` and `*` in the cron.
     """
-    for report_type in REPORT_TYPES:
-        # Should fail for schedules with `-` and `*`
-        for schedule in TEST_SCHEDULES_EVERY_MINUTE:
-            with pytest.raises(ReportScheduleFrequencyNotAllowed):
-                BaseReportScheduleCommand().validate_report_frequency(
-                    schedule,
-                    report_type,
-                )
-
-        # Should work for schedules with single with bigger intervals
-        for schedule in TEST_SCHEDULES_SINGLE_MINUTES:
-            BaseReportScheduleCommand().validate_report_frequency(
-                schedule,
-                report_type,
-            )
+    # Should fail for schedules with `-` and `*`
+    with pytest.raises(ReportScheduleFrequencyNotAllowed):
+        BaseReportScheduleCommand().validate_report_frequency(
+            schedule,
+            report_type,
+        )

From 565998766b73c60627fa42976f07bde4cca85585 Mon Sep 17 00:00:00 2001
From: Vitor Avila <vitor.avila@preset.io>
Date: Fri, 3 May 2024 22:06:45 -0300
Subject: [PATCH 4/5] Fixing tests

---
 superset/config.py                            |  4 +--
 tests/integration_tests/reports/api_tests.py  |  8 +++++
 tests/unit_tests/commands/report/base_test.py | 31 +++++++++++++++++--
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/superset/config.py b/superset/config.py
index 4d16e31ca506b..da491ac433676 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1327,8 +1327,8 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
 # Set a minimum interval threshold between executions (for each Alert/Report)
 # Value should be an integer i.e. int(timedelta(minutes=5).total_seconds())
-ALERT_MINIMUM_INTERVAL = None
-REPORT_MINIMUM_INTERVAL = None
+ALERT_MINIMUM_INTERVAL = 0
+REPORT_MINIMUM_INTERVAL = 0
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "
diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py
index d107cd360403c..4f63a5ab7e3e3 100644
--- a/tests/integration_tests/reports/api_tests.py
+++ b/tests/integration_tests/reports/api_tests.py
@@ -16,6 +16,7 @@
 # under the License.
 # isort:skip_file
 """Unit tests for Superset"""
+
 from datetime import datetime, timedelta
 from unittest.mock import patch
 import json
@@ -1406,6 +1407,13 @@ def test_update_report_schedule_valid_schedule(self) -> None:
             rv = self.put_assert_metric(uri, update_payload, "put")
             assert rv.status_code == 200
 
+        with patch.dict(
+            "superset.commands.report.base.current_app.config",
+            {
+                "ALERT_MINIMUM_INTERVAL": 0,
+                "REPORT_MINIMUM_INTERVAL": 0,
+            },
+        ):
             # Undo changes
             update_payload["crontab"] = previous_cron
             update_payload["type"] = ReportScheduleType.ALERT
diff --git a/tests/unit_tests/commands/report/base_test.py b/tests/unit_tests/commands/report/base_test.py
index 5aa0bd647caaf..499682a1e6f25 100644
--- a/tests/unit_tests/commands/report/base_test.py
+++ b/tests/unit_tests/commands/report/base_test.py
@@ -15,6 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 
+import logging
 from datetime import timedelta
 from functools import wraps
 from typing import Any, Callable
@@ -50,8 +51,8 @@
 
 
 def app_custom_config(
-    alert_minimum_interval: int | None = None,
-    report_minimum_interval: int | None = None,
+    alert_minimum_interval: int | str = 0,
+    report_minimum_interval: int | str = 0,
 ) -> Callable[[Callable[..., Any]], Callable[..., Any]]:
     """
     Decorator to mock the current_app.config values dynamically for each test.
@@ -224,3 +225,29 @@ def test_validate_report_frequency_accepts_every_minute_with_two_raises(
             schedule,
             report_type,
         )
+
+
+@pytest.mark.parametrize("report_type", REPORT_TYPES)
+@pytest.mark.parametrize("schedule", TEST_SCHEDULES)
+@app_custom_config(
+    alert_minimum_interval="10 minutes",
+    report_minimum_interval="10 minutes",
+)
+def test_validate_report_frequency_invalid_config(
+    caplog: pytest.LogCaptureFixture,
+    report_type: str,
+    schedule: str,
+) -> None:
+    """
+    Test the ``validate_report_frequency`` method when the configuration
+    is invalid.
+    """
+    caplog.set_level(logging.ERROR)
+    BaseReportScheduleCommand().validate_report_frequency(
+        schedule,
+        report_type,
+    )
+    expected_error_message = (
+        f"invalid value for {report_type}_MINIMUM_INTERVAL: 10 minutes"
+    )
+    assert expected_error_message.lower() in caplog.text.lower()

From 42e0517d5c8f1e9656da793281ecd81979d164e0 Mon Sep 17 00:00:00 2001
From: Vitor Avila <vitor.avila@preset.io>
Date: Tue, 7 May 2024 16:25:28 -0300
Subject: [PATCH 5/5] addressing PR feedback pt2

---
 superset/commands/report/exceptions.py | 9 ++++-----
 superset/config.py                     | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py
index 2a3a17ae78fbe..495e0bff9a8ae 100644
--- a/superset/commands/report/exceptions.py
+++ b/superset/commands/report/exceptions.py
@@ -15,6 +15,8 @@
 # specific language governing permissions and limitations
 # under the License.
 
+import math
+
 from flask_babel import lazy_gettext as _
 
 from superset.commands.exceptions import (
@@ -104,11 +106,8 @@ def __init__(
         report_type: str = "Report",
         minimum_interval: int = 120,
     ) -> None:
-        interval_in_minutes = (
-            minimum_interval / 60
-            if not minimum_interval % 60
-            else minimum_interval // 60 + 1
-        )
+        interval_in_minutes = math.ceil(minimum_interval / 60)
+
         super().__init__(
             _(
                 "%(report_type)s schedule frequency exceeding limit."
diff --git a/superset/config.py b/superset/config.py
index da491ac433676..8c3d40b7eea34 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -1327,8 +1327,8 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH = 2400
 # Set a minimum interval threshold between executions (for each Alert/Report)
 # Value should be an integer i.e. int(timedelta(minutes=5).total_seconds())
-ALERT_MINIMUM_INTERVAL = 0
-REPORT_MINIMUM_INTERVAL = 0
+ALERT_MINIMUM_INTERVAL = int(timedelta(minutes=0).total_seconds())
+REPORT_MINIMUM_INTERVAL = int(timedelta(minutes=0).total_seconds())
 
 # A custom prefix to use on all Alerts & Reports emails
 EMAIL_REPORTS_SUBJECT_PREFIX = "[Report] "