@diosmosis opened this Pull Request on June 25th 2018 Member

This PR is not done but is ready for an initial review.

Changes:

  • Added new single-metric-value widget (angular based).
  • Add query param filter_last_period_evolution that adds _past/_evolution metrics to any report. (not documented)
  • Found an issue in compileAngularComponents(). The method uses the scope for an element, but in some cases compileAngularComponents() is called again on the same element (like when reloading a widget). This means that the scope is re-used when compiling a widget again, which in turn means its not possible to destroy angular components/directives. In the dashboard I forced the creation of a new scope, not sure if it's the best way to fix.
  • Added Report:: getMetricDocumentationForReport() to access metric documentation for a report. Don't need it if an API.getProcessedReport, but that doesn't show the columns from filter_last_period_evolution.
  • Added a compileAngularComponents() call to widget preview.
  • Noticed when adding a widget it's reloaded several times (and all widgets w/ the same 'unique ID' are reloaded). Removed the extra reloads.

Note: if you'd rather the widget be rendered server side, let me know, just trying to use angular for all new work.

Fixes #12358

CC @mattab / @tsteur for additional changes made + angular related fixes

image

image

@mattab commented on June 26th 2018 Member

Feedback:

  • It's usable and looks very nice to have only one widget with the metric picker :+1:
  • If we move the Sparklines rendering to client side, maybe we should aim to also convert the sparklines twig code to angular, as to avoid having the sparklines logic duplicated. Maybe this would be best done directly in this PR rather than later?
  • some of the metadata is not displayed yet (percentage sign for rate metrics, currency for revenue metrics),
  • sometimes the metric is shown empty for some (instead of zero)
  • is the isReusable flag needed? (the current/desired behavior is to allow adding the same widget multiple times for all widgets)
  • left a code comment re: maybe missing escaping
@diosmosis commented on June 26th 2018 Member

is the isReusable flag needed?

It's used to display the 'Metric' widget w/ black text instead of grey text. Not required, I guess, but I think it's slightly better UX.

@diosmosis commented on July 3rd 2018 Member

sometimes the metric is shown empty for some (instead of zero)

Do you remember which metric? Not currently able to reproduce.

@diosmosis commented on July 3rd 2018 Member

some of the metadata is not displayed yet (percentage sign for rate metrics, currency for revenue metrics),

Some of this is hard to fix. Processed metrics (like percents) I could display via format_metrics=1, but normal metrics like revenue are not currently formatted in API output. I will add a fix in another PR.

@diosmosis commented on July 23rd 2018 Member

@mattab fixed the revenue issue w/ a hacky fix. Needs review, will be adding tests now.

@diosmosis commented on July 24th 2018 Member

Found a couple issues in processed metrics calculation & metric formatting when getting the evolution filter to work:

  • The recursion on subtables would recurse multiple times, once per metric. Now it does this once per table.
  • Processed metrics were not computed in any order, so processed metrics couldn't use other processed metrics. Fixed by using getDependentMetrics() to determine order of metrics.
  • EvolutionMetric did not work correctly on subtables since it would always use the root past datatable when computing evolutions for subtables. Fixed by adding two new non-API hooks to ProcessedMetric: beforeComputeSubtable(), afterComputeSubtable()

Also noticed that new processed metrics will never appear in processed report output because the columns are limited to the metrics/processed metrics in the Report class. This makes me think it might be useful to make the Report class mutable, so DataTablePostProcessor (for instance) can add new processed metrics. What do you think @tsteur / @mattab?

@diosmosis commented on July 27th 2018 Member

@tsteur FYI I changed the strategy here to calculate the evolution data on in the component so the new parameter isn't required. There's less backend changes now, the changes that exist are mostly fixes for issues I found.

@tsteur commented on July 31st 2018 Member

should be fine @diosmosis 👍

@mattab commented on July 31st 2018 Member

@diosmosis Looks good overall, a awesome new feature :+1:

Feedback:

  • KPI metrics for each Goal (Goal conversions, Conversion rate, Revenue). Currently it only has the "overall conversions/revenue.
  • the overall conversion rate (visits with a completed goal) is missing, not sure if it's expected but it would be good to have
  • in general when comparing the list of metrics available in the metric picker VS all the metrics listed in https://github.com/innocraft/plugin-CustomReports/pull/56 -> how would it be possible to have more/all metrics available in this widget picker like they are in Custom Reports?
  • move the "Generic" category above the "Diagnostic" category if possible
@diosmosis commented on July 31st 2018 Member

KPI metrics for each Goal (Goal conversions, Conversion rate, Revenue). Currently it only has the "overall conversions/revenue.

The series picker doesn't let you pick more than a metric (ie, you can't pick a metric + an idGoal). I'll put another select for goals when a goal metric is displayed.

in general when comparing the list of metrics available in the metric picker VS all the metrics listed in innocraft/plugin-CustomReports#56 -> how would it be possible to have more/all metrics available in this widget picker like they are in Custom Reports?

This widget just uses the API.get report. Some metrics used in custom reports are probably metrics that aren't saved "standalone" (ie, maybe they're always w/ a specific report?). Ideally we would add these metrics to API.get to show them.

@mattab commented on July 31st 2018 Member

The series picker doesn't let you pick more than a metric (ie, you can't pick a metric + an idGoal). I'll put another select for goals when a goal metric is displayed.

was hoping we could simply list all the metrics in the list of metrics, ie if the goal name is Affiliate click we'd have the 3 metrics availble: Affiliate click conversions, Affiliate click conversion rate, Affiliate click Revenue. Would this work/doable?

@diosmosis commented on July 31st 2018 Member

@mattab I can probably hack something in like that, but it would create a very large series picker.

@mattab commented on July 31st 2018 Member

It will be OK to have a large picker as long as it's not buggy. in the future, we'll improve the metric picker so it looks like the custom reports metric picker.

@diosmosis commented on August 1st 2018 Member

@mattab added goal metrics, deserves more tests since it's a hacky change. (did not modify the API)

@diosmosis commented on August 1st 2018 Member

Moved the category:

image

@diosmosis commented on August 2nd 2018 Member

@mattab approved on slack, merging after fixing build

This Pull Request was closed on August 2nd 2018
Powered by GitHub Issue Mirror