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

Powered by GitHub Issue Mirror