-
-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Email template create, update, delete views #2467
Email template create, update, delete views #2467
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]> Co-authored-by: Eli Chadwick <[email protected]>
Co-authored-by: Eli Chadwick <[email protected]>
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. |
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.
LGTM now, approved. Added one additional minor comment.
|
||
class ScheduledEmailRescheduleView( | ||
ScheduledEmailLog.objects.create( | ||
details="Scheduled email was changed.", |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
This fixes #2438.
WIP. TODO: