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 and forms signals for plugin integration #1885

Open
wants to merge 3 commits 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 3 times, most recently 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this and moved the hooks to inject HTML and/or forms into the email editor.
Tje hooks are not on the new email composition page now. I'm not yet sure if I also want them there.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid this performance hit, I initially thought about limiting the badges to views with only a few table entries but then opted to remove them completely. I introduced hooks on the email details view / editor instead.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't really like the idea to reorder the page using JS but removed the 'above_xxx' hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... I’d probably restrict this to a pretalx.orga.speaker_form_html signal ...

Personally, I'd prefer to have the signals in the namespace of the ,odels they are related to. But the couly of courde all be moved into the pretalx.orga space if you like that more!

@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch from cc3480a to 4cdfb48 Compare December 9, 2024 14:54
@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch from 4cdfb48 to f46e682 Compare December 9, 2024 15:07
@badbadc0ffee badbadc0ffee force-pushed the add-html-signals-for-plugin-integration branch from f46e682 to ed5ee4a Compare December 9, 2024 15:09
@badbadc0ffee badbadc0ffee changed the title Add html signals for plugin integration Add html and forms signals for plugin integration Dec 9, 2024
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