@diosmosis opened this Pull Request on May 5th 2018 Member

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 commented on May 6th 2018 Member

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

@diosmosis commented on May 21st 2018 Member

@tsteur would you have time to review this PR?

@diosmosis commented on May 24th 2018 Member

After discussing it appears to risky to introduce this change in 3.5.1, will remove this + the related issues from the current milestone.

CC @tsteur / @mattab

@diosmosis commented on May 28th 2018 Member

@mattab If we only show this notification for dates that are < 2 months old, will it still be useful?

For context: we don't really know if an older date will be archived by the next cron:archive run, so if we say data for every date will become available, it's possible some users will just wait forever & then get angry.

@mattab commented on May 29th 2018 Member

If we only show this notification for dates that are < 2 months old, will it still be useful?
For context: we don't really know if an older date will be archived by the next cron:archive run, so if we say data for every date will become available, it's possible some users will just wait forever & then get angry.

I think we are able to know whether a given date in the past will be archived. we wouldn't want to pick a certain date (eg. 2 months) because that depends on the settings of how far log data was purged. In theory (without reading the whole thread) the historical data will be processed for a segment, as long as we still have the raw data (maybe haveLogsBeenPurged is useful there) @diosmosis

@diosmosis commented on May 29th 2018 Member

@mattab So what about min(2 months, $logDataPurgeTime)? so if the log data purges older than 1 month, we check for 1 month, if it's 3 months, we check for 2 months?

CC @tsteur since he's more aware of the limitations of knowing what core:archive will do than I am and can probably explain the issue better.

@tsteur commented on May 29th 2018 Member

In theory (without reading the whole thread) the historical data will be processed for a segment, as long as we still have the raw data

I'm not quite sure since it might depend on when the archiving ran the last time? See https://github.com/matomo-org/matomo/blob/3.5.1/core/CronArchive.php#L1556-L1577

I reckon it may be best to start a document or comment to write all the things done we know influence when something will be archived and when not. For example the process_new_segments_from setting etc.

@mattab commented on May 31st 2018 Member

I'm not quite sure since it might depend on when the archiving ran the last time? See https://github.com/matomo-org/matomo/blob/3.5.1/core/CronArchive.php#L1556-L1577

The historical segments should always be processed within a day or two... because whenever we next call "last2 month" archiving and then "last2 year" archiving, it will pre-process the last ~ 700 days (last 2 years) / last 24 months / etc.

(assuming we have a compatible process_new_segments_from value eg. beginning_of_time)

@mattab commented on May 31st 2018 Member

So what about min(2 months, $logDataPurgeTime)? so if the log data purges older than 1 month, we check for 1 month, if it's 3 months, we check for 2 months?

@diosmosis don't understand the "2 months", AFAIK we don't want to use a fixed date as most of the time it will even pre-process 1 year, or 2 years, or 3 years of historical segment data.

@diosmosis commented on May 31st 2018 Member

That was @tsteur’s suggestion on slack, I was deferring to his knowledge of the archiving process.

@sgiehl commented on July 16th 2018 Member

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

@diosmosis commented on July 16th 2018 Member

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 commented on July 16th 2018 Member

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 commented on July 23rd 2018 Member

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 commented on August 1st 2018 Member

@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?
:+1: 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 commented on August 1st 2018 Member

Change which message to this one?

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

@diosmosis commented on August 3rd 2018 Member

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

@diosmosis commented on August 3rd 2018 Member

Issues should be fixed

@mattab commented on August 7th 2018 Member
@diosmosis commented on August 8th 2018 Member

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

This Pull Request was closed on August 9th 2018
Powered by GitHub Issue Mirror