-
Notifications
You must be signed in to change notification settings - Fork 6
WIP:Add Reminder Email for events #209
base: master
Are you sure you want to change the base?
Conversation
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current solution is a bit complicated.
Furthermore, it seem like none of the new functionality is tested.
item = pickle.load(args) | ||
func_s = "remindermail"+str(item['_id']) | ||
else: | ||
func_s = func_str(func) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems super hacky to me. Is this special case for reminder mails really necessary?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
remindermail, | ||
item) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide default for email text
} | ||
|
||
# Populate content text with fetched infos | ||
email_content = current_app.config['REMINDER_EMAIL_TEXT'] % fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
#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.