dev/Core#53 Add in stats to AB tests and add in clickthrough and open… #11957
Conversation
seamuslee001
force-pushed
the
core_53
branch
2 times, most recently
from
b41f39e to
2d09205
Compare
|
test this please |
|
@seamuslee001 will trade for #11954 |
|
Deal @eileenmcnaughton
…On Mon, 9 Apr 2018 at 9:36 am, Eileen McNaughton ***@***.***> wrote:
@seamuslee001 <https://github.com/seamuslee001> will trade for #11954
<#11954>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11957 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGe_FS0z75d1F-Ul6USs9waJUT9u_TFOks5tmp7-gaJpZM4TLR17>
.
|
CRM/Mailing/BAO/Mailing.php
Outdated
| @@ -1997,12 +1997,16 @@ public static function &report($id, $skipDetails = FALSE, $isSMS = FALSE) { | |||
| $row['bounce_rate'] = (100.0 * $mailing->bounce) / $mailing->queue; | |||
| $row['unsubscribe_rate'] = (100.0 * $row['unsubscribe']) / $mailing->queue; | |||
| $row['optout_rate'] = (100.0 * $row['optout']) / $mailing->queue; | |||
| $row['opened_rate'] = ($row['opened'] / $mailing->delivered * 100.0); | |||
There was a problem hiding this comment.
We should probably add protection against a hard error if delivered === 0 (presumably a very obscure situation but worth handling I think)
There was a problem hiding this comment.
Also - can we stdise bracketting - I prefer but mostly just thing same is better
$row['opened_rate'] = ($row['opened'] / $mailing->delivered) * 100;
with conditional
$row['opened_rate'] = $mailing->delivered ? (($row['opened'] / $mailing->delivered) * 100) : '';
|
@seamuslee001 I agree with this change in principle - ie it adds additional useful information to the A-B stats report and the change is localised to that report & hence low risk. I don't believe it could have a negative impact if it works as intended. Per comments I think each divided by line has a risk of being divided by 0 so worth going through those - in fact my test did hit that on the second mailing (I added the screen shots to the PR description above - but perhaps the after might get replaced again before merge). Visually I think Tracked Opens 1 (100%) is better than Tracked Opens 1 100% I think I'd also prefer the link to be on the number & not the % but from a quick look at the code that looks like more hassle than it's worth to do that. |
|
@eileenmcnaughton thanks for that I think i have now addressed all issues you alluded to in your review. I haven't quite got the () around on AB tests but there is at least now a ui difference between the number and the % |
api/v3/Mailing.php
Outdated
| @@ -769,6 +769,9 @@ function civicrm_api3_mailing_stats($params) { | |||
| break; | |||
| } | |||
| } | |||
| $stats[$params['mailing_id']]['delivered_rate'] = round((100.0 * $stats[$params['mailing_id']]['Delivered']) / CRM_Mailing_Event_BAO_Queue::getTotalCount($params['mailing_id'], $params['job_id']), 2) . '%'; | |||
There was a problem hiding this comment.
could these be subject to division by zero?
|
@seamuslee001 I'm still seeing my Granny on the B side |
…ed % stats to mailing reports Update Test following changes in stats function Fixes following on from review Further divide by zero fixes
|
thanks @eileenmcnaughton hopefully fixed in latest commit |
|
Yes - there is an inconsistency in 0 vs na but that is not new in this PR. The grey power is gone!
|
|
I'm happy this is ready to merge now - the things I observed have been addressed |
|
Merging as it indicates merge-on-pass |
…ed % stats to mailing reports
Overview
This adds Mailing delivery stat percentages to AB tests and also adds in new opened and clickthrough rate stats
Before
No AB testing % stats
After
AB Tests have % stats and rates.