-
Notifications
You must be signed in to change notification settings - Fork 1.9k
slackPagination #42509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
slackPagination #42509
Conversation
|
@content-bot start review |
|
@content-bot start review |
|
@content-bot start review |
|
Validate summary Verdict: PR can be force merged from validate perspective? ✅ |
e65fd68 to
2807c18
Compare
🤖 Content-bot Review DisclaimerThis review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause. |
content-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there, thanks for your contribution to the Slack pack. I've reviewed the changes and have a few points to discuss regarding the configuration and logic.
Please double-check the YAML file for indentation errors and ensure the HasReplies output type matches the implementation. Additionally, review the logging approach to avoid exposing sensitive raw data and verify that the version bump strategy aligns with the scope of these changes.
Great start, looking forward to the updates
@dantavori, @omerKarkKatz please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.
| conversation_name = args.get("conversation_name") | ||
| limit = arg_to_number(args.get("limit")) | ||
| from_time = args.get("from_time") | ||
| page_token = args.get("page_token") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing argument definition in YAML.
|
|
||
| raw_response = send_slack_request_sync(CLIENT, "conversations.history", http_verb="GET", body=body) | ||
| messages = raw_response.get("messages", "") | ||
| demisto.debug(f"Raw response from Slack conversations.history: {raw_response}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Warning: Avoid logging full raw response bodies containing message content.
| messages = [messages] | ||
| if not isinstance(messages, list): | ||
| raise DemistoException( | ||
| f'An error occurred while listing conversation history: {raw_response.get("error")}', res=raw_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message will be unhelpful because ok is True.
| if cursor: | ||
| results.append( | ||
| CommandResults( | ||
| outputs_prefix="SlackPageToken", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure integration YAML is updated.
| - description: | | ||
| Lower bound for conversation history (sent to Slack 'oldest' query parameter). Returns messages with time stamp ≥ this value. Accepts Unix timestamp or ISO strings (e.g., "1727448000.000200", "2025-10-12T09:00:00+03:00"). Results are returned in descending order. | ||
| name: from_time | ||
| - description: Token to retrieve the next page of results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation Error: The page_token argument is indented with 4 spaces, but it should be indented with 6 spaces to align with the other arguments in the list (e.g., from_time at line 498).
| type: string | ||
| - contextPath: Slack.Messages.HasReplies | ||
| description: Whether the message has replies. | ||
| type: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type Mismatch: The HasReplies output is defined as boolean, but the implementation returns a string ("Yes" or "No").
| - contextPath: Slack.Messages.ThreadTimeStamp | ||
| description: The thread timestamp of the message. | ||
| type: string | ||
| - contextPath: SlackPageToken.NextPageToken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Consider nesting the pagination token under the Slack context root (e.g., Slack.PageToken.NextPageToken) instead of creating a new root key SlackPageToken.
| "description": "Interact with Slack API - collect logs, send messages and notifications to your Slack team.", | ||
| "support": "xsoar", | ||
| "currentVersion": "3.5.37", | ||
| "currentVersion": "3.5.38", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version is bumped to 3.5.38, but no other files are changed in this PR. Please ensure the code changes for 'slackPagination' are included. Additionally, if the intended changes add new functionality (e.g. new pagination support), consider if a minor version bump (3.6.0) is more appropriate than a patch bump.
Contributing to Cortex XSOAR Content
Make sure to register your contribution by filling the contribution registration form
The Pull Request will be reviewed only after the contribution registration form is filled.
Status
Related Issues
CRTX-199359
Description
Must have