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

dev/core#2100 Improve A/B test report page and API Mailing stats #20093

Merged

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented Apr 17, 2021

Before: A/B test report pages were confusing and incorrect because API Mailing stats were incomplete or incorrect.

  1. Showed Tracked Opens as the total number of opens.
  2. Showed Clicks-throughs as the total number of clicks.
  3. Showed Unsubscribes as the total number of unsubscribes and opt-outs.
  4. Intended Recipients not shown and not included in API Mailing stats.
  5. Showed n/a for some stats that had 0 results.

After: A/B test report pages are more accurate and API is more complete and correct.

  1. Show Unique Opens as the total number of unique opens (i.e. the number of recipients opening the email).
  2. Show Unique Clicks as the number of unique clicks (i.e. the number of recipients clicking one or more links in the email).
  3. Show Unsubscribes & Opt-outs as the number of unique unsubscribes and opt-outs (i.e. the number of recipients who either unsubscribed or opted out).
  4. Intended Recipients shown and included in API mailing stats.
  5. Shows 0 for all stats with no results (or that aren't applicable).

This makes changes to the API, giving a different results for mailing - stats with distinct:1. I can't see this would be used elsewhere, but I hope someone with more experienced can chime in.

I'm willing to do some more work on this to improve the report page, but need some guidance on what the vision is in the long term. There's the normal mailing report and then there's this A/B mailing report that uses the API. It would make sense to use only one of these only in the future. Does that mean fixing up the A/B /API version and using it for all mailings? Does anyone have thoughts on the way forward here?

API Mailing stats still doesn't return forwards, replies, or opt out and unsub separately, but I can add them. The normal mailing only gives total clicks and not unique, so it might also make sense to add total clicks to the A/B version through a Mailing stats call with distinct:0 (so it would list both the number of contacts who clicked and the number of links clicked). If this was used for the single mailing report, that would be an improvement as that page currently reports click throughs and a click through rate as total clicks / delivered, while this is usually (contacts who clicked at least one link) / delivered.

If we plan to use this for normal mailing reports as well, I could do a little work to make the format of that page a little easier to read.

There is also a time zone issue that results in results not showing until N hours later than they should if server time is UTC - N. I think what's happening is that the time of clicks, opens, etc are being recorded as UTC. However, the API is using the current time in the server time zone here for the query, with the result that if you are in GMT -6, for instance, the query won't return any results from the last six hours because those are in the future relative to server time. What's the solution here? My first thought is do we need to pass the time at all? Perhaps just leaving the time out would give us the full results, but I haven't tried this yet. This has been solved on the normal mailing report page, so I can look there, but welcome any suggestions if someone knows the answer here.

EDIT: Fixed and updated wording of Click-throughs, Unique Clicks in 1 & 2 above

@civibot
Copy link

civibot bot commented Apr 17, 2021

(Standard links)

@civibot civibot bot added the master label Apr 17, 2021
@larssandergreen larssandergreen changed the title dev/core#2100 Improve A/B test report page for unique clicks, unique opens and unsubs dev/core#2100 Improve A/B test report page for unique clicks, unique opens and unsubs, etc Apr 18, 2021
@larssandergreen larssandergreen changed the title dev/core#2100 Improve A/B test report page for unique clicks, unique opens and unsubs, etc dev/core#2100 Improve A/B test report page and API Mailing stats Apr 18, 2021
@seamuslee001
Copy link
Contributor

So this will I think also affect the traditional mailing report statistics have you reviewed that to see if they will still be correct or not?

@larssandergreen
Copy link
Contributor Author

larssandergreen commented Apr 20, 2021

I don't think this touches the traditional mailing report at all, as that one comes from here:

public static function &report($id, $skipDetails = FALSE, $isSMS = FALSE) {

and accesses the DB separately. The single mailing report is quite different from the A/B mailing report.
But I have now checked the single mailing report and everything looks good there. Delivered, bounces, clicks, opens, etc as usual.

@larssandergreen
Copy link
Contributor Author

jenkins, test this please

Show 0 instead of n/a for stats that don't yet have any count

Before: Bounces, Open and Unsubs show n/a if there are 0.
After: All stats show 0 if api returns null.

Add intended recipients to API and A/B mailing report page

Remove in_distinct parameter from Bounce and Delivered in API and BAO

First commit removes is_distinct code from getTotalCount() for Bounces and Delivered in BAO, so we no longer need the is_distinct parameter for these functions, as it doesn't do anything. Changed API to match. These functions do not appear to be used elsewhere.

Fix comments for removed param for getTotalCount

Removing is_distrinct from getTotalCount

Forgot this!

Update mailing stats test

Update label for Tracked opens to Unique Opens, Click-throughs to Unique Clicks

Thanks @sluc23 for pointing this out
@larssandergreen larssandergreen force-pushed the mailings-AB-test-improvements branch from dd0b069 to 2f2240d Compare August 2, 2021 19:36
@eileenmcnaughton
Copy link
Contributor

@agileware-justin @demeritcowboy @seamuslee001 @johntwyman @mattwire @kurund - any ideas how we can get this reviewed? I'm not comfortable enough with a-b stuff to take it on myself so looking to see if someone else is

@agileware-justin
Copy link
Contributor

Is there a Gitlab for this PR?

@eileenmcnaughton
Copy link
Contributor

@agileware-justin - check the PR title - that provides it (that is our preferred way to reference gitlab too)

@agileware-justin
Copy link
Contributor

@agileware-justin - check the PR title - that provides it (that is our preferred way to reference gitlab too)

But the PR title ain't a click target...

@eileenmcnaughton
Copy link
Contributor

annoying isn't it? I'd love us to carve out the time to make the bots add the links for us

@agileware-justin
Copy link
Contributor

I've just tested the A/B mailing process and it's currently broken in 5.40.4 at time of testing. The A mailing sends and shows results, the B mailing apparently has sent but no results.

So yes, happy to test this and see if fixes what is broken and is an improvement.

@eileenmcnaughton
Copy link
Contributor

thanks @agileware-justin !

@agileware-justin
Copy link
Contributor

Confirmed A/B Mailing process is broken in CiviCRM 5.40.4. Related Gitlab, https://lab.civicrm.org/dev/core/-/issues/2810

It's not possible to perform a complete review of this PR until the bug is fixed.

I did apply the PR and check the changes to the reporting UI and they looked good. There's unfortunately a lack of statistics to report on!

@jaapjansma
Copy link
Contributor

@agileware-justin the bug in 5.40.4 is fixed are you willing to test this PR again? Just curious and ok for us if you dont want to test this. We might then do this during our next PR review.

@agileware-justin
Copy link
Contributor

@jaapjansma yes, but might be a week or two before I can get to this one. Happy Australia Day (for tomorrow).

Happy Straya Day

@jaapjansma
Copy link
Contributor

No problem if it takes a while. And Happy Australia Day, enjoy eating your kiwi ;-)

@eileenmcnaughton
Copy link
Contributor

Whoa - @agileware-justin please do not eat any kiwis!

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We did an AB Mailing in a test environment with this patch and in one without this patch and we then checked the reports and clicked on links in the mail.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change sufficiently improves test coverage.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@eileenmcnaughton or @seamuslee001 we (@BettyDolfing and I) have reviewed this PR can you merge this PR?

@jaapjansma jaapjansma added has-test merge ready PR will be merged after a few days if there are no objections labels Feb 28, 2022
@eileenmcnaughton
Copy link
Contributor

Merging based on @jaapjansma & @BettyDolfing review

@eileenmcnaughton eileenmcnaughton merged commit 508bfb8 into civicrm:master Feb 28, 2022
@larssandergreen larssandergreen deleted the mailings-AB-test-improvements branch November 5, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-test master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants