john-bodley commented on code in PR #24628:
URL: https://github.com/apache/superset/pull/24628#discussion_r1256802207
##########
superset/migrations/versions/2023-06-22_13-39_6fbe660cac39_add_on_delete_cascade_for_tables_references.py:
##########
@@ -21,74 +21,45 @@
Create Date: 2023-06-22 13:39:47.989373
"""
-from __future__ import annotations
# revision identifiers, used by Alembic.
revision = "6fbe660cac39"
down_revision = "90139bf715e4"
-import sqlalchemy as sa
Review Comment:
Same logic as before, but using the more generalized shared constraints
logic.
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -282,8 +282,6 @@ def test_import_v1_chart(self, sm_g, utils_g):
assert chart.owners == [admin]
- chart.owners = []
- database.owners = []
Review Comment:
Databases don't have owners so this line was a red herring.
##########
tests/integration_tests/commands_test.py:
##########
@@ -190,10 +187,6 @@ def test_import_v1_dashboard_overwrite(self):
chart = dashboard.slices[0]
dataset = chart.table
database = dataset.database
- dashboard.owners = []
-
- chart.owners = []
- database.owners = []
Review Comment:
See above.
##########
tests/integration_tests/datasets/commands_tests.py:
##########
@@ -400,7 +400,6 @@ def test_import_v1_dataset(self, sm_g, utils_g):
assert column.description is None
assert column.python_date_format is None
- dataset.database.owners = []
Review Comment:
See above.
##########
superset/examples/birth_names.py:
##########
@@ -535,7 +535,6 @@ def create_dashboard(slices: list[Slice]) -> Dashboard:
dash = db.session.query(Dashboard).filter_by(slug="births").first()
if not dash:
dash = Dashboard()
- dash.owners = []
Review Comment:
No need to initialize this.
##########
superset/daos/report.py:
##########
@@ -95,30 +95,6 @@ def find_by_database_ids(database_ids: list[int]) ->
list[ReportSchedule]:
.all()
)
- @classmethod
- def delete(
- cls,
- items: ReportSchedule | list[ReportSchedule],
- commit: bool = True,
- ) -> None:
- item_ids = [item.id for item in get_iterable(items)]
- try:
- # Clean owners secondary table
- report_schedules = (
Review Comment:
I'm not really sure why the reports were refetched as opposed to using the
`items` provides. Note after removing the need to remove the owners this logic
simply reduced to the `BaseDAO.delete` method and thus is unnecessary.
##########
tests/integration_tests/datasets/commands_tests.py:
##########
@@ -528,7 +527,6 @@ def test_import_v1_dataset_existing_database(self, mock_g):
)
assert len(database.tables) == 1
- database.owners = []
Review Comment:
See above.
##########
tests/integration_tests/commands_test.py:
##########
@@ -146,9 +146,6 @@ def test_import_assets(self):
assert dashboard.owners == [self.user]
- dashboard.owners = []
- chart.owners = []
- database.owners = []
Review Comment:
See above.
##########
superset/tables/models.py:
##########
@@ -53,12 +53,12 @@
Model.metadata, # pylint: disable=no-member
sa.Column(
"table_id",
- sa.ForeignKey("sl_tables.id", ondelete="cascade"),
+ sa.ForeignKey("sl_tables.id", ondelete="CASCADE"),
Review Comment:
Using uppercase for consistency.
##########
tests/integration_tests/dashboards/commands_tests.py:
##########
@@ -573,9 +573,6 @@ def test_import_v1_dashboard(self, sm_g, utils_g):
assert dashboard.owners == [admin]
- dashboard.owners = []
- chart.owners = []
- database.owners = []
Review Comment:
See above.
--
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]
