WIP:Add Reminder Email for events - #209
zaettar wants to merge 11 commits into
Conversation
Sorry, something went wrong.
…tive name to make them easier to find for updating events. \<_=_>/
Added function to schedule the sending of the reminder email as a hook to an event creation and a function to update the scheduled remindermail when the event is updated. Progress boiii....
… of the reminder email. Slowly getting there...
…on of an event. |(o.o)|
…re come the commits to let Travis run tests...
…laced with special rule for function name for remindermail functions
…n since introduction of special rule for remindermails scheduling
…tart field is defined
NotSpecial
left a comment
There was a problem hiding this comment.
I think the current solution is a bit complicated.
Furthermore, it seem like none of the new functionality is tested.
Sorry, something went wrong.
| item = pickle.load(args) | ||
| func_s = "remindermail"+str(item['_id']) | ||
| else: | ||
| func_s = func_str(func) |
There was a problem hiding this comment.
This seems super hacky to me. Is this special case for reminder mails really necessary?
Sorry, something went wrong.
| func_s = func_str(func) | ||
| if func_str(func) is "remindermail": | ||
| item = pickle.load(args) | ||
| func_s = "remindermail_"+str(item['_id']) |
There was a problem hiding this comment.
I think this will break the update. the update_one below identifies the function by name, so if the name is changed, the original function cannot be updated anymore.
Sorry, something went wrong.
| remindermail, | ||
| item) | ||
|
|
||
|
|
There was a problem hiding this comment.
We have already discussed this in the chat shortly -- might it not be easier to add a periodic task sending reminder mails for all upcoming events?
Sorry, something went wrong.
NotSpecial
left a comment
There was a problem hiding this comment.
Provide default for email text
Sorry, something went wrong.
| } | ||
|
|
||
| # Populate content text with fetched infos | ||
| email_content = current_app.config['REMINDER_EMAIL_TEXT'] % fields |
There was a problem hiding this comment.
As fair as I can see, REMINDER_EMAIL_TEXT is not in the current app settings. I think we should provide a default, similar to the signup confirmation
Sorry, something went wrong.
|
We have recently moved to templated email messages using jinja2 and send multipart email messages with an HTML and a plaintext version. Due to this change, this pull request must probably be reworked as it is quite old now. |
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
#180
Add the creation and sending of a reminder email for events. The reminder will be sent to all the users signed up for the event.