4
\$\begingroup\$

This is my first programming project with real world application. It's purpose is to take a file from a directory, encrypt it with a predetermined password, and email to appropriate recipient.
ID Email and Password are found in a CSV file that I created EmailDict from. ID and Filename are parsed and dropped into FileDict. Any feedback is valued and appreciated.

import os 
import re
import csv
import PyPDF2
import smtplib
from email.mime.text import MIMEText 
from email.mime.application import MIMEApplication
from email.mime.multipart import MIMEMultipart


# Create a dictionary with the csv values for ID, Email and Password
EmailDict = dict()
with open('commissionrepemaillist.csv', 'r') as infile:
    reader = csv.reader(infile)
    for row in reader :
        REP = row[0]
        EMAIL = row[1]
        PASSWORD = row[2]
        EmailDict[REP] = EMAIL, PASSWORD

# create dictionary of IDs and Pdf Files
FileDict = dict()
path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1"  
for FILE in os.listdir(path):
    split = re.split("[_.]", FILE)
    ID = split[1]
    FileDict[ID] = FILE

# encrypt PDF files in FileDict based on ID and Password from EmailDict
for ID in FileDict:
    if ID in EmailDict:
        path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1\\"
        file = os.path.join(path + FileDict[ID])

        inputStream = PyPDF2.PdfFileReader(file)
        output = PyPDF2.PdfFileWriter()
        output.appendPagesFromReader(inputStream)
        output.encrypt(EmailDict[ID][1])

        with open(file, 'wb') as outputStream:
            output.write(outputStream)  


    else : continue

# email encrypted pdf file to appropriate rep 

for ID in FileDict:
    path = "C:\\Apps\\CorVu\\DATA\\Reports\\Monthly Commission Reports\\Output\\pdcom1\\"
    file = os.path.join(path + FileDict[ID])
    with open(file, 'rb') as pdf :
        pdfAttachment = MIMEApplication(pdf.read(), _subtype = 'pdf')
        pdfAttachment.add_header('content-disposition', 'attachment', filename=('MonthlyPaidCommission_%s.pdf' % ID))
        text = MIMEText(
        """For Rep Number : %s 
        Please use the password previously emailed to open attached file.
        If you have any questions or need assistance please contact  me.
        Thank you""" % ID)
    msg = MIMEMultipart(_subparts = (text, pdfAttachment))   
    msg['SUBJECT'] = "Commission Report"
    msg['FROM'] = "me@gmail.com"
    msg['TO'] = EMAIL

    s = smtplib.SMTP("LOCALHOST:587")
    s.starttls()
    s.login('user', 'pwd')
    s.sendmail('me@123.com', EMAIL, msg.as_string())
    s.quit()
\$\endgroup\$
  • \$\begingroup\$ I've fixed an indentation error for you in Rev 3. Please check your entire question to verify that your code has been posted as intended. \$\endgroup\$ – 200_success Nov 23 '15 at 22:06
4
\$\begingroup\$

Your code is easy enough to follow. All of your open() calls are done using with blocks, which is a good habit.

Your variables — such as REP and EmailDict — are unconventionally named. Use ALL_CAPS for constants, and lower_case for variables. Ironically, the one variable that should be a constant, path, is defined several times, and not written as a constant.

Speaking of path, you are misusing os.path.join(). The whole point of the function is to ensure that the path delimiter is automatically inserted for you between the components. So, you should write os.path.join(path, FileDict[ID]) instead. Also, use raw strings to make your backslashes less cumbersome.


I doubt that there is any value in writing all of those encrypted versions of the files to disk. You could just create a transient encrypted copy in memory, mail it off, and move on to the next recipient. To accomplish that, you can write to a StringIO object instead of a file.

The code to encrypt the PDF and compose the mail message feels like it could belong logically together. I'd define a function for that:

def message_for(unencrypted_pdf_path, rep_id, email, password):
    """
    Creates a MIME message for a rep containing an encrypted version
    of the specified PDF.
    """
    buf = StringIO()
    …
    encrypted_pdf = buf.getvalue()

    text = MIMEText(… % rep_id)

    pdf_attachment = MIMEApplication(encrypted_pdf, _subtype='pdf')
    pdf_attachment.add_header('content-disposition', 'attachment', filename=('MonthlyPaidCommission_%s.pdf' % rep_id))

    msg = MIMEMultipart(_subparts=[text, pdf_attachment])
    msg['SUBJECT'] = 'Commission Report'
    msg['FROM'] = 'me'
    msg['TO'] = rep['EMAIL']
    return msg

Your code would be more expressive if you defined the dictionary entries "all at once" using a dict comprehension. I would also avoid storing the e-mail and password as a tuple, since it makes you write more cryptic code like EmailDict[ID][1] later.

I think that you would be better off not defining FileDict at all. Just process each directory entry as you encounter it. You can even send multiple messages within the same SMTP connection. Just make sure you keep a log of what was sent, in case something goes wrong.

PATH = r'C:\Apps\CorVu\DATA\Reports\Monthly Commission Reports\Output\pdcom1'

COLS = ['rep_id', 'email', 'password']
with open('commissionrepemaillist.csv', 'r') as infile:
    email_dict = {
        row[0]: dict(zip(COLS, row)) for row in csv.reader(infile)
    }

smtp = smtplib.SMTP('localhost:587')
smtp.starttls()
smtp.login(…)

for filename in os.listdir(PATH):
    rep_id = re.split('[_.]', filename)[1]
    rep = email_dict.get(rep_id)
    if rep is None:
        continue
    msg = message_for(os.path.join(PATH, filename), *rep)
    smtp.sendmail('me', rep['email'], msg.as_string())
    print("Sent %s to %s" % (filename, rep['email']))

smtp.quit()
\$\endgroup\$
  • \$\begingroup\$ I appreciate the input. I am trying to figure out how to work in the StingIO(), but am having some difficulty. Also, thank you for the hints. I appreciate the help as I work on my skills. \$\endgroup\$ – AlliDeacon Nov 24 '15 at 15:54
2
\$\begingroup\$

Your first block, reading the csv can be changed in two ways.

  1. You change EmailDict = dict() to email_dict = {}. It's uncommon to do dict() unless there is a requirement, like type conversion. Also Python uses snake_case for variables.

  2. You can change the block to just use a dictionary comprehension. It's simple to setup and voids my above comment.

    All it is is syntatic suger over what you have to increas readability.

    with open('commissionrepemaillist.csv', 'r') as infile:
        reader = csv.reader(infile)
        email_dict = {
            row[0]: row[1], row[2]
            for row in reader
        }
    

    While it removes the information about what the different items are, it makes reading much faster. And you code to look nicer.


You can use another dictionary comprehension to generate file_dict.
I personally have never come across re.split("[_.]", FILE), but it's an neet idea!

    file_dict = {
        re.split("[_.]", file_name)[1]: file_name
        for file_name in os.listdir(path)
    }

You normally want to loop through the smaller dictionary. I assume that the folder has more, if not lots more, files than you wish to send in the email.

This is as the check if ID in EmailDict: is \$O(1)\$ not \$O(e)\$, where as for ID in FileDict is \$O(f)\$. If we get the time it takes to loop through either it will be \$O(e \times 1)\$ for EmailDict, and \$O(f \times 1)\$ for FileDict. And so you want to pick the smaller of the two.

However, it's unlikely that it will have too much of an impact, and so you may ignore this if you wish.

You can use dict.iteritems() to iterate through both the ID and the file name. There is a subtle difference between dict.items and dict.iteritems, the latter is in most cases better, and so became the default in Python3. It's usage is:

for ID, file_name in file_dict.iteritems():
    if ID in email_dict:
        ...

You redefine path in each enumeration to the same static path. This hinders execution time a little, and is not productive.

Also file is python keyword, and so it's highly advised against re-assigning it. This is as from that point on, as you redefined it in the global scope, if you wish to use it you will have to use __builtin__.file, which is definitely not good. I would use file_path instead.

The rest of the block, apart from else: continue, seems good.


Last block now.

There seems to be a bug(?) or an intention to send all files, even un-encrypted ones, to the last person in the csv...

EMAIL is only defined in the first block, or with my changes not at all. And due to the lack of check for if ID in email_dict, it means that even un-encrypted files will be sent, as only files with the ID in both email_dict and file_dict will be encrypted.

Other than the same problems that were highlighted in the other blocks. I would assume that you don't have to create a new smtp connection for each email. Instead I would move the all bar s.sendmail(...) out of the loop.


Higher level stuff:

  • I would do both the encryption and the email in the same loop. However I would encourage the use of a function, to make the logic easier to read.

  • Use 'raw raw string'. A raw string in Python is "Hello world", a 'raw raw string' has an r in-front of it, and prevents character escaping. For example r'\n' is '\\n'.

  • Stop over-writing path, you're making more work for when you change the path! Or want to use this script in a different place.

  • I think over-writing the files is a bad idea, instead I would make a new file, which would be something like file_name + '_cypt'.

  • It's generally better to use str.format over %. This is as % has quirks.

  • As this is a real world application, you would want to only execute if it's the main program. This is just a small security feature.

    if __name__ == '__main__':
        ...
    

And so I would re-write your program to the following:

import os 
import re
import csv
import PyPDF2
import smtplib
from email.mime.text import MIMEText 
from email.mime.application import MIMEApplication
from email.mime.multipart import MIMEMultipart

PATH = r"C:\Apps\CorVu\DATA\Reports\Monthly Commission Reports\Output\pdcom1"

def encrypt(file_path, password):
    input_pdf = PyPDF2.PdfFileReader(file_path)
    output = PyPDF2.PdfFileWriter()
    output.appendPagesFromReader(inputStream)
    output.encrypt(password)

    with open(file, 'wb') as outputStream:
        # Is this a bug? Should it not be the other way around?
        output.write(outputStream)  

def send_email(file_path, email, ID):
    with open(file_path, 'rb') as pdf :
        pdf = MIMEApplication(pdf.read(), _subtype = 'pdf')
        pdf.add_header(
            'content-disposition',
            'attachment',
            filename=('MonthlyPaidCommission_{}.pdf'.format(ID)))

        text = MIMEText(
            """For Rep Number : {} 
            Please use the password previously emailed to open attached file.
            If you have any questions or need assistance please contact  me.
            Thank you""".format(ID))

    msg = MIMEMultipart(_subparts=(text, pdf))
    msg['SUBJECT'] = "Commission Report"
    msg['FROM'] = "me@gmail.com"
    msg['TO'] = email

    s = smtplib.SMTP("LOCALHOST:587")
    s.starttls()
    s.login('user', 'pwd')
    s.sendmail('me@123.com', email, msg.as_string())
    s.quit()

if __name__ == '__main__':
    with open('commissionrepemaillist.csv', 'r') as infile:
        reader = csv.reader(infile)
        emails = {
            row[0]: row[1], row[2]
            for row in reader
        }

    files = {
        re.split('[_.]', file_name)[1]
        for file_name in os.listdir(path)
    }

    for ID, email_info in emails:
        if ID in files:
            file_path = os.path.join(PATH + files[ID])
            encrypt(file_path, email_info[1])
            send_email(file_path, email_info[0], ID)
\$\endgroup\$
  • \$\begingroup\$ thank you SO much for your feedback. I will definitely make the improvements. Also I dont think the outputStream is a bug, as I'm overwriting the original file with the encrypted file (output). \$\endgroup\$ – AlliDeacon Nov 24 '15 at 13:39
  • \$\begingroup\$ @AlliDeacon I've never used PyPDF2, so the only reason I thought it was a bug is as it's normally the other way around in Python. E.g. outputStream.write(output). \$\endgroup\$ – Peilonrayz Nov 24 '15 at 13:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.