feat: distinguish app vs launchpad utm_source when using utm params #21424

marktnoonan · 2022-05-10T13:58:17Z

Closes https://cypress-io.atlassian.net/browse/UNIFY-1436

User facing changelog

No user facing changes, but onlinks that previously had no utm_source, now correctly set a source of 'Binary: App' and 'Binary: Launchpad' depending on where they were clicked.

Additional details

In 9.x the utm_source query param of Test Runner was added on the server as a part of openExternal. This PR removes that functionality completely from the server, and instead applies the same rule on the client side, in our getUrlWithParams helper function, to add the utm_source automatically to URLs that already include any other utm params.

Among other things, this makes the full length URL testable from the client side, and allows us to continue having well-formed links with accurate href attributes.

This PR covers all links in the docs menu and the growth prompt that's part of it, and any future links that use utm params will have the correct source appended automatically.

How to test manually

Click any links in the docs menu in the launchpad and app and verify that when they take you to the docs pages, the utm params are present in the page URL and include the correct utm_source - Binary: App for the app and Binary: Launchpad for the browser.

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?
[na] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-05-10T13:58:21Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-05-10T14:31:40Z

Test summary

4787 • 0 • 61 • 0 • 1

Run details


Project	cypress
Status	Passed
Commit	`873e67d`
Started	May 12, 2022 4:56 PM
Ended	May 12, 2022 5:12 PM
Duration	16:21 💡
OS	Linux Debian - 10.10
Browser	Firefox 93

View run in Cypress Dashboard ➡️

Flakiness

	cypress/e2e/e2e/origin/commands/viewport.cy.ts		1
1	... > syncs the viewport across multiple origins

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

tbiethman

Updated links look good, tested in app/launchpad. Couple things to note:

I noticed that our changelog links don't have UTM params in 10.0, whereas they do in develop. For example: https://docs.cypress.io/guides/references/changelog?source=dgui_footer&utm_medium=Footer&utm_campaign=Changelog&utm_source=Test%20Runner. Not sure if that was intentionally scoped out, but we do launch those from both launchpad and app.
This isn't new to your PR, but we have logic in our openExternal mutation that can also append query params. Seems like this could conflict with our getUrlWithParams implementation if that arg is ever enabled, but it doesn't look like it is 🤷‍♂️ Would be worth removing if it's unused, especially if using it will result in bugs.

lmiller1990

Two comments

lmiller1990 · 2022-05-10T23:50:03Z

packages/frontend-shared/src/utils/getUrlWithParams.ts

@@ -5,6 +5,15 @@ export type LinkWithParams = {

 export const getUrlWithParams = (link: LinkWithParams) => {
  let result = link.url
+  const hasUtmParams = Object.keys(link.params).find((param) => param.startsWith('utm_'))


Can use some to be more clear about the intention. When using find I usually expect the found variable is used, or an error is throw if it's not found. This is more "does it exist" in a true/false sense.

Suggested change

const hasUtmParams = Object.keys(link.params).find((param) => param.startsWith('utm_'))

const hasUtmParams = Object.keys(link.params).some((param) => param.startsWith('utm_'))

This is how Lodash's findKey works. https://github.com/lodash/lodash/blob/master/findKey.js#L23-L36

Or, if you want to be optimal and save a loop...

function keyStartsWith (o, key) { for (const k in o) { if (k.startsWith(key)) return true } return false }

I think some is the best candidate among findKey, find or the for loop.

oh that's awesome, I always forget about some but it's ideal here

lmiller1990 · 2022-05-10T23:51:32Z

packages/server/lib/gui/links.ts

-
-  return require('electron').shell.openExternal(url.href)
+export const openExternal = (url: string) => {
+  return require('electron').shell.openExternal(url)


Any reason we inline the require instead of a top level import?

That was changed as part of #21174.

But good news, upgrading to electron 18 fixes this problem altogether and will allow that extra import to be removed. I'll get that taken out and remove the inline requires when #21418 lands and merges into 10.0-release.

marktnoonan · 2022-05-11T19:27:41Z

packages/graphql/src/schemaTypes/objectTypes/gql-Mutation.ts

        if (args.includeGraphqlPort && process.env.CYPRESS_INTERNAL_GRAPHQL_PORT) {
-          url = `${args.url}?port=${process.env.CYPRESS_INTERNAL_GRAPHQL_PORT}`
+          const joinCharacter = args.url.includes('?') ? '&' : '?'


@estrada9166 I made this small change, might as well future proof this for possible other params. @tbiethman I added a comment about the purpose of this, Alejandro showed me that it is actually used.

lmiller1990

Seems fine to me - CI looks a bit red, but once that's fixed, we are good to go.

* 10.0-release: (22 commits) fix: migrate multiples projects when in global mode (#21458) test: fix flaky cy-in-cy selector validity test (#21360) chore: remove unused codeGenGlobs (#21438) fix: use correct path for scaffolding spec on CT (#21411) fix: remove breaking options from testing type on migration (#21437) fix: test-recording instructions in Component Test mode (#21422) feat: distinguish app vs launchpad utm_source when using utm params (#21424) chore: update stubbed cloud types (#21451) chore: change to yarn registry fix(sessions): refactor flows, fix grouping bugs and align validation fail text (#21379) chore(sessions): more driver tests (#21378) chore: rename domain_fn to origin_fn (#21413) chore: release 9.6.1 (#21404) fix: ensure that proxy logs are updated after the xhr has actually completed (#21373) chore: Re-organize tests in assertions_spec.js (#21283) chore: Distribute tests to desktop-gui containers. Make `desktop-gui` tests faster! (#21305) chore(sessions): add additional tests (#21338) fix: Allow submit button to be outside of the form for implicit submission (#21279) fix(launcher): support Firefox as a snap (#21328) chore(sessions): break out sessions manager code and add unit tests (#21268) ...

marktnoonan added 4 commits May 9, 2022 23:42

stop setting url params on sever

c82b7a8

set utm_source in getUrlWithParams

2a820d1

update tests that check URLs

844e5da

update to include space

9ab0bee

Merge branch '10.0-release' into UNIFY-1346-utm-params

542cd21

marktnoonan requested review from emilyrohrbough, estrada9166 and lmiller1990 and removed request for estrada9166 May 10, 2022 15:48

Merge branch '10.0-release' into UNIFY-1346-utm-params

0554114

marktnoonan marked this pull request as ready for review May 10, 2022 15:49

marktnoonan requested a review from a team as a code owner May 10, 2022 15:49

tbiethman requested review from tbiethman and removed request for emilyrohrbough May 10, 2022 19:00

tbiethman approved these changes May 10, 2022

View reviewed changes

lmiller1990 reviewed May 10, 2022

View reviewed changes

marktnoonan added 4 commits May 11, 2022 12:20

use more appropriate array method

8760e83

Merge branch '10.0-release' into UNIFY-1346-utm-params

f2ad6c2

allow for other params when adding port param

f3fe906

allow for other params when adding port param

da9a3a8

marktnoonan requested a review from tgriesser as a code owner May 11, 2022 19:25

marktnoonan commented May 11, 2022

View reviewed changes

lmiller1990 self-requested a review May 12, 2022 01:33

lmiller1990 approved these changes May 12, 2022

View reviewed changes

Merge branch '10.0-release' into UNIFY-1346-utm-params

873e67d

marktnoonan merged commit a218f96 into 10.0-release May 12, 2022

marktnoonan deleted the UNIFY-1346-utm-params branch May 12, 2022 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: distinguish app vs launchpad utm_source when using utm params #21424

feat: distinguish app vs launchpad utm_source when using utm params #21424

marktnoonan commented May 10, 2022 •

edited

Loading

cypress-bot bot commented May 10, 2022

cypress bot commented May 10, 2022 •

edited

Loading

tbiethman left a comment

lmiller1990 left a comment

lmiller1990 May 10, 2022

marktnoonan May 11, 2022

lmiller1990 May 10, 2022

tbiethman May 11, 2022 •

edited

Loading

marktnoonan May 11, 2022

lmiller1990 left a comment

	const hasUtmParams = Object.keys(link.params).find((param) => param.startsWith('utm_'))
	const hasUtmParams = Object.keys(link.params).some((param) => param.startsWith('utm_'))

feat: distinguish app vs launchpad utm_source when using utm params #21424

feat: distinguish app vs launchpad utm_source when using utm params #21424

Conversation

marktnoonan commented May 10, 2022 • edited Loading

User facing changelog

Additional details

How to test manually

PR Tasks

cypress-bot bot commented May 10, 2022

cypress bot commented May 10, 2022 • edited Loading

Test summary

Run details

Flakiness

tbiethman left a comment

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 May 10, 2022

Choose a reason for hiding this comment

marktnoonan May 11, 2022

Choose a reason for hiding this comment

lmiller1990 May 10, 2022

Choose a reason for hiding this comment

tbiethman May 11, 2022 • edited Loading

Choose a reason for hiding this comment

marktnoonan May 11, 2022

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

marktnoonan commented May 10, 2022 •

edited

Loading

cypress bot commented May 10, 2022 •

edited

Loading

tbiethman May 11, 2022 •

edited

Loading