feat: allow regex patterns and wildcards in survey url #805

Bonitis · 2023-09-22T02:06:43Z

Changes

handles regex pattern and wildcards in survey url
added _isValidRegex util function
all changes use built in JS functionality an should be cross-browser compatible
related to Surveys URL field can handle wildcards/regex posthog#17472

Caveats

Developing locally with a demo, I do not have the Surveys beta feature. What is the best way to have that enabled to fully test these changes in the example nextjs playground?
I'm not 100% confident that checking for / at the beginning and end of the string is a great method for determining whether it is a regex. It requires education for the user to follow that pattern to have their pattern picked up correctly. I would love some feedback on whether that is acceptable.

Checklist

Tests for new code (see advice on the tests we use)
Accounted for the impact of any changes across different browsers

liyiy · 2023-09-25T23:06:07Z

src/__tests__/surveys.js

+            window.location = originalWindowLocation
+
+            // eslint-disable-next-line compat/compat
+            window.location = new URL('https://example.com?name=something')


nice!

With the regex and wildcard options, can we move this into a separate test that specifically deals with URL matching so that this one it line isn't handling too many things now? 🤠

Yes definitely!

@liyiy should the url matching function be moved to a util? Would it be useful in other areas for comparing a url condition with the current href?

liyiy · 2023-09-25T23:07:32Z

src/posthog-surveys.ts

@@ -27,6 +28,23 @@ export class PostHogSurveys {
        }
    }

+    getMatchingUrl(url?: string): boolean {


I believe this will actually require a version update for users in order for it to work for them? @neilkakkar

I'd recommend making this helper pure, i.e pass in URL and regex to match. Makes it easier to test, and remove reference to window.location.href from this function

Sounds good! Does it make sense for that helper to be in utils.ts rather than a class method? At that point it would be a simple regex test of an argument url and pattern.

Yep good call

neilkakkar · 2023-09-26T09:44:51Z

src/posthog-surveys.ts

+        if (!url) return true
+
+        // If the url string starts and ends with / it is meant to be a regular expression
+        if (url.startsWith('/') && url.endsWith('/')) {


not a big fan of this implicit matching, because if someone wanted to test for /test/ , we'll end up matching /test as well. Not too big a deal, but indeed as you say there's an education component involved.

I think we can make this more explicit perhaps, have a toggle with the url field that mentions whether it's a regex or not, so users don't have to worry about adding custom / to the front and back, which will get removed (very confusing as the end user). And just input a clear regex when they select the isRegex checkbox.

So in posthog-js, that would mean you have a second parameter telling you whether it's a regex or not.

@neilkakkar Thank you for the feedback! Would that turn the conditions type into conditions: { url?: string; selector?: string; seenSurveyWaitPeriodInDays?: number, isRegex?: boolean } | null? I wouldn't want to introduce any breaking changes on the url type, but having the isRegex boolean in the conditions object could make it seem like it also applies to the selector. On the other hand, since the checkbox would be controlled within posthog, that might be ok since the user would be explicitly applying it to the url. What do you think?

I don't love having to introduce another parameter for this especially because this is more like an add on than a requirement to using URLs with surveys, but if we do want to properly support url with regex, I agree it's better to go with a more complete solution.

Instead of a checkbox though, we can make it more like our filters where the user has options to choose whether URL "matches regex" or "contains" the property value, similar to this:

Except it would only be "matches regex" and "contains" for now 😅

And the name of the parameter underneath the hood could be like urlMatchType where the match type can be "contains" or "regex" or "exact"

That way we can retain more flexibility than just having isRegex !

It is an extra amount of work though, so feel free to let us know if it is, and the team will be happy to work on it together 😃

Great call, makes more sense 👍

I like the extra flexibility and matching an existing pattern!

It is an extra amount of work though, so feel free to let us know if it is, and the team will be happy to work on it together 😃

I think I can handle the changes in this code easily enough, but may need help on the posthog side. I have been on vacation, which is why I had such a delay replying to your comments. Is there a goal or timeline for having this change in place or hitting a release date? If so, I might not be able to get the posthog side done fast enough.

It's fine to just complete posthog-js and I'm happy to take up the posthog side of things.

Alongside conditions.url I'll pass in a conditions.urlMatchType which currently will be one of regex, exact, contains which you can match by accordingly.

neilkakkar · 2023-09-26T09:46:34Z

src/posthog-surveys.ts

+            }
+        }
+        // If the url string has a wildcard, convert to a regular expression
+        if (url.includes('*')) {


I'd propose removing this magic, and let users use proper regexes if they need regexes.

liyiy · 2023-10-02T04:31:29Z

Developing locally with a demo, I do not have the Surveys beta feature. What is the best way to have that enabled to fully test these changes in the example nextjs playground?

Surveys is under a feature flag where the key is simply surveys, so if you create a feature flag with that key and roll it out fully, it should work ~ But if it doesn't, let me know!

neilkakkar

This looks good, thanks @Bonitis . The companion PR is here: PostHog/posthog#17738

neilkakkar · 2023-10-03T14:21:29Z

I just want to run an end-to-end test and see everything works with both PRs before I merge

liyiy

Awesome work!!!!

Bonitis · 2023-10-04T03:37:38Z

@neilkakkar @liyiy thank you so much for your feedback and guidance in implementing this feature. I'm so excited to see it move forward!

neilkakkar · 2023-10-04T12:40:36Z

Hey @Bonitis , thanks for the contribution! I'd love to send you a merch code to posthog for this :) -> send me an email -> neil @ posthog .com

Also, to get this merged, I had to create a copy with the right credentials. I've made sure to preserve your commits so it's still attributed to you.

Bonitis added 2 commits September 22, 2023 01:33

feat: allow regex patterns and wildcards in survey url

8c87f76

Merge branch 'master' into survey-url

78e2f79

liyiy requested review from liyiy, jurajmajerik and neilkakkar September 25, 2023 23:02

liyiy reviewed Sep 25, 2023

View reviewed changes

neilkakkar reviewed Sep 26, 2023

View reviewed changes

Bonitis added 2 commits October 2, 2023 22:30

update survey validation to include urlMatchType

3d2a402

Merge branch 'master' into survey-url

d47322f

Bonitis requested review from neilkakkar and liyiy October 3, 2023 11:23

neilkakkar mentioned this pull request Oct 3, 2023

feat(surveys): Add regex matching option PostHog/posthog#17738

Merged

neilkakkar approved these changes Oct 3, 2023

View reviewed changes

neilkakkar added the bump minor Bump minor version when this PR gets merged label Oct 3, 2023

liyiy approved these changes Oct 3, 2023

View reviewed changes

neilkakkar mentioned this pull request Oct 4, 2023

feat: allow regex patterns and wildcards in survey url #821

Merged

2 tasks

neilkakkar closed this Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow regex patterns and wildcards in survey url #805

feat: allow regex patterns and wildcards in survey url #805

Bonitis commented Sep 22, 2023

liyiy Sep 25, 2023

Bonitis Sep 25, 2023

Bonitis Sep 25, 2023

liyiy Sep 25, 2023

neilkakkar Sep 26, 2023

neilkakkar Sep 26, 2023

Bonitis Oct 1, 2023

neilkakkar Oct 2, 2023

neilkakkar Sep 26, 2023

Bonitis Oct 1, 2023

liyiy Oct 2, 2023

liyiy Oct 2, 2023

neilkakkar Oct 2, 2023

Bonitis Oct 2, 2023

Bonitis Oct 2, 2023

neilkakkar Oct 2, 2023

neilkakkar Sep 26, 2023

liyiy commented Oct 2, 2023

neilkakkar left a comment

neilkakkar commented Oct 3, 2023

liyiy left a comment

Bonitis commented Oct 4, 2023

neilkakkar commented Oct 4, 2023

feat: allow regex patterns and wildcards in survey url #805

feat: allow regex patterns and wildcards in survey url #805

Conversation

Bonitis commented Sep 22, 2023

Changes

Caveats

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyiy commented Oct 2, 2023

neilkakkar left a comment

Choose a reason for hiding this comment

neilkakkar commented Oct 3, 2023

liyiy left a comment

Choose a reason for hiding this comment

Bonitis commented Oct 4, 2023

neilkakkar commented Oct 4, 2023