@diosmosis opened this Pull Request on March 22nd 2019 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

@mattab commented on May 15th 2019 Member

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 commented on May 16th 2019 Member

Moving to 3.11 since it's not a straightforward fix

@diosmosis commented on July 7th 2019 Member

@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 commented on July 10th 2019 Member

LGTM
(tests need updated before merge)

@tsteur commented on July 14th 2019 Member

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

This Pull Request was closed on July 16th 2019
Powered by GitHub Issue Mirror