-
Notifications
You must be signed in to change notification settings - Fork 17
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 taxonomy topic email signup button #2943
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for picking this up Catalina!
@@ -5,7 +5,7 @@ | |||
> | |||
<%= render "govuk_publishing_components/components/signup_link", { | |||
link_text: "Get emails about this topic", | |||
link_href: "/email-signup/?link=#{base_path}", | |||
link_href: "/email-signup/confirm?topic=#{base_path}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make it clearer that this is an override for a subset of document collection pages ONLY. Someone in the future might use this partial assuming it's just a normal taxon signup link - but actually it's not, it links to the confirmation page.
I reckon the best way to do this is to replace the taxon_signup_link partial with a document collections email signup partial, and put all this logic into there.
We should also have an integration test for document_collection_email_subscriptions, where we could test that the correct signup link is displayed for the variety of different content items that could be passed to this application. By way of documentation really, as this is a complicated thing and could easily be broken!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code. Not sure what other test scenarios should I add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catalina and I were wondering if you could advise us on something here.
Signing up for emails on https://www.gov.uk/money/dealing-with-hmrc-paying-hmrc-shares takes you to:
https://www.gov.uk/email-signup?link=/money/dealing-with-hmrc-paying-hmrc-shares
This PR overrides the email sign up for some document collections, adding a link to sign up to a related taxon instead of adding the single page notification button. And this link will take the user to:
https://www.gov.uk/email-signup/confirm?topic=/money/dealing-with-hmrc-paying-hmrc-shares
Note that one is /confirm
and the other isn't.
You can see that the view rendered at each of those URL's look the same, and they both result in the same email subscription. So from out POV all good. But we wanted to check if there's any relevance of that different entry point on the tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hannako It looks ok to me 👍 It might be worth letting the analysts in our govuk-ga4 Slack channel know that the form action will change, as I think this change would only affect the data coming through to their dashboards rather than breaking anything us devs have written.
@andysellick Was the one who did our email subscription tracking, so he might be able to spot something code wise that I can't see!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andysellick is this something you could take a look at, or something I should pick up with a PA? TLDR: Does it matter to analytics where the user is taken when they click on the email sign up link ie is the destination important. Or is only the click of interest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hannako - I believe he has asked the PAs here https://gds.slack.com/archives/C02HKNAEGAZ/p1698044960768499 - I think it should be OK to go ahead with, as the PAs said they haven't published the email tracking GA4 wise, so if the data coming through is incorrect it wouldn't impact their production dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looping me in @AshGDS I've added a comment to that thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what Ash said! Sorry, forgot about this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all!
When a document collection is tagged to a level one taxon, we want the email signup link to send the user to the confirmation page instead of the select subtopics page.
fd19863
to
4d2c8a4
Compare
When a document collection is tagged to a level one taxon, we want the email signup link to send the user to the confirmation page instead of the select subtopics page.
Example on the email signup link: /email-signup/confirm?topic=/education/inspection-and-performance-of-schools
Trello card: https://trello.com/c/dJvwL0sU/2081-update-taxonomy-topic-email-signup-button-m
Follow these steps if you are doing a Rails upgrade.