Opened 5 years ago

Closed 23 months ago

Last modified 18 months ago

#534 closed New feature (fixed)

Row Evolution: historical view of one or many specific rows in all Piwik reports!

Reported by: matt Owned by:
Priority: critical Milestone: Piwik 1.8
Component: UI - UX (AngularJS, twig, less) Keywords: evolution, graph, multiple values, multi lines, plot, entity, UI pattern
Cc: Sensitive: no

Description (last modified by EZdesign)

Show graph of visits/pages over the last 30 days, for a specific row (keyword, country, etc.)

The plugin would add, in all data tables, an icon next to the keyword/ country / page view (or on hover), that would ajax load an evolution graph of the number of visits for this keyword, country or page, over the last 30 days/weeks/month. The graph would have the 'data export' links.

UI

We should find out how to provide this UI feature without bloating the UI with icons and new functionnality. Note that some rows in Piwik are already clickable (eg. to get the list of keyword by search engine).

One way of doing it would be to show a semi transparent icon when hovering over a row. The icon would be shown at the end of the row.

The new UI would also be used to report the new reports from #756

Report
The report would contain, for a specific keyword or search engine or country or page, the following information:

  • visits in date range + sparkline
  • unique visitors in date range + sparkline
  • bounce rate + sparkline
  • (for pages) time per page, exit rate, etc. + sparkline

Also, we should report Goals conversions, maybe in a different report, available by a click on a tab, or some other mechanism.
This report would show, for this keyword/country/page, the following info:

  • overall conversions / conversion rate / revenue per visit
  • for each goal...
    • conversions / conversion rate/ revenue per visit.

Performance note

Piwik stores data on a "per day" basis. When plotting number of page views for the url 'http://example.org/category/photo' for the last 30 days, it will require selecting data for the 30 days, and navigate through the tree to get the child node data (/photo) - this is all done automatically via filters. In this example, piwik would select 90 blobs of data, and unserialize, load in memory, grep, etc. This is a severe requirement and feasability should be studied before starting the implementation.

When plotting evolution of keyword "piwik" over the last 30 days, this would require 30 blobs of data to be selected/unserialize/loaded/etc.

Note

In the first version, only one data would be plottable. In future versions, we could think about plotting 3 or 4 keywords/countris on the same graph. How would this be available in the UI? Clicking on the icon could "enable" the line to be displayed on the graph, and a "reset" button, or a click on the icon, would disable this line. Requires #397.

Attachments (3)

mockup click on row proposal.png (21.4 KB) - added by matt 3 years ago.
Mockup to show click to launch popover
graph working but not sparklines.png (35.6 KB) - added by matt 2 years ago.
graph working but not sparklines bis.png (36.8 KB) - added by matt 2 years ago.

Download all attachments as: .zip

Change History (96)

comment:1 Changed 5 years ago by matt (mattab)

  • Description modified (diff)

comment:2 Changed 5 years ago by matt (mattab)

  • Description modified (diff)

comment:3 Changed 5 years ago by matt (mattab)

  • Keywords evolution graph multiple values multi lines added

comment:4 Changed 5 years ago by matt (mattab)

  • Description modified (diff)

comment:5 Changed 5 years ago by matt (mattab)

  • Type changed from Bug to New feature

comment:6 Changed 5 years ago by matt (mattab)

  • Component changed from Core to UI (templates, javascript)

comment:7 Changed 5 years ago by matt (mattab)

  • Keywords plot entity UI pattern added
  • Summary changed from Plugin to show number of visits for a specific keyword / page view / country for the last 30 days / weeks / etc to Plugin to show/plot number of visits for a specific keyword / page view / country for the last 30 days / weeks / etc

comment:8 Changed 5 years ago by domtop

comment:9 Changed 4 years ago by matt (mattab)

  • Description modified (diff)
  • Sensitive unset

comment:10 Changed 4 years ago by matt (mattab)

  • Priority changed from major to critical

This would make Piwik a much stronger Analytics tool.

comment:11 Changed 3 years ago by matt (mattab)

  • Description modified (diff)

comment:12 Changed 3 years ago by matt (mattab)

  • Milestone changed from Feature requests to 1.x - Piwik 1.x

comment:13 Changed 3 years ago by matt (mattab)

  • Summary changed from Plugin to show/plot number of visits for a specific keyword / page view / country for the last 30 days / weeks / etc to Show graph of visits/pages over the last 30 days, for a specific row (keyword, country, etc.)

comment:14 Changed 3 years ago by DaSch

to stay on track from #508

comment:15 Changed 3 years ago by matt (mattab)

For reports that have a limited number of rows, for example: #2031, #583, #536, #584

It would be very nice to easily be able to splot the trends of each of these values over time, without having to click on each value separately to see the graph.

The graph plotting history could be shown for all these values at once, maybe with multiple lines on a line plot?

comment:16 Changed 3 years ago by EZdesign (BeezyT)

Page URL, page title, country and keyword are the labels of the DataTables. We could add a parameter "label" to the API. If set, the API plugin filters the table by label. This way, the other plugins don't event need to know about it.

This filtering funcionality has to be available for other plugins as well, eg for Actions.getPageUrl. Maybe it can be implemented in the DataTable class itself. DataTable_Array has to implement a similar function which passes the request to the other tables.

The filtering should provide only one record. If a match is found, we can stop searching. This is especially important for nested DataTables.

If performance is poor (which I suspect), we could add some sort of index to the DataTables. It doesn't have to be super fast so we can pick a lightweight one.

comment:17 Changed 3 years ago by matt (mattab)

The datatable related code must definitely be automatic for all reports, and work consistently across them without need for custom hacks for some reports.

The big challenge of this ticket is the UI of drawing a graph for 1 row, or several rows at once eg.

  • compare performance of 2 key pages over the last 12 months
  • compare performance of the current top 10 keywords over the last 12 months (10 lines on a line plot, confusing but can be useful!)

The UI should make it easy to plot one row, but also plot multiple. How could it look like? how would we "reset" to the previous table view? Where would the graph be displayed? What would be the possible interactions with the graph?

We could ask Marcin our designer to prepare pixel perfect mockup but before would need to come up with wireframe level ideas of such complicated-to-integrate-nicely UI element. Any idea please suggest :)

comment:18 Changed 3 years ago by EZdesign (BeezyT)

I've started implementing the filters for the API using the indexes that are already there on DataTables. The API gets a new parameter "label" and filters the result, if it's a DataTable or DataTable_Array. I also updated the recursive search code of the Actions plugin - works fine.

As for the UI, we could open a properties dialog (similar to right-click in windows) when a row is clicked. There, we can show the available actions like "open url", "show recent development" etc. This is more flexible than icons and clutters the UI less.

comment:19 Changed 3 years ago by matt (mattab)

Proposal for below the graph mini dashboard

= Keyword "Analytics free" =

$SPARKLINE Visits: between 134 - 670 visits per day, +14% over the period
$SPARKLINE Ecommerce orders: between 13 - 44 Ecommerce orders per day, -22% over the period

etc. for all metrics, below, ordered by metrics usefulness
  • Like visible on the screenshot, text for other metrics faded out for other metrics to highlight current one
  • The first number shows the range of values over the period.
  • The second number +14% shows the difference between the first and last day of period. It is green or red based on performance (reusable css styles from All websites dashboard)
  • it will process the % evolution using last period, which can be partial (only first half of current month)... so % will go down and be RED often... maybe we could base it on the last finished period?
  • % between -5 and +5 could be left black instead of colored red/green?
    • Not a big fan of right click for UI, since right click is very hard to access and most people don't expect to use it in web app.
    • Maybe we could display a graph icon on hover on each row, for example this one which is great. I suppose it is not that frequent that users hover on the rows for no reasons.


See attached proposal mockup, workflow could be:

  • show icon + text "Plot" on hover on each row, should be big enough to be easily clickable, but should not take whole width since the row itself is sometimes clickable (eg. to show Custom Variables values for a given row of Custom Variable name)
  • hover on the selector would make it light yellow or something,
  • a click would open the new UI element with graphs etc.
  • A click on the grey outside or pressing ESC closes the popover.

Thoughts? very excited by this feature :)

Changed 3 years ago by matt (mattab)

Mockup to show click to launch popover

comment:20 Changed 3 years ago by matt (mattab)

In the popover, when showing

  • stats for browsers, OS, countries, search engines, etc. the logo (found in the api result) could be displayed
    • in the title next to the $label
    • in the graph in the legend - but maybe icons are too wide for the graph legend?
  • when showing search engines keywords, or page urls evolution, the $label could be linked to the actual search engine result page or page URL (as found in API response)

thoughts?

comment:21 Changed 2 years ago by matt (mattab)

  • Milestone changed from 1.x - Piwik 1.x to 1.8 Piwik 1.8

comment:22 Changed 2 years ago by EZdesign (BeezyT)

(In [5722]) refs #534, #2742 metadata api extensions: more/better translations, actionToLoadSubTables

comment:23 Changed 2 years ago by EZdesign (BeezyT)

(In [5723]) refs #534, #2742 integration tests for previous commit

comment:24 Changed 2 years ago by matt (mattab)

(In [5737]) Remove exit_rate from common metrics Refs #534

comment:25 Changed 2 years ago by matt (mattab)

(In [5739]) * Refs #534 Removing exit_rate from metadata API, it was added in the standard processed metrics causing other reports to display it with 0% values

  • Refs #1465 Leave underscores in test names just to keep tests more readable (even though it is not a best practise to have _ in class prefixes!!)

comment:26 Changed 2 years ago by EZdesign (BeezyT)

(In [5758]) refs #534 recursive label filter for the api

comment:27 Changed 2 years ago by EZdesign (BeezyT)

(In [5759]) refs #534 integration tests for label filter

comment:28 Changed 2 years ago by EZdesign (BeezyT)

The previous commits introduce a label filter. It works for all API methods and filters out a single row - even from a subtable.

This filter will be used to get statistics for a single row in row evolution. In JS, the label (or the parts of the label in a nested table) will be retrived from the data table dom-nodes and an appropriate API request will be made.

Please review this part.

comment:29 Changed 2 years ago by EZdesign (BeezyT)

(In [5760]) refs #534 label filter integration tests for range (=> DataTable_Array)

comment:30 Changed 2 years ago by EZdesign (BeezyT)

(In [5761]) refs #534 removed originalLabel (left over from an earlier version of row evolution, not needed anymore)

comment:31 Changed 2 years ago by EZdesign (BeezyT)

(In [5762]) refs #534 svn properties set on new file DataTableLabelFilter.php

comment:32 Changed 2 years ago by EZdesign (BeezyT)

(In [5767]) refs #534: introducing row actions for data tables

comment:33 Changed 2 years ago by EZdesign (BeezyT)

The previous commit does not change the UI yet becuase I didn't commit the row actions' HTML in the template. I'll do that as soon as the other row evolution code is ready (because the committed code triggers row evolution).

For now, we can discuss the way row actions work. I didn't add them to datatable.js because this code is already pretty long and complex. Instead, datatable.js contains code that loads row action objects based on DOM elements in the table. This way, we can add more actions to the HTML and the existing code will load new actions, which we then have to add to datatable_rowactions.js.

Sounds good?

comment:34 Changed 2 years ago by matt (mattab)

@EZdesign Kuddos!! Review:

  • Great that you put the datatable_rowactions.js in a separate file. As you said the datatable.js is already quite bloated, so that's very welcome..
  • Do we really need to support the DataTable_Array use case? Because, if it is not going to be used by the UI, then there is no need to support the new DataTableLabelFilter (since no API consumer will need it?) If not needed by UI, we can remove the special Datatable_Array code and tests in [5760]
  • in the following code
    if (!plabel) {
    	break;
    }
    labels.push(plabel);
    
    I think the if() could actually maybe be triggered, because of this frequent bug: #1024 So in the case that a "row evolution" report cannot be drawn (data not found in parent table, etc.), it would be nice to test/check that the code fails gracefully (since this edge case will happen).

I will make a few minor modifications as well in the next few hours

comment:35 Changed 2 years ago by EZdesign (BeezyT)

The DataTable_Array case is used every time row evolution is displayed. We want to get statistics for the previous days at once, so that will be an array.

comment:36 Changed 2 years ago by matt (mattab)

(In [5774]) Refs #534

  • To ensure that the "label filtering" works on all types of labels including special characters "' <> etc. I refactored the SafeDecodeLabel filter function.
  • Changed DataTableLabelFilter to use Piwik_API_ResponseBuilder directly rather than duplicating code
  • Added few tests cases

comment:37 Changed 2 years ago by matt (mattab)

One more code review note:

  • would you mind adding a test for Referers Search -> Keywords which would test the case that different APIs are called for parents VS children.

Otherwise looks good, please go ahead with the rest of commit -- I'm excited! :)

comment:38 Changed 2 years ago by matt (mattab)

(In [5848]) Truncation was not working anymore (the tooltip was showing the truncated text rather than full text), reverting part of [5767]

Refs #534

comment:39 Changed 2 years ago by EZdesign (BeezyT)

In ResponseBuilder, we get the label from $this->request. The problem is that the values are trimmed in Piwik_API_Request::getRequestArrayFromString().

The page titles reports has labels starting with a space and therefore the labels can't be found using the index of the DataTable. Can we remove the trimming in Piwik_API_Request?

comment:40 Changed 2 years ago by EZdesign (BeezyT)

In [5848] you reverted backing up the original label to the DOM node's data because it changed the truncation. I can't reproduce the problem and neither jquery.truncate nor jquery.tooltip read data('originalText'). Can you please investigate and tell me how to reproduce the problem?

We need to know the original text in order to send it to the label filter.

comment:41 Changed 2 years ago by EZdesign (BeezyT)

(In [5872]) refs #534 lib / graph updates

  • external series toggles for jqplot
  • row evolution toggle
  • using preloaded DataTable for ViewDataTable
  • new Date methods subWeek and subYear
  • hiding all view icons of a graph
  • template html fixes
  • and some minor things

comment:42 Changed 2 years ago by EZdesign (BeezyT)

(In [5873]) refs #534 the core of row evolution

comment:43 Changed 2 years ago by EZdesign (BeezyT)

The previous commit is the PHP in CoreHome. I didn't want to add 500 lines the CoreHome Controller so I created a folder for DataTable actions. There are more of them to come in the future (e.g. clickpath).

Please review the way it hooks into the Controller and let me know whether you have ideas for improvement.

comment:44 Changed 2 years ago by EZdesign (BeezyT)

(In [5874]) refs #534 svn properties for new files

comment:45 Changed 2 years ago by matt (mattab)

Timo, sorry for the delay, here is the review:

Code review/feedback

50% and 50s should be plotted as 50

$toRemove = array('%', 's');

This will maybe not work for all languages (assuming the s comes from Piwik_Translate('General_Seconds', $value)
So, maybe it should be something like array('%', str_replace('%s','',Piwik_Translate('General_Seconds'))) ?

I didn't want to add 500 lines the CoreHome Controller so I created a folder for DataTable actions. There are more of them to come in the future (e.g. clickpath).

Thanks for not adding 500 lines in CoreHome ;)

TODO this used to be: $_GETlabel? = $this->label = $rowLabel;

Not sure why, but it's good that we don't need the _GET anymore

In ResponseBuilder, we get the label from $this->request. The problem is that the values are trimmed in Piwik_API_Request::getRequestArrayFromString().
The page titles reports has labels starting with a space and therefore the labels can't be found using the index of the DataTable. Can we remove the trimming in Piwik_API_Request?

Removing the trim() in the request will break things and other users apps (it's common for the request string to be put across several lines).

This white space before the label is a ugly hack that I put in like 4 years ago... it is there to differentiate between the Page title "Contact" and "Contact/Hello". The ugly code is in plugins/Actions/Actions.php:716

		if($type != Piwik_Tracker_Action::TYPE_ACTION_NAME )
		{
			$lastPageName = '/' . $lastPageName;
		}
		else
		{
			$lastPageName = ' ' . $lastPageName;
		}

Would it be possible for you, when the requested report is "Actions.getPageTitles", to hack the space before issuing the search?
There is already a special case for " special case: websites report" so it would be OK to do it this way IMO.

In [5848] you reverted backing up the original label to the DOM node's data because it changed the truncation. I can't reproduce the problem and neither jquery.truncate nor jquery.tooltip read data('originalText'). Can you please investigate and tell me how to reproduce the problem?

I'm confused. I'm nearly sure that I didn't dream, but now I tried to put the code back in and truncation seems to work in all reports, subtables, etc. So please put it back in, it seems I was wrong!

$this->metric = Piwik_Common::getRequestVar('metric', , 'string');

for consistency 'metric' should be 'columns' (already found in a few API signatures)

$this->labels = explode('<+MultiRow+>', html_entity_decode($this->label));

the labels should be comma separated, and each label separately URL encoded in JS. So the comma would be eg. %2C

In fact, it should probably be the same for the label filtering, instead of using Piwik_API_DataTableLabelFilter::RECURSIVE_LABEL_SEPARATOR each label could be URL encoded and comma separated?

Please review the way it hooks into the Controller and let me know whether you have ideas for improvement.

I have a big request/feedback: please put all the data fetching code in its own API function in API/API.php
For example, a new function API.getEvolution($idSite, $period, $date, $label, $apiModule, $apiAction, $segment = false, $columns = false, $idGoal = false) for example

It will keep the separation of concepts found in all piwik code.
Also, and this is very important, it will make testing much easier because we will be able to easily write integration tests calling the API and checking the data outputs.

Tests

Please add some integration tests for the API part, at least one for the 2 special cases Referers.getWebsites, Actions.getPageTitles, and maybe one for Actions.getPageUrls ?

Otherwise, looks good, good luck & keep up the great work :)

comment:46 Changed 2 years ago by matt (mattab)

(In [5958]) Refs #534 minor changes

comment:47 Changed 2 years ago by EZdesign (BeezyT)

(In [5981]) refs #534:

  • implementing matts suggestions
  • fixing truncation bug
  • new label encoding
  • extended label filter tests

comment:48 Changed 2 years ago by EZdesign (BeezyT)

(In [5982]) refs #534: moved row evolution data processing to API plugin, added test cases

comment:49 Changed 2 years ago by EZdesign (BeezyT)

(In [5983]) refs #534: different periods

comment:50 Changed 2 years ago by EZdesign (BeezyT)

(In [5986]) refs #534: using API.getRowEvolution in RowEvolution.php

comment:51 Changed 2 years ago by EZdesign (BeezyT)

(In [5989]) refs #534: using Api.getRowEvolution in MultiRowEvolution.php, extended tests

comment:52 Changed 2 years ago by EZdesign (BeezyT)

In the previous commits, I implemented the review notes. I probably introduced some bugs that I will fix later but for now it's important to review the code in API/API.php and the test cases.

comment:53 Changed 2 years ago by EZdesign (BeezyT)

I just noticed a problem I'd like to discuss: When you pass a regular label to API.getRowEvolution that contains a comma, the method will detect that a multi row evolution has been requested. The cleanest way I can think of to solve this is to split the method into one method for regular row evolution and one for multi row evolution. What do you think?

comment:54 Changed 2 years ago by EZdesign (BeezyT)

(In [5990]) refs #534: visibility fix

comment:55 Changed 2 years ago by matt (mattab)

(In [5999]) Refs #534 Minor code style + renaming tests to .test.php to ensure they are picked up by the CI server

comment:56 Changed 2 years ago by EZdesign (BeezyT)

(In [6003]) refs #534: fix the build?

comment:57 Changed 2 years ago by peterb (peterbo)

OT: " TODO: confirm we need this " is nice, it reminds me a bit of this funny thing: http://cobaia.net/2010/09/top-funny-source-code-comments/

*G*

comment:58 Changed 2 years ago by matt (mattab)

(In [6017]) Refs #534

  • Adding "segment" parameter so that row evolution works when we implement #2135
  • still to add: tests

comment:59 Changed 2 years ago by matt (mattab)

(In [6018]) Refs #534

  • Style changes + fix key in XML output
  • rename <data> to <reportData> for consistency with metadata (later, maybe we will enrich the output with more stuff that will make it more metadata like)

comment:60 Changed 2 years ago by matt (mattab)

  • " TODO: confirm we need this " Myabe you used to need this when code wasn't in the API? Now, the SafeDecodeLabel filter will be automatically

applied on output (for XSS securisation) which makes this code obsolete (my guess)

comment:61 Changed 2 years ago by matt (mattab)

Commit in [6019] didn't appear here:

  • Fix last defaultKeyName in XML output
  • Fix bug output for Websites (should use URL). Only using URL for Websites and Actions (otherwise Keywords would show the search engine URL)
    • @Timo: Should I also getUrlsFromWebsiteId in the test in API.php line 1248 ?
  • NOTE: there is a bug when keywords contain the character > eg "free > proprietary" in the test case.
    • The keywords are not matched and return No data
    • Not sure how to fix that bug as I think we lose the information during the filtering in Piwik_API_DataTableLabelFilter ?
  • rename availableColumns to columns

comment:62 Changed 2 years ago by matt (mattab)

(In [6020]) Refs #534

  • This is slightly better

comment:63 Changed 2 years ago by EZdesign (BeezyT)

(In [6095]) refs #534 assets for row evolution

comment:64 Changed 2 years ago by EZdesign (BeezyT)

(In [6096]) refs #534 removed metrics documentation from the actions controller for entry and exit pages. this available from the metadata now.

comment:65 Changed 2 years ago by EZdesign (BeezyT)

(In [6097]) refs #534 reset id tag

comment:66 Changed 2 years ago by EZdesign (BeezyT)

(In [6098]) refs #534 completing [6018]

comment:67 Changed 2 years ago by EZdesign (BeezyT)

(In [6099]) refs #534 small fix for ViewDataTable

comment:68 Changed 2 years ago by matt (mattab)

Feedback

  • + 'RowEvolution_AvailableMetrics' => 'Metrics for', should be + 'RowEvolution_AvailableMetrics' => 'Metrics for %s',

you are getting close aren't you?? I thought it was committed but still don't see the little magic icon ;-)

comment:69 Changed 2 years ago by EZdesign (BeezyT)

(In [6114]) refs #534 handle labels containing &, data => reportData

comment:70 Changed 2 years ago by EZdesign (BeezyT)

(In [6115]) refs #534 fixing row evolution test cases for special characters; they only appeared to be working but the keywords were cut off after the ampersand.

comment:71 Changed 2 years ago by EZdesign (BeezyT)

(In [6116]) refs #534 the url builder update slightly changes the image graph urls => update expected text output

comment:72 Changed 2 years ago by EZdesign (BeezyT)

(In [6117]) refs #534 refactorings: popup => popover, data table action => data table row action

comment:73 Changed 2 years ago by EZdesign (BeezyT)

(In [6118]) refs #534 datatable_rowactions.js: some bug fixes and architecture refactorings for popovers in the URL (browser history) and transitions row action

comment:74 Changed 2 years ago by EZdesign (BeezyT)

(In [6119]) refs #534 using JSON_HEX_QUOT for json_encode. Otherwise encoded double quotes will causes an error (at least in Chrome).

comment:75 Changed 2 years ago by EZdesign (BeezyT)

(In [6120]) refs #534 first working version of row evolution!

  • include trigger in datatable_cell
  • new methods in API_API: getUnit and isLowerValueBetter
  • lower bounce / exit rate is green in the popover
  • minor fixes

comment:76 Changed 2 years ago by peterb (peterbo)

Great feature, Timo! Nice work! - However, I get an exception for core/Common.php on line 1060 (return json_encode($value, JSON_HEX_QUOT);):
1) Notice: Use of undefined constant JSON_HEX_QUOT - assumed 'JSON_HEX_QUOT' in /var/www/vhosts/xxx/httpdocs/statistik/core/Common.php on line 1060
2) Warning: json_encode() expects exactly 1 parameter, 2 given in /var/www/vhosts/xxx/httpdocs/statistik/core/Common.php on line 1060

That's because the options-param and the JSON_HEX_QUOT constant for the "json_encode ( mixed $value [, int $options = 0 ] )"-function was added in PHP 5.3 the first time.

Editing the line to "return json_encode($value);" fixed the problem (at least for having a first look at the feature).

comment:77 Changed 2 years ago by EZdesign (BeezyT)

(In [6121]) refs #534 removed JSON_HEX_QUOT from json_encode

comment:78 follow-up: Changed 2 years ago by EZdesign (BeezyT)

  • Description modified (diff)
  • Summary changed from Show graph of visits/pages over the last 30 days, for a specific row (keyword, country, etc.) to Row Evolution

Thanks for the feedback, Peter. I removed JSON_HEX_QUOT.

The problem I tried to solve is that a double quote in graph data (e.g. a page title with a double quote) is escaped twice. For example, Hi "Peter" would become "Hi \\"Peter\\"". Obviously, this leads to a syntax error in JS.

I did some debugging and noticed that it is not the fault of json_encode. When I echo the properly escaped JSON in Piwik_Visualization_Chart::render, it gets escaped a second time later. The problem is line 157 of Piwik_ViewDataTable_GenerateGraphHTML. It replaces \" with
". Why?? This makes escaping quotes impossible.

comment:79 follow-up: Changed 2 years ago by EZdesign (BeezyT)

After my last commit, Http.test.php is failing. Is that conntected to my commit? I passes on my machine and I can't see any correlation...

comment:80 in reply to: ↑ 79 Changed 2 years ago by matt (mattab)

report from the forum

I tested it a lot earlier and didn't have any problems, but just now I tried out multi-selection on the List of Keywords and soon got the following error.

Fatal error: Maximum function nesting level of '100' reached, aborting! in /home/www/dev5.piwik.org/core/DataTable.php on line 116

The pop up window would open but there wouldn't be any graph. I was also getting this in that window:

Metrics for Keyword: fatal error: exception thrown without a stack frame in unknown on line 0 

Not sure if it's easy possible to replicate?

Replying to EZdesign:

After my last commit, Http.test.php is failing. Is that conntected to my commit? I passes on my machine and I can't see any correlation...

The test failed because the api.piwik.org server was temp unavailable.

comment:81 Changed 2 years ago by matt (mattab)

Timo,
fantastic work. Well done! This feature in its beta state is incredibly useful...

I have been brainstorming and using it a bit and now will do a UX/UI Review.
These are a lot of suggestions, I hope they make sense and very excited to see how useful we can make this feature!!

Suggestions moved to: #3158

comment:82 in reply to: ↑ 78 Changed 2 years ago by matt (mattab)

I did some debugging and noticed that it is not the fault of json_encode. When I echo the properly escaped JSON in Piwik_Visualization_Chart::render, it gets escaped a second time later. The problem is line 157 of Piwik_ViewDataTable_GenerateGraphHTML. It replaces \" with
". Why?? This makes escaping quotes impossible.

Please try removing the escaping of single/double quotes at this line. If all graphs still work then it's good :) it probably should not be there anyway but worth confirming that nothing else breaks.

comment:83 Changed 2 years ago by EZdesign (BeezyT)

(In [6196]) refs #534

  • Proper handling of quotes in labels
  • Better popover title algorithm: don't show urls for search engines, remove protocol and www from URLs, show the entire path - not only the last component - in row evolution (like in multi row evolution)
  • Graph padding: a bit more for small values, make sure %-axes don't go above 100%
  • Series with only 0 are shown in row evolution now (they were excluded because the graph couldn't handle them - now it works and excluding them caused other problems)
  • Fixing issue reported in the forum - row evolution now works on subtables with period=week
  • +100% over the period is not shown anymore when the first value is 0 (i.e. there probably is no data for the first sub period)
  • If a series starts with 0 values, the min/max algorithm ignores them
  • JSON encoding fix for quotes

comment:84 Changed 2 years ago by matt (mattab)

so good to see Row evolution commits :)

Feedback

comment:85 Changed 2 years ago by matt (mattab)

(In [6198]) Refs #534 Fixing popover when period=range. Removed the _GET[period] since it is redundant

comment:86 Changed 23 months ago by matt (mattab)

  • Milestone changed from 1.9 Piwik 1.9 to 1.8

We are going to release Row Evolution Beta as part of the next Piwik release as it's incredibly useful and a must have!

I propose to move all suggestions, ideas, bug reports to another ticket and close this ticket as we reached the beta milestone.

OK to close this ticket, or is there some critical bug to be fixed before stable release?

comment:87 Changed 23 months ago by matt (mattab)

Timo, thanks for all the bug fixes earlier... there are really not any bug I can find now :)

I created the follow up ticket for all other improvements for Row Evolution V2: #3158
Thank you for continuing all discussion over there :)

It would be good to fix the following 2 small bugs before stable release:

  • graphs should not be clickable to change date
  • Clicking on black background should close popover

comment:88 Changed 23 months ago by SteveG (sgiehl)

(In [6383]) refs #534 close row evolution popover on click outside + cancel requested content if popover is closed before loading is finished

comment:89 Changed 23 months ago by SteveG (sgiehl)

(In [6384]) refs #534 disable links in popover evolution charts to switch the date

comment:90 Changed 23 months ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from Row Evolution to Row Evolution: historical view of one or many specific rows in all Piwik reports!

Thanks so much Steve!

ROW EVOLUTION V1 done! :)

well done guys..

comment:91 Changed 23 months ago by matt (mattab)

(In [6410]) Refs #534, Refs #3158

  • Displaying logos when comparing multiple rows. It looks very nice for example comparing Firefox VS Chrome VS IE Vs Safari :)

comment:92 Changed 19 months ago by matt (mattab)

in: [7020] revert partial of [6114] Refs #534 - Fixes #3387 Double encoding issue. Thanks larsen for report!

comment:93 Changed 18 months ago by EZdesign (BeezyT)

(In [7179]) refs #534: bringing the new popover behavior to row evolution

  • the popover parameter is used to remember the current state of row evolution
  • Piwik_Popover is used for controlling the popover
  • ajax helper methods are refactored from Transitions to piwikHelper and reused in row evolution
  • simplified logic for remembering rows for multi row evolution
  • applied code formatter (hence the whitespace changes)
Note: See TracTickets for help on using tickets.