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

Fix Uncaught TypeError: Cannot read property 'type' of undefined at mtc.js:60 at mtc.js:3 at Object.MauticJS.processGatedVideos (mtc.js:60) at MauticJS.initGatedVideo (mtc.js:59) #7365

Merged
merged 2 commits into from
Jun 29, 2020

Conversation

leo108
Copy link
Contributor

@leo108 leo108 commented Mar 29, 2019

closes #7032

Q A
Bug fix? Yes
New feature? No
Automated tests included? No
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #7032
BC breaks? No
Deprecations? No

Steps to reproduce

Easier steps to reproduce mentioned in #7365 (comment)

  1. Add mautic js script tag to website
  2. Create a campaign that triggers a Focus Item that shows a popup
  3. Load/Refresh the page

Log errors

In Console I see the following error:

Uncaught TypeError: Cannot read property 'type' of undefined
at mtc.js:60
at mtc.js:3
at Object.MauticJS.processGatedVideos (mtc.js:60)
at MauticJS.initGatedVideo (mtc.js:59)

Test PR

Re-do it

@npracht npracht added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging labels Mar 29, 2019
@npracht npracht added this to the 2.15.2 milestone Mar 29, 2019
@npracht
Copy link
Member

npracht commented Mar 29, 2019

@Enc3phale you having skills in js, can you have a look at the code ?

Copy link

@quintmouthaan quintmouthaan left a comment

Choose a reason for hiding this comment

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

Please look at my comments and make changes if you agree.

@@ -396,7 +396,11 @@ public function onBuildJsForVideo(BuildJsEvent $event)

MauticJS.iterateCollection(videoElements)(function(node, i){
var playerFeatures = [];
var source = node.getElementsByTagName('source')[0];
var sourceNodes = node.getElementsByTagName('source');
if (sourceNodes.length === 0) {

Choose a reason for hiding this comment

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

by returning here if there is no source node the functionality will still not work, even if the source is specified in the src attribute. I suggest to execute the following couple of lines only if sourceNodes.length > 0 and execute the rest of the function even if sourceNodes.length === 0. That way the functionality will also work for video tags with the src attribute

@kuzmany
Copy link
Member

kuzmany commented May 12, 2019

Wasn't double tested before the 2.15.2 PR merge deadline so pushing to the next release.

@kuzmany kuzmany modified the milestones: 2.15.2, 2.16.0 May 12, 2019
@tenil

This comment has been minimized.

Copy link

@tenil tenil left a comment

Choose a reason for hiding this comment

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

I was able to reproduce the error.
This fix did not work for me.

@npracht npracht modified the milestone: 2.16.0 Jan 23, 2020
@RCheesley
Copy link
Member

If this issue persists could you update the PR @leo108 or if they aren't able to do so, maybe @quintmouthaan could pick it up taking into account the recommendations?

@npracht npracht added Triage M2/M3 and removed code-review-needed PR's that require a code review before merging ready-to-test PR's that are ready to test labels Apr 4, 2020
@npracht
Copy link
Member

npracht commented Apr 4, 2020

Hi there!
It has been decided to not create any extra Mautic 2 versions (except for major security purpose) knowing that Mautic 3 is coming very soon.

We now want to integrate your contribution in the Mautic 3 roadmap as 3.0.1 candidate.

How to do?

  1. Check if the bug still present in Mautic 3
  2. If Yes: rebase your PR against the 3.x branch
  3. If No: please comment on your PR: “This PR only applies to Mautic 2.x”

Please report results by commenting on your PR to make us administration easier.

In case your bugfix only apply to Mautic 2, we'll consider adding it in an extra Mautic 2 version.


You can more information on how to do all of that on this blog post "Getting you PR ready for Mautic 3".

@mautibot
Copy link

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/cors-policy-problem/14291/4

@npracht
Copy link
Member

npracht commented May 20, 2020

Hello @leo108 could you please test mautic 3.0.0-beta2 and tell me if the issue is still existing?

  • If no, i'll tag this issue at M2 only relevant
  • If yes, could you please rebase your branch against 3.x branch ?

Waiting for your feedback, i would very love to merge it in 3.0.1 if still relevant. Thanks !

@npracht npracht changed the base branch from master to 3.x June 10, 2020 07:45
@npracht npracht changed the title Fix issue mautic#7032 Fix Uncaught TypeError: Cannot read property 'type' of undefined at mtc.js:60 at mtc.js:3 at Object.MauticJS.processGatedVideos (mtc.js:60) at MauticJS.initGatedVideo (mtc.js:59) Jun 10, 2020
@npracht npracht added the ready-to-test PR's that are ready to test label Jun 10, 2020
@npracht npracht added this to the 3.0.1 milestone Jun 10, 2020
@RCheesley RCheesley changed the base branch from 3.x to staging June 17, 2020 11:33
@RCheesley
Copy link
Member

@leo108 or @quintmouthaan if this is still an issue could you pick this up

@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #7365 into staging will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             staging    #7365      +/-   ##
=============================================
+ Coverage      35.24%   35.25%   +0.01%     
- Complexity     27884    27889       +5     
=============================================
  Files           1731     1731              
  Lines          96712    96409     -303     
=============================================
- Hits           34088    33991      -97     
+ Misses         62624    62418     -206     
Impacted Files Coverage Δ Complexity Δ
...les/PageBundle/EventListener/BuildJsSubscriber.php 2.94% <ø> (-1.41%) 14.00 <0.00> (ø)
...dles/PageBundle/EventListener/ConfigSubscriber.php 15.38% <0.00%> (-6.05%) 5.00% <0.00%> (ø%)
...pp/bundles/CampaignBundle/Event/ScheduledEvent.php 60.00% <0.00%> (-2.50%) 4.00% <0.00%> (ø%)
...Exchange/Internal/Executioner/OrderExecutioner.php 78.26% <0.00%> (-1.74%) 10.00% <0.00%> (ø%)
...pp/bundles/CampaignBundle/Event/ConditionEvent.php 51.85% <0.00%> (-1.72%) 9.00% <0.00%> (ø%)
...ationsBundle/Sync/DAO/Mapping/ObjectMappingDAO.php 63.63% <0.00%> (-1.59%) 10.00% <0.00%> (ø%)
app/bundles/CampaignBundle/Event/DecisionEvent.php 56.66% <0.00%> (-1.40%) 9.00% <0.00%> (ø%)
app/bundles/PageBundle/Model/RedirectModel.php 23.33% <0.00%> (-1.26%) 19.00% <0.00%> (ø%)
...s/IntegrationsBundle/Sync/Helper/MappingHelper.php 60.86% <0.00%> (-1.24%) 27.00% <0.00%> (ø%)
... and 169 more

@dennisameling
Copy link
Member

dennisameling commented Jun 28, 2020

Easier steps to reproduce:

  • Create a landing page in Mautic
  • Add a "Code Mode" block and add the following video block to it:
<video autoplay src="https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4"></video>

image

  • Open the Landing Page in your browser
  • See the error show up:

image

After applying this PR, the error shouldn't show up anymore.

@dennisameling dennisameling self-requested a review June 28, 2020 18:25
@dennisameling
Copy link
Member

dennisameling commented Jun 28, 2020

Note: this functionality (Gated Video) is designed to only work if the data-form-id and data-gate-time attributes are set on the video element. All other <video> elements should be left intact by Mautic.

Currently, the logic that checks if a data-form-id attribute is set comes too late and should be on top of the function. That is what's causing the issue that's been reported: a video that was embedded on a page that happened to have the Mautic tracking JS on it as well was triggering the reported TypeError. This PR should move up that logic so that the error isn't thrown incorrectly.

Note on @quintmouthaan's comment above:

That way the functionality will also work for video tags with the src attribute

Problem with this is that even though this might work, the developer would need to set a data-form-id on the <video> element to trigger this functionality, as described in the docs. However, when a developer does that, the following logic isn't triggered so the player won't work properly:

node.dataset.mp4 = true;
playerFeatures = ['playpause','progress','current','duration','volume','fullscreen'];

Example:

  • If a dev would add the following video tag to a page: <video controls="" data-form-id="1" data-gate-time="5" src="https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4"></video> the player doesn't work, as this isn't a supported scenario:

image

TL;DR: Mautic should ignore all video tags that don't have the data-form-id and data-gate-time attributes as described in the docs. The logic that checks this in the code currently comes a bit too late which results in a TypeError, which this PR should fix. Please note that Mautic only supports <video> elements with proper <source> elements in it for the Gated Video functionality.

@RCheesley should we mention the TL;DR in the docs as well?

Copy link
Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Tested before with the error and after with no error:

Before:

Screenshot 2020-06-29 at 14 11 54 (2)

After:
Screenshot 2020-06-29 at 14 16 27 (2)

@dennisameling indeed I feel we should ensure that this is documented!

@RCheesley
Copy link
Member

P.S. Awesome video!

@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed ready-to-test PR's that are ready to test labels Jun 29, 2020
@dennisameling dennisameling added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Jun 29, 2020
@RCheesley RCheesley merged commit ebacb5b into mautic:staging Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'type' of undefined in MauticJS.processGatedVideos
8 participants