Saving sms conversations in CrmActivity #98

VSydor · 2022-12-07T12:24:19Z

No description provided.

brmeyer · 2022-12-13T05:44:24Z

src/main/java/com/impactupgrade/nucleus/service/segment/SfdcCrmService.java

@@ -255,6 +296,9 @@ public EnvironmentConfig.CRMFieldDefinitions getFieldDefinitions() {
  }

  protected void setTaskFields(SObject task, CrmTask crmTask) {
+    if (crmTask != null) {


@VSydor Would this ever be null? And if so, there are uses of crmTask below that fall outside of this check.

Should have been crmTask.id check. Will update this line.

src/main/java/com/impactupgrade/nucleus/service/segment/SfdcCrmService.java

brmeyer · 2022-12-13T05:53:51Z

src/main/java/com/impactupgrade/nucleus/controller/TwilioController.java

+  @Path("/webhook/message/onMessageAdded") // TODO: replace with the correct url
+  @POST
+  @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
+  @Produces(MediaType.APPLICATION_XML) //TODO: define what type to return


Twilio webhooks expect APPLICATION_JSON I believe, although this method should simply return a 200 response.

Will update to json and ok empty response

src/main/java/com/impactupgrade/nucleus/controller/TwilioController.java

brmeyer · 2022-12-13T05:54:19Z

src/main/java/com/impactupgrade/nucleus/controller/TwilioController.java

+    addMessage(crmTask, messageSid);
+    upsertCrmTask(env, crmTask);
+
+    return Response.ok().build(); // TODO: what to return?


This is fine as-is

src/main/java/com/impactupgrade/nucleus/controller/TwilioController.java

brmeyer · 2022-12-31T18:26:40Z

src/main/java/com/impactupgrade/nucleus/environment/EnvironmentConfig.java

@@ -206,6 +206,7 @@ public static class Salesforce extends Platform {
    public SalesforceCustomFields customQueryFields = new SalesforceCustomFields();

    public String defaultCampaignId = "";
+    public String defaultTaskAssignee = "";


No longer needed and we're unlikely to need this in practice -- remove?

brmeyer · 2022-12-31T18:29:05Z

src/main/java/com/impactupgrade/nucleus/service/segment/SfdcCrmService.java

@@ -258,11 +308,22 @@ public EnvironmentConfig.CRMFieldDefinitions getFieldDefinitions() {
  }

  protected void setTaskFields(SObject task, CrmTask crmTask) {
+    if (crmTask.id != null) {
+      task.setField("Id", crmTask.id);


Don't set the ID in this method. Instead, have updateTask set it prior to calling setTaskFields. It's a precedent we've set in these services in general, separating the core logic from the specific fields.

brmeyer · 2023-06-25T03:12:24Z

src/main/java/com/impactupgrade/nucleus/service/segment/CrmService.java

@@ -221,6 +221,8 @@ default void batchFlush() throws Exception {
  // MISC
  //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
  String insertTask(CrmTask crmTask) throws Exception;
+  String updateTask(CrmTask crmTask) throws Exception;
+  Optional<CrmTask> getTaskBySubject(String subject) throws Exception;


@VSydor Picky, but precedent setting:

Could we name this something like getTaskByExtRef ("external reference")? It's essentially a way to look up a subject based on an external notion, like a Twilio Conversation ID?

Sure - will rename the method

brmeyer · 2023-06-25T03:14:14Z

src/main/java/com/impactupgrade/nucleus/client/SfdcClient.java

@@ -774,6 +774,13 @@ public Optional<SObject> getUserByEmail(String email, String... extraFields) thr
    return querySingle(query);
  }

+  protected static final String TASK_FIELDS = "Id, WhoId, OwnerId, Subject, description, status, priority, activityDate";
+
+  public Optional<SObject> getTaskBySubject(String subject) throws ConnectionException, InterruptedException {


In EnvironmentConfig, could we instead define a new fieldname that must be set, allowing activities to have a external reference ID (ex: Twilio Conference ID) set as a custom field? Similar to what we do with Stripe charges and the paymentGatewayTransactionId field name definition.

Just concerned about tying this to Subject long-term.

brmeyer · 2023-06-25T03:14:59Z

src/main/java/com/impactupgrade/nucleus/controller/MBTController.java

+      @Context HttpServletRequest request
+  ) throws Exception {
+    Environment env = envFactory.init(request);
+    SecurityUtil.verifyApiKey(env); //TODO: not needed?


Correct, kill this line. No auth for inbound, write-only webhook APIs.

brmeyer · 2023-06-25T03:16:02Z

src/main/java/com/impactupgrade/nucleus/controller/MBTController.java

+        SMS, 
+        inboundMessageWebhookData.message,
+        inboundMessageWebhookData.externalReferenceId,
+        null); // TODO: use customParams to contain conversation id?


Clarify this one for me?

In MBT webhook we don't have conversation id (as we have in Twilio webhook)
So need to get it from somewhere (mb it's possible to configure custom params on MBT side?)

brmeyer · 2023-06-25T03:16:34Z

src/main/java/com/impactupgrade/nucleus/controller/MBTController.java

+      @Context HttpServletRequest request
+  ) throws Exception {
+    Environment env = envFactory.init(request);
+    SecurityUtil.verifyApiKey(env); //TODO: not needed? 


brmeyer · 2023-06-25T03:19:19Z

src/main/java/com/impactupgrade/nucleus/controller/MailchimpController.java

+    Environment env = envFactory.init(request);
+    //SecurityUtil.verifyApiKey(env); //TODO: not needed?
+
+    log.info("Mailchimp message event batch received. Batch size: {}.", webhookPayload.events.size());


Oy! This is actually pretty refreshing! Cool to see they're batching them...

brmeyer · 2023-06-25T03:20:07Z

src/main/java/com/impactupgrade/nucleus/controller/MailchimpController.java


 @Path("/mailchimp")
 public class MailchimpController {

  private static final Logger log = LogManager.getLogger(MailchimpController.class);
+
+  private static final Set<String> SUPPORTED_EVENT_TYPES = Set.of( //TODO: move to env config?


Eh, maybe at some point, but the types are strongly tied to chunks of code, so making this config-driven would be tough. Same issue in StripeController

brmeyer · 2023-06-25T03:21:03Z

src/main/java/com/impactupgrade/nucleus/controller/MailchimpController.java

+    if (event == null) {
+      return;
+    }
+    if (!SUPPORTED_EVENT_TYPES.contains(event.eventType)) {


Is the array of types and this if block really necessary? Since you're explicitly looking for "send" and then doing a "skipping event type..." log for everything else, this whole block technically isn't needed? Same pattern as StripeController

Will follow the pattern from Stripe controller

brmeyer · 2023-06-25T03:21:46Z

src/main/java/com/impactupgrade/nucleus/controller/MailchimpController.java

+          EMAIL, 
+          event.message.subject,
+          event.message.id,
+          event.message.email);


Just to confirm, this will store the actual email body, right? Desirable as a paper trail...

"event.message.email" is used here as a conversation id key (same way as explicit conversation sid in Twilio Controller) to have all the messages sent to the same email address in 1 CrmActivity. Can be too much for 1 activity so we can introduce some additional value (like date for example to contain messages for specific time window)
event.message.email = email address of the recipient

brmeyer · 2023-06-25T03:23:05Z

src/main/java/com/impactupgrade/nucleus/model/CrmTask.java

@@ -16,16 +18,25 @@ public CrmTask() {
  }

  public CrmTask(String targetId, String assignTo, String subject, String description,


Picky, and I'm sorry if we went back and forth on this:

Should we call this something like CrmActivity? Logically, these are all activities. Only in SFDC land are they modeled as "tasks" (which I still think was a weird design choice on their part)

Sure - will rename to CrmActivity.

…roller for manual testing Twilio Conversation webhook added (onMessageAdded) Code review comments pulled logic into MessagingService, convering the controller endpoint into a generic conversations webhook Removed unsuned task config property; Setting task id in update method Added MBT inbound/message-status webhooks; Saving inbound messages into crm activity (tasks) Added basic processing of Mailchimp message webhooks Code review comments Using combinations of webhook data fields as a converstion key for MBT and Mailchimp minor fixes moved the logic to a new ActivityService

VSydor requested a review from brmeyer December 7, 2022 12:24

VSydor self-assigned this Dec 7, 2022

VSydor mentioned this pull request Dec 7, 2022

Saving sms conversations into crm task #72

Closed

VSydor force-pushed the feature/save-sms-conversation-in-crm-task branch 5 times, most recently from 2dc2edf to 11fa2b6 Compare December 12, 2022 10:33

brmeyer reviewed Dec 13, 2022

View reviewed changes

brmeyer force-pushed the feature/save-sms-conversation-in-crm-task branch from ddddff8 to 58ec129 Compare December 18, 2022 17:41

brmeyer reviewed Dec 31, 2022

View reviewed changes

VSydor force-pushed the feature/save-sms-conversation-in-crm-task branch from 29c5e8f to 4742f1f Compare May 31, 2023 09:20

VSydor force-pushed the feature/save-sms-conversation-in-crm-task branch 5 times, most recently from e7fbd7e to 1d17e0e Compare June 21, 2023 14:32

VSydor requested a review from brmeyer June 21, 2023 14:35

brmeyer reviewed Jun 25, 2023

View reviewed changes

VSydor changed the title ~~Saving sms conversations into crm task #2 (Removed CrmActivity)~~ Saving sms conversations into crm task Jun 27, 2023

VSydor force-pushed the feature/save-sms-conversation-in-crm-task branch 4 times, most recently from 11dc384 to 8c31782 Compare June 29, 2023 20:42

VSydor changed the title ~~Saving sms conversations into crm task~~ Saving sms conversations in CrmActivity Jun 29, 2023

VSydor requested a review from brmeyer June 29, 2023 20:45

brmeyer force-pushed the feature/save-sms-conversation-in-crm-task branch 2 times, most recently from d4cc3cc to 7331b7e Compare September 4, 2023 20:38

brmeyer force-pushed the feature/save-sms-conversation-in-crm-task branch from 61bd24c to b1005af Compare December 21, 2023 00:26

brmeyer force-pushed the feature/save-sms-conversation-in-crm-task branch from b1005af to a9127ce Compare December 21, 2023 00:43

brmeyer merged commit 977ddf0 into master Dec 21, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saving sms conversations in CrmActivity #98

Saving sms conversations in CrmActivity #98

VSydor commented Dec 7, 2022 •

edited

Loading

brmeyer Dec 13, 2022

VSydor Dec 13, 2022

brmeyer Dec 13, 2022

VSydor Dec 13, 2022

brmeyer Dec 13, 2022

brmeyer Dec 31, 2022

brmeyer Dec 31, 2022

brmeyer Jun 25, 2023

VSydor Jun 27, 2023

brmeyer Jun 25, 2023

VSydor Jun 27, 2023

brmeyer Jun 25, 2023

brmeyer Jun 25, 2023

VSydor Jun 27, 2023

brmeyer Jun 25, 2023

brmeyer Jun 25, 2023

brmeyer Jun 25, 2023

brmeyer Jun 25, 2023

VSydor Jun 27, 2023

brmeyer Jun 25, 2023

VSydor Jun 27, 2023 •

edited

Loading

brmeyer Jun 25, 2023

VSydor Jun 27, 2023

		@@ -16,16 +18,25 @@ public CrmTask() {
		}

		public CrmTask(String targetId, String assignTo, String subject, String description,

Saving sms conversations in CrmActivity #98

Saving sms conversations in CrmActivity #98

Conversation

VSydor commented Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSydor Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSydor commented Dec 7, 2022 •

edited

Loading

VSydor Jun 27, 2023 •

edited

Loading