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

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

Merged
merged 22 commits into from Aug 9, 2018

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented May 5, 2018

Note: this PR has API changes.

image

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

@diosmosis 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
<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>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not adding this translatable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, forgot

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review and removed Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 5, 2018
@diosmosis diosmosis added this to the 3.5.0 milestone May 6, 2018
@diosmosis
Copy link
Member Author

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

@mattab mattab modified the milestones: 3.5.0, 3.5.1 May 8, 2018
@diosmosis
Copy link
Member Author

@tsteur would you have time to review this PR?

@diosmosis 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 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
}

// get segment
$segment = Common::getRequestVar('segment', false);
Copy link
Member

@tsteur tsteur May 22, 2018

Choose a reason for hiding this comment

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

do we need to use Request::getRawSegmentFromRequest?

@sgiehl
Copy link
Member

sgiehl commented Jul 16, 2018

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

@diosmosis
Copy link
Member Author

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. 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)
  2. this value is saved in the transient cache
  3. 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.
  4. 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...

}

/**
* Returns the current API method being executed, if the current request is an API request.
Copy link
Member

Choose a reason for hiding this comment

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

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.

CHANGELOG.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

@tsteur
Copy link
Member

tsteur commented Jul 16, 2018

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
Copy link
Member Author

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 ?

Copy link
Member

@mattab mattab left a comment

Choose a reason for hiding this comment

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

Feedback

  • please add tests for the API outputs for the 3 use cases covered here:
  1. custom segment with archiving disabled -> won't be ever available
  2. segment created in editor as real-time processed but web archiving is disabled so they need to set the segment to pre-processed
  3. segment created in editor as pre-processed, will be available soon

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Header comment missing

"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."
Copy link
Member

Choose a reason for hiding this comment

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

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

@mattab
Copy link
Member

mattab commented Aug 1, 2018

@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
Copy link
Member Author

Change which message to this one?

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

@diosmosis
Copy link
Member Author

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

@diosmosis
Copy link
Member Author

Issues should be fixed

@mattab
Copy link
Member

mattab commented Aug 7, 2018

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

@diosmosis
Copy link
Member Author

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

@mattab mattab merged commit 2b77bb4 into 3.x-dev Aug 9, 2018
@mattab 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
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…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.
@sgiehl sgiehl deleted the 12256-segment-preprocess-no-data branch November 13, 2018 20:47
@MatomoForumNotifications

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
Copy link

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
Copy link
Member

sgiehl commented Jan 25, 2023

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. 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

6 participants