- Issue created by @dinazaur
- @dinazaur opened merge request.
- πΊπ¦Ukraine dinazaur
Few notes about PR.
I've implemented a few adjustments so it's easier to add new features:
- For the notifications I've implemented a new plugin system, so it's much easier to add new notification types, and we don't repeat a lot of code. So right now we have a two notification plugins: sms, mail.
- All logic that was not related to plugins I've moved to the service
- Implemented ability to process all entities types inside tokens.
- Fixed bug inside scheduled notifier in case when from/to sid was "all". It was not working at all.
What's left to do is:
1. I don't know why we need two identical configuration entity types. We need to remove sms notifier entity and replace it with simple checkboxes for every plugin that is defined in the system inside WorkflowNotificationForm with additional field inside WorkflowNotification something like "notification_plugins". It will eliminate the tone of code.
2. To test sms notifications, I don't have time to do it at all. - Status changed to Needs review
almost 2 years ago 4:28pm 11 August 2023 - π³π±Netherlands johnv
As a co-maintainer, I just now encounter your extensive patch.
Thanks a lot.
As it happens, I just finished the re-factoring of the module myself, by extending the separate WorkflowNotification and WorkflowSmsNotify Classes, and create a WorkflowAbstractNotification parent classe for all common code.Regarding your translation functionality, I will try to deduct from the MR.
- π³π±Netherlands johnv
On the other hand, I think you know better where the beef is.
Leaving it up to you to add a patch with only the translation part.
Also, perhaps π Error upon Translate Workflow Notification message Active is related.