Skip to content

Conversation

@TheJokersThief
Copy link
Contributor

I submitted an issue/feature suggestion (#1181) a while ago asking about interest in adding some slack functionality.

This pull request:

  • Creates a new method to transform an event into a Message
  • Adds ability to update/delete messages
  • Adds ability to send multiple cards at once

These are things we've found useful at HostedGraphite for our ChatOps bot.

I hope this is okay but if it doesn't feel like it would fit in with the bot overall, I totally understand 😊 I've really enjoyed working with the codebase and it's made development much easier for us so I was hoping to contribute some of our home-grown improvements upstream.

Sidenote: I tried running the tox tests on OSX but they ran into an error about being unable to create a new thread and I assume codestyle tests that fail but aren't related to this PR are okay.

@ghost ghost added the in progress label Apr 21, 2018
@TheJokersThief TheJokersThief force-pushed the add-some-slack-functionality branch from e1a0cbe to ac078e2 Compare May 13, 2018 07:58
@TheJokersThief TheJokersThief force-pushed the add-some-slack-functionality branch from ac078e2 to 1714efa Compare May 13, 2018 16:39
Copy link
Contributor

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

I like this a lot, @gbin or @zoni can you review as well?

Copy link
Member

@gbin gbin left a comment

Choose a reason for hiding this comment

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

I would split it this PR. Also, if we need to add a feature that can be supported by other backens, we need to add it in the API of Errbot and not only on the Slack API.

@gbin
Copy link
Member

gbin commented May 16, 2018

So I like the idea and I would implement it fully in Errbot then:

  1. multiple cards can be added to Errbot API. 1 PR for the plugin API, 1 PR for generic support calling the old API, 1 PR implementing it effeciently for Slack
  2. isn't it related to multiple messages? maybe we need to add a mirroring API for self.send. Probably 1 PR for the API + default looping support calling the 1 message API + 1 other PR implementing that for Slack in a more efficient way if possible.
  3. delete, 1 PR for the plugin API with an unsupported exception. 1 PR implementing that for Slack.
  4. update, 1 PR for the plugin API with an unsupported exception. 1 PR implementing that for Slack.

The last thing: Creates a new method to transform an event into a Message <- is this just an internal thing?

@GenPage
Copy link
Contributor

GenPage commented May 16, 2018

That's reasonable. I can help assist, especially with the API portions

  1. Someone already added multiple card support, which is just a for loop inside the send_card message. Which approach do we prefer? Support breaking message body size into parts for Slack attachments. #1118

  2. Can you elaborate mirroring API for self.send? How would this relate to multiple cards?

3 & 4. Sounds good 👍

As for the new method for Slack event message processing, This is internal Slack refactor from what I can tell so that the new method is available to the update/delete methods to call on return.

Copy link
Member

@gbin gbin left a comment

Choose a reason for hiding this comment

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

Can you split this PR into independent ones for each feature? Thank you.

@GenPage
Copy link
Contributor

GenPage commented Oct 19, 2018

@TheJokersThief Are you able to split the work into multiple PRs?

@TheJokersThief
Copy link
Contributor Author

TheJokersThief commented Nov 24, 2018

Hey! Sorry, I completely missed the notification for this.

I can split the work into multiple PRs as I get time, that's no problem :)

With regards your point 1 in the previous message, @GenPage, one of the benefits of slack cards is that they can stack in a single message. For us, we use them to track the progress of a long-running set of tasks. Having all the cards in one message keeps things clean and makes it easy to reference later.

@TheJokersThief
Copy link
Contributor Author

I've added 4 PRs to get some of the easier recommended PRs out of the way :)

@sijis
Copy link
Contributor

sijis commented Jun 18, 2019

I'm closing this as its been broken up into multiple PRs.

@sijis sijis closed this Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants