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

Remove a var_dump /exit #10410

Closed
wants to merge 1 commit into from
Closed

Remove a var_dump /exit #10410

wants to merge 1 commit into from

Conversation

mattab
Copy link
Member

@mattab mattab commented Aug 16, 2016

along with a tiny refactor

@mattab mattab added the Needs Review PRs that need a code review label Aug 16, 2016
@mattab mattab added this to the 3.0.0-b1 milestone Aug 16, 2016
@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 Aug 16, 2016
@sgiehl
Copy link
Member

sgiehl commented Aug 17, 2016

LGTM

private function shouldReportBeIgnored($reportMetadata)
{
// Some plugins may not define report categories
if(empty($reportMetadata['category'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace missing

@tsteur
Copy link
Member

tsteur commented Aug 18, 2016

ideally no merge of this before we haven't merged the UI PR

$reportMetadata['category'] == 'API' ||
$reportMetadata['category'] == Piwik::translate('General_MultiSitesSummary') && $reportMetadata['name'] == Piwik::translate('General_SingleWebsitesDashboard')
) {
if ($this->shouldReportBeIgnored($reportMetadata)) {
Copy link
Member

@tsteur tsteur Aug 18, 2016

Choose a reason for hiding this comment

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

Can you add a unit or an integration test for this method getReportMetadata?

@tsteur
Copy link
Member

tsteur commented Aug 30, 2016

There is now a merge conflict. I think I have removed the var_dump as well when I made the tests green for the Piwik 3 UI branch

@mattab
Copy link
Member Author

mattab commented Sep 19, 2016

Already fixed in 3.x-dev branch

@mattab mattab closed this Sep 19, 2016
@mattab mattab deleted the mattab-patch-1 branch October 1, 2016 21:18
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.

None yet

3 participants