Add setting dates range for Page History #149

Albert221 · 2018-01-10T23:22:07Z

Bug: T181694

MusikAnimal

Looks awesome! I left a few comments. This is not a complete review. I unfortunately might be away much of the day, but I'll definitely get to this as soon as possible.

MusikAnimal · 2018-01-11T12:34:55Z

src/AppBundle/Controller/ArticleInfoController.php

@@ -121,13 +121,22 @@ public function gadgetAction(Request $request)

    /**
     * Display the results.
-     * @Route("/articleinfo/{project}/{article}", name="ArticleInfoResult", requirements={"article"=".+"})
+     * @Route(
+     *     "/articleinfo/{project}/{start}/{end}/{article}", name="ArticleInfoResult",


This routing change will be a problem :( ArticleInfo is very popular and there are a lot of links to it. As optional parameters it'd be better if the start/end dates are at the end, but I suppose there's the caveat that "My article/2017-01-23/2018-01-23" is a valid page title.

Maybe we should use query string parameters instead?

Ideally we could use some trickery to handle the "My article/2017-01-23/2018-01-23" edge case, or perhaps we don't care... Interested to hear your thoughts.

MusikAnimal · 2018-01-11T12:38:05Z

src/Xtools/ArticleInfo.php

+    /** @var false|int From what date to obtain records. */
+    protected $startDate;
+
+    /** @var false|string To what date to obtain records. */


string or int?

MusikAnimal · 2018-01-11T12:43:56Z

src/Xtools/ArticleInfo.php

+        }
+
+        list($start, $end) = $this->translateDatesToYYYYMMDD($this->startDate, $this->endDate);
+        list($start, $end) = $this->applyDatesDefaults($start, $end, $latest);


I'm confused why we're using $latest in this case. The pageviews should be for the same date as what was requested. So if they only gave an end date, it'd use the date of the first revision to the specified end date.

That was what I meant when I told you on IRC that I have minor logical-thinking problems :D

MusikAnimal · 2018-01-11T12:47:51Z

src/Xtools/ArticleInfoRepository.php

+     * @param string $field
+     * @return string
+     */
+    public static function createDatesConditions($start, $end, $tableAlias = '', $field = 'rev_timestamp')


What if we moved this to Repository? Then it's accessible in PageRepository, ArticleInfoRepository, etc., and it doesn't have to be static.

I second that

MusikAnimal · 2018-01-11T22:55:17Z

Now that I'm finally home, I'm able to do a more thorough review. A few more issues, I'm afraid:

"Page size" is not that of the last edit. In the view, instead of calling page.length I would do ai.length, and that method in ArticleInfo will conditionally call $this->page->getLength() or $this->lastEdit->getLength() based on whether a date range was provided. Probably should have some sort of hasDateRange() method in ArticleInfo, too.
"Total edits" does not respect the date range. For this I suppose we need to keep a running total of the number of edits in the while loop of ArticleInfo::parseHistory(), setting it on $this->numRevisionsProcessed, and then hopefully getNumRevisions() will just work (because it is called after ArticleInfo::prepareData() is called).
The "Month counts" section always goes through December, even if you put say, July, as the end date.
Super duper minor priority -- the Pageviews links at the top ("Pageviews (All languages · Redirects)") should pass in the date range to the Pageviews tool. This would mean allowing for start/end parameters in the wiki.pageviewsLinks macro. Definitely don't worry about this! I can do it :)

Some things simply need to be hidden when there is a date range:

"Assessment" and "Bugs" -- both in General stats and the dedicated <section>'s.
"Page watchers"
The "(60 days)" part of the "Pageviews (60 days)" label.
Possibly the "Edits in the past..." fields? The numbers are right, but are relative to the end date, so the language "in the past" might be confusing. I'm definitely OK just hiding them altogether, as I don't think that information is as relevant when working within a date range.

If you need/want help with ANY of this please feel free to ask. I was worried this task was a bit too big, so don't feel like you have to do everything!

Albert221 · 2018-01-11T23:50:05Z

I localized issue of having all months to be here:

xtools/src/Xtools/ArticleInfo.php

Lines 851 to 867 in d0838b9

    
           ]; 
        
           for ($i = 1; $i <= 12; $i++) { 
        
               $timeObj = mktime(0, 0, 0, $i, 1, $editYear); 
        
               // Don't show zeros for months before the first edit or after the current month. 
        
               if ($timeObj < $firstEditTime || $timeObj > strtotime('last day of this month')) { 
        
                   continue; 
        
               } 
        
               $this->yearMonthCounts[$editYear]['months'][sprintf('%02d', $i)] = [ 
        
                   'all' => 0, 
        
                   'minor' => 0, 
        
                   'anon' => 0, 
        
                   'automated' => 0, 
        
               ]; 
        
           }

MusikAnimal

Looking really good!! Just a few more minor complaints :) Thanks so much, I know this is a lot of work!

MusikAnimal · 2018-01-12T00:37:53Z

web/static/css/application.scss

+    @include center('vertical');
+    right: 0;
+    font-size: 70%;
+}


Minor complaint: We could use some mobile-friendliness here, something like:

@media (max-width: $bootstrap-sm) { .date-range { display: block; position: relative; top: 0; transform: none; } }

MusikAnimal · 2018-01-12T00:39:50Z