-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[4.0] Converting several mails to mail templates #28722
Conversation
Results from selected initial tests to see if the marked emails work as before: Mail: "Mail to the new user when creating a new user in the backend” Mail: "Password reset for users in frontend" My test environment is Windows 10, Wampserver 3.20, MySQL 5.7.23 and PHP 7.4.2. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
Changed the Mail Templates – Options: Per Template Mail Settings No --> Yes This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
params of com_mails: {"mail_style":"plaintext","alternative_mailconfig":"1","copy_mails":"0","attachment_folder":""} Function Location1 () JROOT\libraries\src\Mail\MailTemplate.php:198 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
Have you applied the database changes as well? |
Which database changes? I tested #28722 with J4 Patch Tester RC2 and the latest J4 Nightly Build. Yesterday I noticed that the latest Nightly Build was in fact from April 14, which may explain the issue. Asked about it in #27666 and richard67 was going to inform the CMS maintenance team. April 14 is still the latest Nightly Build. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
You can also try the download package in this PR. https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/28722/downloads/31378/ |
@Quy Sorry, I tried but your instructions are not detailed enough. What am I supposed to download from https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/28722/downloads/31378/ ? Links to both .zip files are broken. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
@toivo When testing a Pull Request (PR) it is always good to look first which files it changes. You can see that for this PR here on GitHub: https://github.com/joomla/joomla-cms/pull/28722/files. If a PR adds a new update sql script or changes an existing one, it means the PR also contains database changes. In case of this PR here these are the files I hope these explanations help you a bit to understand the previous comment about database changes of this PR. |
@richard67, that was helpful, thanks. I will apply the two .sql files against the database created by the Nightly Build from April 14 and run the J4 Patch Tester RC2. - Any news when the Nightly Builds might continue with new versions? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
@toivo They are still working on it as far as I know. Regarding SQL, don't forget to replace the "#__" with the table prefix (including the onderscore at the end) before running the statements. |
@Richar67 Just installed the latest Nightly Build from April 14, executed com_admin/sql/updates/mysql/4.0.0-2020-04-16.sql and applied PR #28722 using J4 Patch Tester RC2, then found that the language files are out of date because com_mails is displaying raw language strings, for example com_contact_MAIL_mail_TITLE. I would like to test this PR but where are the updates to the language files? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
@toivo I can't see any language sting with name like |
@Hackwar It seems there is something wrong with the update sql script. First of all there is a warning or error (depending on mysql strict mode settings) telling that columns In addition, @toivo is right: There are strange language sting names instead of real texts shown in the mail templates list view after having run the update SQL. Here just an example with 2 of them, but it's true for all the new records. |
Localhost: Windows 10, Wampserver 3.2.1, Apache 2.4.1, MySQL 8.0.19, PHP 7.4.5 Part 1 "Test if emails still arrive identically to the way they did before" • not tested: com_contact API controller to send a mail • Massmailer mail - error - details below: An error has occurred. Function Location1 () JROOT\libraries\src\Mail\MailTemplate.php:90 MySQL reported two warnings when the SQL update file 4.0.0-2020-04-16.sql was executed: This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28722. |
I've reported this already with my comment above. |
@toivo Thanks for reporting back. Please wait with further tests until the necessary corrections have been made in this Pull Request (PR). I'll give you a notice when the PR can be tested again. |
@Hackwar For the SQL error mentioned in the comments above for updating see Hackwar#40. |
Beside the HTML mode scheck box of the Mass Mail form being ignored, there are other, already known issues with HTML mode and mail templates, see #28524 and #27619 . @Hackwar But the general question remains what to do with the check box options in the Mass Mail form. Maybe they should be removed from this form if these options have to be configured in the mail template? |
@toivo Regarding the update notification email I suggest you do not use any hack. Use a clean 4.0-dev or latest J4 nightly build. Then in the Joomla Update Component's option, change update channel to "Custom URL", set minimum stability to "Development" and enter following custom URL: https://test5.richard-fath.de/j4-nightlies-update-url-patched-for-test.xml, then save. This should cause an update to 4.0.0-beta2-dev be found (which is fake because it just links to the nightly of tonight). The go to the Extension Installer's options and set the cache time to zero and minimum stability to "Development" and save. Then you should get some email, if everything else is set up right. Please test if this works for you and report back the result. |
@toivo joomla-cms/api/components/com_contact/src/Controller/ContactController.php Lines 123 to 126 in 944ffd5
This part was not touched by this PR, so I think it's really unrelated. |
@richard67 The configuration of the workaround to get the Updatenotification email without any tweaks worked all right, but my Super User received four (4) identical emails and they keep coming because of the cache time. I can now apply the patch and continue testing, unless some changes are going to be made overnight. |
@toivo I found the problem: I have to explicitely switch the "Show email form" to "Yes" for the particular contact, then it works. The reason is that the code I've linked in my previous comment checks only the options of the particular contact, and only if Yes, not if any inherited setting. That's definitely an issue not related to this PR. But when I got it work, the email contained the language sting constants and nothing else. I have to check if this happens also without this PR. |
@richard67 Ok, setting the option 'Contact Form' to 'Show' makes sense. The option 'Session Check' is probably not going to be used in the API. |
@toivo When I send the email now with the API to the contact, I get the language constants in the email when having no template for en-GB or when having a template with all buttons to "No" so it uses the language constants. Only when switching the buttons to "Yes" I get the translated text. This could be related to this PR. I will continue to investigate and provide a patch for @Hackwar like before if I find the reason and a valid fix. Update: If I add the language strings COM_CONTACT_COPYSUBJECT_OF, COM_CONTACT_COPYTEXT_OF, COM_CONTACT_ENQUIRY_SUBJECT and COM_CONTACT_ENQUIRY_TEXT to file |
Update: If I add a |
Sounds an ok fix to me. Honestly moving that sendEmail part to a helper function shared by API and site rather than having duplicates in the controller of api and site might be best to avoid the duplication (and that would also have to load the lang file) |
Agree ... but there is anyway potential regarding future PRs for improvement of the Mail Templates component, e.g. regarding HTML mode. Most of the issues are already covered by issues #28524 and #27619 . But this PR should be ok now, I think. Will do a complete test again later. P.S. And also potential of API improvements and simplifications, of course. |
Merging this to get it through in time for beta this weekend. @richard67 if you still have time to report the results of your tests would be good |
@wilsonge @Hackwar Test were all ok with text emails. I've tested for each template without a template created for en-GB, then with template but default using language strings, and finally with own content. HTML templates don't work yet, as stated in issue #28524 , and so the "HTML mode" of the Mass Mail doesn't work, it always sends text mails (content type "text/plain"). Another issue #27619 also discusses the HTML templates. |
Hmm |
@infograf768 Clearly a mistake, should be "by" only, without the "when". Could you make a PR? |
will do as well as another issue concerning com_mails |
Which other issue? If it is the mass mail html mode not working: I wanted to open an issue for it later. |
see #29224 |
This converts several of the core mails to the mail template system.
List of mails send in Joomla
If someone else wants to take over the remaining ones, be my guest.
Testinstructions
Basically, you have to trigger sending the mails in the cases marked as done above and see if they still arrive identically to the way they did before. Then create a template and see if your modifications are reflected.
Comments
I had to extend the MailTemplate class with a few modifications and I guess this wont be the last time this is necessary.