Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Row Evolution shows wrong data when directory and file with identical names exist at same level #4363

Closed
RMastop opened this issue Dec 6, 2013 · 19 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@RMastop
Copy link
Contributor

RMastop commented Dec 6, 2013

Problem exists in the Actions -> Page titles section of Piwik Dashboard.

The customer has a site where a visit to /m/verbind-mobiel has the same result as /m/verbind-mobiel/ (remark the last /)
When we select row evaluation on the /m/verbind-mobiel row, we get the results of the /m/verbind-mobiel/ row.

Is there a way to fix this error.

@RMastop
Copy link
Contributor Author

RMastop commented Dec 6, 2013

Attachment:
pageTitles.jpg

@RMastop
Copy link
Contributor Author

RMastop commented Dec 6, 2013

Attachment:
RowEvaluation.jpg

@mattab
Copy link
Member

mattab commented Dec 8, 2013

Do you also experience the problem with the latest Piwik 2.0 beta? I remember fixing a problem similar to this so please try the latest beta. If you still have the bug we will definitely like to fix it.

@RMastop
Copy link
Contributor Author

RMastop commented Dec 9, 2013

I can confirm that the problem exists in piwik 2.0-b10

@RMastop
Copy link
Contributor Author

RMastop commented Dec 9, 2013

Attachment: row evaluation in piwik 2.0-b10
rowevaluation-in-piwik2.0-b10 .png

@mattab
Copy link
Member

mattab commented Dec 9, 2013

Also, the labels should be m/docs instead of m - docs (using slash separator for URL paths, instead of -)

There has been a regression somewhere along the way, thanks for the report!

@diosmosis
Copy link
Member

In 346ef97: Refs #4363 display correct row evolution title for actions URL reports.

@diosmosis
Copy link
Member

@RMastop I tried to reproduce the error, but couldn't. The data was correct for each row evolution popup. Can you check again? The third screenshot you posted (the one for piwik 2.0-b10) looks correct as well.

If there is still an error, can you post the 'popover' query parameter (the part of the url that's like popover=...) for each row evolution popup?

@RMastop
Copy link
Contributor Author

RMastop commented Dec 10, 2013

Hi capedfuzz,

I will explain a bit better. With some more screenshots.

I open the Page title and click the row pagetitel with the value 7 behind it (the row with the page value)

The result I get is the value of the row with the value 2 (the row with the directory value)

clicking the directory row results in the following URL variable:
popover=RowAction$3ARowEvolution$3AActions.getPageTitles$3A0$3Am$20$3E$20pagetitel

Clicking the page row results in the following URL variable:
popover=RowAction$3ARowEvolution$3AActions.getPageTitles$3A0$3Am$20$3E$20pagetitel

the value for both calls is the same, when the page title path is separated by '/' you could tell the difference between a file and a directory, since the directory will end with a /

@RMastop
Copy link
Contributor Author

RMastop commented Dec 10, 2013

@RMastop
Copy link
Contributor Author

RMastop commented Dec 10, 2013

@RMastop
Copy link
Contributor Author

RMastop commented Jan 2, 2014

Piwik 2.0.3-b3 still shows the same issue

@mattab
Copy link
Member

mattab commented Feb 6, 2014

Removing owner from tickets. from now on, I suggest we assign tickets to ourselves for cases when we we plan to actively work on them in the coming days/weeks. let's discuss if needed during our team call.

@diosmosis
Copy link
Member

In 37c2d8d: Refs #4363, add workaround to fix bug where leaf rows of page titles reports were not accessible via RowEvolution.

@diosmosis
Copy link
Member

Fixed for now, problem is as follows:

In reports, branch + terminal rows for page title reports are differentiated by a ' ' (space) prefix. This space cannot be placed w/i a row evolution 'label' parameter since it will be trimmed away, thus there is no way to differentiate between branch & terminal rows in a row evolution query. The 'correct' fix would be to introduce a difference that can be specified in the 'label' query parameter while still maintaining backwards compatibility. Such a fix would be complicated so waiting to confer for now.

@mattab
Copy link
Member

mattab commented Jun 30, 2014

there is no way to differentiate between branch & terminal rows in a row evolution query

Alternatively of using the appended '+', maybe adding a new parameter to the request that indicates whether the label is a branch or a leaf would be a better solution?

@diosmosis
Copy link
Member

Anything that differentiates between row types should be in the row evolution query itself since selecting the right row is the sole purpose of the 'label' query parameter. So adding a query parameter should be considered just as temporary as the committed solution. I considered using a query parameter but discarded the idea for the following reasons:

  • A query parameter looks official and could be used by others and could be considered final, despite the fact that it is not a true solution.
  • A new query parameter means detecting whether a row is terminal or not in the JS and then manipulating one or more AJAX requests, which is far more complicated than the committed solution.
  • An appended '+' will urldecode to a space which will be trimmed away which makes the committed solution very non-intrusive and easier to remove.

I think the best solution is to add a new symbol to row evolution queries that differentiates. For URL tables, the '/' is effectively used (since 'dir' would be a dir and '/dir' would be a file). Since labels are stored in the database as '/...' for URLs, we can't use '/', but we can use something like '!' or '$' or something else. So, 'm > !verbind-mobiel' would select a terminal as would 'dir > sub > !/page' & 'dir > sub > /page'.

@mattab
Copy link
Member

mattab commented Jul 1, 2014

Sounds good, I'm closing ticket as the bug is fixed, but feel free to append your commits using eg. ! character.

@diosmosis
Copy link
Member

In bcf5595: Fixes #5411, refs #4363 use '%20' to differentiate between terminal + leaf rows in row evolution JS instead of '+' since JS urlDecodeComponent does not decode + chars.

@RMastop RMastop added this to the 2.4.0 - Piwik 2.4.0 milestone Jul 8, 2014
diosmosis pushed a commit that referenced this issue Jul 9, 2014
… leaf rows in row evolution JS instead of '+' since JS urlDecodeComponent does not decode + chars.
diosmosis pushed a commit that referenced this issue Jul 9, 2014
…o get rid of previous hack used to fix #4363. Includes tests and changes to transitions so it will continue working.
sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

3 participants