Skip to content

Ask users to leave a review for Matomo #14432

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.

Sign up for GitHub

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
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 •
edited
Loading

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 •
edited by Findus23
Loading

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

Findus23
Findus23 requested changes May 28, 2019
View reviewed changes
Copy link
Collaborator

@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 added 3 commits June 12, 2019 09:03
@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

mattab
mattab reviewed Jun 18, 2019
View reviewed changes
plugins/Feedback/Controller.php Outdated
{
$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?

mattab
mattab reviewed Jun 18, 2019
View reviewed changes
plugins/Feedback/angularjs/feedback-popup/feedback-popup.directive.html Outdated
<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)

mattab
mattab reviewed Jun 18, 2019
View reviewed changes
plugins/Feedback/Controller.php
*/
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
Closed
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

@Findus23 Findus23 Findus23 requested changes

Assignees
No one assigned
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.
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.

Ask users to leave a review for Matomo
3 participants
@katebutler @mattab @Findus23