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

Responsys new Action: Send to PET + Adjustments in Send Audience as PET #2615

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

Conversation

seg-leonelsanches
Copy link
Contributor

@seg-leonelsanches seg-leonelsanches commented Nov 26, 2024

This new Action was requested by the customer after the tests we performed in their Segment workspace:

  • All actions should work limiting batches of records to 200 each, per Responsys documentation.
  • Audience Key corresponding column in Responsys PET should be restricted to 30 characters only;
  • Improving payload debug in a Segment Source, now checking for the async response if using async endpoints (in other words, calling another endpoint with a requestId to obtain the result);
  • New fields in the mapping to override settings, such as PET Name, Folder Name, Opt In/Out Status;
  • Action upsertListMember received a good amount of updates, following the same improvements implemented in the other two actions;
  • Now all endpoints work with hardcoded rate limits. They don't change too much from customer to customer, so I'm using the highest value compared to three different customers. I also added a function called getRateLimits for future use, when we can fetch these rate limits periodically, but not during the Action execution itself.

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.

…'t exist yet;\n- Adding as an optional parameter in Settings;\n- should be a password field;\n- Profile List Name now it's an optional field.
Copy link
Contributor

github-actions bot commented Nov 26, 2024

New required fields detected

Warning

Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production.

The following required fields were added in this PR:

  • Destination: Responsys (Actions), Action Field(s):userData

Add these new fields as optional instead and assume default values in perform or performBatch block.

@seg-leonelsanches seg-leonelsanches marked this pull request as ready for review December 4, 2024 00:55
@seg-leonelsanches seg-leonelsanches requested a review from a team as a code owner December 4, 2024 00:55
@joe-ayoub-segment
Copy link
Contributor

hi @seg-leonelsanches just FYI - please message me when you'd like me to review the PR.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 81.35593% with 44 lines in your changes missing coverage. Please review.

Project coverage is 78.40%. Comparing base (6955920) to head (c735dc6).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...nation-actions/src/destinations/responsys/utils.ts 78.18% 7 Missing and 5 partials ⚠️
...c/destinations/responsys/sendCustomTraits/index.ts 35.29% 10 Missing and 1 partial ⚠️
.../src/destinations/responsys/sendToPet/functions.ts 85.07% 1 Missing and 9 partials ⚠️
...ions/src/destinations/responsys/sendToPet/index.ts 70.83% 2 Missing and 5 partials ⚠️
...tinations/responsys/sendAudienceAsPet/functions.ts 95.83% 0 Missing and 2 partials ⚠️
...s/src/destinations/responsys/sendAudience/index.ts 75.00% 1 Missing ⚠️
...c/destinations/responsys/upsertListMember/index.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2615      +/-   ##
==========================================
- Coverage   78.40%   78.40%   -0.01%     
==========================================
  Files        1030     1032       +2     
  Lines       18367    18564     +197     
  Branches     3478     3517      +39     
==========================================
+ Hits        14401    14555     +154     
- Misses       2809     2833      +24     
- Partials     1157     1176      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seg-leonelsanches
Copy link
Contributor Author

hi @seg-leonelsanches just FYI - please message me when you'd like me to review the PR.

Hi @joe-ayoub-segment. Thanks for helping me with this.

I tagged you in one unit tests that fails mysteriously. Would like your feedback on it.

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Dec 10, 2024

hi @seg-leonelsanches just FYI - please message me when you'd like me to review the PR.

Hi @joe-ayoub-segment. Thanks for helping me with this.

I tagged you in one unit tests that fails mysteriously. Would like your feedback on it.

Hi @seg-leonelsanches the tests pass when I run them locally.
What command are you using to run them?

I'm using this:

yarn cloud test packages/destination-actions/src/destinations/optimizely-data-platform/customEvent/__tests__

Uploading image.png…

@seg-leonelsanches
Copy link
Contributor Author

Hi @seg-leonelsanches the tests pass when I run them locally.
What command are you using to run them?

Either this command, or debug. I think I'll keep the test skipped for now.

As a next step, I need a deployment in stage, so I can test the three actions.

@joe-ayoub-segment
Copy link
Contributor

Deployed to Staging.

@joe-ayoub-segment
Copy link
Contributor

hi again @seg-leonelsanches . Did you need another deploy to Stage? FYI the CI checks are failing so this won't be able to go to Prod yet.

@seg-leonelsanches
Copy link
Contributor Author

hi again @seg-leonelsanches . Did you need another deploy to Stage? FYI the CI checks are failing so this won't be able to go to Prod yet.

Hi @joe-ayoub-segment. Yes, I need a final deploy in Stage to test 3 scenarios. I want to get this ready this Monday to ask you the approvals for Tuesday deployment in Prod.

@joe-ayoub-segment
Copy link
Contributor

redeployed to staging.

@seg-leonelsanches
Copy link
Contributor Author

@joe-ayoub-segment This is good to go to be deployed in Prod.

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