Skip to content

Conversation

@tomako
Copy link
Contributor

@tomako tomako commented Jul 25, 2025

This PR is about further improvements on event reminders

  • Add WYSIWYG editor for reminder note (and transforms reminder emails to HTML while keeping the text-based one)
  • Introduce custom reminder (with completely customizable subject and email body content)

Implementation details:
I followed your advise regarding using RenderModeMixin for the message field. Old reminders got the RenderMode.plain_text and new reminders gets RenderMode.html. The migration doesn't translate existing text/plain reminders to text/html. So at the end there are 3 types of reminder:

  • Legacy reminder -> text/plain email format only
  • Standard reminder -> both text/html and text/plain email format
  • Custom reminder -> text/html email format only

In fact, the newly introduced ReminderType enum is only standard or customized. I didn't want to embed the legacy one.

@github-actions github-actions bot added the alembic Contains database changes label Jul 25, 2025
@tomako tomako changed the title Introduce custom reminder Introduce custom event reminder Jul 25, 2025
@tomako tomako force-pushed the introduce_custom_reminder branch from f01a67f to a931ead Compare July 25, 2025 20:58
background-color: white;
display: block;
min-width: 250px;
min-width: 700px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How likely is it that this does not break the display in some other place...? 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a bad idea, indeed. Unfortunately the preview looks often silly with the auto size.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If needed, the ajaxDialog has an dialogClasses argument which can be used to add a custom CSS class to the parent element of the exclusivePopup

<hr>
<a href="{% block footer_url %}{{ url_for_index(_external=true) }}{% endblock %}">Indico</a>
:: {% block footer_title %}{% trans %}Email Notifier{% endtrans %}{% endblock %}<br>
<p><a href="{% block footer_url %}{{ url_for_index(_external=true) }}{% endblock %}">Indico</a> :: {% block footer_title %}{% trans %}Email Notifier{% endtrans %}{% endblock %}</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Also it's much less readable now with such a long lone...

@tomako tomako force-pushed the introduce_custom_reminder branch 2 times, most recently from 5096942 to 9774b99 Compare August 1, 2025 23:16
@ThiefMaster ThiefMaster force-pushed the introduce_custom_reminder branch from 9774b99 to 5d601ed Compare August 8, 2025 09:41
@ThiefMaster ThiefMaster force-pushed the introduce_custom_reminder branch from e5cf2cd to 65a8dc0 Compare August 8, 2025 14:09
@ThiefMaster ThiefMaster added this to the v3.3 milestone Aug 8, 2025
@ThiefMaster ThiefMaster enabled auto-merge (squash) August 8, 2025 16:20
@ThiefMaster ThiefMaster force-pushed the introduce_custom_reminder branch from 5306aee to 95add1c Compare August 8, 2025 16:25
@ThiefMaster ThiefMaster merged commit 9b226f3 into indico:master Aug 8, 2025
10 checks passed
Comment on lines +111 to +112
self.message.validators.append(DataRequired())
self.message.flags.required = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is incorrect, I also missed it when reviewing it - when you modify the validators it leaks into every other instance of that form within the same Python process.

The correct way to do this is using our inject_validators util. I fixed it in fe1b369

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed that yesterday and tried fixing it when working on #7115. I guess that grabbed your eyes to this problem :)
Thanks for fixing that I didn't know about that utility function which seems pretty robust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i noticed it while testing #7094

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alembic Contains database changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants