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

[BB-7904] fix: youtube race condition when GTM loads #591

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

viadanna
Copy link
Member

This PR fixes a race condition that might prevent YouTube videos from loading when Google Tag Manager is used.

Both Open edX and GTM implement onYouTubeIframeAPIReady and run already existing implementations of this callback when set up.

The issue arises when GTM overwrites it, as the callback implemented by Open edX expects a resolve() attribute that isn't implemented by GTM. This change ensures the original global is used by Open edX so it's never overwritten.

Testing

  1. Set up a devstack with this branch and Google Analytics, or use this sandbox.
  2. Navigate to a Unit containing a YouTube video and open your browser Development Tools.
  3. Add the following breakpoint:
    image
  4. Reload the page and wait a couple seconds before pressing F8 to continue running. This ensures onYouTubeIframeAPIReady is overwritten by GTM.
  5. Make sure the YouTube video loads correctly.
  6. Disable the breakpoint and reload, again making sure the Youtube video loads when the callback set by Open edX is used.

@viadanna viadanna self-assigned this Sep 27, 2023
@Cup0fCoffee Cup0fCoffee force-pushed the viadanna/youtube-race-condition branch from 98c8029 to fc9b6af Compare September 29, 2023 19:36
@ArturGaspar
Copy link
Member

👍

  • I tested this: reproduced both the cases where GTM and Open edX set the callback first, tested that video works
  • I read through the code
  • I checked for accessibility issues - N/A
  • Includes documentation - N/A
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository. - N/A

@viadanna viadanna merged commit 7d52828 into opencraft-release/palm.1 Oct 13, 2023
41 checks passed
@viadanna viadanna deleted the viadanna/youtube-race-condition branch October 13, 2023 14:12
@0x29a
Copy link
Member

0x29a commented Apr 10, 2024

Upstream PR: openedx#33649

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.

3 participants