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

Update email notification engine #164

Closed
WillStrohl opened this issue Nov 18, 2022 · 5 comments · Fixed by #752
Closed

Update email notification engine #164

WillStrohl opened this issue Nov 18, 2022 · 5 comments · Fixed by #752
Assignees
Labels
broad scope Non-specific issues that address multiple areas enhancement New feature or request epic Placeholder for large development which may include multiple issues and/or pull requests localization Updates to support multi-lingual websites performance An update likely to improve site performance
Milestone

Comments

@WillStrohl
Copy link
Member

Is your feature request related to a problem?

There is a lot of code around how emails are generated and sent. A lot of this code is legacy and updated over the years to saitisy many use cases surrounding how DNN expected you to connect to the email API and the various conditions and workarounds that have been necessary over the years.

Since then, the email API has been updated a lot to support a lot of the more advanced things that previously required custom code.

Describe the solution you'd like

Rewrite the email classes to use the current DNN email API directly, and remove all unnecessary legacy logic.

Describe alternatives you've considered

None yet.

Additional context

This step will likely be necessary before we do anything else with email. Such as the recent issue #163 for adding digests.

@WillStrohl WillStrohl added the enhancement New feature or request label Nov 18, 2022
@johnhenley
Copy link
Collaborator

#I would push back on this since I actually spent a lot of time in the email code to get portal-specific SMTP working because the core API doesn't work correctly:). The only change I would make is to remove the option to send immediately vs using the queue and just always use the scheduler/queue.

@WillStrohl
Copy link
Member Author

Maybe the description could be better written. I was attempting to write it based on what I remember from our previous conversations and updates. Though, isn't there still legacy email code that can be removed?

@johnhenley
Copy link
Collaborator

@WillStrohl I've studied this a bit and I thinking up a better title :)

@johnhenley johnhenley changed the title Update the Email API Logic Update email notification engine Apr 13, 2023
@johnhenley
Copy link
Collaborator

The original implementation has some drawbacks that are rooted in very old limitations in the DNN scheduler. In particular, the scheduler could only handle small units of work. The use of the mail queue is optional, and even when used, the logic for queuing up notifications is called from the UI thread. This imposes a performance penalty on a forum with lots of email subscribers, since a poster has to wait while notifications are queued. Additionally, the email notification logic is spread across multiple classes.

The proposed changes include:

  • removing the "use queue" option and always use it. This will make some code simpler
  • Implement a more generic queuing controller which will become the base for any type of queued work
  • implement a "new post" queue which handles any workflow actions currently handled in the UI and forces the user to wait. Examples are the reordering of previous/next pointers between topics, and updating last update timestamps, etc. Adding an single entry to "new post" queue should be all that is done vs current logic which looks up subscribers and queues each individually. 🙃
  • Move any notification code out of the UI and into the "new post" queue. This includes handling moderation notifications as well as regular notifications.

@WillStrohl @Timo-Breumelhof @skamphuis do you agree with this direction? Not going to happen all at once but it's not insurmountable.

@johnhenley johnhenley added localization Updates to support multi-lingual websites performance An update likely to improve site performance epic Placeholder for large development which may include multiple issues and/or pull requests broad scope Non-specific issues that address multiple areas labels Apr 13, 2023
@johnhenley johnhenley self-assigned this Apr 13, 2023
@Timo-Breumelhof
Copy link
Contributor

Sounds good to me :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broad scope Non-specific issues that address multiple areas enhancement New feature or request epic Placeholder for large development which may include multiple issues and/or pull requests localization Updates to support multi-lingual websites performance An update likely to improve site performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants