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: Preserve Redirection Links for Images in Emails #18397

Open
wants to merge 3 commits into
base: 10.0/bugfixes
Choose a base branch
from

Conversation

MyvTsv
Copy link
Contributor

@MyvTsv MyvTsv commented Nov 27, 2024

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

It fixes !35308

When an image is sent by email, it is possible to link it in order to redirect the user to another page. SharePoint uses this system which links to images. The problem is that if the user sends an image containing a redirect link, GLPI displays the image in the DOM but does not redirect the user to the associated link.

@MyvTsv MyvTsv requested a review from stonebuzz November 28, 2024 10:24
@MyvTsv MyvTsv marked this pull request as ready for review November 28, 2024 10:25
@MyvTsv MyvTsv requested a review from Rom1-B November 28, 2024 10:25
@stonebuzz
Copy link
Contributor

Maybe a new test should be added (instead of update it) to test the regex format (permissive now)

To test that it accepts any type of content before or after <img>, including HTML tags.

<a href="example.com">
  <span>Balise imbriquée</span>
  <img src="image.jpg" alt="Image" class="tag-class" />
</a>
<a href="example.com">
  Une image simple
  <img src="image.jpg" alt="Image" class="tag-class" />
  Plus de texte ici
</a>
<a href="example.com">
  <div>
    Contenu complexe
    <img src="image.jpg" class="tag-class" />
  </div>
</a>

@trasher
Copy link
Contributor

trasher commented Nov 28, 2024

I'm not sure I understand this fix... It talks about keeping redireciton link, and tests was adapted to drop one :D

Also, it seems the previous fix was successfully used by the customer; is this the same case as the "original" one?

Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous test case should be preserved.

Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other format should be test too

@@ -943,7 +943,7 @@ public function testConvertTagToImageAlreadyInLink()
$expected_result = <<<HTML
Some contents with <a href="http://example.org/">a link</a>
and a first image <a href="{$expected_url_1}" target="_blank"><img alt="{$img_1_tag}" width="10" src="{$image_1_src}" /></a> inside a link
then a second image surrounded by links <a href="http://www.example.org/">link1</a> <a href="{$image_2_src}" target="_blank" ><img alt="{$img_2_tag}" width="10" src="{$image_2_src}" /></a> <a href="http://www.example.org/2">link2</a>
then a second image surrounded by links <a href="http://www.example.org/">link1</a> <img alt="{$img_2_tag}" width="10" src="{$image_2_src}" /> <a href="http://www.example.org/2">link2</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
then a second image surrounded by links <a href="http://www.example.org/">link1</a> <img alt="{$img_2_tag}" width="10" src="{$image_2_src}" /> <a href="http://www.example.org/2">link2</a>
then a second image surrounded by links <a href="http://www.example.org/">link1</a> <a href="{$image_2_src}" target="_blank" ><img alt="{$img_2_tag}" width="10" src="{$image_2_src}" /></a> <a href="http://www.example.org/2">link2</a>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep original test

Copy link
Contributor

@Rom1-B Rom1-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add these test cases without modifying the originals

Copy link
Contributor

@Rom1-B Rom1-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests case

@cedric-anne cedric-anne linked an issue Feb 6, 2025 that may be closed by this pull request
2 tasks
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.

Sharepoint/one drive links are removed
5 participants