@sgiehl opened this Pull Request on November 11th 2021 Member

Description:

This change would actually remove the time on page for all actions other than pageviews. For pageviews the time on page would be summed up till the next pageview.

This is an approach to fix #18270

Did the changes in a couple of minutes while checking for a possible reason. To finish this some manual testing and updating/adding tests would be needed.

@tsteur If this is something that should be worked on further, feel free to assign the PR to a milestone and I will resume the work then.

Review

@tsteur commented on November 11th 2021 Member

@sgiehl how much more time would it take to finish it? I guess originally the idea was to eventually fix the root cause of https://github.com/matomo-org/matomo/issues/9198 which then would fix this as well. As this might take a while until we work on https://github.com/matomo-org/matomo/issues/9198 could maybe get this done now if it's real quick.

There's a bit of a problem that then eg people who aggregate the page view time manually will find it's different to the reported average time. But we could probably ignore this.

@sgiehl commented on November 12th 2021 Member

@tsteur I think the code in the PR should actually work. It would remove the "time on page" metric for all non page view actions in the visits log API. The time on page views should be summed up.
To ensure that really works as expected I would need to build a simple system test case that tracks multiple different actions and if that works also adjust the now failing tests. Guess that would be doable in a couple of hours.

@tsteur commented on November 14th 2021 Member

@sgiehl this already changes quite a lot of system tests so maybe no new system test would need to be written ? https://app.travis-ci.com/github/matomo-org/matomo/jobs/547727804

@sgiehl commented on November 18th 2021 Member

I'd rather like to write a very simple new test that tracks some different actions and validate if the resulting number are correct and then simply update the expected results of all other tests assuming they are correct.
Could also try to do that with the existing tests, but might be more effort to look through the fixtures and check which results would be expected, as we often don't turn time forward when tracking additional actions.

@sgiehl commented on November 23rd 2021 Member

This PR should be ready for a first review. The time on page should now only be calculated for page views. For all other action types the timeSpent and timeSpentRef columns are removed. If a visit doesn't contain a page view, there will actually be no action that contains a timeSpent

Some System tests of submodule plugins are still failing, but those can be updated once this one would be ready to merge.

@sgiehl commented on November 24th 2021 Member

@tsteur shall we merge this one for 4.7 then?

@tsteur commented on November 24th 2021 Member

OK sounds good

@sgiehl commented on November 26th 2021 Member

@AltamashShaikh This most likely will break the tests for various plugins. The time spent in visit details may change in various tests. Those can be simply updated, if there are no other changes.

@AltamashShaikh commented on November 26th 2021 Contributor

Okay Thanks,
Will keep this in mind

This Pull Request was closed on November 26th 2021
Powered by GitHub Issue Mirror