-
Notifications
You must be signed in to change notification settings - Fork 271
[Recombee] Add internalAdditionalData field to actions #2856
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
[Recombee] Add internalAdditionalData field to actions #2856
Conversation
73c6f5e
to
15264d7
Compare
packages/destination-actions/src/destinations/recombee/addBookmark/index.ts
Outdated
Show resolved
Hide resolved
Hi @joe-ayoub-segment , I refactored the implementation to be all within the class constructors, as you suggested. I also added a few more PayloadValidationErrors, so that the error message is more descriptive than the one from the API. I generated a new instance of |
Hi @mstieranka I'm still not following the PR properly. Maybe we could catch up on a call to walk through it? Here is my Calendly link: https://calendly.com/joe_ayoub Can you schedule a call please? |
Talked to @mstieranka who explained the changes. All makes sense. Tests included. |
Hi @joe-ayoub-segment , I implemented the minor updates we talked about and we've also done an internal round of code review to make sure this is what we want to go out. This PR is therefore ready for you to merge and release. |
Hi @mstieranka PR deployed. Please confirm that you are happy with the change. |
This PR adds a new field,
internalAdditionalData
, to all applicable actions within the Recombee destination.The goal is for us to be able to track the types of events that are sent using our destination, and being able to use this data in our analytics, as well as assist our customers with any issues they may be having.
We set this new field up with
unsafe_hidden
set totrue
, as end users should never be able to modify the values within this field. If this parameter doesn't prevent the user from changing the values within this object, or if there's a better way to obtain$.type
and$.event
within actions, please let us know.Additionally, the original
additionalData
field (that should be present to all users) has had itsdefaultObjectUI
set tokeyvalue:only
, as we want to discourage users from setting the value of this field to be an entire object.Testing
These changes were tested end-to-end to ensure the event data is sent in all applicable actions, and unit test snapshots were modified to account for the above-mentioned changes.
The new field is not required, meaning this is not a breaking change.