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

Jump to bottom

Issue #11450 - Email clarification for new version #11542

Merged
merged 5 commits into from Mar 28, 2017
Merged

Issue #11450 - Email clarification for new version #11542

merged 5 commits into from Mar 28, 2017

Conversation

fbrzozowski
Copy link
Contributor

That should resolve #11450
Add clarification in emails about new version for super users. Unfortunately I don't know all languages so it needs to be translated.

@fbrzozowski
Issue #11450
ed5d25a 
Add clarification in emails about new version for super users
@sgiehl
Copy link
Member

sgiehl commented Mar 27, 2017

Thanks for the PR.
Please do not change any language files other than en.json. this will be automatically done later

@fbrzozowski
Copy link
Contributor Author

@sgiehl Sorry this is my first time - didn't know that. Thanks for info/advice. :)

@mattab
Copy link
Member

mattab commented Mar 27, 2017

Thanks and congrats @fbrzozowski on your first PR 👍

Here is my feedback:

  • remove the languages changes except in en.json (our translators use transifex to translate PIwik from English and we have automated commands to fetch translations from there)
  • in the english string, change it to [...] on the Piwik at: %s and then in the code, replace the placeholder by the value. This is better than appending the value because in some languages they want the value in the middle or some other places and it couldn't be done if we manually put it at the end.

@mattab mattab added this to the 3.0.3 milestone Mar 27, 2017
@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Mar 27, 2017
mattab
mattab reviewed Mar 28, 2017
View reviewed changes
plugins/CoreUpdater/UpdateCommunication.php Outdated
@@ -79,6 +79,8 @@ protected function sendNotifications()
$message .= "\n\n";
}

$message .= Piwik::translate('CoreUpdater_ReceiveEmailBecauseIsSuperUser', $host);
$message .= "\n";
Copy link
Member

Choose a reason for hiding this comment

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

Change this to \n\n to fix the test

@mattab
Copy link
Member

mattab commented Mar 28, 2017

Changes look good, and now: there are now a couple integration tests failing:

1) Piwik\Plugins\CoreUpdater\Test\UpdateCommunicationTest::test_sendNotifications_shouldSentCorrectEmail

Expectation failed for method name is equal to <string:sendEmailNotification> when invoked 1 time(s)

Parameter 1 for invocation Piwik\Plugins\CoreUpdater\UpdateCommunication::sendEmailNotification('CoreUpdater_NotificationSubje...Update', 'ScheduledReports_EmailHello\n\n...ntact/') does not match expected value.

Failed asserting that two strings are equal.

--- Expected

+++ Actual

@@ @@

 CoreUpdater_ReceiveEmailBecauseIsSuperUser

-

 CoreUpdater_FeedbackRequest

 http://piwik.org/contact/'

/home/travis/build/piwik/piwik/plugins/CoreUpdater/UpdateCommunication.php:88

/home/travis/build/piwik/piwik/plugins/CoreUpdater/UpdateCommunication.php:55

/home/travis/build/piwik/piwik/plugins/CoreUpdater/Test/Integration/UpdateCommunicationTest.php:118

/home/travis/build/piwik/piwik/plugins/CoreUpdater/Test/Integration/UpdateCommunicationTest.php:87

2) Piwik\Plugins\CoreUpdater\Test\UpdateCommunicationTest::test_sendNotifications_shouldNotIncludeChangelogIfNotMajorVersionUpdate

Expectation failed for method name is equal to <string:sendEmailNotification> when invoked 1 time(s)

Parameter 1 for invocation Piwik\Plugins\CoreUpdater\UpdateCommunication::sendEmailNotification('CoreUpdater_NotificationSubje...Update', 'ScheduledReports_EmailHello\n\n...ntact/') does not match expected value.

Failed asserting that two strings are equal.

--- Expected

+++ Actual

@@ @@

 CoreUpdater_ReceiveEmailBecauseIsSuperUser

-

 CoreUpdater_FeedbackRequest

 http://piwik.org/contact/'

@mattab mattab merged commit f9c555e into matomo-org:3.x-dev Mar 28, 2017
@mattab
Copy link
Member

mattab commented Mar 28, 2017

Well done @fbrzozowski for your first PR. We're looking forward to your next ones 🎉

@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattab mattab mattab left review comments

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
3.0.3
Development

Successfully merging this pull request may close these issues.

Clarify that emails about new versions are sent to Super Users
3 participants
@fbrzozowski @sgiehl @mattab