search

rubyforgood / share_christmas

Share Your Holiday Application
4 stars 3 forks source link

Org Admin Send Emails #144

Closed craig-riecke closed 8 years ago

craig-riecke commented 8 years ago

Implements and fixes #90. Note I shut off mailing in DEV altogether since Gmail now requires you to jump through hoops to use it as an SMTP mailer. Automated tests mostly capture that mailing will do the right thing in prod.

atzorvas commented 8 years ago

I would prefer using a separate controller with views in order to handle the mailing stuff.

craig-riecke commented 8 years ago

Makes sense, but I'm not sure where to put it right now. Constructing the email needs an Org and a Campaign, since that's how it builds the sample subject and body. Sending an email needs to know the Org to get the members list for sending, at the very least. If you called the resource POST /email or something, then emails would need an :id.

atzorvas commented 8 years ago

I was thinking of a notifications_controller, available as @/mail@.

While creating a notification, we could give the type of the mail we want to create, which would result into creating an object with the same class name. This class could have all the required and optional elements for constructing the notification as attributes, and composing it would be easy.

We could store the composed notifications as records in the database and thus having an ID to request them later on (probably as a "view online" feature) but we could also ignore storing them, at least for now.

That way we could simplify our controllers from having "send email" methods and we could have a clear way on what's required and what's optional while using controllers & views taking advantage of convention over configuration.

We could also skip this for now, and leave it as a future idea, and I'll probably create a PR at some time :smiley:

craig-riecke commented 8 years ago

@atzorvas Yeah, that was just the idea I needed. Thanks! Refactored into /mail/appeals, built a model for it (non-persisting for now), and restructured tests. Much nicer.

Can someone take a look before I merge?