korbit-ai[bot] commented on code in PR #32336:
URL: https://github.com/apache/superset/pull/32336#discussion_r1964413618
##########
superset/commands/report/execute.py:
##########
@@ -139,15 +139,21 @@ def update_report_schedule_slack_v2(self) -> None:
slack_recipients =
json.loads(recipient.recipient_config_json)
# we need to ensure that existing reports can also fetch
# ids from private channels
+ current_target_count =
slack_recipients["target"].split(",")
+ channels = get_channels_with_search(
+ slack_recipients["target"],
+ types=[
+ SlackChannelTypes.PRIVATE,
+ SlackChannelTypes.PUBLIC,
+ ],
+ exact_match=True,
+ )
+ if len(current_target_count) != len(channels):
+ raise UpdateFailedError("Not all channels could be
found")
Review Comment:
### Non-Specific Error Message <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message doesn't provide specifics about which channels were not
found, making it difficult to diagnose issues.
###### Why this matters
Without detailed information about missing channels, administrators would
have difficulty troubleshooting failed migrations and fixing the underlying
channel configuration issues.
###### Suggested change ∙ *Feature Preview*
Enhance error message with specific channel information:
```python
if len(current_target_count) != len(channels):
missing_channels = set(current_target_count) - {channel["name"] for
channel in channels}
raise UpdateFailedError(f"Could not find the following channels: {',
'.join(missing_channels)}")
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cb6bd7a0-ad45-4673-b4cd-1caa8c9de67b?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:6d27d7bb-96bc-41ff-b6e1-8fcf5eb25a4c -->
##########
superset/commands/report/execute.py:
##########
@@ -139,15 +139,21 @@
slack_recipients =
json.loads(recipient.recipient_config_json)
# we need to ensure that existing reports can also fetch
# ids from private channels
+ current_target_count =
slack_recipients["target"].split(",")
Review Comment:
### Unsafe String Split Operation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The split operation assumes the target string contains only comma-separated
values, but could fail if the target string contains malformed data or is empty.
###### Why this matters
This could cause the migration to fail with an unhandled exception if the
target string is malformed or empty, preventing successful migration of Slack
recipients.
###### Suggested change ∙ *Feature Preview*
Add input validation and safe handling of the target string:
```python
current_target_count = slack_recipients.get("target", "").split(",") if
slack_recipients.get("target") else []
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7f89236c-59fc-4530-b7b6-826e0e116496?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:29157ecb-3038-4c52-80e0-d5c104d6e303 -->
--
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]
