Notifications about page changes have wrong link #237

New issue

Open

opened 2019-04-09 08:46:25 +00:00 by tammoterhark · 1 comment

tammoterhark commented

2019-04-09 08:46:25 +00:00

(Migrated from github.com)

Whenever people change a page, certain users receive mails with notifications about which page was changed and a link to see the changes.

The link to the page is OK, but the link to the changes only shows the front page of the hypha instance.

<div style="font-family: sans; font-size: 12pt;"><div style="font-size: 14pt;"><b>De Stadsbron</b>: hypha samenvatting van de laatste 1 dagen</div>2103 page-visits<hr/><div>8-04-19, 12:31 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div>
<div>8-04-19, 16:44 Arjeh Kalmann heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/Podcast_Stadhuisplein_April_2019">nl/Podcast_Stadhuisplein_April_2019</a> - <a href="http://www.destadsbron.nl/#5ca89f777b2d8">view changes</a></div>
<div>9-04-19, 03:34 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div>
<div>9-04-19, 03:35 Theater Hark heeft de volgende pagina bewerkt: <a
href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div>
<div>9-04-19, 03:35 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/tijdlijn">nl/tijdlijn</a> - <a href="http://www.destadsbron.nl/#5c83c790cf0f9">view changes</a></div>
<div>9-04-19, 03:37 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div>
<a name="5b0d573837e4d"/>

Whenever people change a page, certain users receive mails with notifications about which page was changed and a link to see the changes. The link to the page is OK, but the link to the changes only shows the front page of the hypha instance. ``` <div style="font-family: sans; font-size: 12pt;"><div style="font-size: 14pt;"><b>De Stadsbron</b>: hypha samenvatting van de laatste 1 dagen</div>2103 page-visits<hr/><div>8-04-19, 12:31 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div> <div>8-04-19, 16:44 Arjeh Kalmann heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/Podcast_Stadhuisplein_April_2019">nl/Podcast_Stadhuisplein_April_2019</a> - <a href="http://www.destadsbron.nl/#5ca89f777b2d8">view changes</a></div> <div>9-04-19, 03:34 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div> <div>9-04-19, 03:35 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div> <div>9-04-19, 03:35 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/tijdlijn">nl/tijdlijn</a> - <a href="http://www.destadsbron.nl/#5c83c790cf0f9">view changes</a></div> <div>9-04-19, 03:37 Theater Hark heeft de volgende pagina bewerkt: <a href="http://www.destadsbron.nl/nl/home">nl/home</a> - <a href="http://www.destadsbron.nl/#5b0d573837e4d">view changes</a></div> <a name="5b0d573837e4d"/> ```

matthijskooijman commented

2019-04-25 10:33:06 +00:00

(Migrated from github.com)

I've had a look a tthis, the problem is that addBaseUrl is a bit overzealous. These links are supposed to be href="#5b0d573837e4d" and refer to anchors inside the e-mail, but addBaseUrl` instead lets them refer to the online page.

This is a bit of a challenge, since addBaseUrl cannot really distinguish between links that are intended to self-reference the e-mail and links that are intended to refer to pages online. We could special-case anchor-only links (e.g. starting with #), but that does not feel so nice. A more elegant approach would be to explicitly opt-out these links by adding a class="no-base-url" or something like that.

Currently, addBaseUrl still uses regex-based replacement. For the above exception to be reliably implemented, this should be switched to an xpath-based implementation (#190). It would probably be good to change the mail-sending code (which now does dewikify_html and then addBaseUrl to convert the html string to a DomDocument node once, and then pass it to dewikify and (the rewritten) addBaseUrl, so the string -> node conversion only happens once. It might even be useful to change the sendMail function to build up a complete DomDocument for the message body, rather than the string concatenation it does now. This would make it even easier to convert the string message to a node.

I've had a look a tthis, the problem is that `addBaseUrl` is a bit overzealous. These links are supposed to be `href="#5b0d573837e4d" and refer to anchors inside the e-mail, but `addBaseUrl` instead lets them refer to the online page. This is a bit of a challenge, since `addBaseUrl` cannot really distinguish between links that are intended to self-reference the e-mail and links that are intended to refer to pages online. We could special-case anchor-only links (e.g. starting with #), but that does not feel so nice. A more elegant approach would be to explicitly opt-out these links by adding a `class="no-base-url"` or something like that. Currently, `addBaseUrl` still uses regex-based replacement. For the above exception to be reliably implemented, this should be switched to an xpath-based implementation (#190). It would probably be good to change the mail-sending code (which now does dewikify_html and then addBaseUrl to convert the html string to a DomDocument node once, and then pass it to dewikify and (the rewritten) addBaseUrl, so the string -> node conversion only happens once. It might even be useful to change the `sendMail` function to build up a complete DomDocument for the message body, rather than the string concatenation it does now. This would make it even easier to convert the string message to a node.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha#237

No description provided.

Rows
Columns