-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Installer: Fix issues with newsletter signup #20705
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
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.
Pull Request Overview
This PR refactors the installer's user creation step to improve newsletter subscription handling and logging. The changes modernize the newsletter subscription process by updating the endpoint and improving error handling with proper logging.
Key changes:
- Adds logger injection to CreateUserStep for better observability of newsletter subscription operations
- Updates newsletter subscription to use new endpoint (
emailcollector.umbraco.io/api/EmailProxy) with proper data model - Replaces silent failure handling with structured logging for subscription attempts
- Removes username logging from telemetry consent for privacy reasons
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Umbraco.Infrastructure/Installer/Steps/CreateUserStep.cs | Added logger parameter, new obsolete constructor for backward compatibility, updated newsletter subscription logic with new endpoint and EmailModel, improved error handling with logging, removed unused using statements |
| src/Umbraco.Core/Services/MetricsConsentService.cs | Removed username from telemetry level logging and formatted code for readability |
| src/Umbraco.Cms.Api.Management/ViewModels/Installer/UserInstallRequestModel.cs | Changed SubscribeToNewsletter property from get-only to get/set to allow proper data binding |
Migaroez
left a comment
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.
Looks good and works as described.
Prerequisites
Please also read the internal chat in the CMS group Slack channel.
Description
Investigating the newsletter sign-up feature on the installation screen I found a few issues:
false.FormUrlEncodedwas incorrectly JSONThis PR fixes this by:
One little addition - I removed the logging of the user name (possibly email address) when recording the telemetry selection, as we shouldn't really have this in the logs.
Also fixed indentation, so best to review with "hide whitespace".
Testing
Install Umbraco and tick the box to sign-up with an email address that won't already be signed up for Umbraco newsletters.
You should get an email from "Lars from Umbraco" indicating that the sign-up has completed.
The success or otherwise is also written to the log file.