Skip to content

Commit

Permalink
Add notification when report w/ segment has no data, but segment is u…
Browse files Browse the repository at this point in the history
…nprocessed (#12823)

* When a report has no data, check if it is for an unprocessed segment and display an explanatory notification.

* Remove transient notifications on reporting page change, allow datatable views to add notifications, use to display unprocessed segment message.

* Update changelog.

* Internationalize unprocessed segment message.

* Parse notification divs in dashboardWidget.js when setting widget content.

* Tweak message.

* Change PR to use different approach: throw exception when no archives found and segment is used, then detect exception in new event.

* Update changelog + document new event.

* Unfinished review changes

* more review fixes

* Do not show notification w/ custom segments.

* apply pr review

* Apply review.

* undo escaping since it was not needed & get test to pass

* Different strategy + new test.

* Fix tests.

* Update two expected screenshots.
  • Loading branch information
diosmosis authored and mattab committed Aug 9, 2018
1 parent 6112cc2 commit 2b77bb4
Show file tree
Hide file tree
Showing 33 changed files with 795 additions and 51 deletions.
50 changes: 44 additions & 6 deletions core/API/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@
*/
class Request
{
/**
* The count of nested API request invocations. Used to determine if the currently executing request is the root or not.
*
* @var int
*/
private static $nestedApiInvocationCount = 0;

private $request = null;

/**
Expand Down Expand Up @@ -223,6 +230,7 @@ public function process()
$shouldReloadAuth = false;

try {
++self::$nestedApiInvocationCount;

// IP check is needed here as we cannot listen to API.Request.authenticate as it would then not return proper API format response.
// We can also not do it by listening to API.Request.dispatch as by then the user is already authenticated and we want to make sure
Expand Down Expand Up @@ -258,6 +266,8 @@ public function process()
Log::debug($e);

$toReturn = $response->getResponseException($e);
} finally {
--self::$nestedApiInvocationCount;
}

if ($shouldReloadAuth) {
Expand Down Expand Up @@ -297,11 +307,11 @@ public static function getClassNameAPI($plugin)
/**
* @ignore
* @internal
* @param bool $isRootRequestApiRequest
* @param string $currentApiMethod
*/
public static function setIsRootRequestApiRequest($isRootRequestApiRequest)
public static function setIsRootRequestApiRequest($currentApiMethod)
{
Cache::getTransientCache()->save('API.setIsRootRequestApiRequest', $isRootRequestApiRequest);
Cache::getTransientCache()->save('API.setIsRootRequestApiRequest', $currentApiMethod);
}

/**
Expand All @@ -313,8 +323,22 @@ public static function setIsRootRequestApiRequest($isRootRequestApiRequest)
*/
public static function isRootRequestApiRequest()
{
$isApi = Cache::getTransientCache()->fetch('API.setIsRootRequestApiRequest');
return !empty($isApi);
$apiMethod = Cache::getTransientCache()->fetch('API.setIsRootRequestApiRequest');
return !empty($apiMethod);
}

/**
* Checks if the currently executing API request is the root API request or not.
*
* Note: the "root" API request is the first request made. Within that request, further API methods
* can be called programmatically. These requests are considered "child" API requests.
*
* @return bool
* @throws Exception
*/
public static function isCurrentApiRequestTheRootApiRequest()
{
return self::$nestedApiInvocationCount == 1;
}

/**
Expand All @@ -329,11 +353,25 @@ public static function isRootRequestApiRequest()
* @throws Exception
*/
public static function isApiRequest($request)
{
$method = self::getMethodIfApiRequest($request);
return !empty($method);
}

/**
* Returns the current API method being executed, if the current request is an API request.
*
* @param array $request eg array('module' => 'API', 'method' => 'Test.getMethod')
* @return string|null
* @throws Exception
*/
public static function getMethodIfApiRequest($request)
{
$module = Common::getRequestVar('module', '', 'string', $request);
$method = Common::getRequestVar('method', '', 'string', $request);

return $module === 'API' && !empty($method) && (count(explode('.', $method)) === 2);
$isApi = $module === 'API' && !empty($method) && (count(explode('.', $method)) === 2);
return $isApi ? $method : null;
}

/**
Expand Down
6 changes: 5 additions & 1 deletion core/Archive.php
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,12 @@ protected function get($archiveNames, $archiveDataType, $idSubtable = null)
$dataNames, $archiveDataType, $this->params->getIdSites(), $this->params->getPeriods(), $defaultRow = null);

$archiveIds = $this->getArchiveIds($archiveNames);

if (empty($archiveIds)) {
/**
* Triggered when no archive data is found in an API request.
* @ignore
*/
Piwik::postEvent('Archive.noArchivedData');
return $result;
}

Expand Down
2 changes: 1 addition & 1 deletion core/ArchiveProcessor/Rules.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static function shouldProcessReportsAllPlugins(array $idSites, Segment $s
* @param $idSites
* @return array
*/
private static function getSegmentsToProcess($idSites)
public static function getSegmentsToProcess($idSites)
{
$knownSegmentsToArchiveAllSites = SettingsPiwik::getKnownSegmentsToArchive();

Expand Down
21 changes: 20 additions & 1 deletion core/Plugin/Visualization.php
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,12 @@ public function render()
}

$view = new View("@CoreHome/_dataTable");
$view->assign($this->templateVars);

if (!empty($loadingError)) {
$view->error = $loadingError;
}

$view->assign($this->templateVars);
$view->visualization = $this;
$view->visualizationTemplate = static::TEMPLATE_FILE;
$view->visualizationCssClass = $this->getDefaultDataTableCssClass();
Expand Down Expand Up @@ -243,10 +243,29 @@ public function render()
$view->reportLastUpdatedMessage = $this->reportLastUpdatedMessage;
$view->footerIcons = $this->config->footer_icons;
$view->isWidget = Common::getRequestVar('widget', 0, 'int');
$view->notifications = [];

if (empty($this->dataTable) || !$this->hasAnyData($this->dataTable)) {
/**
* @ignore
*/
Piwik::postEvent('Visualization.onNoData', [$view]);
}

return $view->render();
}

private function hasAnyData(DataTable\DataTableInterface $dataTable)
{
$hasData = false;
$dataTable->filter(function (DataTable $table) use (&$hasData) {
if ($table->getRowsCount() > 0) {
$hasData = true;
}
});
return $hasData;
}

/**
* @internal
*/
Expand Down
10 changes: 10 additions & 0 deletions core/View.php
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,16 @@ public function __isset($name)
return isset($this->templateVars[$name]);
}

/**
* Unsets a template variable.
*
* @param string $name The name of the template variable.
*/
public function __unset($name)
{
unset($this->templateVars[$name]);
}

/** @var Twig */
static $twigCached = null;

Expand Down
2 changes: 1 addition & 1 deletion plugins/API/API.php
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ public function registerEvents()

public function detectIsApiRequest()
{
Request::setIsRootRequestApiRequest(Request::isApiRequest($request = null));
Request::setIsRootRequestApiRequest(Request::getMethodIfApiRequest($request = null));
}

public function getStylesheetFiles(&$stylesheets)
Expand Down
2 changes: 1 addition & 1 deletion plugins/CoreHome/CoreHome.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@ public function getJsFiles(&$jsFiles)
$jsFiles[] = "libs/jqplot/jqplot-custom.min.js";
$jsFiles[] = "plugins/CoreHome/javascripts/color_manager.js";
$jsFiles[] = "plugins/CoreHome/javascripts/notification.js";
$jsFiles[] = "plugins/CoreHome/javascripts/notification_parser.js";
$jsFiles[] = "plugins/CoreHome/javascripts/numberFormatter.js";
$jsFiles[] = "plugins/CoreHome/javascripts/zen-mode.js";
$jsFiles[] = "plugins/CoreHome/javascripts/noreferrer.js";
Expand Down Expand Up @@ -237,6 +236,7 @@ public function getJsFiles(&$jsFiles)

$jsFiles[] = "plugins/CoreHome/angularjs/notification/notification.controller.js";
$jsFiles[] = "plugins/CoreHome/angularjs/notification/notification.directive.js";
$jsFiles[] = "plugins/CoreHome/angularjs/notification/notification.service.js";

$jsFiles[] = "plugins/CoreHome/angularjs/ajax-form/ajax-form.controller.js";
$jsFiles[] = "plugins/CoreHome/angularjs/ajax-form/ajax-form.directive.js";
Expand Down
53 changes: 53 additions & 0 deletions plugins/CoreHome/angularjs/notification/notification.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*!
* Piwik - free/libre analytics platform
*
* @link http://piwik.org
* @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
*/
(function () {
angular.module('piwikApp').factory('notifications', NotificationFactory);

NotificationFactory.$inject = [];

function NotificationFactory() {

return {
parseNotificationDivs: parseNotificationDivs,
clearTransientNotifications: clearTransientNotifications,
};

function parseNotificationDivs() {
var UI = require('piwik/UI');

var $notificationNodes = $('[data-role="notification"]');

$notificationNodes.each(function (index, notificationNode) {
$notificationNode = $(notificationNode);
var attributes = $notificationNode.data();
var message = $notificationNode.html();

if (message) {
var notification = new UI.Notification();
attributes.animate = false;
notification.show(message, attributes);
}

$notificationNodes.remove();
});
}

function clearTransientNotifications() {
$('[piwik-notification][type=transient]').each(function () {
var $element = angular.element(this);
$element.scope().$destroy();
$element.remove();
});
}
}

angular.module('piwikApp').run(['notifications', function (notifications) {
$(function () {
notifications.parseNotificationDivs();
});
}]);
})();
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
(function () {
angular.module('piwikApp').controller('ReportingPageController', ReportingPageController);

ReportingPageController.$inject = ['$scope', 'piwik', '$rootScope', '$location', 'reportingPageModel', 'reportingPagesModel'];
ReportingPageController.$inject = ['$scope', 'piwik', '$rootScope', '$location', 'reportingPageModel', 'reportingPagesModel', 'notifications'];

function ReportingPageController($scope, piwik, $rootScope, $location, pageModel, pagesModel) {
function ReportingPageController($scope, piwik, $rootScope, $location, pageModel, pagesModel, notifications) {
pageModel.resetPage();
$scope.pageModel = pageModel;

Expand Down Expand Up @@ -40,6 +40,8 @@
currentCategory = category;
currentSubcategory = subcategory;

notifications.clearTransientNotifications();

if (category === 'Dashboard_Dashboard' && $.isNumeric(subcategory) && $('[piwik-dashboard]').length) {
// hack to make loading of dashboards faster since all the information is already there in the
// piwik-dashboard widget, we can let the piwik-dashboard widget render the page. We need to find
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
(function () {
angular.module('piwikApp').directive('piwikWidgetLoader', piwikWidgetLoader);

piwikWidgetLoader.$inject = ['piwik', 'piwikUrl', '$http', '$compile', '$q', '$location'];
piwikWidgetLoader.$inject = ['piwik', 'piwikUrl', '$http', '$compile', '$q', '$location', 'notifications'];

function piwikWidgetLoader(piwik, piwikUrl, $http, $compile, $q, $location){
function piwikWidgetLoader(piwik, piwikUrl, $http, $compile, $q, $location, notifications){
return {
restrict: 'A',
transclude: true,
Expand Down Expand Up @@ -147,6 +147,8 @@
}

$compile(currentElement)(newScope);

notifications.parseNotificationDivs();
})['catch'](function () {
if (thisChangeId !== changeCounter) {
// another widget was requested meanwhile, ignore this response
Expand Down
31 changes: 0 additions & 31 deletions plugins/CoreHome/javascripts/notification_parser.js

This file was deleted.

8 changes: 8 additions & 0 deletions plugins/CoreHome/templates/_dataTable.twig
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@
</div>
</div>

{% if notifications is not empty and notifications|length %}
{% for notificationId, n in notifications %}

{{ n.message|notification({'id': notificationId, 'type': n.type, 'title': n.title, 'noclear': n.hasNoClear, 'context': n.context, 'raw': n.raw}, false) }}

{% endfor %}
{% endif %}

{% if showCardTableIsEmpty %}
</div></div>
{% endif %}
Expand Down
4 changes: 4 additions & 0 deletions plugins/Dashboard/javascripts/dashboardWidget.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@
}
$widgetContent.removeClass('loading');
$widgetContent.trigger('widget:create', [self]);

angular.element(document).injector().invoke(['notifications', function (notifications) {
notifications.parseNotificationDivs();
}]);
}

// Reading segment from hash tag (standard case) or from the URL (when embedding dashboard)
Expand Down
10 changes: 9 additions & 1 deletion plugins/SegmentEditor/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,15 @@ public function getAllSegmentsForAllUsers($idSite = false)
return $segments;
}

public function getSegmentByDefinition($definition)
{
$sql = $this->buildQuerySortedByName("definition = ?");
$bind = [$definition];

$segment = $this->getDb()->fetchRow($sql, $bind);
return $segment;
}

public function deleteSegment($idSegment)
{
$db = $this->getDb();
Expand Down Expand Up @@ -178,5 +187,4 @@ public static function install()

DbHelper::createTable(self::$rawPrefix, $segmentTable);
}

}
Loading

0 comments on commit 2b77bb4

Please sign in to comment.