PlanBCode / hypha

1 stars 8 forks source link

Notifications about page changes have wrong link #237

Open tammoterhark opened 5 years ago

tammoterhark commented 5 years ago

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 5 years ago

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, butaddBaseUrl` 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.