Skip to content

Alec segment connector #2726

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

alecbatch
Copy link

@alecbatch alecbatch commented Feb 5, 2025

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
  • 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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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

@joe-ayoub-segment
Copy link
Contributor

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

@joe-ayoub-segment joe-ayoub-segment self-assigned this May 2, 2025
@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented May 2, 2025

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
Copy link
Contributor

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
Copy link
Contributor

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

@alecbatch
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants