betodealmeida commented on a change in pull request #16375:
URL: https://github.com/apache/superset/pull/16375#discussion_r695243414
##########
File path: superset/reports/dao.py
##########
@@ -113,6 +113,24 @@ def bulk_delete(
db.session.rollback()
raise DAODeleteFailedError(str(ex)) from ex
+ @staticmethod
+ def validate_unique_creation_method(
+ user_id: int, dashboard_id: Optional[int] = None, chart_id:
Optional[int] = None
+ ) -> bool:
+ """
+ Validate if the user already has a chart or dashboard
+ with a report attached form the self subscribe reports
+ """
+
+ query =
db.session.query(ReportSchedule).filter_by(created_by_fk=user_id)
+ if query is not None and dashboard_id is not None:
Review comment:
`query` can't be `None`, since it's defined as something else in line
125.
```suggestion
if dashboard_id is not None:
```
##########
File path: superset/reports/commands/create.py
##########
@@ -59,7 +60,10 @@ def validate(self) -> None:
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")
+ chart_id = self._properties.get("chart")
+ dashboard_id = self._properties.get("dashboard")
+ user_id = self._actor.id
Review comment:
Nit: leave the empty line separating variable definitions from the test
below.
```suggestion
user_id = self._actor.id
```
##########
File path: superset/reports/dao.py
##########
@@ -113,6 +113,24 @@ def bulk_delete(
db.session.rollback()
raise DAODeleteFailedError(str(ex)) from ex
+ @staticmethod
+ def validate_unique_creation_method(
+ user_id: int, dashboard_id: Optional[int] = None, chart_id:
Optional[int] = None
+ ) -> bool:
+ """
+ Validate if the user already has a chart or dashboard
+ with a report attached form the self subscribe reports
+ """
+
+ query =
db.session.query(ReportSchedule).filter_by(created_by_fk=user_id)
+ if query is not None and dashboard_id is not None:
+ query = query.filter(ReportSchedule.dashboard_id == dashboard_id)
+
+ if query is not None and chart_id is not None:
Review comment:
Same here:
```suggestion
if chart_id is not None:
```
##########
File path: superset/reports/commands/exceptions.py
##########
@@ -167,6 +167,19 @@ def __init__(self) -> None:
super().__init__([_("Name must be unique")], field_name="name")
+class ReportScheduleCreationMethodUniquenessValidationError(ValidationError):
+ """
+ Marshmallow validation error for Report Schedule with creation method
charts
+ or dashboards already existing on a report.
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
Review comment:
```suggestion
status = 409
super().__init__(
```
##########
File path: superset/reports/dao.py
##########
@@ -113,6 +113,24 @@ def bulk_delete(
db.session.rollback()
raise DAODeleteFailedError(str(ex)) from ex
+ @staticmethod
+ def validate_unique_creation_method(
+ user_id: int, dashboard_id: Optional[int] = None, chart_id:
Optional[int] = None
+ ) -> bool:
+ """
+ Validate if the user already has a chart or dashboard
+ with a report attached form the self subscribe reports
+ """
+
+ query =
db.session.query(ReportSchedule).filter_by(created_by_fk=user_id)
+ if query is not None and dashboard_id is not None:
+ query = query.filter(ReportSchedule.dashboard_id == dashboard_id)
+
+ if query is not None and chart_id is not None:
+ query = query.filter(ReportSchedule.chart_id == chart_id)
+ print(query)
+ return not db.session.query(query.exists()).scalar()
Review comment:
Leave an empty line to logically separate the return from the previous
test.
```suggestion
return not db.session.query(query.exists()).scalar()
```
##########
File path: tests/integration_tests/reports/api_tests.py
##########
@@ -790,6 +790,96 @@ def test_no_dashboard_report_schedule_schema(self):
== "Please save your dashboard first, then try creating a new
email report."
)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "create_report_schedules"
+ )
+ def test_create_multiple_creation_method_report_schedule_charts(self):
+ """
+ ReportSchedule Api: Test create multiple reports with the same
creation method
+ """
+ self.login(username="admin")
+ chart = db.session.query(Slice).first()
+ dashboard = db.session.query(Dashboard).first()
+ example_db = get_example_database()
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name4",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.CHARTS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "chart": chart.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ print(data)
+ assert rv.status_code == 201
+
+ # this second time it should receive an error because the chart has an
attached report
+ # with the same creation method from the same user.
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name5",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.CHARTS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "chart": chart.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ assert rv.status_code == 422
+ assert data["message"]["creation_method"] == [
+ "Resource already has an attached report."
+ ]
+
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "create_report_schedules"
+ )
+ def test_create_multiple_creation_method_report_schedule_dashboards(self):
+ """
+ ReportSchedule Api: Test create multiple reports with the same
creation method
+ """
+ self.login(username="admin")
+ chart = db.session.query(Slice).first()
+ dashboard = db.session.query(Dashboard).first()
+ example_db = get_example_database()
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name4",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.DASHBOARDS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "dashboard": dashboard.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ print(data)
Review comment:
```suggestion
```
##########
File path: tests/integration_tests/reports/api_tests.py
##########
@@ -790,6 +790,96 @@ def test_no_dashboard_report_schedule_schema(self):
== "Please save your dashboard first, then try creating a new
email report."
)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "create_report_schedules"
+ )
+ def test_create_multiple_creation_method_report_schedule_charts(self):
+ """
+ ReportSchedule Api: Test create multiple reports with the same
creation method
+ """
+ self.login(username="admin")
+ chart = db.session.query(Slice).first()
+ dashboard = db.session.query(Dashboard).first()
+ example_db = get_example_database()
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name4",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.CHARTS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "chart": chart.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ print(data)
+ assert rv.status_code == 201
+
+ # this second time it should receive an error because the chart has an
attached report
+ # with the same creation method from the same user.
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name5",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.CHARTS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "chart": chart.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ assert rv.status_code == 422
Review comment:
I think returning a 409
(https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/409) makes more sense
here. All you need to do is to set the attribute `status = 409` in your new
exception.
##########
File path: tests/integration_tests/reports/api_tests.py
##########
@@ -790,6 +790,96 @@ def test_no_dashboard_report_schedule_schema(self):
== "Please save your dashboard first, then try creating a new
email report."
)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "create_report_schedules"
+ )
+ def test_create_multiple_creation_method_report_schedule_charts(self):
+ """
+ ReportSchedule Api: Test create multiple reports with the same
creation method
+ """
+ self.login(username="admin")
+ chart = db.session.query(Slice).first()
+ dashboard = db.session.query(Dashboard).first()
+ example_db = get_example_database()
+ report_schedule_data = {
+ "type": ReportScheduleType.REPORT,
+ "name": "name4",
+ "description": "description",
+ "creation_method": ReportCreationMethodType.CHARTS,
+ "crontab": "0 9 * * *",
+ "working_timeout": 3600,
+ "chart": chart.id,
+ }
+ uri = "api/v1/report/"
+ rv = self.client.post(uri, json=report_schedule_data)
+ data = json.loads(rv.data.decode("utf-8"))
+ print(data)
Review comment:
```suggestion
```
##########
File path: superset/reports/dao.py
##########
@@ -113,6 +113,24 @@ def bulk_delete(
db.session.rollback()
raise DAODeleteFailedError(str(ex)) from ex
+ @staticmethod
+ def validate_unique_creation_method(
+ user_id: int, dashboard_id: Optional[int] = None, chart_id:
Optional[int] = None
+ ) -> bool:
+ """
+ Validate if the user already has a chart or dashboard
+ with a report attached form the self subscribe reports
+ """
+
+ query =
db.session.query(ReportSchedule).filter_by(created_by_fk=user_id)
+ if query is not None and dashboard_id is not None:
+ query = query.filter(ReportSchedule.dashboard_id == dashboard_id)
+
+ if query is not None and chart_id is not None:
+ query = query.filter(ReportSchedule.chart_id == chart_id)
+ print(query)
Review comment:
```suggestion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
