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

feat(FEC-10041): playAdsWithMSE with DAI detach the playback and ad #408

Merged
merged 7 commits into from
Feb 4, 2021

Conversation

Yuvalke
Copy link
Contributor

@Yuvalke Yuvalke commented Feb 2, 2021

Description of the Changes

Issue: ad and playback have the same content URI.
Solution: check if ads are imadai and exclude the logic for attaching detach.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

Issue: ad and playback has same cotnet URI.
Solution: check if ads is imadai and exclude the logic for attach detach.
@Yuvalke Yuvalke requested a review from a team February 2, 2021 06:54
@Yuvalke Yuvalke self-assigned this Feb 2, 2021
Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

Maybe do it more generic, the plugin decorator can expose data if requires detach/attach and then we ask to get current active decorator and ask it if it needs detach/attach.
Base will always return true and plugins implementing decorator can override and return false

@Yuvalke
Copy link
Contributor Author

Yuvalke commented Feb 2, 2021

@OrenMe I thought about that but why wouldn't we pass it on adLoaded params? sounds a bit less robust I think

@OrenMe
Copy link
Contributor

OrenMe commented Feb 2, 2021

@Yuvalke my point here is to make it more generic and not check in kaltura-player for specific plugin names. If it makes more sense to pass it in another manner then raise it and we can consider.

yairans
yairans previously approved these changes Feb 4, 2021
@Yuvalke Yuvalke merged commit d7b5e09 into master Feb 4, 2021
@Yuvalke Yuvalke deleted the FEC-10041 branch February 4, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants