Threads #1312
Threads #1312
Conversation
lib/cog/chat/adapter.ex
Outdated
| @@ -139,19 +139,19 @@ defmodule Cog.Chat.Adapter do | |||
| result | |||
| end | |||
|
|
|||
| def send(provider, target, message) do | |||
| def send(provider, target, message, thread_id) do | |||
There was a problem hiding this comment.
What would you think about replacing thread_id with an options keyword list?
def send(provider, target, message, opts \\ [])
There was a problem hiding this comment.
Good idea. To keep our api surface area down it makes sense to always send our metadata field here whether or not it's empty. We can ignore it in the non-Slack adapters for now.
lib/cog/chat/message.ex
Outdated
| @@ -7,6 +7,7 @@ defmodule Cog.Chat.Message do | |||
| field :user, [object: Cog.Chat.User], required: true | |||
| field :text, :string, required: true | |||
| field :provider, :string, required: true | |||
| field :thread_id, :string, required: false | |||
There was a problem hiding this comment.
Similar to above I'm thinking we should replace :thread_id with a metadata module and field like so:
field :metadata, [object: Cog.Chat.MessageMetadata], required: false
vanstee
force-pushed
the
vanstee/threads
branch
3 times, most recently
from
9ffb538 to
b8b5cff
Compare
|
vanstee
force-pushed
the
vanstee/threads
branch
2 times, most recently
from
094680a to
482c9fc
Compare
There was a problem hiding this comment.
I like the overall look of things, but have a concern about the "metadata" implementation.
There aren't any tests, either, which gives me pause, particularly with such a new and apparently unfinished feature as threads are.
Maybe we should hold off on merging this until we have tests or threads mature a bit?
| @@ -0,0 +1,6 @@ | |||
| defmodule Cog.Chat.MessageMetadata do | |||
There was a problem hiding this comment.
Some documentation on this module in particular would be nice, emphasizing that (for now, at least) it's a bit of a grab-bag of "stuff" that only really applies to Slack, and not more broadly.
On further reflection, I wonder if we're perhaps over-architecting things by making this a Conduit-validated module, particularly given its grab-bag nature. Maybe it should just be a keyword list until a broader and more general use case becomes apparent?
vanstee
force-pushed
the
vanstee/threads
branch
from
482c9fc to
b59cb69
Compare
vanstee
force-pushed
the
vanstee/threads
branch
from
b59cb69 to
74ba414
Compare
lib/cog/chat/adapter.ex
Outdated
| case Cog.Chat.MessageMetadata.from_map(metadata) do | ||
| {:ok, metadata} -> | ||
| metadata | ||
| _error -> |
There was a problem hiding this comment.
Conversion error should be logged.
lib/cog/chat/adapter.ex
Outdated
| %Cog.Chat.MessageMetadata{} | ||
| end | ||
| _ -> | ||
| %Cog.Chat.MessageMetadata{} |
There was a problem hiding this comment.
Should log that we're ignoring non-map metadata so future us doesn't go insane trying to figure out why metadata isn't getting included :)
|
Added logging and comments about the metadata struct. |
Probably not going to end up merging this, but figured I'd open a PR for some discussion.
This PR makes Cog respond to the message it was sent with a new thread response. All we need to do is set
thread_tswith the value oftsfrom the original message. There are still some issues on Slack's end to work out, like attachments missing from responses and such. Also on our end we need to disable this when redirecting to another room since threads can only be created and responded to in a single room. Either way, this was a cool experiment and was really only a minimal amount of work.