Skip to content

Add method for enabling form event tracking

Jeremy Jackson requested to merge jejacks0n/add-form-tracking into master

What does this MR do?

It properly hooks into snowplow form event tracking in such a way that standard and experiment contexts can be added to them.

THIS METHOD SHOULD NOT BE CALLED WITHOUT A CONFIG THAT SPECIFIES A WHITELIST! (Whitelist is a term that's used by snowplow, and we don't obscure that even though https://about.gitlab.com/handbook/communication/top-misused-terms/#whitelist states it's a term we don't want to use)

Meaning that we should never instruct snowplow to enable form tracking without including a list of enabled forms or fields in the config (search for enableFormTracking).

Whitelists: This is an array of strings used to turn on certail [sic]. Any form with a CSS class in the array will be tracked. Any field in a tracked form whose name property is in the array will be tracked. All other elements will be ignored.

I have added a basic layer of protection and will encourage the product intelligence team to review all of the form tracking behaviors, as there's several things that are incorrect with how this is currently handled.

If you look at https://gitlab.com/gitlab-org/gitlab/-/blob/0327e789ffe73c7f9d4044cd8e33c747f6f2f392/lib/gitlab/tracking.rb#L28 (this is enabled on gitlab.com) you'll realize that it doesn't work -- AND SHOULDN'T the way it's setup without a list of forms and inputs, to be honest. When we follow this code forward we end up at https://gitlab.com/gitlab-org/gitlab/-/blob/0327e789ffe73c7f9d4044cd8e33c747f6f2f392/app/assets/javascripts/tracking.js#L199, where this call thankfully does nothing. Per the docs about enableFormTracking:

This will only work for form elements which exist when it is called.

Thankfully this call happens before the DOM is ready, and I don't think we should ever just add form tracking without being very intentional about it. I've added a commented out line that would make some of this behave nicer, but think it needs more approval and thought from the product intelligence team before it's something we permit.

Conformity

Availability and Testing

Well, we really don't want to just enable form tracking everywhere, without being very intentional about it -- so we've implemented a couple layers to try to ensure this isn't abused. There might be some useful linting rules to be added?

Edited by Jeremy Jackson

Merge request reports