-
-
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
Improve scheduled email views #2448
Improve scheduled email views #2448
Conversation
42260ef
to
b293efd
Compare
There's a limitation: it's only one to many and it's only for models with numeric based PKs.
f080b43
to
dd0dc6f
Compare
@elichad Ready for your review! I skipped the markdown->HTML conversion preview for now, I don't think we need it in MVP. |
I decided to implement email cancelling view + add missing tests. There are still some business cases that were not addressed:
I don't think they're required for MVP, though. |
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.
These views are looking good so far. Some suggestions to improve accessibility.
helper = BootstrapHelper(submit_label="Update") | ||
|
||
|
||
class ScheduledEmailCancelForm(forms.Form): |
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.
We should also think about this for the reschedule form - could a user misinterpret the meaning of the "Cancel" button there 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.
I think that grey "Cancel" button used here is common on other forms as well. That's why I wanted to keep it the same, and instead extend the left cancel button (you can see it has a long label).
I am open to suggestions, but I'm not so sure "Return without cancelling" is the best choice. Maybe we can figure out different labels.
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.
Yeah, finding something that works well alongside our existing patterns is tricky, and we should make sure this is included in user testing.
Perhaps we could use a question like "Are you sure you want to cancel this email?" with Yes/No buttons to get away from the confusion altogether?
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.
Sounds good to me. This would work for cancelling the scheduled email, but not so much for the rescheduling form.
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.
<div class="alert alert-{% if scheduled_email.state == 'failed' %}danger{% elif scheduled_email.state == 'succeeded'%}success{% elif scheduled_email.state == 'cancelled' %}secondary{% else %}info{% endif %}"> | ||
<p> | ||
Email was scheduled to run | ||
<relative-time datetime="{{ scheduled_email.scheduled_at.isoformat }}"></relative-time>. | ||
</p> | ||
<p> | ||
Current status: <strong>{{ scheduled_email.state }}</strong>. | ||
</p> | ||
</div> |
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 block is helpful! I especially like the use of <relative-time>
here
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.
Thanks! Please note that relative-time
requires a JS plugin, if I remember correctly.
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.
Is this left empty deliberately?
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.
Yes, there's a story for improving it: #2438.
# This generic relation is limited only to single relation, and only to models | ||
# defining their PK as numeric. | ||
generic_relation_content_type = models.ForeignKey( | ||
ContentType, | ||
on_delete=models.SET_NULL, | ||
null=True, | ||
blank=True, | ||
) | ||
generic_relation_pk = models.PositiveIntegerField(null=True, blank=True) | ||
generic_relation = GenericForeignKey( | ||
"generic_relation_content_type", "generic_relation_pk" | ||
) | ||
|
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.
How do you see us making use of this generic object relation? Is this what will help us to e.g. check a Person's email address is up to date when sending an email?
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.
For now it's only going to point at the main object, for which the email was created (e.g. a person for "persons_merged" signal, or a membership for "membership quarterly check"). I admit it's a subject to change, as we can only have 1 object in this generic relationship, and sometimes we would like to have more than that (e.g. for instructor not placed in a workshop we could have a person and an event).
@elichad Please take a look again :) Thanks! |
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.
Looks good! Approved, with one aesthetic suggestion
Co-authored-by: Eli Chadwick <[email protected]>
This fixes #2437.
This depends on #2434 and #2447. Once they are merged, this PR should become clearer.WIP! TODO:generic object relationrescheduling jobspossibly markdown -> html generation