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

Ask users to leave a review for Matomo #14432

Merged
merged 16 commits into from Jun 18, 2019
Merged

Ask users to leave a review for Matomo #14432

merged 16 commits into from Jun 18, 2019

Conversation

katebutler
Copy link

@katebutler katebutler commented May 9, 2019

Fixes #14086

@katebutler katebutler added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 9, 2019
@katebutler katebutler added this to the 3.11.0 milestone May 9, 2019
@katebutler
Copy link
Author

katebutler commented May 12, 2019

Sample screenshot below. @Findus23 Thomas has suggested that you may be able to help with the logos for the review sites - the Product Hunt one is OK as they provide official branding resources, but the others I've just taken off the internet. They need transparent backgrounds and we may need to play around with the size a bit as well, particularly for SaasWorthy.

review-matomo-popup

Copy link
Member

@Findus23 Findus23 left a comment

Choose a reason for hiding this comment

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

Hi @katebutler,

Could you please update the images with the SVGs I sent you?
Thanks

@mattab
Copy link
Member

mattab commented Jun 10, 2019

@katebutler can you check @Findus23 comment from 13 days ago?

@mattab mattab added the Needs Review PRs that need a code review label Jun 10, 2019
@katebutler
Copy link
Author

Have replaced the icons with the SVGs. Has caused quite a few UI tests to fail because of the popup showing up over the top of the expected dashboard. My own test that the popup doesn't show up when it's not expected is also failing so hopefully once I fix that it will sort the others out.

@katebutler katebutler removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jun 18, 2019
@katebutler
Copy link
Author

Ready for review/merge

{
$nextReminder = Common::getRequestVar('nextReminder');
// -1 means "never remind me again", otherwise add the interval onto today's date
if ((int)$nextReminder !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe could use a const named appropriately and remove the comment?

<div ng-include="'plugins/Feedback/angularjs/feedback-popup/review-links.directive.html'"></div>

<div class="footer">
<input type="button" value="Remind me later" role="yes"/>
Copy link
Member

Choose a reason for hiding this comment

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

this string is not translated yet (line below as well)

*/
public function updateFeedbackReminderDate()
{
$nextReminder = Common::getRequestVar('nextReminder');
Copy link
Member

Choose a reason for hiding this comment

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

maybe add auth check, that user is not anonymous?

@mattab
Copy link
Member

mattab commented Jun 18, 2019

@katebutler noticed tests are failing with:

There were 3 errors:
1) Piwik\Plugins\Feedback\tests\Unit\FeedbackTest::test_shouldPromptForFeedback_nextReminderDateInPast
Exception: General_ExceptionInvalidDateBeforeFirstWebsite: 2019
/home/travis/build/matomo-org/matomo/core/Date.php:161
/home/travis/build/matomo-org/matomo/plugins/Feedback/Feedback.php:96
/home/travis/build/matomo-org/matomo/plugins/Feedback/tests/Integration/FeedbackTest.php:100
2) Piwik\Plugins\Feedback\tests\Unit\FeedbackTest::test_shouldPromptForFeedack_nextReminderDateToday
Exception: General_ExceptionInvalidDateBeforeFirstWebsite: 2019
/home/travis/build/matomo-org/matomo/core/Date.php:161
/home/travis/build/matomo-org/matomo/plugins/Feedback/Feedback.php:96
/home/travis/build/matomo-org/matomo/plugins/Feedback/tests/Integration/FeedbackTest.php:108
3) Piwik\Plugins\Feedback\tests\Unit\FeedbackTest::test_shouldPromptForFeedack_nextReminderDateInFuture
Exception: General_ExceptionInvalidDateBeforeFirstWebsite: 2019
/home/travis/build/matomo-org/matomo/core/Date.php:161
/home/travis/build/matomo-org/matomo/plugins/Feedback/Feedback.php:96
/home/travis/build/matomo-org/matomo/plugins/Feedback/tests/Integration/FeedbackTest.php:116

@mattab
Copy link
Member

mattab commented Jun 18, 2019

PR looks good otherwise, looking forward to merging this 👍

@katebutler
Copy link
Author

FeedbackTest is now fixed.

@mattab mattab merged commit 57f7184 into 3.x-dev Jun 18, 2019
@mattab mattab deleted the 14086 branch June 18, 2019 04:45
@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 Jun 29, 2019
@tsteur tsteur mentioned this pull request Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask users to leave a review for Matomo
3 participants