Add notification when report w/ segment has no data, but segment is unprocessed #12823

diosmosis · 2018-05-05T02:49:51Z

Note: this PR has API changes.

This PR makes two changes to the way notifications are handled/displayed:

Transient notifications are now removed when the reporting page changes.
ViewDataTables have a view property called 'notifications' which can be used to add new notifications when loading a report. This is not in the Config so is not API. (The actual creation of notifications is done in the widget loader directive.)

Individual changes:

Remove 'notification_parser.js' and replace w/ notification.service.js angular service.
Add 'notifications' property to ViewDataTable's View & handle in _dataTable.twig. The widget loader directive initiates the "parse" of the divs outputted in _dataTable.twig. (Also done by widget loading logic in dashboard code.)
In reporting page controller, clear transient notifications after page load.
Added getSegmentByDefinition() function to SegmentEditor Model.
Add new event Visualization.onLoadingError that is triggered when there an exception is caught when loading report data.
Add new event Archive.noArchivedData that is triggered when during an API request no archive data can be found.
Use new event to display a notification when a report has no data and is for an unprocessed segment.

Fixes #12256
Fixes #12255

…and display an explanatory notification.

…ble views to add notifications, use to display unprocessed segment message.

sgiehl · 2018-05-05T08:31:19Z

plugins/SegmentEditor/templates/_unprocessedSegmentMessage.twig

+    <a href="{{ visitorLogLink }}" target="_blank">In the Visitor Log</a>, you can test whether your segment will match your users
+    correctly by applying it there. When applied, you can see immediately which visits and actions were matched by your segment.
+    This can help you confirm your Segment matches the users and actions you expect it to.
+</p>


Any reason for not adding this translatable?

Oh, right, forgot

…tent.

diosmosis · 2018-05-06T19:48:37Z

Adding to 3.5.0 milestone in case it can go in, if not can remove it.

diosmosis · 2018-05-21T00:02:07Z

@tsteur would you have time to review this PR?

… found and segment is used, then detect exception in new event.

tsteur · 2018-05-22T21:10:03Z

plugins/SegmentEditor/SegmentEditor.php

+        }
+
+        // get segment
+        $segment = Common::getRequestVar('segment', false);


do we need to use Request::getRawSegmentFromRequest?

sgiehl · 2018-07-16T13:26:41Z

@diosmosis @tsteur @mattab does this one need another review or anything else so it can be merged?

diosmosis · 2018-07-16T16:15:53Z

I think @tsteur had some bigger issues w/ this one specifically regarding the inability to determine when a segment will be archived in the future. Probably needs more discussion w/ @tsteur & @mattab.

tsteur · 2018-07-16T20:42:09Z

core/API/Request.php

+    public static function isRootApiRequestHandlingMethod($methodToCheck = null)
+    {
+        if (empty($methodToCheck)) {
+            $methodToCheck = Common::getRequestVar('method', '', 'string');


I'm not quite sure if this works / makes too much sense? $_GET/$_POST['method'] will be overwritten during API calls within the code etc. And it seems to be only called once but without a parameter? And what is the difference to isRootRequestApiRequest() when called without a parameter or so? I've tried to understand what is happening here but couldn't.

The process is split over a couple functions, I'll explain it below:

on request start via Platform.initialized event, Request::setIsRootRequestApiRequest() is called w/ the api method being called (see plugins/API/API.php detectIsApiRequest() method in this PR)

this value is saved in the transient cache

later some code wants to see if the current request is the root API method so they call isRootApiRequestHandlingMethod() w/o any value. this finds the current executing API method & checks if its the same as the value that was cached before. if it is, then this is the root request (though I suppose this won't work w/ a recursive API method...). if it isn't, then this was invoked within another API request.

later some code wants to see if the root request is a specific method, say Actions.getPageTitles, then they call isRootApiRequestHandlingMethod('Actions.getPageTitles').

anyway, since this won't work w/ recursive methods I'll have to change it...

tsteur · 2018-07-16T20:44:02Z

core/API/Request.php

+    }
+
+    /**
+     * Returns the current API method being executed, if the current request is an API request.


I wonder if we need to clarify API request vs root API request... in this case here we would be also returning a method when an API request is being executed as part of a regular request... maybe we also need some wordings for it to better talk about it? not sure though.

tsteur · 2018-07-16T20:54:30Z

CHANGELOG.md

@@ -9,6 +9,8 @@ The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)*
 ### New APIs

 * Added new method `Piwik\API\Request::isRootRequestApiRequest()` to detect if the root request is an API request.
+* New event `Archive.noArchivedData` which can be used to execute code when no archive data is found for an API method.


Can you remove the events from changelog again as we "ignore" them and don't consider them API?

tsteur · 2018-07-16T20:56:00Z

Had a look through the code again but to really say whether it covers all cases or not, what else we need to consider etc I would need a day or two to understand things fully. As long as nothing is API and we can change things easily later I presume it should be fine to merge as long as we don't say "Data will become available later" when it actually maybe won't.

diosmosis · 2018-07-23T05:32:44Z

Applied some changes, but nothing w/ regard to when the message is displayed or not.

Maybe the following changes would help:

Only show the message for a recent period (where recent could be within 2 months or some other date).
Change the message to display "Data for this Segment will become available in a few hours when processing completes. If it does not, there may be a problem."

What do you think @mattab / @tsteur ?

mattab

Feedback

please add tests for the API outputs for the 3 use cases covered here:

custom segment with archiving disabled -> won't be ever available
segment created in editor as real-time processed but web archiving is disabled so they need to set the segment to pre-processed
segment created in editor as pre-processed, will be available soon

tests will ensure we don't regress this key information/new user feedback.

mattab · 2018-08-01T23:16:59Z

plugins/SegmentEditor/templates/_unprocessedSegmentMessage.twig

+{% set visitorLogLinkHtml %}<a href="{{ visitorLogLink }}" target="_blank">{% endset %}
+{% if isSegmentToPreprocess %}
+<p>
+    {{ 'SegmentEditor_UnprocessedSegmentNoData1'|translate('<strong>(' ~ segmentName ~ ')</strong>')|raw }}


Wondering about the |raw here, maybe it could lead to XSS for specially crafted segments, ie. maybe segmentName needs to be escaped?

mattab · 2018-08-01T23:18:44Z

plugins/SegmentEditor/UnprocessedSegmentException.php

@@ -0,0 +1,87 @@
+<?php
+/**
+ * Created by PhpStorm.


Header comment missing

mattab · 2018-08-01T23:22:17Z

plugins/SegmentEditor/lang/en.json

+        "CustomUnprocessedSegmentApiError4": "Once created the segment in the editor (or via API), this error message will disappear and within a few hours you will see your segmented report data, after the segment data has been pre-processed.",
+        "CustomUnprocessedSegmentApiError5": "Please note that you can test whether your segment will work without having to wait for it to be processed by using the Live.getLastVisitsDetails API.",
+        "CustomUnprocessedSegmentApiError6": "When using this API method, you will see which users and actions were matched by your &segment= parameter.",
+        "CustomUnprocessedSegmentNoData2": "To see data for this segment, you must create this segment manually in the Segment Editor, then wait a couple hours for preprocessing to complete."


maybe this could be renamed to CustomUnprocessedSegmentNoData as there was no other key called this?

mattab · 2018-08-01T23:40:01Z

@diosmosis

Maybe the following changes would help:
Only show the message for a recent period (where recent could be within 2 months or some other date).
Change the message to display "Data for this Segment will become available in a few hours when processing completes. If it does not, there may be a problem."

Change which message to this one?
👍 otherwise for the proposed message. but we need to display this message only when we know logs are still available for this date/period and haven't been purged yet (according to the Data retention configured in the Privacy settings in Matomo admin). Also check the message wouldn't be displayed on date/periods before the site creation date.

diosmosis · 2018-08-01T23:48:15Z

Change which message to this one?

Both, since they both tell the user when data will become available.

diosmosis · 2018-08-03T08:20:25Z

Found issues w/ this one, don't merge yet.

diosmosis · 2018-08-03T20:02:52Z

Issues should be fixed

mattab · 2018-08-07T06:59:39Z

@diosmosis Build is failing at least in https://travis-ci.org/matomo-org/matomo/jobs/411881882

diosmosis · 2018-08-08T00:46:23Z

Build should be passing (UI test failures are unrelated to this PR, will have to be fixed in another PR)

…nprocessed (matomo-org#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.

MatomoForumNotifications · 2023-01-24T22:41:34Z

This pull request has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/custom-segment-archiving-question/49322/3

metalocator · 2023-01-25T15:49:34Z

Hi,

Really wishing there was a "force" option only on the API side.

We can't allow customers to create segments via the web because they're often too data intensive, risking an outage with a complex segment with millions of records.

However, we have in-app dashboards that are generated dynamically based on API calls, and to create every dashboard segment beforehand wouldn't be practical. We have dashboards that can report on a per profile basis, and one customer might have 500,000 profiles. Also then if they delete profiles, we need to keep the segment list in sync, not to mention we would be required to maintain thousands of segments many of which would never be used.

sgiehl · 2023-01-25T16:16:45Z

@metalocator If you have a feature request or suggestion for improvement please consider open a new issue, if non exist yet.
Comments on older and closed pull requests or issues, will not get any attention when considering future improvements...

diosmosis added 3 commits May 3, 2018 17:57

When a report has no data, check if it is for an unprocessed segment …

0559142

…and display an explanatory notification.

Remove transient notifications on reporting page change, allow datata…

b213a4b

…ble views to add notifications, use to display unprocessed segment message.

Merge branch '3.x-dev' into 12256-segment-preprocess-no-data

d29cc85

diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels May 5, 2018

Update changelog.

cc65995

sgiehl reviewed May 5, 2018

View reviewed changes

diosmosis added 3 commits May 5, 2018 15:39

Internationalize unprocessed segment message.

4f83b06

Parse notification divs in dashboardWidget.js when setting widget con…

cd750d1

…tent.

Tweak message.

385d12e

diosmosis mentioned this pull request May 6, 2018

When a custom segment is used in an API request and browser_archiving_disabled_enforce=1, throw an exception explaining to create the segment first #12255

Closed

diosmosis added this to the 3.5.0 milestone May 6, 2018

mattab modified the milestones: 3.5.0, 3.5.1 May 8, 2018

Merge branch '3.x-dev' into 12256-segment-preprocess-no-data

de4495d

diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels May 21, 2018

diosmosis added 2 commits May 21, 2018 18:53

Change PR to use different approach: throw exception when no archives…

5e72560

… found and segment is used, then detect exception in new event.

Update changelog + document new event.

94fb3a8

diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 22, 2018

tsteur reviewed May 22, 2018

View reviewed changes

tsteur reviewed Jul 16, 2018

View reviewed changes

diosmosis added 2 commits July 22, 2018 21:19

Merge branch '3.x-dev' into 12256-segment-preprocess-no-data

c7d73bd

apply pr review

82922d1

mattab requested changes Aug 1, 2018

View reviewed changes

Apply review.

0e9b2dc

diosmosis added 2 commits August 3, 2018 01:32

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

870af25

Different strategy + new test.

b3c5be8

diosmosis added 3 commits August 7, 2018 15:10

Merge branch '3.x-dev' into 12256-segment-preprocess-no-data

65fc0c2

Fix tests.

5211c4f

Update two expected screenshots.

7f34dc9

mattab merged commit 2b77bb4 into 3.x-dev Aug 9, 2018

mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Aug 9, 2018

sgiehl deleted the 12256-segment-preprocess-no-data branch November 13, 2018 20:47

bx80 mentioned this pull request Jan 17, 2023

Segment just created, and real-time processing of segments is disabled, And Segment has no data, the message is not displayed at the top to warn it may take some time display #20060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notification when report w/ segment has no data, but segment is unprocessed #12823

Add notification when report w/ segment has no data, but segment is unprocessed #12823

diosmosis commented May 5, 2018 •

edited

Loading

sgiehl May 5, 2018

diosmosis May 5, 2018

diosmosis commented May 6, 2018

diosmosis commented May 21, 2018

tsteur May 22, 2018 •

edited

Loading

sgiehl commented Jul 16, 2018

diosmosis commented Jul 16, 2018

tsteur Jul 16, 2018

diosmosis Jul 23, 2018

tsteur Jul 16, 2018

tsteur Jul 16, 2018

tsteur commented Jul 16, 2018

diosmosis commented Jul 23, 2018

mattab left a comment

mattab Aug 1, 2018

mattab Aug 1, 2018

mattab Aug 1, 2018

mattab commented Aug 1, 2018

diosmosis commented Aug 1, 2018

diosmosis commented Aug 3, 2018

diosmosis commented Aug 3, 2018

mattab commented Aug 7, 2018

diosmosis commented Aug 8, 2018

MatomoForumNotifications commented Jan 24, 2023

metalocator commented Jan 25, 2023

sgiehl commented Jan 25, 2023

Add notification when report w/ segment has no data, but segment is unprocessed #12823

Add notification when report w/ segment has no data, but segment is unprocessed #12823

Conversation

diosmosis commented May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented May 6, 2018

diosmosis commented May 21, 2018

tsteur May 22, 2018 • edited Loading

Choose a reason for hiding this comment

sgiehl commented Jul 16, 2018

diosmosis commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Jul 16, 2018

diosmosis commented Jul 23, 2018

mattab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Aug 1, 2018

diosmosis commented Aug 1, 2018

diosmosis commented Aug 3, 2018

diosmosis commented Aug 3, 2018

mattab commented Aug 7, 2018

diosmosis commented Aug 8, 2018

MatomoForumNotifications commented Jan 24, 2023

metalocator commented Jan 25, 2023

sgiehl commented Jan 25, 2023

diosmosis commented May 5, 2018 •

edited

Loading

tsteur May 22, 2018 •

edited

Loading