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

Update newsletter/firefox page (issue #15075) #15101

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

alexgibson
Copy link
Member

@alexgibson alexgibson commented Sep 10, 2024

One-line summary

Important

This needs to be live before Monday 16th Sept

  • Updates the /newsletter/firefox/ page with simplified layout and new copy.
  • Adds A/B header image test for en-US, de, and fr languages.

Issue / Bugzilla link

#15075

Testing

Version A (fox tail image):

http://localhost:8000/en-US/newsletter/firefox/?v=1 (en-US)
http://localhost:8000/en-GB/newsletter/firefox/ (en-GB)
http://localhost:8000/de/newsletter/firefox/?v=1 (de)
http://localhost:8000/fr/newsletter/firefox/?v=1 (fr)
http://localhost:8000/es-ES/newsletter/firefox/ (es-ES)
http://localhost:8000/it/newsletter/firefox/ (it)
http://localhost:8000/nl/newsletter/firefox/ (nl)
http://localhost:8000/pl/newsletter/firefox/ (pl)
http://localhost:8000/el/newsletter/firefox/ (other locales with fallback copy)

Version B (alternate images):

http://localhost:8000/en-US/newsletter/firefox/?v=2 (en-US)
http://localhost:8000/de/newsletter/firefox/?v=2 (de)
http://localhost:8000/fr/newsletter/firefox/?v=2 (fr)

Experiment URL:

http://localhost:8000/en-US/newsletter/firefox/

  • Redirects to experiment variation for en-US, de, and fr (test with DNT/GPC disabled)

@alexgibson alexgibson force-pushed the firefox-newsletter-landing branch 2 times, most recently from 62f4b20 to 9d18938 Compare September 10, 2024 12:26
@alexgibson alexgibson added P2 Second level priority - Should have Needs Review Awaiting code review Review: XS Code review time: up to 30min Frontend HTML, CSS, JS... client side stuff labels Sep 10, 2024
@alexgibson alexgibson marked this pull request as ready for review September 10, 2024 12:31
@reemhamz reemhamz self-assigned this Sep 11, 2024
@alexgibson alexgibson added P1 First level priority - Must have and removed P2 Second level priority - Should have labels Sep 11, 2024
@craigcook craigcook assigned craigcook and unassigned reemhamz Sep 12, 2024
@stephaniehobson stephaniehobson self-assigned this Sep 12, 2024
Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

We've changed how we record experiment data for GA4, please update to use the new syntax. (also, if you copy and pasted this code from somewhere, we should file an issue to update that too).

Also, there was a problem getting it running locally for me. Not sure if it was me or if you'll see it to if you stop and then re-start your dev server.

Oh... and it needs a rebase ;)

# Firefox newsletter A/B test. See issue 15075
path(
"newsletter/firefox/",
VariationTemplateView.as_view(
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I the error NameError: name 'VariationTemplateView' is not defined when I try to start bedrock.

I fixed it by adding from bedrock.utils.views import VariationTemplateView to this file so I could do the review. I'm not sure I am doing something wrong or if that is actually necessary. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, looks like the line was removed in https://github.com/mozilla/bedrock/pull/15119/files#diff-05181ef311764e79e4e0f540afdb3d341a0294fed3347b5c4386a810b73c0783

I'll rebase and add it back here, thanks!

@alexgibson alexgibson removed the Needs Review Awaiting code review label Sep 13, 2024
@alexgibson
Copy link
Member Author

@stephaniehobson updated thanks

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.83%. Comparing base (6fa3b0e) to head (1377242).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15101   +/-   ##
=======================================
  Coverage   77.83%   77.83%           
=======================================
  Files         162      162           
  Lines        8458     8459    +1     
=======================================
+ Hits         6583     6584    +1     
  Misses       1875     1875           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

R+ 🦊

@stephaniehobson stephaniehobson merged commit cea46a3 into mozilla:main Sep 13, 2024
6 checks passed
@alexgibson alexgibson deleted the firefox-newsletter-landing branch September 13, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend HTML, CSS, JS... client side stuff P1 First level priority - Must have Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants