-
Notifications
You must be signed in to change notification settings - Fork 510
Introduce custom event reminder #6989
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
Conversation
f01a67f to
a931ead
Compare
| background-color: white; | ||
| display: block; | ||
| min-width: 250px; | ||
| min-width: 700px; |
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 likely is it that this does not break the display in some other place...? 😱
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 was a bad idea, indeed. Unfortunately the preview looks often silly with the auto size.
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.
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> |
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.
Why? Also it's much less readable now with such a long lone...
indico/modules/events/reminders/templates/emails/event_reminder.html
Outdated
Show resolved
Hide resolved
5096942 to
9774b99
Compare
9774b99 to
5d601ed
Compare
It does not seem particularly useful anywhere else
e5cf2cd to
65a8dc0
Compare
Otherwise e.g. links are simply lost, which is not great.
- add missing css to html email preview - use editor-output styling for preview dialog
The subject isn't HTML, tags are fine there
5306aee to
95add1c
Compare
| self.message.validators.append(DataRequired()) | ||
| self.message.flags.required = True |
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.
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
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, 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.
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.
no, i noticed it while testing #7094
This PR is about further improvements on event reminders
Implementation details:
I followed your advise regarding using
RenderModeMixinfor the message field. Old reminders got theRenderMode.plain_textand new reminders getsRenderMode.html. The migration doesn't translate existingtext/plainreminders totext/html. So at the end there are 3 types of reminder:In fact, the newly introduced
ReminderTypeenum is only standard or customized. I didn't want to embed the legacy one.