Skip to content
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

First push with CustomEvents Destination Action #2570

Merged
merged 33 commits into from
Nov 26, 2024

Conversation

wtnelso
Copy link
Contributor

@wtnelso wtnelso commented Nov 6, 2024

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.

@wtnelso wtnelso requested a review from a team as a code owner November 6, 2024 17:09
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment
Copy link
Contributor

Hi @wtnelso thanks for raising this PR!
I'll schedule some time to review.

Could you please remove the yarn version upgrade please?
The PR contains changes to the package.json and yarn.lock files, which looks like it might have been a mistake.

label: 'Phone',
description: 'Phone number of the user associated with the action. E.164 format is required. This field is required if either email or an externalIdentifier is not provided.',
type: 'string',
required: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this default mapping for phone:

default: {
        '@if': {
          exists: { '@path': '$.properties.phone' },
          then: { '@path': '$.properties.phone' },
          else: { '@path': '$.context.traits.phone' }
        }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to the file and committed.

@joe-ayoub-segment
Copy link
Contributor

hi @wtnelso I just went through the code and would like to make some changes to make the UI simpler. Do you mind granting me permission to push changes to your repo please? joe-ayoub-segment is my Github handle.

@joe-ayoub-segment
Copy link
Contributor

hi @wtnelso just following up on this. Are you still interested in having these changes deployed?

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment
Copy link
Contributor

hi @wtnelso
Would you be able to schedule a call with me so we can fix up this Destination and get it deployed please?
Here's my Calendly: https://calendly.com/joe_ayoub/

I look forward to catching up.
Joe

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@wtnelso
Copy link
Contributor Author

wtnelso commented Nov 19, 2024

Hi @joe-ayoub-segment, i added a new unit test. other than that new one, not sure there are others to add.

One more question. I used the command yarn cloud test packages/destination-actions/src/destinations/attentive and get the below and then following error. Is there anything I need to do different next time?
image
Is there something else I need to configure on my end?

@joe-ayoub-segment
Copy link
Contributor

Hi @wtnelso - which folder did you execute the command from? You should be in the repo root folder.

@wtnelso
Copy link
Contributor Author

wtnelso commented Nov 20, 2024

@joe-ayoub-segment I've tried at the attentive folder and its sub folder. packages/destination-actions/src/destinations/attentive

besides this one command, are there other changes needed for this PR? If we're good on this one, I'll begin the work on the other endpoints we want to add next

@joe-ayoub-segment
Copy link
Contributor

Hi @wtnelso - the new test doesn't work.
No probs though, just leave it. I'll fix or remove it.
Feel free to work on the other Actions.

@wtnelso
Copy link
Contributor Author

wtnelso commented Nov 20, 2024

@joe-ayoub-segment great, and thank you. please let me know if anything else is needed for this PR.

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment
Copy link
Contributor

Hi @wtnelso I've prepared this PR for deploy.
If you are going to write any more code (for the other Actions) please do them in a separate branch.

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@joe-ayoub-segment joe-ayoub-segment merged commit b1c9e56 into segmentio:main Nov 26, 2024
10 of 12 checks passed
@joe-ayoub-segment
Copy link
Contributor

We're in a deploy freeze so next deploy scheduled for 3-Dec

@wtnelso wtnelso deleted the attentive branch December 2, 2024 15:46
@joe-ayoub-segment
Copy link
Contributor

hi @wtnelso this PR has been deployed. I'll email you next steps.

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.

2 participants