feat:base_attributes_for_media_events

[TheOnlyRantis]

Instructions

1. PR target branch should be against development
2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

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

• Was this tested locally? If not, explain why.
• Yes, this was tested locally. It appears to function as expected with no errors.

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

• Closes https://go.mparticle.com/work/REPLACEME

cc: @gkelley-motortrend

[alexs-mparticle]

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.

[alexs-mparticle]

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.

Suggested change

	public baseCustomAttributes = {}
	public baseCustomAttributes: ModelAttributes = {}

[TheOnlyRantis]

@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; }

[alexs-mparticle]

@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

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[TheOnlyRantis]

@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?

[alexs-mparticle]

@TheOnlyRantis

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.

[alexs-mparticle]

@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.