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

Jump to bottom

Keep top level nb_visits_converted metric #14253

Merged
merged 22 commits into from Jul 16, 2019
Merged

Keep top level nb_visits_converted metric #14253

merged 22 commits into from Jul 16, 2019

Conversation

diosmosis
Copy link
Member

Currently we delete this metric "because it could differ from the sum of each goal conversions", but it should, since nb_visits_converted is not the sum of each goal's conversions. Conversions can happen multiple times per visit, so this would not be accurate.

Also, it is not the sum of each goals' nb_visits_converted, since visits can share conversions for different goals. Keeping it puts the correct value of conversion_rate in scheduled reports.

Fixes #10109

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Mar 22, 2019
@diosmosis diosmosis added this to the 3.10.0 milestone Mar 22, 2019
mattab
mattab reviewed May 15, 2019
View reviewed changes
plugins/CoreHome/Columns/Metrics/ConversionRate.php Outdated
$nbVisitsConverted = 0;
foreach ($goals as $idGoal => $values) {
if ($idGoal === 0
|| $idGoal === '0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there maybe a CONST we could reference instead of hard coding the value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do

@mattab
Copy link
Member

mattab commented May 15, 2019

Feedback

  • in general i'm not sure if this is accurate. "Nb visits converted" should be number of visits with at least one converted goal. but this logic takes the "max" value of "nb visits converted" for any given goal. The "Real" accurate value of "Nb visits converted" will be equal or higher to the value processed here. For example we may have 5 visits converted to goal X and 10 visits converted to goal Y, this PR will say that 10 visits are converted, but it could be that anywhere between 10 and 15 visits are converted. So it wouldn't show accurate numbers. Is there maybe another way to have this accurate nb_visits_converted metric?
  • A sub-module change is included in the PR which I suppose is not expected

@diosmosis
Copy link
Member Author

Moving to 3.11 since it's not a straightforward fix

@diosmosis diosmosis modified the milestones: 3.10.0, 3.11.0 May 16, 2019
@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jun 25, 2019
@diosmosis
Copy link
Member Author

@mattab removed the code you were talking about (that was for showing the conversion rate for old reports, but of course that won't work).

From testing manually and looking at automated tests, the nb_visits_converted metric that's not in the goals array looks accurate. It's computed by queryVisitsByDimension() which aggregates over the log_visit table, so I'm not sure why it would be inaccurate. Don't think there's an issue in keeping it.

@mattab
Copy link
Member

mattab commented Jul 10, 2019

LGTM
(tests need updated before merge)

@tsteur
Copy link
Member

tsteur commented Jul 14, 2019

Some tests are still failing @diosmosis . Feel free to merge once the build is green

@diosmosis diosmosis merged commit e4c1f1f into 3.x-dev Jul 16, 2019
@diosmosis diosmosis deleted the conversion-rate-keep branch July 16, 2019 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@mattab mattab mattab left review comments

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.

On email reports the conversion rate is zero always
3 participants
@diosmosis @mattab @tsteur