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 option to disable UTM query param #227

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Conversation

jrlarano
Copy link
Contributor

@jrlarano jrlarano commented Apr 5, 2024

@jrlarano jrlarano requested review from klarstrup and mortenbo April 5, 2024 11:34
Copy link
Member

@mortenbo mortenbo left a comment

Choose a reason for hiding this comment

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

It feels a bit opposite deleting utm - vs. not setting it if disabled.

@jrlarano
Copy link
Contributor Author

jrlarano commented Apr 8, 2024

Yeah, but this is because the webshop_link from offer details coming from Squid always contain UTMs. We only added an option to overwrite its default values and an option to remove it if it's disabled.

@mortenbo
Copy link
Member

mortenbo commented Apr 8, 2024

Okay. Why the option to remove the params in the first place?

@jrlarano
Copy link
Contributor Author

jrlarano commented Apr 8, 2024

It's because one of our clients don't like having UTM params when they redirect to the webshop_link

Copy link
Member

@mortenbo mortenbo left a comment

Choose a reason for hiding this comment

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

I think the real solution is to add a new field in Squid so you have two URLs: with and without UTM and then the SDK uses the one without, but this is fine for now.

@mortenbo
Copy link
Member

mortenbo commented Apr 8, 2024

Actually, maybe we should completely remove UTM parameters in general using the SDK instead of it being an option? They are not needed when it's the client themselves integrating.

@mortenbo
Copy link
Member

mortenbo commented Apr 8, 2024

Nvm my last comment. We'll make it default in the CMS instead, Jakob informed me.

@jrlarano
Copy link
Contributor Author

jrlarano commented Apr 8, 2024

@mortenbo I see. Does this mean that we won't need this change now?

@mortenbo
Copy link
Member

mortenbo commented Apr 8, 2024

Yes, as long as we in the CMS add disable-utm param as a default param in the integration output.

@jrlarano jrlarano closed this Apr 8, 2024
@Jakob-tjek
Copy link

Jakob-tjek commented Apr 11, 2024

There has been some misunderstandings here. This PR is still valid.
But another PR for the CMS is required and will add "data-component-publication-disable-utm="true" to the integration code if no custom UTM is set:
image
This will make sure we don't change any existing logic for the integrations out there, but at the same time give clients the option to add this line if they want to disable UTM. And new integrations will not get UTM's because of this extra line in the integration code.

@Jakob-tjek Jakob-tjek reopened this Apr 11, 2024
@jrlarano jrlarano merged commit 7c9d206 into develop Apr 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants