Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ele 4067 slack messaging integration #1822

Merged
merged 14 commits into from
Feb 19, 2025

Conversation

ofek1weiss
Copy link
Contributor

@ofek1weiss ofek1weiss commented Feb 16, 2025

null

Copy link

linear bot commented Feb 16, 2025

Copy link
Contributor

👋 @ofek1weiss
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@@ -70,7 +80,7 @@ class DividerBlock(BaseBlock):

class LineBlock(BaseBlock):
type: Literal["line"] = "line"
inlines: List[InlineBlock]
inlines: Sequence[InlineBlock]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to sequence for better typing

@@ -162,12 +183,6 @@ def _add_expandable_block(self, block: ExpandableBlock) -> None:
Expandable blocks are not supported in Slack Block Kit.
However, slack automatically collapses a large section block into an expandable block.
"""
self._add_block(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the button label as a title in slack looked really bad, looks better without it

timestamp=response["ts"],
)

def _handle_send_err(self, err: SlackApiError, channel_name: str) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from this point down, mainly adapted from the existing clients, with some tweaks

@@ -70,13 +68,3 @@ def send_message(

def supports_reply(self) -> bool:
return False

def reply_to_message(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed, the base class does this

lines=[
LineBlock(
inlines=[
MentionBlock(user="user1"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added test for new inline types

"LineBlock",
]

LineBlock.update_forward_refs()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed to join a subsection of the line with a different sep (the owners with a ',')

integration=integration,
config=self.config,
metadata=metadata,
override_config_defaults=self.override_config_defaults,
)
return integration.send_message(destination=destination, body=body)

def _send_test_message(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing return types

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in _send_alert too.

ONE_SECOND = 1


class SlackWebhookMessagingIntegration(BaseMessagingIntegration[None, None]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to align the type annotation with the Teams webhook integration.
In the Teams webhook integration, the webhook acts as a destination that can be provided to "send_message," whereas here, it serves as a parameter to initiate the integration.

@ofek1weiss ofek1weiss merged commit c96bcf2 into master Feb 19, 2025
4 checks passed
@ofek1weiss ofek1weiss deleted the ele-4067-slack-messaging-integration branch February 19, 2025 11:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants