@katebutler opened this Pull Request on May 15th 2019 Member

Fixes #14178

@mattab commented on May 16th 2019 Member

Quick feedback:

  • would have expected the line unset($result[4]); // remove 'range' period to be removed as well, so that when you don't specify --periods it will also invalidate all ranges overlapping the invalidated date.
@mattab commented on May 16th 2019 Member
@katebutler commented on June 17th 2019 Member

Am working on integration tests for invalidating all overlapping date ranges when "all" is passed.

@katebutler commented on June 19th 2019 Member

Ready for review/merge

@tsteur commented on July 17th 2019 Member

@katebutler @mattab

just tested and debugged to see how it works. I might have actually misunderstood and not quite sure which behaviour is expected.

So it seems right now when using eg ./console core:invalidate-report-data --periods=range --dates=2018-01-02,2018-02-02 it will invalidate the range period for each individual date. So it would invalidate period=range,date=2018-01-02,2018-01-02;period=range,date=2018-01-03,2018-01-03;.... period=range,date=2018-02-02,2018-02-02 but I would have now maybe expected it invalidates the exact date range whereperiod=range, date=2018-01-02,2018-02-02. So basically something like this code:

diff --git a/core/Archive/ArchiveInvalidator.php b/core/Archive/ArchiveInvalidator.php
index 1f03a07dda..f3dd0a68c2 100644
--- a/core/Archive/ArchiveInvalidator.php
+++ b/core/Archive/ArchiveInvalidator.php
@@ -232,11 +232,8 @@ class ArchiveInvalidator

         // Get the individual dates for a range, we need to check all of them
         if ($periodType === 'range') {
-            $allDates = array();
-            for ($date = $dates[0]; $date->getTimestamp() <= $dates[1]->getTimestamp(); $date = $date->addDay(1)) {
-                $allDates[] = $date;
-            }
-            $dates = $allDates;
+            $periodsToInvalidate[] = Period\Factory::build($periodType, $dates[0]->toString() . ',' . $dates[1]->toString());
+            return $periodsToInvalidate;
         }

         foreach ($dates as $date) {

@mattab not quite sure what is supposed to happen?

@tsteur commented on July 29th 2019 Member

@mattab still need your input here

@mattab commented on August 9th 2019 Member

@tsteur i would have also expected what you propose... @katebutler do you maybe remember the idea behind the current PR implementation?

@katebutler commented on August 26th 2019 Member

Logic has been re-implemented so that:

  • If the range period type is passed, only archives whose date ranges exactly match the input range(s) will be invalidated. This should be more intuitive to the user, as we are only invalidating exactly what we've been asked to.
  • If the all period type is passed, all archives whose date ranges overlap the input range(s) will be invalidated. This is the correct behaviour when the user has recently removed some data (e.g. for GDPR compliance) and needs all archives that may contain that data to be invalidated.
@tsteur commented on October 14th 2019 Member

@katebutler lets discuss details tmrw. Also waiting to here from @mattab the comments. I reckon though it be good to let it behave the same way when --periods=range is used and when --periods=all. We don't really want it to behave differently.

@tsteur commented on October 14th 2019 Member

It's hard to explain what I meant so quickly made 3-4 changes... lets discuss later and see what makes sense... I meant something like this:

https://github.com/matomo-org/matomo/compare/14178_2?expand=1

see my last commit https://github.com/matomo-org/matomo/commit/df13cbed17d8786396ce90a4c952e2244cba63c7 where we basically handle period range separately. (didn't test it at all)... there are still some things that would need to be adjusted and restored maybe such as https://github.com/matomo-org/matomo/compare/14178_2?expand=1#diff-27aba6f51c03fb57f660eeb0b2a18484L234 maybe... seems to be making the whole PR easier though

This Pull Request was closed on October 16th 2019
Powered by GitHub Issue Mirror