Page MenuHomePhabricator

Add functionality for Line Graph
ClosedPublic

Authored by fdans on Jul 27 2017, 12:58 PM.

Details

Maniphest Tasks
T171766: Productionise line graph
Reviewers
Milimetric
mforns
Nuria
fdans
Commits
rWIKISTATSe64209a2da39: Add functionality for Line Graph
Patch without arc
git checkout -b D730 && curl -L https://phabricator.wikimedia.org/D730?download=true | git apply
Summary

Ref T171766

  • Made the line graph reactive to the breakdown values and with data, depending on metric config.
  • Enabled line graphs in dashboard widgets.
  • Refactored MetricWidgets to remove a lot of repeated logic.

Diff Detail

Repository
rWIKISTATS analytics-wikistats-new
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fdans created this revision.Jul 27 2017, 12:58 PM
fdans edited the summary of this revision. (Show Details)Jul 27 2017, 1:02 PM
Milimetric edited edge metadata.Aug 1 2017, 9:38 PM

In general in the future you should mention how to test a change. In this case the default screen doesn't have unique devices data (because there is none for all-projects), so you should mention it needs to be tested on a specific wiki or something like that.

src/components/dashboard/MetricWidget.vue
35

Right now this is showing the month from 12 months ago, so two thoughts:

  1. That data is wrong for additive metrics, because this says Year Total. Take a look at the design and consultation and figure out what exactly you want to show here.
  2. Year Total is not possible for Unique Devices, you can't add up months, so you'll need to have conditional labels here. There's a discussion on the internal list with Erik Zachte about this Unique Devices widget, take a look at that.
65

if we're gonna use this, let's put it in config instead of repeating it.

154

When this metric isn't possible to compute because there is no data for 'all-projects', Vue still attempts to render it and throws errors in these computeds. You should figure out a more graceful way to fail so it still renders and says "NO DATA" or something like that. This disabled thing is ok, but it's combining multiple conditions in one flag.

src/components/detail/Detail.vue
129

The range selector should always have the range you need. You can pass how many years you want by default as a parameter to it when you render it from the parent. That way you don't have to do this else.

fdans updated this revision to Diff 1937.Aug 3 2017, 9:33 PM
  • moves default range generation to TimeRangeSelector
  • makes sure that previous graph is erased in dashboard when wiki changes
fdans accepted this revision.Aug 3 2017, 9:46 PM
fdans marked an inline comment as done.
fdans added inline comments.
src/components/dashboard/MetricWidget.vue
65

I'd prefer to minimise the places where we access the config, and I like this better here than in a nondescript utils file or something like that.

This revision is now accepted and ready to land.Aug 3 2017, 9:46 PM
fdans added a comment.Aug 3 2017, 9:49 PM

Sorry, I accidentally accepted the revision from my end so dismiss that.

fdans updated this revision to Diff 1939.Aug 4 2017, 11:12 PM
  • aggregates last year data correctly
  • computes average if metric is not additive
fdans marked an inline comment as done.Aug 4 2017, 11:18 PM

@Milimetric thanks for reviewing! all your comments addressed (apparently if you push a change that outdates the diff, I can't mark your comment as done πŸ™„)

src/components/dashboard/MetricWidget.vue
154

Totally, but let's handle this in T171487 since this is a bit out of the scope of this review

mforns edited edge metadata.Aug 11 2017, 3:00 PM

Some comments inline.
In general also, we have to decide if we go for ; at the end of instructions or not.
And also maybe decide to follow a style guide like lint (without necessarily linting the code).

src/components/TimeRangeSelector.vue
51

Do we need 2 years and 1 month? Shouldn't it be 2 years exact?
Is it because of and [inclusive-exclusive) interval?
If so, we might want to leave that logic to each specific api no?
They might behave differently, even with different granularities.

src/components/dashboard/MetricWidget.vue
27

I think the metric-list-widget does not fit into this refactor.
The list would be the top most edited pages, or something similar.
It would be not possible to aggregate a monthly or yearly total of that in a number.
We don't have a list metric yet, so we can leave that for later, dunno. Thoughts?

65

I would also put this in the config file for now to avoid repetition of code.
This code is already in 3 places: here, the column widget and the line widget.
(I know column and line widgets contain abbreviations, but it sill feels to me like they all could be stored together).
Also, hopefully not very far away from now, we'll want to have more languages, and this code will be somewhere separate for localization.

fdans marked an inline comment as done.Aug 17 2017, 12:30 PM
fdans added inline comments.
src/components/TimeRangeSelector.vue
51

Yeah it's not really 2 years and one month, it's "show me the 24 months available until now". I agree that it makes sense to leave it up to the behaviour of each api, but with that change we should change granularity to daily in lower ranges, as requested in T173372

src/components/dashboard/MetricWidget.vue
27

The approach we've taken so far is to refactor and change according to the metrics that we gradually add. So far we don't have a list metric so, while you're totally right it's a change that is out of the scope of this revision.

fdans updated this revision to Diff 1987.Aug 17 2017, 12:32 PM
  • puts month array in config
fdans updated this revision to Diff 1988.Aug 17 2017, 12:33 PM
  • Merge branch 'develop' of ssh://git-ssh.wikimedia.org/source/wikistats into line-graph
mforns added inline comments.Aug 21 2017, 2:08 PM
src/components/TimeRangeSelector.vue
51

I think this piece of code should not have to care about granularity.
And that it should return just the raw start and end, without thinking whether it needs to go one month back, or one week back. Just return the plain exact timestamp, and let each api apply those adjustments.
What do you think?

src/components/dashboard/MetricWidget.vue
27

Ok, makes sense.
In this case, I would prefer having this code commented out with an explanatory comment.
Otherwise, a new developer that looks into this later on in the project would have to know which parts of the code are effectively working or are just remainders of previous refactors.
I know we're still moving from prototype code to productionized code in many places (like metrics modal), but ideally all code should be working-code.

fdans marked an inline comment as done.Aug 23 2017, 9:40 AM
fdans added inline comments.
src/components/TimeRangeSelector.vue
51

Yeah that makes sense, let's do that

fdans updated this revision to Diff 1991.Aug 23 2017, 9:41 AM
  • sets time ranges to be independent of api details
  • comments out list metric widget while we don't have a matching metric
mforns accepted this revision.Aug 24 2017, 2:39 PM

LGTM @fdans :]

The only thing that could be improved IMO is to do a consistent instruction ending: either all instructions end with semi-colon, or all without.
Dan thinks semi-colon is better (even necessary), because the language has still some ambiguities without semi-colons.
Hearing that, I would also lean towards semi-colon. Would that be OK for you?
Nevertheless, I think we can leave this semi-colon hell to other patches, because this here involves many files.
But definitely we should stick to one convention in future patches.

fdans updated this revision to Diff 1998.Aug 24 2017, 5:18 PM
  • Merge branch 'develop' of ssh://git-ssh.wikimedia.org/source/wikistats into line-graph
This revision was automatically updated to reflect the committed changes.