korbit-ai[bot] commented on code in PR #31631:
URL: https://github.com/apache/superset/pull/31631#discussion_r1898451547
##########
superset-frontend/src/features/tags/BulkTagModal.tsx:
##########
@@ -68,23 +68,24 @@
},
})
.then(({ json = {} }) => {
- const skipped = json.result.objects_skipped;
- const tagged = json.result.objects_tagged;
- if (skipped.length > 0) {
- addSuccessToast(
+ const skipped = json.result.objects_skipped || [];
+ const tagged = json.result.objects_tagged || [];
+ if (tagged.length > 0) {
+ addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
+ }
+ if (skipped.length > 0 && tagged.length === 0) {
+ addDangerToast(
t(
'%s items could not be tagged because you donβt have edit
permissions to all selected objects.',
skipped.length,
resourceName,
),
);
}
- addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
})
.catch(err => {
addDangerToast(t('Failed to tag items'));
});
Review Comment:
### Improve error handling for bulk tag creation API. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The current error handling for the bulk tag creation API call can be
improved. Right now it just shows a generic 'Failed to tag items' message on
any error. Instead, consider logging the actual error details returned by the
API for easier debugging. Also, analyze the error response and show a more
specific, informative message to the user about what might have gone wrong,
like invalid tag names, permission issues, etc. This will help users better
understand the problem.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help
Korbit improve your reviews.
<!--- korbi internal id:4b0e251f-994e-49f2-a4a3-20e1458f7940 -->
##########
superset/commands/tag/create.py:
##########
@@ -91,20 +91,15 @@ def validate(self) -> None:
for obj_type, obj_id in objects_to_tag:
object_type = to_object_type(obj_type)
- # Validate object type
- 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:
- if 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
- self._skipped_tagged_objects.add((obj_type, obj_id))
+ if not object_type:
+ exceptions.append(TagInvalidError(f"invalid object type
{object_type}"))
+ continue
+
+ try:
+ if model := to_object_model(object_type, obj_id):
+ security_manager.raise_for_ownership(model)
+ except SupersetSecurityException:
+ self._skipped_tagged_objects.add((obj_type, obj_id))
Review Comment:
### Improper handling of SupersetSecurityException. <sub></sub>
<details>
<summary>Tell me more</summary>
​
The code is catching a SupersetSecurityException and simply adding the
object to a skipped list without properly handling the exception. Ignoring
security exceptions can potentially lead to unauthorized access. Consider
logging the exception, notifying the user about the lack of access, and
evaluating if execution should be halted in such cases. Skipping objects due to
security exceptions should be done cautiously with proper logging.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help
Korbit improve your reviews.
<!--- korbi internal id:6e289e9f-bcf4-42e2-92cc-9c31e44d81c6 -->
##########
superset-frontend/src/features/tags/BulkTagModal.tsx:
##########
@@ -68,23 +68,24 @@
},
})
.then(({ json = {} }) => {
- const skipped = json.result.objects_skipped;
- const tagged = json.result.objects_tagged;
- if (skipped.length > 0) {
- addSuccessToast(
+ const skipped = json.result.objects_skipped || [];
+ const tagged = json.result.objects_tagged || [];
+ if (tagged.length > 0) {
+ addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
+ }
+ if (skipped.length > 0 && tagged.length === 0) {
+ addDangerToast(
t(
'%s items could not be tagged because you donβt have edit
permissions to all selected objects.',
skipped.length,
resourceName,
),
);
}
- addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
})
.catch(err => {
addDangerToast(t('Failed to tag items'));
});
Review Comment:
### Lost Error Context in Catch Block <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The catch block discards the error information (err) and only shows a
generic error message.
###### Why this matters
Without logging or displaying the specific error details, debugging issues
becomes more difficult and users receive less helpful error feedback.
###### Suggested change
.catch(err => {
console.error('Bulk tagging failed:', err);
addDangerToast(t('Failed to tag items: %s', err.message || 'Unknown
error'));
});
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help
Korbit improve your reviews.
<!--- korbi internal id:5622cf95-1663-4af9-b670-2dc1bc95b496 -->
##########
superset-frontend/src/features/tags/BulkTagModal.tsx:
##########
@@ -68,23 +68,24 @@ const BulkTagModal: FC<BulkTagModalProps> = ({
},
})
.then(({ json = {} }) => {
- const skipped = json.result.objects_skipped;
- const tagged = json.result.objects_tagged;
- if (skipped.length > 0) {
- addSuccessToast(
+ const skipped = json.result.objects_skipped || [];
+ const tagged = json.result.objects_tagged || [];
+ if (tagged.length > 0) {
+ addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
+ }
+ if (skipped.length > 0 && tagged.length === 0) {
Review Comment:
### Incomplete Error Notification for Skipped Items <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message for skipped items is only shown when no items were
successfully tagged. Users should be notified about skipped items even when
some items were successfully tagged.
###### Why this matters
Users might miss important information about permissions issues with some
items, leading to confusion about why not all selected items were tagged.
###### Suggested change
Remove the tagged.length === 0 condition:
if (skipped.length > 0) {
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help
Korbit improve your reviews.
<!--- korbi internal id:bf2bc837-31ef-4feb-904f-d6ad7efb9ad0 -->
##########
superset-frontend/src/features/tags/BulkTagModal.tsx:
##########
@@ -68,23 +68,24 @@
},
})
.then(({ json = {} }) => {
- const skipped = json.result.objects_skipped;
- const tagged = json.result.objects_tagged;
- if (skipped.length > 0) {
- addSuccessToast(
+ const skipped = json.result.objects_skipped || [];
+ const tagged = json.result.objects_tagged || [];
+ if (tagged.length > 0) {
+ addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
+ }
+ if (skipped.length > 0 && tagged.length === 0) {
+ addDangerToast(
t(
'%s items could not be tagged because you donβt have edit
permissions to all selected objects.',
skipped.length,
resourceName,
),
);
}
- addSuccessToast(t('Tagged %s %ss', tagged.length, resourceName));
})
.catch(err => {
addDangerToast(t('Failed to tag items'));
});
Review Comment:
### Generic Error Message in Catch Block <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handler doesn't utilize the error information from the caught
exception, showing only a generic message.
###### Why this matters
Users won't know the specific reason for the tagging failure, making it
difficult to address the underlying issue.
###### Suggested change
.catch(err => {
addDangerToast(t('Failed to tag items: %s', err.message || 'Unknown
error'));
});
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a π or π to help
Korbit improve your reviews.
<!--- korbi internal id:d218721a-61c8-4fd5-aa85-041bfa5e182d -->
--
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]
