Python API Query + Email Notification

Question

I wrote this to query the Trello API to find the cards that haven't been updated in 7 days and send an email notification to the card's members as a digest.

I'm a Node.JS & client-side JS dev, and this is my first Python script. I'd really appreciate any suggestions around best practices, how it could be more modular/reusable, etc.

import os
from os.path import join, dirname
from dotenv import load_dotenv
from urllib import urlencode
import json
import urllib2
from datetime import datetime
import dateutil.parser
import pytz
import mandrill
from jinja2 import Environment, PackageLoader

# Load environment variables
dotenv_path = join(dirname(__file__), '.env')
load_dotenv(dotenv_path)

MIN_DAYS = os.environ.get("MIN_DAYS")
TRELLO_KEY = os.environ.get("TRELLO_KEY")
TRELLO_TOKEN = os.environ.get("TRELLO_TOKEN")
TRELLO_BOARD = os.environ.get("TRELLO_BOARD")
MANDRILL_KEY = os.environ.get("MANDRILL_KEY")
MSG_FROM_EMAIL = os.environ.get("MSG_FROM_EMAIL")
MSG_FROM_NAME = os.environ.get("MSG_FROM_NAME")
MSG_SUBJECT = os.environ.get("MSG_SUBJECT")
MEMBER_EMAILS = os.environ.get("MEMBER_EMAILS")

mandrill_client = mandrill.Mandrill(MANDRILL_KEY)

# Prepare template
template_env = Environment(loader=PackageLoader(__name__, "templates"))
template = template_env.get_template("reminder_email.html")

# Split member emails into a dict
member_emails = {}
member_email_lines = MEMBER_EMAILS.split(",")
for member_email_line in member_email_lines:
    member_email_line_parts = member_email_line.split(":")
    member_emails[member_email_line_parts[0].strip()] = member_email_line_parts[1].strip()

# Querystring parameters
params = {
    "key": TRELLO_KEY,
    "token": TRELLO_TOKEN,
    "fields": "name,url,dateLastActivity",
    "members": "true"
}

# Construct trello URL and fetch
url = "https://api.trello.com/1/boards/" + TRELLO_BOARD + "/cards?" + urlencode(params)
cards = json.load(urllib2.urlopen(url))

today = datetime.now(pytz.utc)
members = {}

# For each card that's older than a week
for card in cards:
    dateLastActivity = dateutil.parser.parse(card[u'dateLastActivity'])
    if abs(today - dateLastActivity).days > int(MIN_DAYS):
        # Add card to each member's list
        for member in card[u'members']:
            username = str(member[u'username'])
            if username not in members:
                members[username] = []
            members[username].append(card)

# For each member
for username in members:
    if username in member_emails:
        # Build message data
        message = {
            "subject": MSG_SUBJECT,
            "from_email": MSG_FROM_EMAIL,
            "from_name": MSG_FROM_NAME or MSG_FROM_EMAIL,
            "to": [{
                "email": member_emails[username],
                "name": username,
                "type": "to"
            }],
            # Construct HTML from template
            "html": template.render(num_days=MIN_DAYS, cards=members[username])
        }

        # Send email
        try:
            result = mandrill_client.messages.send(message=message)
            print result
        except mandrill.Error, e:
            print 'A mandrill error occurred: %s - %s' % (e.__class__, e)
            raise

Answer 1

Looks good at a first glance. For reusability the code should be divided into functions instead of just being a single script file.

• I'd load the multiple environment keys in a loop and into a separate dictionary instead of globals. That's nicer to refactor later and it's easy to pass around, or even update from command line arguments as well (e.g. using argparse). You can also look at dotmap or something similar to get a bit nicer syntax. E.g.:

  options = {}
  for name in ["MIN_DAYS",
              "TRELLO_KEY",
              "TRELLO_TOKEN",
              "TRELLO_BOARD",
              "MANDRILL_KEY",
              "MSG_FROM_EMAIL",
              "MSG_FROM_NAME",
              "MSG_SUBJECT",
              "MEMBER_EMAILS"]:
      options[name] = os.environ.get(name)
  

• The loop at the end can be exited early with if ... not in ...: continue to remove one level of indentation.

• The print should be using function call syntax for forwards compatibility with Python 3 and similarly the except handler should also use the except ... as e: syntax instead. I.e.:

  ...
          print(result)
      except mandrill.Error as e:
  ...
  

• The iteration over members can use iteritems to get both name and cards in the same step.

• For the date comparison I'd recommend using a dedicated library, that way both the code will be easier to read and one less wheel is reinvented. E.g. I had good experiences with arrow.
• For the HTTP requests you can also look at requests for a nicer interface.

Answer 2

You've done a good job of matching Python style, especially the names. If you haven't already, read the Python Style guide, it's an invaluable resource that comprehensively covers all the style notes.

Organise your imports to be grouped together by type of import: first party, third party and then relative. You also may as well just import os rather than importing the whole module for environ.get then just grabbing join and dirname.

When using .split to get two elements, it's often neater to using Python's multiple assignment. You can have your split line like this:

key, value = member_email_line.split(":")

Then key and value can be used as variables directly:

member_emails[key.strip()] = value.strip()

It's better to use more applicable names that actually tell you something about what these values are for. Note that if there is more than one colon in the string you'll now get errors when trying to assign this way. This should only be a problem if you

It's more Pythonic not to test for a key's existence when you expect it to be there. Instead, attempt to append to the key and if it doesn't exist catch the error:

try:
    members[username].append(card)
except KeyError:
    members[username] = [card]

A KeyError is raised if username isn't in members. Since Python's error handling is fast and exceptions aren't going to be very common, your script is faster now.

Lastly, don't use this format for exceptions:

except mandrill.Error, e:

This was removed in Python3, for good reason. It obfuscates the ability to except multiple exceptions. Imagine this:

except KeyError, ValueError:

In this case, you catch a KeyError with the name ValueError. This is confusing behaviour that will throw people off. Instead, use the newer except Exception as e syntax, which explicitly references the exception with the name e.

except mandrill.Error as e: