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

Support additional device content tags in conviva integration #67

Conversation

zigavehovec
Copy link
Contributor

@zigavehovec zigavehovec commented Jun 3, 2024

Problem

Conviva has a list of predefined standard device and content tags that this integration currently doesn't support reporting.

Solution

  • Add MetadataOverrides.additionalStandardTags that allows to set additional standard tags for the session.
  • Update the readme and example app

Notes

Android integration PR: bitmovin/bitmovin-player-android-analytics-conviva#56

Checklist

  • I added an update to CHANGELOG.md file
  • I ran all the tests in the project and they succeed

@zigavehovec zigavehovec changed the title Pi 3398/support additional device content tags in conviva integration Support additional device content tags in conviva integration Jun 4, 2024
@zigavehovec zigavehovec self-assigned this Jun 4, 2024
@zigavehovec zigavehovec force-pushed the PI-3398/support-additional-device-content-tags-in-conviva-integration branch from e77f293 to 059bee6 Compare June 4, 2024 13:29
@zigavehovec zigavehovec requested a review from stonko1994 June 4, 2024 13:30
@zigavehovec zigavehovec marked this pull request as ready for review June 4, 2024 13:30
Copy link
Contributor

@stonko1994 stonko1994 left a comment

Choose a reason for hiding this comment

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

Thank you for taking on this 💪 Just minor stuff.

I guess the failing unit tests are unrelated to your changes 🤔 I'll trigger a re-run and see how they behave.

@@ -85,6 +93,9 @@ class ContentMetadataBuilder: CustomStringConvertible {
if let custom = self.custom {
contentInfo.merge(custom) { $1 }
}
if let additionalStandardTags = self.additionalStandardTags {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: (no action required)

Since Swift 5.7 you could write this like this:

Suggested change
if let additionalStandardTags = self.additionalStandardTags {
if let additionalStandardTags {

However, the Swift version specified in the podspec file is set to 5.0. We could update it since it's quite old already (we are on 5.10 no in the Player SDK) Not relevant for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool, didn't know about that Swift feature 👍

zigavehovec and others added 4 commits June 5, 2024 14:11
Co-authored-by: David Steinacher <david.steinacher@bitmovin.com>
Co-authored-by: David Steinacher <david.steinacher@bitmovin.com>
@zigavehovec zigavehovec requested a review from stonko1994 June 5, 2024 13:40
@zigavehovec zigavehovec merged commit 71671b8 into develop Jun 6, 2024
3 checks passed
@zigavehovec zigavehovec deleted the PI-3398/support-additional-device-content-tags-in-conviva-integration branch June 6, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants