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

Partner: blend ai: Add authorization header #2189

Merged
merged 22 commits into from
Jul 29, 2024

Conversation

mihaa1
Copy link
Contributor

@mihaa1 mihaa1 commented Jul 18, 2024

A summary of your pull request, including the what change you're making and why.

  • added authorization header
  • small conflict fix

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
  • [Segmenters] Tested in the staging environment

spiderman and others added 2 commits July 18, 2024 10:49
@mihaa1 mihaa1 changed the title Add authorization header Partner: blend ai: Add authorization header Jul 18, 2024
Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment left a comment

Choose a reason for hiding this comment

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

Hi @mihaa1 ,

It looks like you were sending all settings and mapping config in the body of your request (as well as payload data). I don't think you intended to do this. So the correct way to send just the payload is as below.

  perform: (request, {payload, settings}) => {
    return request(baseUrl + 'sendData', {
      headers: { Authorization: `Bearer ${settings.apiKey}` },
      json: { payload }
    })
  }

@mihaa1
Copy link
Contributor Author

mihaa1 commented Jul 18, 2024

Hi @mihaa1 ,

This code will break.

Please do the following:

  perform: (request, {payload, settings}) => {
    return request(baseUrl + 'sendData', {
      headers: { Authorization: `Bearer ${settings.apiKey}` },
      json: { payload }
    })
  }

Done!

@joe-ayoub-segment
Copy link
Contributor

hi @mihaa1 - I just looked at this again.
Your original code will work fine.

Usually the payload object will have data in it, but in your case you don't have any fields defined to populate the payload data. So you are probably just grabbing all the data you need from the rawData object.

This isn't the way we designed the platform to be used, but since you already have it that way we should leave as is.

Can you revert back to your original PR and I'll approve it please?

@mihaa1
Copy link
Contributor Author

mihaa1 commented Jul 18, 2024

hi @mihaa1 - I just looked at this again. Your original code will work fine.

Usually the payload object will have data in it, but in your case you don't have any fields defined to populate the payload data. So you are probably just grabbing all the data you need from the rawData object.

This isn't the way we designed the platform to be used, but since you already have it that way we should leave as is.

Can you revert back to your original PR and I'll approve it please?

@joe-ayoub-segment thanks for the feedback
We are currently rewriting this part of our code.
If this is the correct way of doing things, I would like to change it. We dont have code relying on this currently.

@joe-ayoub-segment
Copy link
Contributor

Ah OK, if you are rewriting it to work in the way our platform expects, then you should define some fields to specify which bits of data you want to collect from the payload.

If you want to grab the entire payload in a single field you can, but again that's not recommended.

For example here is a single field capturing the entire payload:

  fields: {
    data: {
      label: 'Payload',
      description: 'All payload data',
      type: 'object',
      required: true,
      default: { '@path': '$.' }
    }
 }

To drop this into the body of your request you would do this in the perform function:

  perform: (request, {payload, settings}) => {
    return request(baseUrl + 'sendData', {
      headers: { Authorization: `Bearer ${settings.apiKey}` },
      json: payload.data
    })
  }

Again, the 'correct' thing to do is to be selective about the parts of the payload you want to send to your platform, and have separate fields which pick out just those bits of data. You can then compose the json body you want to send to your platform in the perform() function.

Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment left a comment

Choose a reason for hiding this comment

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

Hi @mihaa1 - left some comments.

@joe-ayoub-segment
Copy link
Contributor

hi @mihaa1 - I think the neatest way to do this would be to add a new Action which has all the fields defined for data points you want to allow customers to send to your platform.
You could then label your existing Action as 'deprecated' by updating the label name. You could also update the 'preset' to point to the new Action to make it more likely that customers who are setting up the Integration in the figure future will use the new Action.

Does that make sense?

There is no good way to update the existing Action to work in both the old and new way unfortunately.

Best regards,
Joe

@mihaa1
Copy link
Contributor Author

mihaa1 commented Jul 22, 2024

hi @mihaa1 - I think the neatest way to do this would be to add a new Action which has all the fields defined for data points you want to allow customers to send to your platform. You could then label your existing Action as 'deprecated' by updating the label name. You could also update the 'preset' to point to the new Action to make it more likely that customers who are setting up the Integration in the figure future will use the new Action.

Does that make sense?

There is no good way to update the existing Action to work in both the old and new way unfortunately.

Best regards, Joe

That sounds great - I created a new action, and reverted all changes in old one.
A question - is it possible to update just the url for the old integration?
I would like to change it from segment-api.blnd.ai to api.blnd.ai

@mihaa1
Copy link
Contributor Author

mihaa1 commented Jul 25, 2024

@joe-ayoub-segment fixed everything!

Comment on lines 37 to +43
{
name: 'Send Data to Blend',
name: 'Send Data to Blend - Depracated',
subscribe: 'type = "identify" or type = "page" or type = "screen" or type = "track"',
partnerAction: 'sendData',
mapping: defaultValues(sendData.fields),
type: 'automatic'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @mihaa1 - just remove this preset please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will the events from existing clients still be sent if I remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the events from existing clients still be sent if I remove this?

yes they will

@joe-ayoub-segment joe-ayoub-segment merged commit 7b54a7b into segmentio:main Jul 29, 2024
5 of 11 checks passed
@joe-ayoub-segment
Copy link
Contributor

hi @mihaa1 this PR has been deployed. Can you please confirm that the change looks good?

@mihaa1
Copy link
Contributor Author

mihaa1 commented Jul 29, 2024

hi @mihaa1 this PR has been deployed. Can you please confirm that the change looks good?

@joe-ayoub-segment the old integration works. We will need to test with new 1 to see.
Thank you so much for your help

marinhero pushed a commit that referenced this pull request Aug 2, 2024
* Update README.md

* x

* blend updated version

* new update

* fixing preset

* * add auth header
* resolve conflict

* fix from comment

* modify sending structure

* refactor & formatting

* Remove .tool-versions file

* remove required on new fields

* * created new action /trackEvents - to replace /sendData action

* more revert

* remove

* comment

* added properties according to comments

* generated types

* * added depreacted to sendData action
* more fixes

---------

Co-authored-by: Yuval Zandbank <126495947+yuvlaz@users.noreply.github.com>
Co-authored-by: Yuval <yuval@blnd.ai>
Co-authored-by: joe-ayoub-segment <45374896+joe-ayoub-segment@users.noreply.github.com>
Co-authored-by: spiderman <spiderman@Michaels-MacBook-Pro.local>
harsh-joshi99 pushed a commit that referenced this pull request Aug 16, 2024
* Update README.md

* x

* blend updated version

* new update

* fixing preset

* * add auth header
* resolve conflict

* fix from comment

* modify sending structure

* refactor & formatting

* Remove .tool-versions file

* remove required on new fields

* * created new action /trackEvents - to replace /sendData action

* more revert

* remove

* comment

* added properties according to comments

* generated types

* * added depreacted to sendData action
* more fixes

---------

Co-authored-by: Yuval Zandbank <126495947+yuvlaz@users.noreply.github.com>
Co-authored-by: Yuval <yuval@blnd.ai>
Co-authored-by: joe-ayoub-segment <45374896+joe-ayoub-segment@users.noreply.github.com>
Co-authored-by: spiderman <spiderman@Michaels-MacBook-Pro.local>
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