-
Notifications
You must be signed in to change notification settings - Fork 0
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
Messaging abstraction updates #134
base: master
Are you sure you want to change the base?
Conversation
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.
@Levi-Lehman Headed the right direction!
src/main/java/com/impactupgrade/nucleus/controller/MessagingController.java
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/controller/MessagingController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/controller/MessagingController.java
Outdated
Show resolved
Hide resolved
}; | ||
new Thread(thread).start(); | ||
|
||
//TODO removed OG Twilio response, revisit after sorting out MB |
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.
We might need to shake up the whole flow to make this happen.
- Remove the use of a new thread in this whole endpoint.
- Break apart the processing code in MessagingService. Spin up a new thread there instead.
- Allow a vender-specific response to be created and bubbled up from the underlying segment service. Something like TwilioService (whatever we call it) -> MessagingService -> MessagingController.
However, you receive bonus points for the use of "OG".
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.
Started implementing this and am getting a bit lost in the separation of everything. It seems to me like there is nothing super Twilio-specific besides how the response is being generated. Do we just need something like
textingService.buildResponse()
for now? Could be misunderstanding
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.
I assume we will have more complex needs as we get deeper into MessageBird flows though
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.
A buildResponse()
would work for the default "yup I got it" responses, but we might need something else more specific for certain circumstances.
In Twilio land, webhooks out to external services are expected to respond back with TwiML, which is just an XML schema used to tell Twilio what to do next. So in some situations, we might return a reply with message "foobar"
. It's more useful for things like decision trees being handled by external services. But in our cases, often we're simply returning back empty TwiML since there's nothing extra to do.
That part is definitely Twilio-specific -- not sure if we'll need something similar in MB land.
env.endJobLog(jobName); | ||
} | ||
} | ||
//TODO removed OG Twilio response, revisit after sorting out MB |
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.
Same as above
src/main/java/com/impactupgrade/nucleus/controller/MessagingController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/environment/EnvironmentConfig.java
Outdated
Show resolved
Hide resolved
@@ -164,6 +166,10 @@ public CrmService messagingCrmService() { | |||
return crmService(getConfig().crmMessaging); | |||
} | |||
|
|||
public TextingService textingService(){ | |||
return segmentService(getConfig().textingService, TextingService.class); |
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.
Not sold on TextingService as the name, since it's not really different than MessagingService, but I don't have any better ideas either. Let's circle back on it...
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.
Yeah wasn't sold on it myself either... words are hard
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.
What if we simply used MessagingLogicService for logic and MessagingSegmentService for segment?
No description provided.