Alec segment connector

[alecbatch]

A summary of your pull request, including the what change you're making and why.

Testing

Include any additional information about the testing you have completed to ensure your changes behave as expected. For a speedy review, please check any of the tasks you completed below during your testing.

• [ ] Added unit tests for new functionality
• [x] Tested end-to-end using the local server
• [ ] [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
• [ ] [Segmenters] Tested in the staging environment
• [ ] [Segmenters] [If applicable for this change] Tested for regression with Hadron.

[marinhero]

Hi @alecbatch. Is this ready for review? I see a bunch of code being commented out and I'm unsure if you intended to open a draft instead. LMK and I'll take a look. Thanks!

[alecbatch]

Hi @marinhero I've just come back from holiday, thanks for your message. Yes, the code is ready for review, there's no need to take into account the bunch commented.

[alecbatch]

Hello @marinhero, did you get a chance to have a look?

[joe-ayoub-segment]

hi @alecbatch I'll be reviewing this PR in the next 24h. FYI @marinhero .

[joe-ayoub-segment]

Hi @alecbatch, I intend to do a full review of this PR tomorrow - however I just did a quick look over it and noticed these issues:

1. the package.json file should not be changed
2. You should only change code within the folder for your new Integration. No need to change code here: packages/destination-subscriptions/src/types.ts
3. Please don't add files which contain only comments: packages/destination-actions/src/destinations/batch/trackEvent/tests/index.test.ts
4. Please don't define any field names starting with the $ character. .e.g $language
5. I think there might be a mistake in the way you are handling batches. The perform() function calls this buildProfileJsonWithEvents function to prepare the payload. However the performBatch() function does not. Why is this?
6. Please remove any console.logs
7. There's no need to change the yarn.lock file.

Also can you tell me if you have registered Batch as a Segment Partner yet?

Kind regards, Joe

[joe-ayoub-segment]

hi @alecbatch - Would you be available to catch up on a call to run through this PR with me? Here's my Calendly link:

[joe-ayoub-segment]

hi again @alecbatch please let me know if you'd like to proceed with this PR or not.

[alecbatch]

Hi @joe-ayoub-segment, just back from vacation – apologies for the delay, and thank you so much for your feedback and availability!

I suggest you take the time to rectify the points and, if necessary, we can organise a review call together if that suits you? I'll get back to you by Thursday at the latest.

[joe-ayoub-segment]

Thanks for getting back to me @alecbatch . I take it you meant that you'll take a look at the comments I left, and after that you might schedule a call. If so I look forward to catching up!

[joe-ayoub-segment]

Hi @alecbatch - just following up on this. Would you like some assistance? If so please schedule a call with me here https://calendly.com/joe_ayoub

[alecbatch]

hello @joe-ayoub-segment, thanks for your reminders ! Apologies for the delay, time off has stacked up on my end and I’ve been handling this topic solo. I’ve now made the necessary corrections, I’ll be unvailable again next week, I’m available this week or from June 23rd onward.

[joe-ayoub-segment]

Hi @alecbatch , I reviewed the PR, but there were too many design issues with it so I ended up creating another branch and refactoring the code.

You can see the updated code here: https://github.com/segmentio/action-destinations/pull/2995

I think you only need 1 Action for this Integration. It can sync both profile data and events at the same time. I also added batching support, set to a default of 200 events in size.

Please take a look, copy the code over to your branch, and make any necessary changes.

I removed the timetsamp field as I couldn't see it in the Batch API. https://doc.batch.com/developer/api/cep/profiles/update

Another thing that will need to be done is to add some unit tests.

Best regards, Joe

[joe-ayoub-segment]

Hi @alecbatch just following up on this. Have you had a look at the internal PR I created here? https://github.com/segmentio/action-destinations/pull/2995

[alecbatch]

Hello @joe-ayoub-segment, thanks for the design corrections. I rectified the code by correcting the elements cited on PR #2995 and pushed here.

[joe-ayoub-segment]

Hi @alecbatch. I reviewed the PR - seems fine apart from the unit test.

I can't really comment on if the performBatch() and perform() functions work. Did you send some test data through them?

Regarding the unit test: the unit test is supposed to test the JSON you expect to send and the response you expect to receive. You need to use nock to prevent any actual HTTP requests from being made.

I did make a few changes which I pushed to this PR. https://github.com/segmentio/action-destinations/pull/2995

You can probably just copy the folder from that PR over yours, then fix up the unit test.

There's plenty of examples in the other Destinations which you can reference. If you have trouble please schedule a call and we'll get it done.

Best regards, Joe

[joe-ayoub-segment]

hi @alecbatch Are you interested in completing this Integration? I think we only have some minor items to wrap up before I deploy it. If you'd like to do this please schedule a call https://calendly.com/joe_ayoub

Otherwise I'll close this PR in the next few days for housekeeping reasons. Kind regards, Joe

[joe-ayoub-segment]

Moved to https://github.com/segmentio/action-destinations/pull/3127