-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add html signals for plugin integration #1885
base: main
Are you sure you want to change the base?
Add html signals for plugin integration #1885
Conversation
1961712
to
fccae90
Compare
25ae434
to
e133588
Compare
f0fcd47
to
13c5df6
Compare
https://github.com/badbadc0ffee/pretalx-signals-demo contains a small demo that emits a sample for each of the suggested signals |
html_below_mail_subject = EventPluginSignal() | ||
""" | ||
This signal is sent out to display additional information related to emails in | ||
the internal organiser area. |
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 is not correct as implemented – the mail_log.html
file is show to the speaker aswell.
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.
That is right! - And should not necessarily be that way.
There obviously is already a differentiation, because the 'accordion' cannot be opened on the public profile page.
Makes a lot of sense, not to add the information there - although it might also make at least some sense to let the plugin decide.
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.
Minor additional remart: the accordion doesn't open in the public view but the HTML is the same as in the organiser backend.
@@ -102,6 +103,7 @@ <h2> | |||
{% elif mail.template == request.event.question_template %} | |||
<span class="badge color-info">{% translate "Unanswered questions reminder" %}</span> | |||
{% endif %} | |||
{% html_signal "pretalx.mail.signals.html_after_mail_badge" sender=request.event request=request mail=mail %} |
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 worry about the performance hit for calling a signal on every row of a potentially 100+ row table, especially as plugins can't add fields to be prefetched/preselected, which sounds like a recipe for n+1 queries.
This kind of request makes me think that maybe we should move to using django-tables2
(or a ripoff) regardless of the unfortunate performance penalty detailed here, as then we could give plugins the option to register proper table columns instead.
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.
That is definitely a good idea, I think.
You are absolutely right - you can see a significant difference in huge table views.
I am also attending the DemoCon this year. They have plenty of great sessions and sent out hundreds, almost a thousand e-amils through RT already. With erdgeists hurt-me-plenty patch, all of these render on a single page.
On my small old laptop that takes almost exactly 1.5 seconds on a vanilla 2024.3.1 - running in my debugger...
With the signals added but no one using it, we end up at 2.8 seconds - which you can already honestly feel. Installing the RT plugin but leaving it inactive for the event doesn't add anything. It actually looks like it runs a fraction of a second quicker which cannot really explained. Activating the plugin and showing the RT information with almost all of the mails sent, we add another bit: her I end up at approx. 3.4 seconds which is quite a bit more than the original 1.4 seconds.
On the other hand: when I use a paging that makes more sense, with 50 mails per page, adding the (unused signals raises the time from 770 ms to 780 ms, adding and activating the plugin adds ~100 ms. That would be fine for me.
event. Additionally, the signal will be called with the ``request`` it is | ||
processing, and the ``person`` which is currently displayed. | ||
The receivers are expected to return HTML. | ||
""" |
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’m not sure I like this – this way we’d accumulate a lot of these signals. I think it’s worth taking a look at what pretix is doing in instances like this – pretix allows plugins to add more forms to given pages, or to even replace the form class of a certain page, see e.g. pretix.control.signals.item_forms
, pretix.control.signals.voucher_form_class
.
In any case, I’d probably restrict this to a pretalx.orga.speaker_form_html
signal that is rendered below the form, and if there is a super urgent need for the plugin to add content to the top of the page, it could still move content up via JS, I suppose.
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 a lot for pointing that out! That approach is definitely more elegant. I'll dig into that.
The reason for me to put one hook above and one below here was that we actually have the usecase that we have some information that belongs below the form. Information from other systems like RT in particular but also Zammad and Frab. They are obviously all read-only now, btw. which could be addressed with the approach you suggested.
But we will also have some information that will be 'urgent' and should be on top of the page: during the congress, when a speaker has not shown up short before the presentation, that is the kind of indication I'd like to see above anything else.
This is an initial draft / a proposal:
I'd like to add some signals to be able to integrate information related to emails and submissions from a plugin.
@rixx: is this the way you'd like to allow plugins to add information to the orga pages?