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

[MOLOCO] Add a new field Platform Name in settings #2114

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

heonjang
Copy link
Contributor

@heonjang heonjang commented Jun 26, 2024

Testing

  • [✅ ] Added unit tests for new functionality
  • [✅ ] Tested end-to-end using the local server
  • [Segmenters] Tested in the staging environment

Summary

This PR adds a new field to settings: platformName

Previously, platform was used for both URL creation and in the request header, but now platformName will be used for URL creation, and platformId will be used for the header separately.

Note: there are lots of changes automatically created due to pre-commit linter

@heonjang heonjang requested a review from a team as a code owner June 26, 2024 18:30
@heonjang heonjang changed the title Add platform name [MOLOCO] Update fields in settings Jun 26, 2024
'Content-Type': 'application/json'
}
private getEndpoint() {
return `https://${this.platformName.replace(/_/g, '-')}-evt.rmp-api.moloco.com/cdp/SEGMENT`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pointer: Now uses platformName instead of platform

Copy link
Contributor

Choose a reason for hiding this comment

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

hi @heonjang - we'll need to handle the case where platformName is undefined. This will be the case for all customers who have the Integration already in use.

async sendEvent(body: Record<string, any>): Promise<ModifiedResponse> {
const headers = {
'x-api-key': this.apiKey,
'x-platform-id': this.platformId,
Copy link
Contributor Author

@heonjang heonjang Jun 26, 2024

Choose a reason for hiding this comment

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

Pointer: Now uses platformId instead of platform

@@ -32,6 +32,12 @@ const destination: DestinationDefinition<Settings> = {
type: 'string',
required: true
},
platformName: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Newly added platformName field

@heonjang heonjang changed the title [MOLOCO] Update fields in settings [MOLOCO] Add a new field Platform Name in settings Jun 26, 2024
label: 'Platform Name',
description: 'Name of the platform',
type: 'string',
required: true
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @heonjang - we can't add a new 'required' field to code which is already in use by customers. It will result in all events being dropped and not processed by customers who have already set up the Destination.

Can we set required:false, and then in code ensure that we don't fail if this field is undefined?

@joe-ayoub-segment
Copy link
Contributor

hi @heonjang thanks for raising this PR.
Please see the comments. It's important that we don't add a new 'required' field to code which is already in use by customers. For these customers field will resolve to 'undefined', which will cause an error in your code.

Thanks,
Joe

@heonjang
Copy link
Contributor Author

heonjang commented Jul 1, 2024

hi @heonjang thanks for raising this PR. Please see the comments. It's important that we don't add a new 'required' field to code which is already in use by customers. For these customers field will resolve to 'undefined', which will cause an error in your code.

Thanks, Joe

@joe-ayoub-segment Thanks for the review! I've updated the code and made that field optional.

It will now use platformId if platformName is not passed when forging a URL.
Screenshot 2024-07-01 at 4 42 45 PM

@joe-ayoub-segment
Copy link
Contributor

joe-ayoub-segment commented Jul 9, 2024

hi @heonjang - looks like a bunch of failing CI checks. Any chance you could take a look please?
Try running this command:

yarn jest --testPathPattern='./packages/destination-actions/src/destinations/moloco-rmp'

Looks like you need to do something like this:

this.platformName = settings.platformName ?? ''

in the common/request-client.ts file

@joe-ayoub-segment joe-ayoub-segment merged commit 78de0cd into segmentio:main Jul 9, 2024
6 of 11 checks passed
@joe-ayoub-segment
Copy link
Contributor

Nevermind - I'll merge it now and apply the fix in main

@joe-ayoub-segment
Copy link
Contributor

#2146

@joe-ayoub-segment
Copy link
Contributor

hi @heonjang this PR has been deployed. Please confirm you are happy with the change.

@heonjang
Copy link
Contributor Author

heonjang commented Jul 9, 2024

hi @heonjang this PR has been deployed. Please confirm you are happy with the change.

Everything looks great. Thank you for your work!

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