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

Emxd 3336 add app video ctv #1529

Merged
merged 9 commits into from
Oct 21, 2020

Conversation

EMXDigital
Copy link
Contributor

Adding video support to EMX Digital adapter

@EMXDigital
Copy link
Contributor Author

Juts following up here, the checks seem to be failing due to an independent/non-related part of the package pubstack_module_test

FAIL: TestPubstackModuleSuccess (0.17s) pubstack_module_test.go:223: Error Trace: pubstack_module_test.go:223 pubstack_module_test.go:205 Error: Should receive an event, but did NOT Test: TestPubstackModuleSuccess Messages: setUID events are only published when logging a setUID object with setUID feature on FAIL

EMX Digital adapter looks to be okay and has > 90% testing coverage

@bsardo
Copy link
Collaborator

bsardo commented Oct 9, 2020

I'll take a look at that test failure since I recently worked on that test.

@EMXDigital
Copy link
Contributor Author

@bsardo looks like all the checks are passing now 👍

Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Looks good. I have a few minor comments. Please review the docs for this adapter to check if any changes are needed: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/emx_digital.md

adapters/emx_digital/emx_digital.go Outdated Show resolved Hide resolved
adapters/emx_digital/emx_digital.go Outdated Show resolved Hide resolved
@SyntaxNode SyntaxNode self-assigned this Oct 13, 2020
@EMXDigital
Copy link
Contributor Author

@SyntaxNode Updated code based on feedback and discussion with team. Ready for another round

@SyntaxNode
Copy link
Contributor

Is the intent to support the /openrtb2/video long form video endpoint? If so, please look at the "Long-Form Video Support" section of the new adapter docs to be sure you are setting the necessary fields.

@EMXDigital
Copy link
Contributor Author

@SyntaxNode No, we are not looking to support long form video at this time, just the vanilla video integrations

@SyntaxNode
Copy link
Contributor

@SyntaxNode No, we are not looking to support long form video at this time, just the vanilla video integrations

Gotcha. Sometimes CTV = Long Form Video, so I wanted to ask. Thank you.

@hhhjort hhhjort merged commit 21a9076 into prebid:master Oct 21, 2020
@EMXDigital EMXDigital deleted the EMXD_3336_add_app_video_ctv branch October 21, 2020 17:35
@EMXDigital
Copy link
Contributor Author

Thanks @hhhjort for merging this in - will there be an official release coming out in the coming days that we can use to message to Publishers for what version of Prebid S2S supports this new functionality?

@hhhjort
Copy link
Collaborator

hhhjort commented Oct 21, 2020

We don't have a set in stone schedule for releases, but there is likely to be a release tomorrow. If not then there should be a release in the middle of next week. The version will be 0.134.0 of course.

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.

6 participants