You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hughhhh (via GitHub)" <gi...@apache.org> on 2023/09/29 23:12:55 UTC
[GitHub] [superset] hughhhh opened a new pull request, #25470: fix(tags): fix clears delete on Tags Modal
hughhhh opened a new pull request, #25470:
URL: https://github.com/apache/superset/pull/25470
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1344764921
##########
superset/tags/commands/create.py:
##########
@@ -96,13 +94,10 @@ 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):
- exceptions.append(TagInvalidError())
-
+ if objects_to_tag := self._properties.get("objects_to_tag", []):
Review Comment:
same here.. is this if statement necessary if objects_to_tag is an empty array, then the for loop before will do nothing.
##########
superset/tags/commands/create.py:
##########
@@ -96,13 +94,10 @@ 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):
- exceptions.append(TagInvalidError())
-
+ if objects_to_tag := self._properties.get("objects_to_tag", []):
Review Comment:
same here.. is this if statement necessary if objects_to_tag is an empty array, then the for loop below will do nothing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh merged PR #25470:
URL: https://github.com/apache/superset/pull/25470
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25470:
URL: https://github.com/apache/superset/pull/25470#issuecomment-1743392723
@hughhhh would you mind filling out the PR description.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1344753535
##########
superset/tags/commands/create.py:
##########
@@ -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"]
Review Comment:
no, this makes sense, thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1344761175
##########
superset/tags/commands/create.py:
##########
@@ -96,13 +94,10 @@ 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):
- exceptions.append(TagInvalidError())
-
+ if objects_to_tag := self._properties.get("objects_to_tag", []):
# 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 = []
Review Comment:
why is skipped_tagged_objects defined twice, once above, and then again in this for loop?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1343219427
##########
superset/tags/commands/create.py:
##########
@@ -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", []):
+ if any(obj_id == 0 for obj_type, obj_id in objects_to_tag):
Review Comment:
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?
##########
superset/tags/commands/create.py:
##########
@@ -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", []):
+ if any(obj_id == 0 for obj_type, obj_id in objects_to_tag):
Review Comment:
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?
##########
superset/daos/tag.py:
##########
@@ -394,7 +394,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
Review Comment:
I think you can replace all of this with:
```python
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1342990347
##########
superset/daos/tag.py:
##########
@@ -394,7 +394,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
Review Comment:
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1344764026
##########
superset/tags/commands/create.py:
##########
@@ -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"]
+ 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
Review Comment:
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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1346568419
##########
superset/tags/commands/create.py:
##########
@@ -95,31 +92,23 @@ 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):
- exceptions.append(TagInvalidError())
-
- # Validate object type
- skipped_tagged_objects: list[tuple[str, int]] = []
- for obj_type, obj_id in self._objects_to_tag:
- skipped_tagged_objects = []
- object_type = to_object_type(obj_type)
-
- if not object_type:
- exceptions.append(
- TagInvalidError(f"invalid object type {object_type}")
- )
- try:
- model = to_object_model(object_type, obj_id) # type: ignore
- security_manager.raise_for_ownership(model)
- except SupersetSecurityException:
- # 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(
- skipped_tagged_objects
- )
+ objects_to_tag = self._properties.get("objects_to_tag", [])
+ skipped_tagged_objects: list[tuple[str, int]] = []
+ for obj_type, obj_id in objects_to_tag:
+ object_type = to_object_type(obj_type)
+
+ if not object_type:
+ exceptions.append(TagInvalidError(f"invalid object type {object_type}"))
+ try:
+ model = to_object_model(object_type, obj_id) # type: ignore
+ security_manager.raise_for_ownership(model)
+ except SupersetSecurityException:
+ # skip the object if the user doesn't have access
+ skipped_tagged_objects.append((obj_type, obj_id))
+
+ self._properties["objects_to_tag"] = set(objects_to_tag) - set(
+ skipped_tagged_objects
+ )
Review Comment:
```suggestion
self._properties["objects_to_tag"] = objects_to_tag - skipped_tagged_objects
```
##########
superset/tags/commands/create.py:
##########
@@ -95,31 +92,23 @@ 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):
- exceptions.append(TagInvalidError())
-
- # Validate object type
- skipped_tagged_objects: list[tuple[str, int]] = []
- for obj_type, obj_id in self._objects_to_tag:
- skipped_tagged_objects = []
- object_type = to_object_type(obj_type)
-
- if not object_type:
- exceptions.append(
- TagInvalidError(f"invalid object type {object_type}")
- )
- try:
- model = to_object_model(object_type, obj_id) # type: ignore
- security_manager.raise_for_ownership(model)
- except SupersetSecurityException:
- # 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(
- skipped_tagged_objects
- )
+ objects_to_tag = self._properties.get("objects_to_tag", [])
+ skipped_tagged_objects: list[tuple[str, int]] = []
+ for obj_type, obj_id in objects_to_tag:
+ object_type = to_object_type(obj_type)
+
+ if not object_type:
+ exceptions.append(TagInvalidError(f"invalid object type {object_type}"))
+ try:
+ model = to_object_model(object_type, obj_id) # type: ignore
+ security_manager.raise_for_ownership(model)
+ except SupersetSecurityException:
+ # skip the object if the user doesn't have access
+ skipped_tagged_objects.append((obj_type, obj_id))
Review Comment:
```suggestion
skipped_tagged_objects.add((obj_type, obj_id))
```
##########
superset/tags/commands/create.py:
##########
@@ -95,31 +92,23 @@ 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):
- exceptions.append(TagInvalidError())
-
- # Validate object type
- skipped_tagged_objects: list[tuple[str, int]] = []
- for obj_type, obj_id in self._objects_to_tag:
- skipped_tagged_objects = []
- object_type = to_object_type(obj_type)
-
- if not object_type:
- exceptions.append(
- TagInvalidError(f"invalid object type {object_type}")
- )
- try:
- model = to_object_model(object_type, obj_id) # type: ignore
- security_manager.raise_for_ownership(model)
- except SupersetSecurityException:
- # 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(
- skipped_tagged_objects
- )
+ objects_to_tag = self._properties.get("objects_to_tag", [])
+ skipped_tagged_objects: list[tuple[str, int]] = []
Review Comment:
You can create these as sets, instead of converting later:
```suggestion
objects_to_tag = set(self._properties.get("objects_to_tag", []))
skipped_tagged_objects: set[tuple[str, int]] = set()
```
##########
superset/daos/tag.py:
##########
@@ -390,7 +390,12 @@ 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
+
+ tagged_objects_to_delete = (
+ current_tagged_objects
+ if not objects_to_tag
+ else current_tagged_objects - updated_tagged_objects
+ )
Review Comment:
If `objects_to_tag` is falsy then `updated_tagged_objects` is an empty set, so this should work:
```suggestion
tagged_objects_to_delete = current_tagged_objects - updated_tagged_objects
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1342991572
##########
superset/tags/commands/create.py:
##########
@@ -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"]
Review Comment:
why is name accessed directly as opposed to the other properties that are using .get?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1343167357
##########
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())),
Review Comment:
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1347707980
##########
superset/daos/tag.py:
##########
@@ -390,7 +390,12 @@ 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
+
+ tagged_objects_to_delete = (
+ current_tagged_objects
+ if not objects_to_tag
+ else current_tagged_objects - updated_tagged_objects
+ )
Review Comment:
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
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1342993293
##########
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())),
Review Comment:
does this change the api? I know this is behind a feature flag, but in your opinion, is this a breaking change?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1343166039
##########
superset/tags/commands/create.py:
##########
@@ -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"]
Review Comment:
required vs. not required, right now with the current PR name is the only required to create the object.
Let me know if you'd want me to just use .get for everything
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): fix clears delete on Tags Modal [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25470:
URL: https://github.com/apache/superset/pull/25470#discussion_r1344764921
##########
superset/tags/commands/create.py:
##########
@@ -96,13 +94,10 @@ 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):
- exceptions.append(TagInvalidError())
-
+ if objects_to_tag := self._properties.get("objects_to_tag", []):
Review Comment:
same here.. is this if statement necessary? If objects_to_tag is an empty array, then the for loop below will do nothing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org