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

Add tracking for the contact form block #13688

Merged
merged 19 commits into from
Oct 29, 2019
Merged

Conversation

scruffian
Copy link
Member

Right now we can track how many contact forms are submitted, but it's not possible to distinguish which ones are from the shortcode and which ones are from the block. This adds a tracks event specifically for the block, so that we can track it independently of the shortcode.

Changes proposed in this Pull Request:

  • Relies on the hasFormSettingsSet attribute which is set by the block
  • When this attribute exists we add a hidden field to the form with the name is_block
  • If this hidden field is submitted we then add a value to the extra_values array
  • If is_block is set in the extra_values array, then send a jetpack_contact_form_gutenberg_block_message_sent event to Tracks

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Adding features to Jetpack

Testing instructions:

  • Create a new post and add a contact form block
  • From the front end of your site, submit the contact form
  • Go to Tracks and search for the event jetpack_contact_form_gutenberg_block_message_sent
  • Now create another post and add the contact form shortcode
  • From the front end of your site, submit the contact form
  • Go to Tracks and search for the event jetpack_contact_form_gutenberg_block_message_sent - you should not see it.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello scruffian! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33693-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Oct 8, 2019

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 012227f

@scruffian scruffian added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 8, 2019
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Tracks labels Oct 8, 2019
jeherve
jeherve previously requested changes Oct 8, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

All tests are currently failing, so they'll need to be updated:

WP_Test_Grunion_Contact_Form::test_process_submission_will_store_a_feedback_correctly_with_default_form
Undefined index: is_block

Are those events triggered even when the site is in development mode, when the site owner has not given their consent for tracking yet?

modules/contact-form/tracks-events.php Outdated Show resolved Hide resolved
modules/contact-form/grunion-contact-form.php Outdated Show resolved Hide resolved
modules/contact-form/tracks-events.php Outdated Show resolved Hide resolved
modules/contact-form/tracks-events.php Outdated Show resolved Hide resolved
modules/contact-form/tracks-events.php Outdated Show resolved Hide resolved
modules/contact-form/tracks-events.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 8, 2019
@scruffian
Copy link
Member Author

Are those events triggered even when the site is in development mode, when the site owner has not given their consent for tracking yet?

The contact form won't work unless Jetpack is connected, so we are safe in that respect.

@jeherve
Copy link
Member

jeherve commented Oct 9, 2019

The contact form won't work unless Jetpack is connected

Jetpack's Contact Form is one of the features that is accessible when Jetpack is in development mode, so we do need to take it into account imo.

@matticbot
Copy link
Contributor

scruffian, Your synced wpcom patch D33693-code has been updated.

1 similar comment
@matticbot
Copy link
Contributor

scruffian, Your synced wpcom patch D33693-code has been updated.

@matticbot
Copy link
Contributor

scruffian, Your synced wpcom patch D33693-code has been updated.

@jeherve jeherve added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 10, 2019
@jeherve
Copy link
Member

jeherve commented Oct 10, 2019

Ongoing discussions about this here:
p1570722249055600-slack-jetpack-crew

If we don't do that we can't separate the 2 when looking at the data later on.
@jeherve jeherve added this to the 7.9 milestone Oct 11, 2019
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Oct 11, 2019
@jeherve
Copy link
Member

jeherve commented Oct 11, 2019

This should now be ready for a new review. The WordPress.com diff is also updated.

  • We now include the blog ID when possible, to get valid Tracks events.
  • We now fallback to the Jetpack Master User information when a contact form is submitted from a non-logged in user.
  • We now have a different prefix for the event name that will be fired, depending on the platform. This will allow us to differentiate data coming from WordPress.com sites vs. Jetpack sites.

@scruffian
Copy link
Member Author

Thanks @jeherve these changes look great.

The only one I am not sure about is:

We now have a different prefix for the event name that will be fired, depending on the platform. This will allow us to differentiate data coming from WordPress.com sites vs. Jetpack sites.

I worry that this might make it harder to get overall number for contact form submissions. @huguesvincent, what do you think?

@huguesvincent
Copy link

Would it allow us to get the volume on DotCom alone?
That's really what I was hoping to get here. I don't think we'll be considering to do any sort of tiring or carve a different experience depending on the volume you get. We might on DotCom particularly as ZBS integration comes along.

@jeherve
Copy link
Member

jeherve commented Oct 15, 2019

Would it allow us to get the volume on DotCom alone?

Yes, you'd be able to track both Jetpack usage and WordPress.com usage separately, or together with a %_contact_form_block_message_sent wildcard.

@scruffian
Copy link
Member Author

or together with a %_contact_form_block_message_sent wildcard.

Oh I didn't know about this. That's great 👍

@jeherve jeherve dismissed their stale review October 25, 2019 20:32

Changes were made to address feedback.

jeherve
jeherve previously approved these changes Oct 25, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This works well in my tests, here and on wpcom, but I'd appreciate another review before merge since I contributed to this PR.

Thank you!

@jeherve jeherve self-assigned this Oct 25, 2019
@matticbot
Copy link
Contributor

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well in my tests. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 29, 2019
@jeherve jeherve merged commit c2dd31d into master Oct 29, 2019
@jeherve jeherve deleted the add/contact-form-tracking branch October 29, 2019 11:22
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 29, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Contact Form [Feature] Tracks Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants