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

Config Additions: session_recording, mask_all_element_attributes, mask_all_text #209

Merged
merged 25 commits into from
Apr 9, 2021

Conversation

yakkomajuri
Copy link
Contributor

@yakkomajuri yakkomajuri commented Mar 30, 2021

Changes

So I was operating on a few hours of sleep today and put through #207 which, while preventing rrweb from capturing input values as stated, it doesn't prevent PostHog from doing so, because..... we already don't. 🤦 I confused our approach with rrweb's and went ahead and censored element attributes instead.

Nevertheless, this PR now adds multiple config options:

  • session_recording: an object containing a subset of rrweb options that we can expose to our users
  • mask_all_text: Prevents us from capturing textContent as per @lharress's request (and this makes a lot of sense)
  • mask_all_element_attributes: prevents autocapture from capturing any element attributes (apparently you'd need this too @lharress?)

Checklist

  • Tests for new code (if applicable)
  • TypeScript definitions (module.d.ts) updated and in sync with library exports (if applicable)

@yakkomajuri yakkomajuri added the bump patch Bump patch version when this PR gets merged label Mar 30, 2021
@lharress
Copy link

Thanks, I think you missed one?

if (usefulElements.indexOf(tag_name) > -1) props['$el_text'] = getSafeText(elem)

The second kinda works for us (insert sledgehammer scalpel analogy here) in that we have some custom attributes that don't fall within the input sensitivity check. The 3 options (exclude attributes, textContent and the built in input conservativeness) will definitely help us. Thanks!

@yakkomajuri
Copy link
Contributor Author

Fixed, thanks a lot for keeping an eye on this!

@posthog-bot please add @lharress for ideas and test

@posthog-contributions-bot
Copy link

@yakkomajuri

I've put up a pull request to add @lharress! 🎉

@lharress
Copy link

Thanks for jumping on it so fast!

Copy link
Contributor

@macobo macobo left a comment

Choose a reason for hiding this comment

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

We currently lack any E2E testing for autocapture functionality (and this would have introduced a bug there) - how do you feel about extending the testcafe test for this?

src/autocapture.js Outdated Show resolved Hide resolved
src/autocapture.js Outdated Show resolved Hide resolved
src/__tests__/extensions/sessionrecording.js Outdated Show resolved Hide resolved
src/__tests__/extensions/sessionrecording.js Outdated Show resolved Hide resolved
src/__tests__/autocapture.js Show resolved Hide resolved
@yakkomajuri
Copy link
Contributor Author

Thanks! I have no time today but can hopefully find time to improve the tests tomorrow!

@yakkomajuri
Copy link
Contributor Author

I've addressed everything except testcafe tests - can get to it on this PR or a later one.

cc @macobo

@macobo
Copy link
Contributor

macobo commented Apr 6, 2021

Might be worth getting in here, but would also be OK with merging as-is if you're confident in it. :)

@yakkomajuri
Copy link
Contributor Author

Comment came before I had access to browserstack - I now do so I'll play around with it

testcafe/e2e.spec.js Outdated Show resolved Hide resolved
@yakkomajuri
Copy link
Contributor Author

Definitely happy to update/change this again, but I've now structured E2E tests in 3 parts, to "set the tone" for future tests:

  • Custom events work
  • Autocapture works (captures events and the right props)
  • Config options correctly affect capture behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants