Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(tags): fix clears delete on Tags Modal #25470

Merged
merged 13 commits into from
Oct 5, 2023
6 changes: 5 additions & 1 deletion superset/daos/tag.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,11 @@ def create_tag_relationship(
updated_tagged_objects = {
(to_object_type(obj[0]), obj[1]) for obj in objects_to_tag
}
tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects

if not objects_to_tag:
tagged_objects_to_delete = current_tagged_objects
else:
tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe simplify with a ternary? Something like:
tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects if objects_to_tag else current_tagged_objects
just slightly easier to read imo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can replace all of this with:

tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects

Since if objects_to_tag is falsy then updated_tagged_objects is an empty set.


for object_type, object_id in updated_tagged_objects:
# create rows for new objects, and skip tags that already exist
Expand Down
30 changes: 14 additions & 16 deletions superset/tags/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,23 @@ def validate(self) -> None:

class CreateCustomTagWithRelationshipsCommand(CreateMixin, BaseCommand):
def __init__(self, data: dict[str, Any], bulk_create: bool = False):
self._tag = data["name"]
self._objects_to_tag = data.get("objects_to_tag")
self._description = data.get("description")
self._properties = data.copy()
self._bulk_create = bulk_create

def run(self) -> None:
self.validate()

try:
tag = TagDAO.get_by_name(self._tag.strip(), TagTypes.custom)
if self._objects_to_tag:
TagDAO.create_tag_relationship(
objects_to_tag=self._objects_to_tag,
tag=tag,
bulk_create=self._bulk_create,
)
tag_name = self._properties["name"]
eschutho marked this conversation as resolved.
Show resolved Hide resolved
tag = TagDAO.get_by_name(tag_name.strip(), TagTypes.custom)
TagDAO.create_tag_relationship(
objects_to_tag=self._properties.get("objects_to_tag", []),
tag=tag,
bulk_create=self._bulk_create,
)

if self._description:
tag.description = self._description
if description := self._properties.get("description"):
tag.description = description
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can use less "if" statements, like in here, I would suggest removing it, too. "If" statements IMO tend to break up the flow and make the code more difficult to read.

Can this be simplified to:
tag.description = self._properties.get("description")
would that change anything if self._properties.get("description") is falsy?


db.session.commit()

Expand All @@ -96,13 +94,13 @@ def run(self) -> None:
def validate(self) -> None:
exceptions = []
# Validate object_id
if self._objects_to_tag:
if any(obj_id == 0 for obj_type, obj_id in self._objects_to_tag):
if objects_to_tag := self._properties.get("objects_to_tag", []):
Copy link
Member

@eschutho eschutho Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.. is this if statement necessary? If objects_to_tag is an empty array, then the for loop below will do nothing.

if any(obj_id == 0 for obj_type, obj_id in objects_to_tag):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we checking for id 0 here? Is this something that could happen realistically? Why not check for negative numbers, or integers — can we move this validation to the Marshmallow schema?

exceptions.append(TagInvalidError())

# Validate object type
skipped_tagged_objects: list[tuple[str, int]] = []
for obj_type, obj_id in self._objects_to_tag:
for obj_type, obj_id in objects_to_tag:
skipped_tagged_objects = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is skipped_tagged_objects defined twice, once above, and then again in this for loop?

object_type = to_object_type(obj_type)

Expand All @@ -117,7 +115,7 @@ def validate(self) -> None:
# skip the object if the user doesn't have access
skipped_tagged_objects.append((obj_type, obj_id))

self._objects_to_tag = set(self._objects_to_tag) - set(
self._properties["objects_to_tag"] = set(objects_to_tag) - set(
skipped_tagged_objects
)

Expand Down
10 changes: 4 additions & 6 deletions superset/tags/commands/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,10 @@ def __init__(self, model_id: int, data: dict[str, Any]):
def run(self) -> Model:
self.validate()
if self._model:
if self._properties.get("objects_to_tag"):
# todo(hugh): can this manage duplication
TagDAO.create_tag_relationship(
objects_to_tag=self._properties["objects_to_tag"],
tag=self._model,
)
TagDAO.create_tag_relationship(
objects_to_tag=self._properties.get("objects_to_tag", []),
tag=self._model,
)
if description := self._properties.get("description"):
self._model.description = description
if tag_name := self._properties.get("name"):
Expand Down