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
Added functionality to filter getReportMetadata and getSegmentsMetada in SystemTestCase #18141
Conversation
…ta API response, #DEV-2366
…set and get, #DEV-2366
@AltamashShaikh I think it might be useful to add or change a test in core that uses that new possibility so we can directly see how and if it works. That might also prevent future regressions. You could for example change the custom dimension tests:
|
Piwik::addAction('API.API.getSegmentsMetadata.end', function (&$reports, $info) use ($allowedModuleForApiSegmentsReport) { | ||
$this->filterReportsCallback($reports, $info, $allowedModuleForApiSegmentsReport); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be moved to the setUpBeforeClass
method. Calling it in setUp
causes each test to add a new observer. So running 10 tests would end up having 10 event observers in the last test.
If the event observer is added in the setUpBeforeClass
, you could also always add it like:
Piwik::addAction('API.getReportMetadata.end', function (&$reports, $info) {
$allowedModuleForApiMetadataReport = self::getAllowedModuleToFilterApiResponse('API.getReportMetadata');
if ($allowedModuleForApiMetadataReport) {
self::filterReportsCallback($reports, $info, $allowedModuleForApiMetadataReport);
}
});
Piwik::addAction('API.API.getSegmentsMetadata.end', function (&$reports, $info) {
$allowedModuleForApiSegmentsReport = self::getAllowedModuleToFilterApiResponse('API.getSegmentsMetadata');
if ($allowedModuleForApiSegmentsReport) {
self::filterReportsCallback($reports, $info, $allowedModuleForApiSegmentsReport);
}
});
Note: EvenObserver will be cleared when a Fixture is teared down, so the shouldn't be a need to remove the observers manually.
…or getReportMetadata, #DEV-2366
…case file, #DEV-2366
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now. Once travis tests are running again and we can see if all tests are still passing, this should be good to merge
Added functionality to filter getReportMetadata and getSegmentsMetada in SystemTestCase
Issue: #DEV-2366
Description:
Please include a description of this change and which issue it fixes. If no issue exists yet please include context and what problem it solves.
Review