Email template create, update, delete views #2467
Conversation
There was a problem hiding this comment.
The new views look good on the whole. I have a couple of suggested improvements, and I did run into one bug that I now can't replicate.
I also realized some of the names of the new classes and URLs in this module don't match the conventions that we use elsewhere - most of the comments are about that.
| body = MarkdownxFormField( | ||
| label=EmailTemplate._meta.get_field("body").verbose_name, | ||
| help_text=EmailTemplate._meta.get_field("body").help_text, | ||
| widget=forms.Textarea, |
There was a problem hiding this comment.
When I first tried to create a template, the preview section showed me the AMY login page. When I refreshed, I was logged out fully. After logging in, the view worked as expected. I haven't been able to replicate this, so not sure what happened.
There was a problem hiding this comment.
Interesting! I have never seen this behavior either. This system is used in comments across AMY, so (I hope) it was well tested. Didn't hear from anyone about issues with preview.
There was a problem hiding this comment.
Hmm. Let's chalk it up to one-off weirdness in my local instance and see if it ever crops up again.
amy/emails/urls.py
Outdated
| include([ | ||
| path( | ||
| "", | ||
| views.EmailTemplateDetailView.as_view(), |
There was a problem hiding this comment.
Another convention point - we don't usually include View in our view class names, and detail views are typically ModelNameDetails (plural)
Co-authored-by: Eli Chadwick <eli.chadwick.256@gmail.com> Co-authored-by: Eli Chadwick <eli.chadwick.256@gmail.com>
Co-authored-by: Eli Chadwick <eli.chadwick.256@gmail.com>
pbanaszkiewicz
force-pushed
the
feature/2438-email-template-create-update-delete
branch
from
70fd6f2
to
a3c038b
Compare
|
@elichad Thank you for the review and suggestions. I changed the views as you requested. Please take another look at this PR. |
|
|
||
| class ScheduledEmailRescheduleView( | ||
| ScheduledEmailLog.objects.create( | ||
| details="Scheduled email was changed.", |
There was a problem hiding this comment.
Could you include the user that made the change here?
This would be useful for reschedule/cancel log entries too.
There was a problem hiding this comment.
It's a good idea to add author to these log items. I have created a ticket for that: #2484 (it would require changes in multiple places so I didn't want to work on it in this PR).
pbanaszkiewicz
deleted the
feature/2438-email-template-create-update-delete
branch
This fixes #2438.
WIP. TODO: