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 hooks for email poller plugins #21384

Merged
merged 1 commit into from Jun 26, 2023

Conversation

AlessioC31
Copy link
Contributor

Hello,

This PR is child of #20445: while it would be better to support OAUTH2 with pop3, that PR can't be merged until pop3 merges the support for XOAUTH (ruby/net-pop#16).

For this reason, I would like to add in discourse the support for mail pollers plugins. Doing so, it would be possible to write a plugin which then uses other ways (microsoft graph sdk for example) to poll emails from a mailbox.

The idea is that a plugin would define a class which inherits from Email::Poller and defines a poll_mailbox static method which returns an array of strings. Then the plugin could call register_mail_poller(<class_name>) to have it registered. All the configuration (oauth2 tokens, email, etc) could be managed by sitesettings defined in the plugin.

Please let me know if this solution works for you (I tried to have a look at other "plugin hooks" and based my solution on those) and if you think I should change something, thanks!

@AlessioC31 AlessioC31 force-pushed the add_poll_plugin_hooks branch 3 times, most recently from 3803416 to 18910a0 Compare May 9, 2023 15:31
@AlessioC31
Copy link
Contributor Author

I've slightly changed the design of the API. Now discourse core gives a callback to the plugin to call each time to process an inbound mail. I thought this was the best idea to easily give the control over the lifecycle of an email to the plugin.

@AlessioC31 AlessioC31 force-pushed the add_poll_plugin_hooks branch 2 times, most recently from 862e7c7 to 08111a7 Compare May 10, 2023 10:14
@SamSaffron
Copy link
Member

the biggest thing missing here is some tests, we are adding an extensibility point here, if we don't cover it with tests we are more likely to mistakenly remove or break it in future.

@AlessioC31
Copy link
Contributor Author

the biggest thing missing here is some tests, we are adding an extensibility point here, if we don't cover it with tests we are more likely to mistakenly remove or break it in future.

I've added a couple of tests, let me know if you think we should add more, thanks!

@AlessioC31 AlessioC31 force-pushed the add_poll_plugin_hooks branch 2 times, most recently from 79d7fcd to a017563 Compare June 5, 2023 10:40
@CLAassistant
Copy link

CLAassistant commented Jun 5, 2023

CLA assistant check
All committers have signed the CLA.

@AlessioC31 AlessioC31 force-pushed the add_poll_plugin_hooks branch 2 times, most recently from 8a9461f to 6002f02 Compare June 5, 2023 11:23
@AlessioC31
Copy link
Contributor Author

I saw that the PR failed, the error didn't look like it was something related to my changes. I rebased again and pushed

@AlessioC31
Copy link
Contributor Author

AlessioC31 commented Jun 6, 2023

Hello, I've added the fact that now the validator for reply_by_email_enabled checkes also custom mail pollers.
So that it's possible to enable the reply by email feature while having manual polling / pop3 disabled but at least one custom mail poller

@AlessioC31 AlessioC31 force-pushed the add_poll_plugin_hooks branch 2 times, most recently from bf3cc7e to f5705b8 Compare June 6, 2023 11:25
@trobiyo
Copy link

trobiyo commented Jun 9, 2023

With the following configuration, we've been able to reply by mail (private message included) and send an email to a category with successful results:

Settings (core)

  • reply by mail enabled: checked
  • reply by email address: account+%{reply_key}@example.com
  • email_in: checked
  • manual polling enabled: unchecked
  • pop3 polling enabled: unchecked

Settings (plugin)

Emails are automatically auto-purged from Exchange Online, which is great.

Unless Discourse Team gives us some more feedback, this is ready.

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/microsoft-graph-mail-poller/268611/1

Comment on lines +121 to +123
def self.register_mail_poller(mail_poller)
self.mail_pollers << mail_poller
end
Copy link
Contributor

@nattsw nattsw Jun 21, 2023

Choose a reason for hiding this comment

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

Just need a small test in https://github.com/discourse/discourse/blob/main/spec/lib/discourse_plugin_registry_spec.rb. Ideally we want to test the empty set.

If you could add an explicit empty test in poll_mailbox_spec.rb describe "poller plugin" that would be good too - though I get that it would be covered implicitly in the tests above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I added the test for the empty set!

@nattsw
Copy link
Contributor

nattsw commented Jun 21, 2023

Heya @AlessioC31, took this for a spin locally! This generally looks very good, thanks. I'll be happy to approve this with one small addition. Will keep this PR open 👀 so we can get this in this week.

@nattsw
Copy link
Contributor

nattsw commented Jun 26, 2023

Very funny/shady but the test failure is mailer related but has got nothing to do with your change. Will re-run

  1) UserNotifications.user_replied number of tags shown in subject line max_tags_per_email_subject siteSetting disabled should match max_tags_per_topic
     Failure/Error: expect(mail.subject).to match(/Taggie/)
     
       expected "[Discourse] [India] Teggo Taggo Super cool topic" to match /Taggie/
       Diff:
       @@ -1 +1 @@
       -/Taggie/
       +"[Discourse] [India] Teggo Taggo Super cool topic"       

@nattsw nattsw merged commit 5671850 into discourse:main Jun 26, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants