Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

Add configurable email/message classes. #18

Merged
merged 2 commits into from
Dec 1, 2012

Conversation

kmtracey
Copy link
Contributor

Refs #16, #8. Can be used to allow installations to configure various
specifics of "email" generation and sending, per drip.

Refs zapier#16, zapier#8. Can be used to allow installations to configure various
specifics of "email" generation and sending, per drip.
@bryanhelmig
Copy link
Member

This looks really good, I'll pull it down and give it a spin this weekend. Have a good Thanksgiving!

@kmtracey
Copy link
Contributor Author

Hi there -- just curious if you've had a chance to look at this more closely? Anything I can do to help get it integrated? Thank!

@brad
Copy link
Contributor

brad commented Nov 30, 2012

Hi @kmtracey, I just saw this today. It looks really great, thank you for your effort. My only complaint is that it is still email specific. My main goal was to use this to send SMS messages and I can't save a phone number in an EmailField. My suggestion in #16 was to make that a CharField and name it from_address and the other field from_address_name. Are you open to that idea? //cc @bryanhelmig
I will try this out today.

@bryanhelmig
Copy link
Member

Sorry guys for the slow reply. I will merge this shortly.

@bryanhelmig
Copy link
Member

@brad @kmtracey is a switch to a generic CharField doable?

@kmtracey
Copy link
Contributor Author

kmtracey commented Dec 1, 2012

Brad, sorry, I did miss that detail of the changes you had proposed. At the database level an email field is just a char field anyway. so the only functional difference is in the automatic form validation being done in the admin. I think (without actually trying it) that you could accomplish storing a phone number in the existing field by installing a custom DripAdmin that inherits from the provided one and specifies use of a custom form (inheriting from the provided DripForm) which overrides the form field type for that field. That, of course, is a bit of a pain...but turning it into a plain char field in the base means anyone who does want validation as an email address has to add it manually using the same override the default admin approach.

So I guess it boils down to whether the expectation is that most people who use this field will be putting an email address or something else in there? If the answer is "email" then likely it's best to leave it as an email address field and add some docs on how to replace/bypass the email address validation if it's desired to store something else in there (assuming my untested belief that it can be done as described above is verified via actually trying it). If "something else" then best to change it to a char field and add some docs noting that no sort of validation is done on the value, and on how to add in whatever sort of field validation you may want for your own install. This wasn't a field I had any part in adding (nor any anticipated use of) so I have no idea which of these answers is more likely to be more common by users.

On the field name, at this point since the field has already been added by a committed-to-master migration (it predates this pull request), I'd suggest just changing its verbose name for display in admin and not messing with the field name/DB column name. If absolutely necessary to change the field/DB column name (and possibly max length?) that should be done in an added migration that renames the column, not by changing the migration that added the column. People who have already pulled and applied the migration from master would have to manually recover from the effects of changing the code and the already-run migration...it'd be much easier on them to make such changes in a new migration.

I think the field type issue is really separate from this pull request, since that field was added earlier (other than this pull request doesn't quite accomplish the full configurability we were aiming for). Adding doc, changing verbose name, possibly changing the field type, all could be done in a subsequent change after this one goes in...

@bryanhelmig
Copy link
Member

My opinion is that email is the desired medium and I don't see support for out of the box SMS drips in this library. @brad I might suggest either following @kmtracey's recommendation on docs for monkey-patching or maintaining your own customized fork really specialized for SMS (which likely needs custom sender classes, Twilio wrappers and more anyways).

bryanhelmig added a commit that referenced this pull request Dec 1, 2012
Add configurable email/message classes.
@bryanhelmig bryanhelmig merged commit e3906ac into zapier:master Dec 1, 2012
@brad
Copy link
Contributor

brad commented Dec 5, 2012

Works for me, thanks guys!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants