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

fix: shorten form acquisition URLs - WD-6294 #13244

Conversation

pedoch
Copy link
Contributor

@pedoch pedoch commented Oct 13, 2023

Done

  • Shorten acquisition URLs on submitted forms as described by the task: WD-6294
    • The task is to ensure the acquisition URLs are not longer than 255 characters, if they are the fbclid and gclid parameters should be removed.

QA

Quick link you can test

Issue / Card

@webteam-app
Copy link

Demo starting at https://ubuntu-com-13244.demos.haus

@pedoch pedoch requested a review from laszlokajtar October 13, 2023 16:31
@pedoch pedoch changed the title shorten form acquisition urls fix: shorten form acquisition URLs - WD-6294 Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #13244 (1a1ed4d) into main (e019fbd) will not change coverage.
Report is 5 commits behind head on main.
The diff coverage is n/a.

❗ Current head 1a1ed4d differs from pull request most recent head 068368e. Consider uploading reports for the commit 068368e to get more accurate results

@@           Coverage Diff           @@
##             main   #13244   +/-   ##
=======================================
  Coverage   74.43%   74.43%           
=======================================
  Files         107      107           
  Lines        2840     2840           
  Branches      948      948           
=======================================
  Hits         2114     2114           
  Misses        702      702           
  Partials       24       24           

Copy link
Contributor

@laszlokajtar laszlokajtar left a comment

Choose a reason for hiding this comment

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

lgtm

@pedoch
Copy link
Contributor Author

pedoch commented Oct 16, 2023

@carkod do you mind taking a look at this PR when you have the time? I ask since you commented on the previous two which I closed.

@pedoch
Copy link
Contributor Author

pedoch commented Oct 16, 2023

@laszlokajtar were you able to QA the PR (submitting a form and checking the acquisition URL on marketo)?

@laszlokajtar
Copy link
Contributor

@pedoch oh sorry, i misunderstood and wasn't sure that's required at this point.

tested now on two ubuntu.com URLs and neither test submissions had acquisition url. when i submitted a form without URL parameters, the acqusition URL was correctly identified.

@laszlokajtar
Copy link
Contributor

@pedoch on checking, it seems that the enrichment submit is not successful when there are URLs parameters. the submit arrives with the non-enrichment form (e.g. 3230) but doesn't for the enrichment one (1257)

@pedoch
Copy link
Contributor Author

pedoch commented Oct 17, 2023

@pedoch oh sorry, i misunderstood and wasn't sure that's required at this point.

@laszlokajtar I only asked if you were to QA because I'm not able to navigate marketo properly to check the result of the submission.

@pedoch on checking, it seems that the enrichment submit is not successful when there are URLs parameters. the submit arrives with the non-enrichment form (e.g. 3230) but doesn't for the enrichment one (1257)

If I understand correctly, the acquisition URL is dropped by marketo when it has URL parameters?

@carkod
Copy link
Contributor

carkod commented Oct 23, 2023

Submission of engage page forms used to work in the demo, but now it's breaking? May be related to what @laszlokajtar is saying.

@carkod
Copy link
Contributor

carkod commented Oct 23, 2023

https://ubuntu.com/engage/case-study-gmo-pepabo submission form is not working.

@pedoch
Copy link
Contributor Author

pedoch commented Oct 25, 2023

@carkod the demo seems to be up now. I replace the link with an engage page that works (the previous one seems to have a problem on prod too, may be from marketo) so could you check this again. Thank you!

@carkod
Copy link
Contributor

carkod commented Oct 25, 2023

Ideally, we should create a demo on jp.ubuntu.com pointing to this PR so we can test it, because ubuntu.com doesn't need acquisition url.

Optionally, would you be willing to write a unit test for the shorten_acquisition_url function? Basically just your QA in a test.

@pedoch
Copy link
Contributor Author

pedoch commented Oct 25, 2023

@carkod Sure, I can write a unit test for the shorten_acquisition_url function, and I'll look into creating a demo on jp.ubuntu.com that points at this PR

@pedoch pedoch force-pushed the WD-6294-modify-jp-ubuntu-com-cn-ubuntu-com-acquisition-url-to-respect-length-limit branch from d5f5046 to 1983741 Compare November 2, 2023 11:24
@pedoch pedoch force-pushed the WD-6294-modify-jp-ubuntu-com-cn-ubuntu-com-acquisition-url-to-respect-length-limit branch from 1983741 to 1a1ed4d Compare November 2, 2023 11:25
tests/test_views.py Outdated Show resolved Hide resolved
@carkod
Copy link
Contributor

carkod commented Nov 2, 2023

Left a comment in the demo PR

Based on failed test cases and sentry submission errors
@pedoch pedoch force-pushed the WD-6294-modify-jp-ubuntu-com-cn-ubuntu-com-acquisition-url-to-respect-length-limit branch from 1a1ed4d to 1534255 Compare November 2, 2023 16:33
@pedoch
Copy link
Contributor Author

pedoch commented Nov 2, 2023

@carkod I was finally able to access sentry and I figured out what the problem was and fixed it. You can test the jp.ubuntu.com submission again.

Copy link
Contributor

@carkod carkod left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks. LGTM

@pedoch pedoch merged commit 3ab727d into canonical:main Nov 2, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants