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

Add calculated fields to API 4 for total mailing stats #26689

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented Jun 30, 2023

Overview

This adds calculated fields to API 4 for mailing stats (intended recipients, successful deliveries, unique and total opens, unique and total clicks, bounces, unsubscribes, opt outs, forwards, replies). This is mostly copy-paste from the API 3 version, with a little finessing and then combined into one function for most of the mailing events.

I thought I'd put these up as one PR and then finish up the rest afterwards, as I have a couple questions:

1. Is there any way to pass a parameter into the method in setSqlRenderer? Then some of these could be re-used for other stats. For example, countUnsubscribes could take an org_unsubscribe param so that it could also be used for optouts with org_unsubscribe = 1. Similarly, we could do distinct and non-distinct click counts with a param.
2. I've copy-pasted ->getTableName from API 3, but I wonder if this is worth the cost to load all these classes versus just hardcoding the table names.

@civibot
Copy link

civibot bot commented Jun 30, 2023

(Standard links)

@civibot civibot bot added the master label Jun 30, 2023
@larssandergreen
Copy link
Contributor Author

@colemanw could you take a look at this?

@colemanw
Copy link
Member

Nice work @larssandergreen

  1. Is there any way to pass a parameter into the method in setSqlRenderer?

Not quite in the way you mean, there are no user-params passed into a field, however more than one field could share a sqlRenderer callback function and it can differentiate between them via it's $field argument. For example, your countClicks, countBounces and countUnsubscribes functions look identical except for the table they join to, so they could be a single function that picks that table based on the name of the $field param. You could also add a few more fields like stats_unsubscribe_org and have the callback function tweak the sql accordingly.

I noticed one thing that makes them not identical is that some filter out is_test records and others don't. Per #23724 I would say consistently "don't".

  1. I've copy-pasted ->getTableName from API 3, but I wonder if this is worth the cost to load all these classes versus just hardcoding the table names.

I think the performance cost is negligible as it doesn't even have to instantiate the objects just read a static variable from them. OTOH, it's kind of pointless as those table names will never change. I don't have any objection to hard-coding them into the queries.

@larssandergreen larssandergreen marked this pull request as draft June 30, 2023 21:13
@larssandergreen larssandergreen changed the title Add calculated fields to API 4 for some mailing overall stats Add calculated fields to API 4 for total mailing stats Jun 30, 2023
@larssandergreen larssandergreen force-pushed the Calculated-field-for-mailing-successful-deliveries branch from b733ab7 to e223d29 Compare June 30, 2023 23:37
@larssandergreen
Copy link
Contributor Author

Thanks @colemanw, that's very helpful. Now covers all the stats!

@larssandergreen larssandergreen marked this pull request as ready for review June 30, 2023 23:38
@larssandergreen
Copy link
Contributor Author

Regarding is_test: I think in this case we definitely do want to filter these out. The tests would be the test emails you've sent to yourself or your boss and I can't see why we'd want to include opens or link clicks from those in the stats for the overall mailing. It doesn't make sense to include these, in the same way as we don't count test contributions in reports showing contribution totals.

@colemanw
Copy link
Member

colemanw commented Jul 1, 2023

OK that makes sense about is_test.
This code looks perfect to me. What about adding a test? Probably needs a new class like tests/phpunit/api/v4/Entity/MailingEvent.php

@larssandergreen larssandergreen force-pushed the Calculated-field-for-mailing-successful-deliveries branch 3 times, most recently from df2fc79 to b6f532d Compare July 1, 2023 17:28
@larssandergreen larssandergreen force-pushed the Calculated-field-for-mailing-successful-deliveries branch from b6f532d to 695d787 Compare July 1, 2023 17:31
@larssandergreen
Copy link
Contributor Author

Yes, I should have just added tests from the start — because it wasn't perfect. Should be better now.

@colemanw colemanw merged commit 9b63c8c into civicrm:master Jul 1, 2023
3 checks passed
@colemanw
Copy link
Member

colemanw commented Jul 1, 2023

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants