Add newsletter signup button to user settings page #14466
Conversation
katebutler
added
the
Pull Request WIP
# Conflicts: # plugins/Installation/Controller.php # tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png
katebutler
added
Needs Review
|
Ready for review/merge |
plugins/UsersManager/angularjs/personal-settings/personal-settings.controller.js
Outdated
Show resolved
Hide resolved
plugins/UsersManager/angularjs/personal-settings/personal-settings.controller.js
Outdated
Show resolved
Hide resolved
| ng-click="personalSettings.signupForNewsletter()" | ||
| >{{ 'General_Yes'|translate }}</button> | ||
| </div> | ||
| {% endif %} |
There was a problem hiding this comment.
@mattab Should there be any specific copy in this new section? The screenshot makes the section look a bit bare.
|
In the personal settings page, should we mention this block maybe very first? and I suppose we need to maybe show a privacy notice, similar to on our website https://matomo.org/newsletter : |
|
fyi @katebutler see previous comment |
|
Actually, I reckon by the time we show the privacy notice it'll take too much space. Also to be safe re GDPR it might be good to show an unchecked checkbox and the button be disabled initially ( |
|
Have added a short privacy notice and a checkbox. |
| public static function signupForNewsletter($userLogin, $email, $matomoOrg = false, $professionalServices = false) | ||
| { | ||
| // Don't bother if they aren't signing up for at least one newsletter | ||
| if (! ($matomoOrg || $professionalServices)) { |
There was a problem hiding this comment.
We will also need to return if Piwik\SettingsPiwik::isInternetEnabled() is disabled
There was a problem hiding this comment.
@katebutler I left a few comments and also converted the PR to use angular instead of jquery, and our built-in form components and notification component see 8d83f66 Can show you what it does tmrw. Now also when the checkbox is disabled, the confirm button will be disabled.
plugins/UsersManager/Controller.php
Outdated
| @@ -183,6 +185,9 @@ public function userSettings() | |||
| $view->userTokenAuth = Piwik::getCurrentUserTokenAuth(); | |||
| $view->ignoreSalt = $this->getIgnoreCookieSalt(); | |||
|
|
|||
| $newsletterSignupOptionKey = 'UsersManager.newsletterSignup.' . $userLogin; | |||
There was a problem hiding this comment.
Be good to use a constant for this value 'UsersManager.newsletterSignup.'
plugins/UsersManager/Controller.php
Outdated
| @@ -183,6 +185,9 @@ public function userSettings() | |||
| $view->userTokenAuth = Piwik::getCurrentUserTokenAuth(); | |||
| $view->ignoreSalt = $this->getIgnoreCookieSalt(); | |||
|
|
|||
| $newsletterSignupOptionKey = 'UsersManager.newsletterSignup.' . $userLogin; | |||
| $view->showNewsletterSignup = Option::get($newsletterSignupOptionKey) === false; | |||
There was a problem hiding this comment.
We could also not show the box if Piwik\SettingsPiwik::isInternetEnabled() is disabled
plugins/UsersManager/Controller.php
Outdated
| @@ -405,6 +410,24 @@ public function recordUserSettings() | |||
| return $toReturn; | |||
| } | |||
|
|
|||
| public function newsletterSignup() | |||
There was a problem hiding this comment.
@katebutler ideally we would put this into an API so we could implement for example the same action in the mobile app or somewhere else. (of course the mobile API would also need an API method whether it has been submitted already or not but that's not needed for now).
Once this is in the API, it'll be simply return true/false and it should simplify things. Only need to slightly adjust the JavaScript that calls it like {module: 'API', method: 'UsersManager.signupNewsletter'}
|
BTW there's also a merge conflict |
# Conflicts: # tests/UI/expected-screenshots/UIIntegrationTest_admin_user_settings.png
|
@katebutler could you fix/update the expected screenshots for these 3 screenshots and then merge? thanks
|
|
@katebutler there's still a failing test BTW: https://builds-artifacts.matomo.org/matomo-org/matomo/3.x-dev/35326/UserSettings_signup_success.png The expected message is no longer showing as the success message is now shown in a success notification. We can usually take a screnshot of that using |
Fixes #14118