Improve scheduled email views #2448

pbanaszkiewicz · 2023-06-24T14:43:55Z

This fixes #2437.

~~This depends on #2434 and #2447. Once they are merged, this PR should become clearer.~~

~~WIP! TODO:~~

~~generic object relation~~
~~rescheduling jobs~~
~~possibly markdown -> html generation~~

There's a limitation: it's only one to many and it's only for models with numeric based PKs.

pbanaszkiewicz · 2023-07-03T21:46:34Z

@elichad Ready for your review! I skipped the markdown->HTML conversion preview for now, I don't think we need it in MVP.

pbanaszkiewicz · 2023-07-05T20:40:39Z

I decided to implement email cancelling view + add missing tests.

There are still some business cases that were not addressed:

Markdown -> HTML conversion (preview)
Editing (edit/reschedule/cancel) a locked email
Re-enabling a cancelled email

I don't think they're required for MVP, though.

elichad

These views are looking good so far. Some suggestions to improve accessibility.

elichad · 2023-07-07T10:30:37Z

amy/emails/forms.py

+    helper = BootstrapHelper(submit_label="Update")
+
+
+class ScheduledEmailCancelForm(forms.Form):


The buttons on this form could be confusing, as both of them say "Cancel". Could you reword the usual "Cancel" button to "Return without cancelling" or similar?

We should also think about this for the reschedule form - could a user misinterpret the meaning of the "Cancel" button there too?

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.

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?

Sounds good to me. This would work for cancelling the scheduled email, but not so much for the rescheduling form.

Changed to:

amy/emails/views.py

elichad · 2023-07-07T11:43:30Z

amy/templates/emails/scheduled_email_detail.html

+<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>


This block is helpful! I especially like the use of <relative-time> here

Thanks! Please note that relative-time requires a JS plugin, if I remember correctly.

amy/templates/emails/scheduled_email_detail.html

elichad · 2023-07-07T12:09:48Z

amy/templates/emails/email_template_detail.html

Is this left empty deliberately?

Yes, there's a story for improving it: #2438.

elichad · 2023-07-07T12:16:53Z

amy/emails/models.py

+    # 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"
+    )
+


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?

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).

… accessibility

pbanaszkiewicz · 2023-07-07T19:59:20Z

@elichad Please take a look again :) Thanks!

elichad

Looks good! Approved, with one aesthetic suggestion

amy/templates/emails/scheduled_email_detail.html

Co-authored-by: Eli Chadwick <eli.chadwick.256@gmail.com>

pbanaszkiewicz added the type: new feature label Jun 24, 2023

pbanaszkiewicz added this to the v4.2 milestone Jun 24, 2023

pbanaszkiewicz self-assigned this Jun 24, 2023

pbanaszkiewicz force-pushed the feature/2437-improve-scheduled-email-views branch 3 times, most recently from 42260ef to b293efd Compare June 28, 2023 21:02

pbanaszkiewicz marked this pull request as ready for review July 3, 2023 21:43

pbanaszkiewicz requested a review from elichad July 3, 2023 21:43

pbanaszkiewicz added 10 commits July 3, 2023 23:44

[#2437] Add empty detail view for templates, scheduled emails

b08de34

[#2437] Correct casing in some views' titles

b05538b

[#2437] More details in scheduled email view

16bbab9

[#2437] Base views for edit, reschedule and cancel

6d3518b

[#2437] Edit scheduled email view+template

d371225

[#2437] Switch from "object" to "scheduled_email" template variable

067d194

[#2437] Controller: add method for rescheduling an email

cb97130

[#2437] Reschedule view to use non-model form

24606b7

[#2437] Add generic relation to scheduled email

e1159e1

There's a limitation: it's only one to many and it's only for models with numeric based PKs.

[#2437] Improve message link to scheduled email details

dd0dc6f

pbanaszkiewicz force-pushed the feature/2437-improve-scheduled-email-views branch from f080b43 to dd0dc6f Compare July 3, 2023 21:44

pbanaszkiewicz added 2 commits July 5, 2023 22:30

[#2437] Add email cancel view+form

05dce87

[#2437] Add missed tests

91bcb3b

elichad suggested changes Jul 7, 2023

View reviewed changes

pbanaszkiewicz added 3 commits July 7, 2023 15:57

[#2437] Unify scheduled email header details template

b1e3028

[#2437] Move the history sub-table out into separate table for better…

d363e1b

… accessibility

[#2437] Change form for cancelling the email

71f206a

pbanaszkiewicz requested a review from elichad July 7, 2023 19:58

elichad approved these changes Jul 10, 2023

View reviewed changes

amy/templates/emails/scheduled_email_detail.html Outdated Show resolved Hide resolved

[#2437] Update amy/templates/emails/scheduled_email_detail.html

c73d862

Co-authored-by: Eli Chadwick <eli.chadwick.256@gmail.com>

pbanaszkiewicz merged commit 8a25f8f into develop Jul 10, 2023
4 checks passed

pbanaszkiewicz deleted the feature/2437-improve-scheduled-email-views branch August 12, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve scheduled email views #2448

Improve scheduled email views #2448

pbanaszkiewicz commented Jun 24, 2023 •

edited

pbanaszkiewicz commented Jul 3, 2023

pbanaszkiewicz commented Jul 5, 2023

elichad left a comment

elichad Jul 7, 2023

elichad Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

elichad Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

elichad Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

elichad Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

elichad Jul 7, 2023

pbanaszkiewicz Jul 7, 2023

pbanaszkiewicz commented Jul 7, 2023

elichad left a comment

		helper = BootstrapHelper(submit_label="Update")


		class ScheduledEmailCancelForm(forms.Form):

Improve scheduled email views #2448

Improve scheduled email views #2448

Conversation

pbanaszkiewicz commented Jun 24, 2023 • edited

pbanaszkiewicz commented Jul 3, 2023

pbanaszkiewicz commented Jul 5, 2023

elichad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbanaszkiewicz commented Jul 7, 2023

elichad left a comment

Choose a reason for hiding this comment

pbanaszkiewicz commented Jun 24, 2023 •

edited