Skip to content

Conversation

@brianteeman
Copy link
Contributor

When mail templates are set to HTML it is not possible to SEE the senders email address as it sees the wrapping < > as html markup

This PR simply removes the < > from the string so that the email address is displayed.

The alternative would be to create a more complicated option that uses a different language string for the HTML version of the mail template

Pull Request for Issue #44791 .

Testing Instructions

Set the mail templates to use html and then send a test mail using a contact form

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

When mail templates are set to HTML it is not possible to SEE the senders email address as it sees the wrapping < > as html markup

This PR simply removes the < > from the string so that the email address is displayed.

The alternative would be to create a more complicated option that uses a different language string for the HTML version of the mail template
@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on 592eaf9

Thank you @brianteeman for this quick fix. I tested it and now it works as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44792.

@drmenzelit
Copy link
Contributor

I have tested this item ✅ successfully on 592eaf9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44792.

@drmenzelit drmenzelit added RTC This Pull Request is Ready To Commit Language Change This is for Translators PR-5.2-dev and removed Language Change This is for Translators PR-5.2-dev labels Jan 27, 2025
@richard67
Copy link
Member

System tests are failing for both MySQL and PostgreSQL, and it seems related:

  Running:  api/com_contact/Contacts.cy.js                                               (93 of 129)

  Test that contacts API endpoint
    ✓ can deliver a list of contacts (887ms)
    ✓ can deliver a single contact (754ms)
    ✓ can create a contact (503ms)
    ✓ can update a contact (1177ms)
    ✓ can delete a contact (379ms)

    1) can submit a contact form

  5 passing (9s)
  1 failing

  1) Test that contacts API endpoint
       can submit a contact form:
     AssertionError: Timed out retrying after 4000ms: expected 'This is an enquiry email via http://localhost/cmysql/api/ from:\njane doe admin@example.com\n\nautomated test message\n' to contain 'jane doe <admin@example.com>'
      at  (webpack://joomla/./tests/System/integration/api/com_contact/Contacts.cy.js:65:29)

It seems the system tests need some adjustment, too.

@richard67
Copy link
Member

Back to pending as system tests need to be adjusted to the changes in this PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44792.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 27, 2025
@richard67
Copy link
Member

richard67 commented Jan 27, 2025

@LadySolveig Could you help here with adjusting the system tests (see my previous comment)? Not needed anymore, Brian has done that.

@alikon
Copy link
Contributor

alikon commented Jan 27, 2025

i'm unable to replicate the issue ##44791 without the pr

image

update system tests
@brianteeman
Copy link
Contributor Author

System tests updated

@richard67
Copy link
Member

RTC as previous human tests are still valid, only system tests were adjusted meanwhile.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44792.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 27, 2025
@Hackwar
Copy link
Member

Hackwar commented Feb 2, 2025

First of all thank you for working on this. Unfortunately I'm a bit unhappy with the approach you took, because with your approach we are relying on the transformation of the non-HTML text into HTML. At the same time we do have the possibility to create entirely different templates for HTML mails than for text mails.

What I'm trying to say here is, that I'd rather would want to see us introducing a new translation string and insert that into the htmlbody column of the template instead of modifying the existing templates. Could you please change this PR in that manner? Thank you! 😄

@brianteeman
Copy link
Contributor Author

with your approach we are relying on the transformation of the non-HTML text into HTML.

what transformation?

@Hackwar
Copy link
Member

Hackwar commented Feb 2, 2025

If the HTML body is empty, it tries to convert the plain text content into an HTML content. See line 328 of /libraries/src/Mail/MailTemplate.php

@brianteeman
Copy link
Contributor Author

yes but there is nothing special happening. the email address is just plain text now and its all it would be in an htmlbody template. So your reasoning is lost on me

@Hackwar
Copy link
Member

Hackwar commented Feb 2, 2025

When the email adress is wrapped in those brackets in a plaintext mail, at least in my program the content of the brackets is displayed as a link. With your change that is gone and I just get a plain text. There is a reason why we have the html body field and I would rather have a proper template in there than to convert the current plain text template into HTML, even if it is a simple conversion right now.

@brianteeman
Copy link
Contributor Author

They're not a link in my email client.

Doing it your way has a lot of b/c issues for anyone that has customised their email and that's not something easily addressed and not something I am prepared to work on

@coolcat-creations
Copy link
Contributor

I would make it a priority to get this fix, because websites can lose their leads without the fix. Afterward, you could discuss improving it.

@Hackwar
Copy link
Member

Hackwar commented Feb 2, 2025

UPDATE #__mail_templates SET htmlbody = 'COM_CONTACT_ENQUIRY_TEXT_HTML' WHERE template_id = 'com_contact.mail' AND htmlbody = '' AND language = '*'
That is fully b/c, doesn't break any existing modifications and is the correct solution to fixing this. I'm not going to merge this PR the way it is.

@coolcat-creations no, companies are not losing their leads. The data is still there. They can look at the source code of the mail and will see the address there. Yes, I know that this is cumbersome.

@brianteeman
Copy link
Contributor Author

That is only b/c if the email has not been customised

@rdeutz
Copy link
Contributor

rdeutz commented Feb 27, 2025

I think this is a valid fix for the problem and I don't see why we cant't merge it, will add it to the Wednesday meeting document

@LadySolveig LadySolveig changed the base branch from 5.2-dev to 5.3-dev March 12, 2025 18:09
@LadySolveig LadySolveig changed the title [5.2] display email address in contact [5.3] display email address in contact Mar 12, 2025
@brianteeman
Copy link
Contributor Author

I think this is a valid fix for the problem and I don't see why we cant't merge it, will add it to the Wednesday meeting document

has this been discussed yet?

@richard67
Copy link
Member

has this been discussed yet?

@brianteeman Yes. We have rebased it to 5.3-dev because there will not be a 5.2.x release anymore (if no urgent security fix), so the next regular one will be 5.3.0. As it was a clean rebase, RTC is still valid, and we can merge it as soon as drone is happy.

@richard67 richard67 merged commit 0d93740 into joomla:5.3-dev Mar 12, 2025
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 12, 2025
@richard67
Copy link
Member

Thanks all.

@richard67 richard67 added this to the Joomla! 5.3.0 milestone Mar 12, 2025
@brianteeman
Copy link
Contributor Author

thanks

@brianteeman brianteeman deleted the patch-3 branch March 12, 2025 18:41
Kostelano added a commit to JPathRu/localisation that referenced this pull request Apr 14, 2025
joomla/joomla-cms#41496 - (upmerge с 5.2х)
joomla/joomla-cms#42530 +
joomla/joomla-cms#43994 - (upmerge с 5.2х)
joomla/joomla-cms#44009 - (upmerge с 5.2х)
joomla/joomla-cms#44010 - (upmerge с 5.2х)
joomla/joomla-cms#44161 +
joomla/joomla-cms#44187 - (upmerge с 5.2х)
joomla/joomla-cms#44207 - (upmerge с 5.2х)
joomla/joomla-cms#44271 +
joomla/joomla-cms#44273 +
joomla/joomla-cms#44288 - (только для en-GB)
joomla/joomla-cms#44348 - (upmerge с 5.2х)
joomla/joomla-cms#44366 +
joomla/joomla-cms#44367 - (upmerge с 5.2х)
joomla/joomla-cms#44434 - (upmerge с 5.2х)
joomla/joomla-cms#44448 - (upmerge с 5.2х)
joomla/joomla-cms#44462 +
joomla/joomla-cms#44487 - (upmerge с 5.2х)
joomla/joomla-cms#44587 +
joomla/joomla-cms#44600 +
joomla/joomla-cms#44604 +
joomla/joomla-cms#44621 - (upmerge с 5.2х)
joomla/joomla-cms#44623 +
joomla/joomla-cms#44632 +
joomla/joomla-cms#44640 - (позже был REVERT joomla/joomla-cms#44845)
joomla/joomla-cms#44714 - (upmerge с 5.2х)
joomla/joomla-cms#44756 +
joomla/joomla-cms#44768 - (upmerge с 5.2х)
joomla/joomla-cms#44792 - (только для en-GB)
joomla/joomla-cms#44813 +
joomla/joomla-cms#44822 - (upmerge с 5.2х)
joomla/joomla-cms#44839 +
joomla/joomla-cms#44871 +
joomla/joomla-cms#44954 +
joomla/joomla-cms#45034 - (upmerge с 5.2х)
joomla/joomla-cms#45058 - (только для en-GB)
joomla/joomla-cms#45064 +
joomla/joomla-cms#45078 - (только для en-GB)
joomla/joomla-cms#45130 - (upmerge с 5.2х)
joomla/joomla-cms#45240 - (upmerge с 5.2х)
joomla/joomla-cms#45246 - (только для др. пакетов)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants