Addressing PR feedback

superset-frontend/src/components/Tags/utils.test.tsx

@@ -21,15 +21,15 @@ import { tagToSelectOption } from 'src/components/Tags/utils';
 describe('tagToSelectOption', () => {
   test('converts a Tag object with table_name to a SelectTagsValue', () => {
     const tag = {
-      id: '1',
+      id: 1,
       name: 'TagName',
       table_name: 'Table1',
     };
 
     const expectedSelectTagsValue = {
-      value: 'TagName',
+      value: 1,
       label: 'TagName',
-      key: '1',
+      key: 1,
     };
 
     expect(tagToSelectOption(tag)).toEqual(expectedSelectTagsValue);

superset-frontend/src/components/Tags/utils.tsx

@@ -37,17 +37,17 @@ const cachedSupersetGet = cacheWrapper(
 );
 
 type SelectTagsValue = {
-  value: string | number | undefined;
-  label: string;
-  key: string | number | undefined;
+  value: number | undefined;
+  label: string | undefined;
+  key: number | undefined;
 };
 
 export const tagToSelectOption = (
-  item: Tag & { table_name: string },
+  tag: Tag & { table_name: string },
 ): SelectTagsValue => ({
-  value: item.name,
-  label: item.name,
-  key: item.id,
+  value: tag.id,
+  label: tag.name,
+  key: tag.id,
 });
 
 export const loadTags = async (

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

@@ -110,10 +110,9 @@ const PropertiesModal = ({
   const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
 
   const tagsAsSelectValues = useMemo(() => {
-    const selectTags = tags.map(tag => ({
-      value: tag.name,
+    const selectTags = tags.map((tag: { id: number; name: string }) => ({
+      value: tag.id,
       label: tag.name,
-      key: tag.name,
     }));
     return selectTags;
   }, [tags.length]);
@@ -362,13 +361,14 @@ const PropertiesModal = ({
     });
 
     const moreOnSubmitProps: { roles?: Roles } = {};
-    const morePutProps: { roles?: number[]; tags?: string[] } = {};
+    const morePutProps: { roles?: number[]; tags?: (number | undefined)[] } =
+      {};
     if (isFeatureEnabled(FeatureFlag.DashboardRbac)) {
       moreOnSubmitProps.roles = roles;
       morePutProps.roles = (roles || []).map(r => r.id);
     }
     if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
-      morePutProps.tags = (tags || []).map(r => r.name);
+      morePutProps.tags = tags.map(tag => tag.id);
     }
     const onSubmitProps = {
       id: dashboardId,
@@ -565,12 +565,12 @@ const PropertiesModal = ({
     }
   }, [dashboardId]);
 
-  const handleChangeTags = (values: { label: string; value: number }[]) => {
-    // triggered whenever a new tag is selected or a tag was deselected
-    // on new tag selected, add the tag
-
-    const uniqueTags = [...new Set(values.map(v => v.label))];
-    setTags([...uniqueTags.map(t => ({ name: t }))]);
+  const handleChangeTags = (tags: { label: string; value: number }[]) => {
+    const parsedTags: TagType[] = ensureIsArray(tags).map(r => ({
+      id: r.value,
+      name: r.label,
+    }));
+    setTags(parsedTags);
   };
 
   return (

superset-frontend/src/explore/components/PropertiesModal/index.tsx

@@ -30,6 +30,7 @@ import {
   isFeatureEnabled,
   FeatureFlag,
   getClientErrorObject,
+  ensureIsArray,
 } from '@superset-ui/core';
 import Chart, { Slice } from 'src/types/Chart';
 import withToasts from 'src/components/MessageToasts/withToasts';
@@ -75,10 +76,9 @@ function PropertiesModal({
   const [tags, setTags] = useState<TagType[]>([]);
 
   const tagsAsSelectValues = useMemo(() => {
-    const selectTags = tags.map(tag => ({
-      value: tag.name,
+    const selectTags = tags.map((tag: { id: number; name: string }) => ({
+      value: tag.id,
       label: tag.name,
-      key: tag.name,
     }));
     return selectTags;
   }, [tags.length]);
@@ -169,7 +169,7 @@ function PropertiesModal({
       ).map(o => o.value);
     }
     if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
-      payload.tags = tags.map(tag => tag.name);
+      payload.tags = tags.map(tag => tag.id);
     }
 
     try {
@@ -227,12 +227,12 @@ function PropertiesModal({
     }
   }, [slice.slice_id]);
 
-  const handleChangeTags = (values: { label: string; value: number }[]) => {
-    // triggered whenever a new tag is selected or a tag was deselected
-    // on new tag selected, add the tag
-
-    const uniqueTags = [...new Set(values.map(v => v.label))];
-    setTags([...uniqueTags.map(t => ({ name: t }))]);
+  const handleChangeTags = (tags: { label: string; value: number }[]) => {
+    const parsedTags: TagType[] = ensureIsArray(tags).map(r => ({
+      id: r.value,
+      name: r.label,
+    }));
+    setTags(parsedTags);
   };
 
   const handleClearTags = () => {

superset-frontend/src/types/TagType.ts

@@ -20,7 +20,7 @@
 import { MouseEventHandler } from 'react';
 
 export interface TagType {
-  id?: string | number;
+  id?: number;
   type?: string | number;
   editable?: boolean;
   onDelete?: (index: number) => void;

superset/charts/schemas.py

@@ -27,7 +27,6 @@
 from superset import app
 from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType
 from superset.db_engine_specs.base import builtin_time_grains
-from superset.tags.models import TagType
 from superset.utils import pandas_postprocessing, schema as utils
 from superset.utils.core import (
     AnnotationType,
@@ -144,12 +143,6 @@
 }
 
 
-class TagSchema(Schema):
-    id = fields.Int()
-    name = fields.String()
-    type = fields.Enum(TagType, by_value=True)
-
-
 class ChartEntityResponseSchema(Schema):
     """
     Schema for a chart object
@@ -285,9 +278,7 @@ class ChartPutSchema(Schema):
     )
     is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
     external_url = fields.String(allow_none=True)
-    tags = fields.List(
-        fields.String(metadata={"description": tags_description}, allow_none=True)
-    )
+    tags = fields.List(fields.Integer(metadata={"description": tags_description}))
 
 
 class ChartGetDatasourceObjectDataResponseSchema(Schema):

superset/commands/chart/update.py

@@ -78,7 +78,7 @@ def validate(self) -> None:
         exceptions: list[ValidationError] = []
         dashboard_ids = self._properties.get("dashboards")
         owner_ids: Optional[list[int]] = self._properties.get("owners")
-        tag_names: Optional[list[str]] = self._properties.get("tags")
+        tag_ids: Optional[list[int]] = self._properties.get("tags")
 
         # Validate if datasource_id is provided datasource_type is required
         datasource_id = self._properties.get("datasource_id")
@@ -109,7 +109,7 @@ def validate(self) -> None:
 
         # validate tags
         try:
-            validate_tags(self._model.__class__.__name__, self._model.tags, tag_names)
+            validate_tags(ObjectType.chart, self._model.tags, tag_ids)
         except ValidationError as ex:
             exceptions.append(ex)
 

superset/commands/dashboard/update.py

@@ -77,7 +77,7 @@ def validate(self) -> None:
         owner_ids: Optional[list[int]] = self._properties.get("owners")
         roles_ids: Optional[list[int]] = self._properties.get("roles")
         slug: Optional[str] = self._properties.get("slug")
-        tag_names: Optional[list[str]] = self._properties.get("tags")
+        tag_ids: Optional[list[int]] = self._properties.get("tags")
 
         # Validate/populate model exists
         self._model = DashboardDAO.find_by_id(self._model_id)
@@ -105,7 +105,7 @@ def validate(self) -> None:
 
         # validate tags
         try:
-            validate_tags(self._model.__class__.__name__, self._model.tags, tag_names)
+            validate_tags(ObjectType.dashboard, self._model.tags, tag_ids)
         except ValidationError as ex:
             exceptions.append(ex)
 

superset/commands/utils.py

@@ -110,9 +110,9 @@ def get_datasource_by_id(datasource_id: int, datasource_type: str) -> BaseDataso
 
 
 def validate_tags(
-    object_type: str,
+    object_type: ObjectType,
     current_tags: list[Tag],
-    new_tags: Optional[list[str]],
+    new_tag_ids: Optional[list[int]],
 ) -> None:
     """
     Helper function for update commands, to validate the tags list. Users
@@ -121,38 +121,33 @@ def validate_tags(
     only allowed to manage existing existing tags' associations with the object.
 
     :param current_tags: list of current tags
-    :param new_tags: list of tags specified in the update payload
+    :param new_tag_ids: list of tags specified in the update payload
     """
 
-    # Classname override
-    class_overrides = {"Slice": "Chart"}
-    object_type = class_overrides.get(object_type, object_type)
-
     # `tags` not part of the update payload
-    if new_tags is None:
-        return
-
-    # User has perm to create new tags
-    if security_manager.can_access("can_write", "Tag"):
+    if new_tag_ids is None:
         return
 
     # No changes in the list
-    custom_tags = [tag.name for tag in current_tags if tag.type == TagType.custom]
-    if Counter(custom_tags) == Counter(new_tags):
+    current_custom_tags = [tag.id for tag in current_tags if tag.type == TagType.custom]
+    if Counter(current_custom_tags) == Counter(new_tag_ids):
         return
 
     # No perm to tags assets
-    if not security_manager.can_access("can_tag", object_type):
+    if not (
+        security_manager.can_access("can_write", "Tag")
+        or security_manager.can_access("can_tag", object_type.name.capitalize())
+    ):
         validation_error = (
-            f"You do not have permission to manage tags on {object_type.lower()}s"
+            f"You do not have permission to manage tags on {object_type.name}s"
         )
         raise TagForbiddenError(validation_error)
 
     # Validate if new tags already exist
-    additional_tags = [tag for tag in new_tags if tag not in custom_tags]
-    for tag in additional_tags:
-        if not TagDAO.find_by_name(tag):
-            validation_error = f"Tag not found: {tag}"
+    additional_tags = [tag for tag in new_tag_ids if tag not in current_custom_tags]
+    for tag_id in additional_tags:
+        if not TagDAO.find_by_id(tag_id):
+            validation_error = f"Tag ID {tag_id} not found"
             raise TagNotFoundValidationError(validation_error)
 
     return
@@ -162,24 +157,31 @@ def update_tags(
     object_type: ObjectType,
     object_id: int,
     current_tags: list[Tag],
-    new_tags: list[str],
+    new_tag_ids: list[int],
 ) -> None:
     """
     Helper function for update commands, to update the tag relationship.
 
     :param object_id: The object (dashboard, chart, etc) ID
     :param object_type: The object type
     :param current_tags: list of current tags
-    :param new_tags: list of tags specified in the update payload
+    :param new_tag_ids: list of tags specified in the update payload
     """
 
-    current_custom_tags = [
-        tag.name for tag in current_tags if tag.type == TagType.custom
+    current_custom_tags = [tag for tag in current_tags if tag.type == TagType.custom]
+    current_custom_tag_ids = [
+        tag.id for tag in current_tags if tag.type == TagType.custom
     ]
-    tags_to_delete = [tag for tag in current_custom_tags if tag not in new_tags]
-    tags_to_add = [tag for tag in new_tags if tag not in current_custom_tags]
 
+    tags_to_delete = [tag for tag in current_custom_tags if tag.id not in new_tag_ids]
     for tag in tags_to_delete:
-        TagDAO.delete_tagged_object(object_type, object_id, tag)
-    if tags_to_add:
-        TagDAO.create_custom_tagged_objects(object_type, object_id, tags_to_add)
+        TagDAO.delete_tagged_object(object_type, object_id, tag.name)
+
+    tag_ids_to_add = [
+        tag_id for tag_id in new_tag_ids if tag_id not in current_custom_tag_ids
+    ]
+    if tag_ids_to_add:
+        tags_to_add = TagDAO.find_by_ids(tag_ids_to_add)
+        TagDAO.create_custom_tagged_objects(
+            object_type, object_id, [tag.name for tag in tags_to_add]
+        )

superset/dashboards/schemas.py

@@ -371,7 +371,7 @@ class DashboardPutSchema(BaseDashboardSchema):
     is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
     external_url = fields.String(allow_none=True)
     tags = fields.List(
-        fields.String(metadata={"description": tags_description}, allow_none=True)
+        fields.Integer(metadata={"description": tags_description}, allow_none=True)
     )