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

Salesforce custom object external #2485

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

Conversation

seg-leonelsanches
Copy link
Contributor

This modification:

  • Updates Salesforce base API from version v53 to v62;
  • Adds a new mapping to upsert a custom object by its Id.

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.

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Oct 16, 2024

hi @brennan - This is a Salesforce PR raised by an SA, so passing this PR on to you so it can be triaged.

Copy link
Contributor

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: Salesforce (Actions), Action Field(s):operation,customObjectName,externalIdField,externalIdValue

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 October 18, 2024 23:44
@seg-leonelsanches seg-leonelsanches requested a review from a team as a code owner October 18, 2024 23:44
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.00%. Comparing base (2574714) to head (487f516).

Files with missing lines Patch % Lines
...nations/salesforce/customObjectExternalId/index.ts 75.00% 2 Missing ⚠️
.../src/destinations/dynamic-yield-audiences/index.ts 0.00% 1 Missing ⚠️
.../destinations/loops/createOrUpdateContact/index.ts 80.00% 1 Missing ⚠️
...tions/src/destinations/salesforce/sf-operations.ts 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2485   +/-   ##
=======================================
  Coverage   77.99%   78.00%           
=======================================
  Files         991      992    +1     
  Lines       17383    17394   +11     
  Branches     3281     3281           
=======================================
+ Hits        13558    13568   +10     
- Misses       2737     2738    +1     
  Partials     1088     1088           

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

Copy link
Contributor

@rvadera12 rvadera12 left a comment

Choose a reason for hiding this comment

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

Left a couple of comments. Can we also clean up the PR to only include the changes you made and not other syntax changes from other destinations.

Could you also include a testing doc or provide proof of the tests you ran to verify


return sf.upsertCustomObject(payload, payload.customObjectName)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we are not implementing a performBatch method here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvadera12 The batch doesn't make sense for this endpoint. Each record requires one separate API call. The API endpoint format is /services/data/v61.0/sobjects/${entityType}/${entityKey}/${entityKeyValue}. There's no way we can aggregate N records in 1 API call.

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.

3 participants