Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

badbadc0ffee
Copy link
Contributor

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?

@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch from 1961712 to fccae90 Compare November 14, 2024 17:25
@badbadc0ffee badbadc0ffee marked this pull request as ready for review November 14, 2024 17:34
@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch 2 times, most recently from 25ae434 to e133588 Compare November 16, 2024 16:04
@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch from f0fcd47 to 13c5df6 Compare November 20, 2024 11:33
@badbadc0ffee badbadc0ffee changed the title Draft proposal: Add html signals for plugin integration Add html signals for plugin integration Nov 20, 2024
@badbadc0ffee
Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 %}
Copy link
Member

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.

Copy link
Contributor Author

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.
"""
Copy link
Member

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.

Copy link
Contributor Author

@badbadc0ffee badbadc0ffee Dec 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants