User tests: Successful: Unsuccessful:
Pull Request for Issue #37745 #37446 .
This change adds the option to select email as a notification type. Users can send notifications to:
First you need to apply this PR. Since there are database changes you will need to run those as well.
For MySQL users run this on your database:
INSERT IGNORE INTO `#__mail_templates` (`template_id`, `extension`, `language`, `subject`, `body`, `htmlbody`, `attachments`, `params`) VALUES
('plg_workflow_notification.mail', 'plg_workflow_notification', '', 'PLG_WORKFLOW_NOTIFICATION_EMAIL_ON_TRANSITION_SUBJECT', 'PLG_WORKFLOW_NOTIFICATION_EMAIL_ON_TRANSITION_MSG', '', '', '{"tags":["siteurl","title","performer","transitionName","toStage","extraText"]}');
For Postgres users run this on your database:
INSERT INTO "#__mail_templates" ("template_id", "extension", "language", "subject", "body", "htmlbody", "attachments", "params") VALUES
('plg_workflow_notification.mail', 'plg_workflow_notification', '', 'PLG_WORKFLOW_NOTIFICATION_EMAIL_ON_TRANSITION_SUBJECT', 'PLG_WORKFLOW_NOTIFICATION_EMAIL_ON_TRANSITION_MSG', '', '', '{"tags":["siteurl","title","performer","transitionName","toStage","extraText"]}')
ON CONFLICT DO NOTHING;
Private Messages
is the old and default way of sending notifications. Clicking on the dropdown now also let's you select Email
as an option. Multiple options are possible as wellUnpublished
The sending of the emails uses the mail templates in Joomla. The mail template can be customized via these steps:
workflow
The following questions I have:
performer
Any suggestions for a better name?@brianteeman Could you please check the language files?
Thanks to @bembelimen and @softforge for the idea to add this. Thanks to @brianteeman to point out to use mail templates.
I know this isn't perfect code so looking for feedback to get this into Joomla to make transitions more useful as they are quite neat.
Messages are only sent to users who can receive private messages
Messages can now also be send to users with an account and email address.
Yes, need to document the addition of the email option.
Status | New | ⇒ | Pending |
Category | ⇒ | SQL Administration com_admin Postgresql Language & Strings Installation Front End Plugins |
Doesn't make any sense to keep the old broken behaviour as an option You're introducing a new method that fixes the bugs in the old method but still leave the old method there with its bugs
You cannot have a required field that is conditionaly displayed.
You have made the notification type a required field but is only shown when send email is set to yes.
I really dont undrstand why you have kept the broken private messages version. There is no explanation or why you would use method email or method private messags and you can still select groups and users to receive private messages which will neveer be sent.
It needs to be clearer that you can customise the mail further if you are using the email method by using th email templates BUT that it is a generic template for all transition notification emails and you must put anything specific to the transition in the notification config only.
In what scenario would anyone ever send a message by email AND private message?
Labels |
Added:
Language Change
?
|
The "Additional Message Text" field must be removed.
This cannot be removed as it will be a backwards compatible break.
Secondly, if we add text in this field and we select both methods of receiving a notification, then we will receive this additional text in personal messages, but we will NOT receive it in email, since this additional text will not be in the letter template.
You can add the additional text to the email. This is a valid point that the extra text should be in the default mail template. I have updated this PR with that change.
Thirdly, in this field we do not see the real message (only the description), but we see it in the email template.
I do not understand this. Can you explain this further?
In short, we really need to leave 1 place for setting the "body" of the notification and this place in the email templates.
That is something I was thinking about but really have no idea how we should do this. First I cannot remove this addition message text due to backwards compatibility break. Second, we could create a mail template for every transition but these cannot be in core because we have no idea which transitions there will be.
Doesn't make any sense to keep the old broken behaviour as an option
I cannot remove sending messages to private messages because that would be a backwards compatible break. I would not say it is broken but rather limited as we can only send messages to users with access to private messages.
You cannot have a required field that is conditionaly displayed.
Actually you can. The required condition is only active when the field is displayed.
There is no explanation or why you would use method email or method private messags
Any suggestions for a text to add?
you can still select groups and users to receive private messages which will neveer be sent.
I saw your other issue about that but this is not in scope of this PR. I can look at that in a separate PR.
It needs to be clearer that you can customise the mail further if you are using the email method by using th email templates BUT that it is a generic template for all transition notification emails and you must put anything specific to the transition in the notification config only.
See my remark above for Kostelano, he basically says the same thing. Any suggestions on how to improve this are welcome.
In what scenario would anyone ever send a message by email AND private message?
I knew this was coming :) If I chose a single dropdown the request would come to make it multiple, make it multiple and the request will come for a single dropdown. Ghehehehe.
So my reasoning is that the transition message can be send but may not arrive in the mail but you can see it in your private message box. Why limit the options for the user? We can let the user decide what they want to use, we do not have to decide for them.
Doesn't make any sense to keep the old broken behaviour as an option
I cannot remove sending messages to private messages because that would be a backwards compatible break. I would not say it is broken but rather limited as we can only send messages to users with access to private messages.
No its not B/C break you are just changing from a completely broken method to a working method
You cannot have a required field that is conditionaly displayed.
Actually you can. The required condition is only active when the field is displayed.
I do not write things just for the sake of writing words. I tested the PR. (and I know that we already have the same bug elsewhere.)
you can still select groups and users to receive private messages which will neveer be sent.
I saw your other issue about that but this is not in scope of this PR. I can look at that in a separate PR.
Of course it is in the scope of this PR. Otherwise it doesn't fix the bug you state this pr fixes.
@Kostelano you need the "additional text" field. Without it you cannot have a unique message for each notification.
Mail templates are for general customisations of all notification emails
No its not B/C break you are just changing from a completely broken method to a working method
It works on the site where we are using workflow. Messages go to the private message box.
I do not write things just for the sake of writing words. I tested the PR. (and I know that we already have the same bug elsewhere.)
I also tested this of course but did not read between your lines that there should not be an option selected. I removed the requirement.
I have tested this item
Does not fix the reported problems
There is no br tag in private messages.
Good catch. I moved it to the private message where it comes from.
So I have updated this PR to also check if there is any recipient selected when saving the transition and if not, an error message is shown and the transition is not saved. The same goes if for some reason a user manages to save with no notification types selected. That should fix the issue of enabling the notification but not selecting any recipients.
Labels |
Added:
Conflicting Files
|
This pull requests has been automatically converted to the PSR-12 coding standard.
Labels |
Added:
?
Removed: Conflicting Files |
Rebased to 4.3
Hi,
i'v upgraded a CLEAN installation of joomla 4.2.0 to the last one 4.2.2.
I'v upgrade by the file upload (administrator/index.php?option=com_joomlaupdate&view=upload) and not by the automatic update
The check of the database by the internal function .../administrator/index.php?option=com_installer&view=database shows two errors of database:
One of this was the XXXX_mail_templates has MEDIUMTEXT character set.
I solved with this SQL command:
ALTER TABLE XXXX_mail_templates
CHANGE htmlbody
htmlbody
MEDIUMTEXT CHARACTER SET utf8 COLLATE utf8_unicode_ci NOT NULL;
Labels |
Added:
PR-4.3-dev
Removed: ? ? |
I have tested this item
Still no mail received after a transition. The private message notification works, but not the email.
Title |
|
This pull request has been automatically rebased to 5.0-dev. No new features will be merged into Joomla! 4.3 series. Joomla! 4.4 series is a bridge release to make migration from Joomla! 4 to 5 as smooth as possible.
@micker If there is no other super user besides the one who causes the notification, then there is nobody to send email to. Create another user user and that other one should get the email if the first super user changes something and your email sending is set up right.
I have tested this item
thanks @richardnet its great now
@roland-d tested and validate !
maybe 2 return
Title |
|
This pull request has been automatically rebased to 5.1-dev.
This pull request has been automatically rebased to 5.2-dev.
Title |
|
I'm trying to apply the patch to test this but I get this error:
The file marked for modification does not exist: plugins/workflow/notification/notification.php
It seems the notification plugin was updated to the new architecture and you need to update your pr @roland-d . Can you please check?
Labels |
Added:
Feature
Conflicting Files
Updates Requested
PR-5.0-dev
PR-5.2-dev
Removed: PR-4.3-dev |
Just a heads up, I have merged the 5.2 dev into this branch but I have not tested anything yet. More to follow later.
Labels |
Removed:
PR-5.0-dev
|
I have tested this item ? unsuccessfully on 1f172e8
I followed all of the steps - selecting the notification to go to the superuser group.
Upon saving the article this was the error message:
Menu
Frontend
An error has occurred.
0 Joomla\Plugin\Workflow\Notification\Extension\Notification::getUsersFromGroup(): Argument #1 ($data) must be of type Joomla\Plugin\Workflow\Notification\Extension\stdClass, stdClass given, called in /home/basicjoo/j5core.basicjoomla.com/stageit/plugins/workflow/notification/src/Extension/Notification.php on line 195
Call Stack
1 () JROOT/plugins/workflow/notification/src/Extension/Notification.php:392
This pull request has been automatically rebased to 5.3-dev.
Title |
|
Great addition.
I tested a little, I'll leave a comment.
The "Additional Message Text" field must be removed. First, we find ourselves in a similar situation, which I described in the issue #37493. Secondly, if we add text in this field and we select both methods of receiving a notification, then we will receive this additional text in personal messages, but we will NOT receive it in email, since this additional text will not be in the letter template. Thirdly, in this field we do not see the real message (only the description), but we see it in the email template.
In short, we really need to leave 1 place for setting the "body" of the notification and this place in the email templates. We can mention this in the description for the "Notification Type" parameter.