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

hughhhh · 2023-09-29T23:12:52Z

SUMMARY

Currently the clear dropdown functionality doesn't work in production, this fix properly handles the logic to properly to untag certain entities

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-10-02.at.5.03.20.PM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2023-10-02T16:53:55Z

@hughhhh would you mind filling out the PR description.

eschutho · 2023-10-02T17:56:34Z

superset/daos/tag.py

+        if not objects_to_tag:
+            tagged_objects_to_delete = current_tagged_objects
+        else:
+            tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects


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

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.

superset/tags/commands/create.py

eschutho · 2023-10-02T18:00:03Z

superset/tags/schemas.py

@@ -58,7 +58,7 @@ class TagObjectSchema(Schema):
    name = fields.String()
    description = fields.String(required=False, allow_none=True)
    objects_to_tag = fields.List(
-        fields.Tuple((fields.String(), fields.Int())), required=False
+        fields.Tuple((fields.String(), fields.Int())),


does this change the api? I know this is behind a feature flag, but in your opinion, is this a breaking change?

good call out, this is a breaking change it's prolly safer to just keep as not required i've already have the proper .get() default value in place

…ags-fix-clear-delete

betodealmeida · 2023-10-02T22:15:34Z

superset/daos/tag.py

+        if not objects_to_tag:
+            tagged_objects_to_delete = current_tagged_objects
+        else:
+            tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects


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.

betodealmeida · 2023-10-02T22:21:31Z

superset/tags/commands/create.py

-        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", []):
+            if any(obj_id == 0 for obj_type, obj_id in objects_to_tag):


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?

eschutho · 2023-10-03T21:24:57Z

superset/tags/commands/create.py

            # 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 = []


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

eschutho · 2023-10-03T21:28:08Z

superset/tags/commands/create.py

-            if self._description:
-                tag.description = self._description
+            if description := self._properties.get("description"):
+                tag.description = description


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?

eschutho · 2023-10-03T21:29:14Z

superset/tags/commands/create.py

-            if any(obj_id == 0 for obj_type, obj_id in self._objects_to_tag):
-                exceptions.append(TagInvalidError())
-
+        if objects_to_tag := self._properties.get("objects_to_tag", []):


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

eschutho

Much easier to read now.. looks great!

betodealmeida

Looks good, left a few suggestions.

betodealmeida · 2023-10-04T23:01:36Z

superset/daos/tag.py

+        tagged_objects_to_delete = (
+            current_tagged_objects
+            if not objects_to_tag
+            else current_tagged_objects - updated_tagged_objects
+        )


If objects_to_tag is falsy then updated_tagged_objects is an empty set, so this should work:

Suggested change

tagged_objects_to_delete = (

current_tagged_objects

if not objects_to_tag

else current_tagged_objects - updated_tagged_objects

)

tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects

so this logic won't work since updated tags includes both prev and new tags, so if we get a empty objects_to_tag we the difference logic won't catch:

In [1]: current_tagged_object = [1, 2, 3] In [2]: updated_tagged_object = [1] In [3]: updated_tagged_object = [1, 4] In [4]: current_tagged_object - updated_tagged_object In [5]: set(current_tagged_object) - set(updated_tagged_object) Out[5]: {2, 3} In [6]: updated_tagged_object = [] In [7]: set(current_tagged_object) - set(updated_tagged_object) Out[7]: {1, 2, 3} >> won't delete anything

superset/tags/commands/create.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

properly handle clear deletes

6fb1348

pull-request-size bot added the size/M label Sep 29, 2023

john-bodley self-requested a review October 2, 2023 16:54

eschutho reviewed Oct 2, 2023

View reviewed changes

superset/tags/commands/create.py Show resolved Hide resolved

eschutho reviewed Oct 2, 2023

View reviewed changes

hughhhh added 2 commits October 2, 2023 21:05

Merge branch 'master' of https://github.com/preset-io/superset into t…

3e15726

…ags-fix-clear-delete

address comments

7e1150b

hughhhh force-pushed the tags-fix-clear-delete branch from ba65e3b to 7e1150b Compare October 2, 2023 21:18

betodealmeida reviewed Oct 2, 2023

View reviewed changes

hughhhh added 5 commits October 3, 2023 00:34

fix pre-commit

25cda87

Merge branch 'master' into tags-fix-clear-delete

88af0ef

address concerns and update test

49ae961

move validation to marshmallow

58ca613

fix linting

a8de326

eschutho reviewed Oct 3, 2023

View reviewed changes

hughhhh added 2 commits October 3, 2023 23:41

remove test

3fb54be

remove condition

2bb6584

pull-request-size bot added size/L and removed size/M labels Oct 3, 2023

eschutho approved these changes Oct 4, 2023

View reviewed changes

betodealmeida approved these changes Oct 4, 2023

View reviewed changes

hughhhh and others added 3 commits October 5, 2023 12:41

Update superset/tags/commands/create.py

3f073e8

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/tags/commands/create.py

49bfd45

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/tags/commands/create.py

4ac3d57

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

hughhhh merged commit dcfebfc into apache:master Oct 5, 2023

hughhhh deleted the tags-fix-clear-delete branch October 5, 2023 17:37

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

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

6cc2b36

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

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

634596b

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hughhhh commented Sep 29, 2023 •

edited

Loading

john-bodley commented Oct 2, 2023

eschutho Oct 2, 2023

betodealmeida Oct 2, 2023

eschutho Oct 2, 2023

hughhhh Oct 2, 2023

betodealmeida Oct 2, 2023

betodealmeida Oct 2, 2023

eschutho Oct 3, 2023

eschutho Oct 3, 2023

eschutho Oct 3, 2023 •

edited

Loading

eschutho left a comment

betodealmeida left a comment

betodealmeida Oct 4, 2023

hughhhh Oct 5, 2023

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

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

Conversation

hughhhh commented Sep 29, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

john-bodley commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughhhh commented Sep 29, 2023 •

edited

Loading

eschutho Oct 3, 2023 •

edited

Loading