Twilio receive text message agent #1418
Conversation
|
Cool! Do you think it would make sense to merge this into the existing Twilio Agent? I'm generally a fan of fewer agents. |
|
If this were part of the existing Twilio agent it would need a |
albertsun
force-pushed
the
feature/twilio-receive-texts
branch
from
2a3073b
to
35779ee
Compare
|
I generally prefer a couple simple agents over one complicated one that does a lot, especially when the separation can be made clear in the agent name/description. While writing the LocalFile and CsvAgent I was unsure and went with the We could introduce a general |
|
If the goal is to have a more navigable number of agents on the New Agent screen, what about grouping agents into related modules like |
albertsun
force-pushed
the
feature/twilio-receive-texts
branch
from
4ee13dc
to
0a12b86
Compare
|
That seems like a good idea @albertsun. I think I'm still biased towards just having a TwilioAgent that can do a couple things (e.g., one agent per integration instead of one agent per integration behavior), but I don't feel strongly. |
albertsun
force-pushed
the
feature/twilio-receive-texts
branch
from
0a12b86
to
b82fb1c
Compare
| if method(:receive_web_request).arity == 1 | ||
| handled_request = receive_web_request(request) | ||
| else | ||
| handled_request = receive_web_request(params, request.method_symbol.to_s, request.format) |
There was a problem hiding this comment.
Do we need request.format.to_s to match what was in the controller?
don't need securerandom don't assume ENV['DOMAIN'] exists update TwilioReceiveTextAgent to use new receive_web_request method signature
albertsun
force-pushed
the
feature/twilio-receive-texts
branch
from
b82fb1c
to
29d4691
Compare
|
Now that #1415 is merged I rebased this on master so there's just one commit. As for one agent that does many things vs many agents doing one thing each, I took a look through the @dsander 's CsvAgent and LocalFileAgent and the code around "mode" seems a little more complicated than I like. It's almost like writing two agents in one with conditionals and feels like it'll have harder to use tests and documentation than having simpler agents. Maybe if some of the Either way, wondering if we can just merge this agent, and discuss that further in a broader issue |
| #{post_url} | ||
| ``` | ||
|
|
||
| #{'The placeholder symbols above will be replaced by their values once the agent is saved.' unless id} |
|
Agreed, I don't want to block this on the larger discussion about |
|
Comment removed. Hope this is ready to merge now. (As for mode and one vs many discussion, perhaps that is also part of the pulling agents into gems discussion — there could be a huginn-twilio gem that has multiple |
|
Nice work! |
To be merged after #1415
An agent for receiving text messages from Twilio. It requires somewhat different logic from the existing webhook agent.