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

Fix actionlogs information emails containing HTML links #40033

Merged
merged 4 commits into from Dec 28, 2023

Conversation

ManuelHu
Copy link
Contributor

@ManuelHu ManuelHu commented Mar 5, 2023

Pull Request for Issue #39851. This is essentially my strategy "3" as outlined in the issue, with the difference that the two replacement arrays are not merged. As I did not receive any comments on the issue, I just went ahead to implement this solution....

Summary of Changes

This needed multiple changes:

  1. MailTemplate now supports an extra set of replacement values for the plain-text mode. This will only be used if set explicitely, and will fall back to the old behaviour otherwise. The 'default' data and the plain data are not merged. Callers have to ensure that both sets of values contain the same keys, or otherwise output might be missing.
  2. ActionlogModel uses this new facility to provide values without HTML links.
  3. ActionlogsHelper::getHumanReadableLogMessage() already had a parameter $generateLinks, that did not do what was documented. It only controlled the conversion from relative to absolute URLs. Now it will also remove link tags from the message templates, if set to false.

Note: This also affects the CSV export feature of the actionlogs components. Until now, the exported files contained relative urls in the administrator application (which is - I think - quite useless anyway). With this PR, links are also stripped. (This is the the only other call site that changed the value of $generateLinks to false.)

Testing Instructions

  • Install a recent 4.3.0 nighly build, that already has [4.2] Fix MailTemplate array regression from 4.2.7 #39850 upmerged.
  • Make sure action logs for user events are enabled and configure working system email sending
  • In the user settings for a user with a valid email address (you can receive mail on), enable user action information emails for the user component.
  • Log out and log back in
  • Look at the emails you should have received

Actual result BEFORE applying this Pull Request

Plain text emails contain HTML links

Expected result AFTER applying this Pull Request

Plain text emails do not contain HTML links and only the link text.

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

@Hackwar Hackwar added the Feature label Apr 8, 2023
@HLeithner HLeithner changed the base branch from 4.3-dev to 5.0-dev May 8, 2023 15:01
@HLeithner
Copy link
Member

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.

@ManuelHu
Copy link
Contributor Author

ManuelHu commented May 9, 2023

From a users perspective I would not classify this a s a new feature, but I also don't want to argue about that :-) (From the code point of view it is a new feature...)

To get this finally into 5.0, I would like to have some feedback if the slight b/c break here is accepatable:

  • Adding a new parameter with a defult value should not even be a problem?
  • The $generateLinks parameter of ActionlogsHelper::getHumanReadableLogMessage() changes it's semantics, but should not be covered by the b/c policy (as the function is part of a component helper, and not a core library)
  • --> this has effects on the CSV export which now will also not contain links.

ManuelHu and others added 3 commits June 30, 2023 19:26
This needed multiple changes:
* MailTemplate now supports an extra set of replacement values for the
  plain-text mode. This will only be used if set explicitely, and will
  fall back to the old behaviour otherwise.
  The 'default' data and the plain data are _not_ merged. Callers have
  to ensure that both sets of values contain the same keys, or otherwise
  output might be missing.
* ActionlogModel uses this new facility to provide values without HTML
  links.
* ActionlogsHelper::getHumanReadableLogMessage already had a parameter
  $generateLinks, that did not do what was documented. It only controlled
  the conversion from relative to absolute URLs. Now it will also remove
  link tags from the message templates, if set to false.
Co-authored-by: Quy <quy@nomonkeybiz.com>
@ManuelHu
Copy link
Contributor Author

I have rebased this to the current 5.0-dev. Can someone test this that we can get this timely for 5.0? @Quy

@ceford
Copy link
Contributor

ceford commented Sep 14, 2023

I have tested this item ✅ successfully on b8995aa

Works fine: without patch email is hard to read because of links; with patch it is plain text.


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

@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:50
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.1-dev.

@Hackwar Hackwar self-assigned this Oct 25, 2023
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on b8995aa


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

@Quy
Copy link
Contributor

Quy commented Nov 15, 2023

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 15, 2023
@Quy Quy added PR-5.1-dev and removed PR-5.0-dev labels Nov 15, 2023
@Razzo1987 Razzo1987 merged commit 9b943e3 into joomla:5.1-dev Dec 28, 2023
2 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 28, 2023
@Razzo1987
Copy link
Contributor

Thanks you!

@Razzo1987 Razzo1987 added this to the Joomla! 5.1.0 milestone Dec 28, 2023
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.

None yet

9 participants