delta / mailer

Command line mail utility
MIT License
1 stars 5 forks source link

Added Campaign, Mailer classes. Fixed README. Wrote tests. #2

Closed thakkarparth007 closed 9 years ago

thakkarparth007 commented 9 years ago

I've added 3 main classes and a couple of factory functions to enable creating objects of those types. The classes are described below. Tinu will use the CampaignFactory function to create a Campaign object, and will then call send/preview as per the arguments given.

  1. Mail - A dummy object, just to contain all the attributes needed by the Mailer object. Could have been simply a dictionary, however, I've used a class to make sure that Mailer gets a well defined interface to deal with Mail objects.
  2. Mailer - The object that will handle sending of the mails. Requires SMTP authentication information.
  3. Campaign - Used to create mail objects easily. Two types of campaigns exist - BulkMailCampaign (for bulk mails) and TransactionMailCampaign (for personalised mails). Use the CampaignFactory to create a Campaign object. Call the send or preview methods on these objects.

TODOs:

Nearby goals:

  1. Handle cases where the preview location isn't empty.
  2. Opening a preview mail in browser
  3. Warning the user when the template file contains references to variables not defined in the variables file.
  4. Test the code.

Distant goals:

  1. Adding attachment support.
  2. Embedding images in the mail (or should a server be used? Server will help in tracking if a mail was opened)
sananth12 commented 9 years ago

Have you tested this? Good job on the clean PR, but next time try to split the PRs into smaller ones so that we can catch bugs/bottlenecks earlier :)

thakkarparth007 commented 9 years ago

Nope. Yet to be tested.

And, I'll have smaller PRs from next time! :D

ssundarraj commented 9 years ago

@thakkarparth007 Please test before sending a PR.

thakkarparth007 commented 9 years ago

I've written the tests, and the code seems to be working fine.

What is left however is the Mailer class. I'm unable to test the send() method in the Mailer class because I'm unable to deal with SMTP server as of yet. I'll talk to Kousik Sathish, as I was told by @ssundarraj, and get that resolved within a day or two.

The rest of the code is tested, except for the previewOneInBrowser() method of the Campaign class. This method is yet to be implemented, and currently just raises an NotImplementedError.

Most of the code is auto-tested. However, previewing requires to manually check the output files for correctness. Will probably make this automatic too in future.

vigneshmanix commented 9 years ago

@thakkarparth007 check out https://mailtrap.io/

thakkarparth007 commented 9 years ago

Thanks. This will be useful!

sananth12 commented 9 years ago

You have quite a few lint errors:

***** Module deltamail.campaign C: 91, 0: Unnecessary parens after 'raise' keyword (superfluous-parens) C:151, 0: Trailing whitespace (trailing-whitespace) W: 73, 0: Anomalous backslash in string: '\/'. String constant might be missing an r prefix. (anomalous-backslash-in-string) C: 1, 0: Missing module docstring (missing-docstring) W: 4, 0: Relative import 'mail', should be 'deltamail.mail' (relative-import) C: 10, 0: Old-style class defined. (old-style-class) C: 20, 4: Invalid method name "_createMailObjects" (invalid-name) C: 20, 4: Invalid argument name "mailingList" (invalid-name) C: 20, 4: Invalid argument name "templateStr" (invalid-name) C: 20, 4: Invalid argument name "globalVars" (invalid-name) C: 27, 4: Invalid argument name "mailingList" (invalid-name) C: 27, 4: Invalid argument name "templateStr" (invalid-name) C: 27, 4: Invalid argument name "globalVars" (invalid-name) C: 41,12: Invalid variable name "m" (invalid-name) C: 81, 4: Invalid method name "previewOneInBrowser" (invalid-name) W: 90, 8: Unreachable code (unreachable) W: 91,12: Raising a string exception (raising-string) W: 96, 0: Method 'previewOneInBrowser' is abstract in class 'Campaign' but is not overridden (abstract-method) W:126, 0: Method 'previewOneInBrowser' is abstract in class 'Campaign' but is not overridden (abstract-method) R:167, 0: Too many local variables (17/15) (too-many-locals) C:167, 0: Invalid function name "CampaignFactory" (invalid-name) C:167, 0: Invalid argument name "mailingList" (invalid-name) C:167, 0: Invalid argument name "templateFile" (invalid-name) C:167, 0: Invalid argument name "globalVarsFile" (invalid-name) C:195, 4: Invalid variable name "templateStr" (invalid-name) C:209, 4: Invalid variable name "globalVars" (invalid-name) C:218, 8: Invalid variable name "mailingListFile" (invalid-name) C:245, 8: Invalid variable name "mailConstructor" (invalid-name) C:248, 8: Invalid variable name "mailConstructor" (invalid-name) ***** Module deltamail C: 1, 0: Missing module docstring (missing-docstring) W: 1, 0: Relative import 'main', should be 'deltamail.main' (relative-import) ***** Module deltamail.mailer C: 1, 0: Missing module docstring (missing-docstring) C: 5, 0: Old-style class defined. (old-style-class) W: 16,52: Redefining name 'email' from outer scope (line 2) (redefined-outer-name) W: 33, 8: Unreachable code (unreachable) R: 5, 0: Too few public methods (1/2) (too-few-public-methods) ***** Module deltamail.mail C: 1, 0: Missing module docstring (missing-docstring) C: 20, 8: Invalid attribute name "mailingList" (invalid-name) C: 4, 0: Old-style class defined. (old-style-class) C: 8, 4: Invalid argument name "mailingList" (invalid-name) R: 4, 0: Too few public methods (0/2) (too-few-public-methods) C: 24, 0: Invalid function name "MailFactory" (invalid-name) C: 24, 0: Invalid argument name "mailingList" (invalid-name) C: 24, 0: Invalid argument name "templateStr" (invalid-name) C: 38, 4: Invalid variable name "subjectTempl" (invalid-name) W: 39,14: Used * or * magic (star-args) C: 41, 4: Invalid variable name "bodyTemplate" (invalid-name) W: 42,11: Used * or * magic (star-args) ***** Module deltamail.main C: 1, 0: Missing module docstring (missing-docstring) C: 1, 0: Missing function docstring (missing-docstring) R: 5, 0: Abstract class not referenced (abstract-class-not-used)

Please use a lint tool like http://docs.pylint.org/. You don't have to fix everything(at least for now), but it's good practice to use a lint tool from the very beginning.

thakkarparth007 commented 9 years ago

I'm using the Anaconda plugin (for Sublimetext) for the linting. It didn't show any messages for the code (whatever it showed, I had fixed it). I'll make the necessary changes. :)

thakkarparth007 commented 9 years ago

Btw, I'm using camel casing, however the linter output seems to suggest that one shouldn't use that. I hope that isn't an issue.

sananth12 commented 9 years ago

CamelCase is more like java. Consider following the standard style guide https://www.python.org/dev/peps/pep-0008/

thakkarparth007 commented 9 years ago

The Campaign.send() function is working now (tested using mailtrap.io, that was suggested by @vigneshmanix). Only thing left now is implementing the previewOneInBrowser method. However, I'm confused regarding the preview functionality.

There are 2 alternatives.

  1. I can have a preview(location="") method that creates a folder email-preview in the current directory or the specified location, where all the mails being sent will be stored as separate html files. Besides this, I can have another previewOneInBrowser() that'll fire up the browser and show a mail in that (since mail is just an html page). The mail file will be a temp file. The folder created by preview method will, however, be permanent.
  2. The other alternative is, have a single method for previewing, that'll open up browser too. The browser will list all the mail files being sent (same as the preview preview method, except that we open the browser directly and set its url to the folder where the preview files are stored).

However, the folders must be permanent. That sucks, because no one really wants to store preview mails on his disk. Any suggestions for this?

Other than this, once Tinu does his part, we can have version 1 out. My part is now done completely for version 1. (I will handle the preview-in-browser in next version :P)

ssundarraj commented 9 years ago

https://www.python.org/dev/peps/pep-0257/ Use standard Python docstrings. Change all docstrings to this format.