-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat:base_attributes_for_media_events #540
feat:base_attributes_for_media_events #540
Conversation
pass to all subsequent events and merge in attributes passed to the logX functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for providing creating a pull request to update our Media SDK. This looks like a fairly straightforward request, but I have gone ahead and added some comments and requests.
As you are adding a new attribute that will effect all events within a session, I would request that you add some tests to this feature.
Specifically, I would add unit tests to the MediaSession class to simply verify that the custom attributes are optional, and that the createMediaEvent
method will properly add the custom attributes to events.
I would also add some integration tests to make sure that the end-to-end event creation process is tested.
Lastly, creating a simple type definition for this new object would be helpful, but you can use a simple dictionary or our ModelAttributes
definition as well.
@@ -140,6 +142,7 @@ export class MediaSession { | |||
readonly streamType: MediaStreamType, | |||
public logPageEvent = false, | |||
public logMediaEvent = true, | |||
public baseCustomAttributes = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a type definition here.
We provide ModelAttributes
within src/types.ts
or create one specific for SessionCustomAttribute
that conforms to any sort of Dictionary.
public baseCustomAttributes = {} | |
public baseCustomAttributes: ModelAttributes = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexs-mparticle Using ModelAttributes for the type seems fine. Do you know if there's a reason that the value for the dictionary doesn't accept booleans? The data collection end of the product supports booleans so I would think the SDK should accept that as well.
Existing Code:
export interface ModelAttributes { [key: string]: string | number; }
Theoretical Code:
export interface ModelAttributes { [key: string]: string | number | boolean; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheOnlyRantis It might be an oversight on our part when we first created this definition. Booleans should work, but most of our attributes are strings and numbers, so I would verify that adding a boolean value shows up as expected in your LiveStream
Co-authored-by: Alex S <49695018+alexs-mparticle@users.noreply.github.com>
Co-authored-by: Alex S <49695018+alexs-mparticle@users.noreply.github.com>
Co-authored-by: Alex S <49695018+alexs-mparticle@users.noreply.github.com>
Kudos, SonarCloud Quality Gate passed! |
@alexs-mparticle For testing whether the new custom attributes are optional wouldn't the existing tests cover that since they don't currently pass any value into new constructor argument? I can create a test that constructs the media session then ends the media session to validate whether the end and media Session Summary events also have the custom attributes set in the constructor. I'm not very familiar with how the integrations work behind the scenes besides wrapping other SDKs. Do you have a suggestion on how to test this more specifically? |
I think your suggestion makes sense. For me, "End-to-End" really just means creating a media session, and verifying that the media summary events have the custom attributes, so your approach is the way to go. I think my original suggestion was just to have some extra coverage in case of future changes, but you are correct that our existing tests should cover this. |
@TheOnlyRantis We're closing this PR as we've rolled it into #559. Thanks for helping to kick off this change. This will be part of the next media-sdk release, and it will also be rolled into the iOS and Android sdks shortly after. |
Instructions
development
Summary
Added support for base attributes that can be passed into createMediaSession and get passed into each subsequent event sent by the media player. If attributes are passed into a specific event function like logPlay() they will override the base custom attributes if they share the same key name.
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)
cc: @gkelley-motortrend