osuosl / formsender

Simple script to email form submissions.
Apache License 2.0
3 stars 1 forks source link

Set outgoing email address #48

Closed athai closed 8 years ago

athai commented 8 years ago

47.

athai commented 8 years ago

@matthewrsj - What should I redirect to in EnvironBuilder?

https://github.com/osuosl/formsender/blob/develop/tests.py#L69

matthewrsj commented 8 years ago

@athai redirect shouldn't matter for tests as long as the field exists.

matthewrsj commented 8 years ago

@athai I made one last comment on that test. After that I think it is good.

One thing to think about, though, is if this will be a required field. If it is required, you will need to add the send_to field to all other tests that test validation methods (like this one: https://github.com/osuosl/formsender/blob/develop/tests.py#L91 or any other test that call on_form_page()). If it is not required, you will need to set a default for when that field isn't present, which would require another test.

@Kennric thoughts on required vs optional with default?

Kennric commented 8 years ago

@matthewrsj I'd say make it optional with a sensible default like support.

matthewrsj commented 8 years ago

I agree. Then @athai you only have to make one more test that doesn't include that field and assert it's set to the default

matthewrsj commented 8 years ago

The tests cover the cases:

+1 from me

Kennric commented 8 years ago

I think that's got it. +1

athai commented 8 years ago

@matthewrsj @Kennric Is there anything I need to add to the formsender docs?

I was thinking that it may be useful to update http://formsender.readthedocs.org/en/latest/usage.html to display a dict in the email field since that better reflects what we're doing. Alongside that, adding 'send_to' to the optional fields section (http://formsender.readthedocs.org/en/latest/form_setup.html#optional-fields).

matthewrsj commented 8 years ago

@athai yup that first link needs to be updated. Also update the EMAIL description below that to show how to use it.

And yes the optional field section needs to be updated.

matthewrsj commented 8 years ago

@athai just a few documentation things. The implementation looks great, the optional parameter really kept things simple.

Kennric commented 8 years ago

Looks good. +1

matthewrsj commented 8 years ago

+1 from me too. Thanks for cleaning up my test docstrings for me.