-
Notifications
You must be signed in to change notification settings - Fork 132
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
EveryAction: Add email endpoint methods to retrieve email stats from TargetedEmail #1003
EveryAction: Add email endpoint methods to retrieve email stats from TargetedEmail #1003
Conversation
For a little more context, the get_email_stats method uses the get_emails and get_email method in order to unpack a bunch of information. You need to get a list of all emails first with get_emails(). Then you use foreginMessageId with the get_email() method to get the stats. The data is nested because with any ab testing that was done. So for instance, inside the emailMessageContentDistributions section, there is a list of dictionary objects with email_A, email_B, email_c and the email_winner. The get_email_stats method combines all these to get the final open count, unsubs, etc. |
Another thing to note, which I would take suggestions for, is that you need to request that this endpoint is turned on for your api key. They don't expose it by default. So we will want to document that somewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts looking at the code. Very excited for this!
email = self.get_email(fmid) | ||
email_list.append(email) | ||
|
||
for email in email_list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to have some progress indicator? tqdm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it runs quick enough that this isn't necessary. Plus I don't believe we use tqdm or progress bars for many (if any) connectors in Parsons and not sure we want to add another dependency to our tangled mess.
`Args:` | ||
emails : list | ||
A list of email message details. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion - maybe we could include start_date
and end_date
as optional parameters? I know that this won't affect get_emails
but maybe might save some time for the L88 loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we care about passing through orderby
?
d["machineOpenCount"] = 0 | ||
d["openCount"] = 0 | ||
d["unsubscribeCount"] = 0 | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why do we need a try block here? What issues have we run into?
- If we need it, would it maybe be better to have each of these assignments get their own try block so that - if something breaks with
bounceCount
(for example) - it doesn't affect all of the other assignment commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the problems I was running into was that when emails weren't sent yet, these fields did not exist. This was also code I wrote pretty early on in my career and a few months ago added it to parsons without looking too close into it because I was about to lose access to EveryAction and wanted to contribute before that went away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Do we expect all of these fields to be missing simultaneously in the same i
block?
Another question - are there cases where we would expect to loop multiple times? Does this happen for emails that have A/B tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so.
Yes, if there are A/B tests it loops multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then the try block should be fine.
Would it be possible to have the option of not aggregating over each email? For the reporting we do for Tech for Campaigns, we report on how each send did independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could create another method or could have a param that gives the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our discussion on 2024-05-14 we can defer this request to a later PR.
The `get_emails` method now accepts an optional `ascending` parameter to specify the sorting order for the `dateModified` field. By default, the emails are sorted in ascending order. This change allows users to retrieve emails in either ascending or descending order based on their preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request adds an email endpoint to the VANConnector class, allowing users to retrieve email messages and their stats. It also includes test cases for getting email messages and email stats.