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

fix insights widget "Others" percentage #17656

Merged
merged 2 commits into from Jun 9, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Jun 8, 2021

Description:

fixes #17615

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this Jun 8, 2021
@flamisz flamisz added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jun 8, 2021
@diosmosis
Copy link
Member

code looks good 👍

@diosmosis diosmosis merged commit 63190d1 into 4.x-dev Jun 9, 2021
@diosmosis diosmosis deleted the 17615-insight-plugin-regression branch June 9, 2021 06:16
@MichaIng
Copy link
Contributor

MichaIng commented Jun 9, 2021

Many thanks. I applied the new code, but I think I have to wait for the next report generation to see the results.

Where is the new function getPastRowFromCurrent actually used?
EDIT: Ah sorry, I see now it is an override.

@MichaIng
Copy link
Contributor

MichaIng commented Jun 9, 2021

Okay, it does not fix the issue in our case. I added the code, verified it, cleared all caches, but it still shows:

Others +100%
Others -100%

on all page title lists.

Are there other companion changes required, committed outside of this PR?

@diosmosis
Copy link
Member

@MichaIng This by itself seemed to fix it for me locally (and the test works on this branch but fails on 4.x-dev). I'll reopen the original issue.

@diosmosis
Copy link
Member

@MichaIng could you post the following screenshots to help us debug?

  • a screenshot of the insights overview page w/ the issue visible (and the period / date visible)
  • a screenshot of the page titles report for the same period/date, sorted in descending order so the Others row is shown first
  • a screenshot of the page titles report (as before) but for the previous period/date (again sorted so we see the Others row)

@MichaIng
Copy link
Contributor

insights

The page titles report does not show "Others" at all, neither for the current period (this is one week), nor for any previous period, long or short.

@diosmosis
Copy link
Member

@MichaIng I'll try to reproduce this again. Would you be willing to provide view access to your Matomo in the event I can't?

@MichaIng
Copy link
Contributor

Yes, I can do that.

@diosmosis
Copy link
Member

diosmosis commented Jun 11, 2021

@MichaIng can you email dizzy@innocraft.com with access details? I'll take a closer look today.

@MichaIng
Copy link
Contributor

MichaIng commented Jun 11, 2021

Okay that is strange, the issue seems to fade currently, while all I just did is updating another app. Is the "Others" entry preserved in reports and because of this still shown even that the code has been fixed? The very same time range the screenshot above is from now does not shot "Others" anymore, the "PAGE TITLES" section in the "Insights Overview" widget is gone completely instead. I still see "Others" when switching to "day" or "week" view (above is "date range", covering last seven days).

But I'll send you access details regardless.

@diosmosis
Copy link
Member

@MichaIng

Is the "Others" entry preserved in reports and because of this still shown even that the code has been fixed?

It is preserved in reports, but the Insights report is not stored in the database. It's built dynamically using stored reports for the current period and previous period, for example, the page titles reports. So if the data in the periods for other reports (like the page titles reports) change, then the Insights reports should display different data immediately. I'm not sure why it would show nothing now... I'll take a look later and see what I can find.

@diosmosis
Copy link
Member

Resolved via email, the PR does work.

@MichaIng
Copy link
Contributor

Yes, many thanks. The entries from the old records basically needed to fade out.

@Findus23 Findus23 changed the title fix regression fix Insights widget "Others" percentage Jun 13, 2021
@Findus23 Findus23 changed the title fix Insights widget "Others" percentage fix insights widget "Others" percentage Jun 13, 2021
@sgiehl
Copy link
Member

sgiehl commented Jun 13, 2021

@diosmosis so we can close #17615 again?

@diosmosis
Copy link
Member

@sgiehl yes 👍. Closed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insights widget shows only "Others" page titles
4 participants