webplatform / annotation-service

Hypothes.is’ container project to customize for notes.webplatform.org
1 stars 0 forks source link

On a spec, when there’s an annotation, send an email to the archive #20

Closed renoirb closed 9 years ago

renoirb commented 9 years ago

When we make an annotation on a spec, be it hosted on w3.org, or specs.webplatform.org, we should have an email sent to an archive mailing list.

Expected outcome

Use email specified in the spec HTML, e.g.

<a rel="reply-to"  href="mailto:public-audio-comments@w3.org">foo</a></code>

Backend code to send an email with the contents of the annotation to the archive list

tilgovi commented 9 years ago

This should be enabled. We have to check the logs if it's not working. It may be that the mail relay is rejecting messages because they come from "Username@notes.webplatform.org"?

renoirb commented 9 years ago

I’ll look at it tomorrow.

tilgovi commented 9 years ago

Closed as of beadfa17e34380d14eda7d9a52008d99407759e7

renoirb commented 9 years ago

We have an issue when we publish a spec on specs.webplatform.org. Can we make exceptions to allow archiving when on domains [*.webplatform(|staging).org, w3.org]. Thanks!

renoirb commented 9 years ago

This issue is still unresolved @tilgovi and the regex isn’t the solution.

If you remember, during TPAC I talked to you about it and also told you that nothing was getting out to the SMTP server. You explained to me that sending an email is somewhere internal (you didn’t gave any direct link) and that a try-catch clause silently prevents the email message to be sent.

renoirb commented 9 years ago

I just tried to run without the lines 44-45 and I still don’t see the emails.

tilgovi commented 9 years ago

We have seen some emails work. We know the SMTP works. There was once a problem with transactional emails and aborted transactions, but we fixed that IIRC. If things are not being sent the regex is incorrect or the markup is incorrect.

renoirb commented 9 years ago

To give more context.

When the component code taking care of sending the archive email. The email body says from which page it got the annotation.

For example, this annotation archive, at the link number 2 is from a spec hosted on w3.org. But, if you look at one (1) of the three (2) annotations (3) I made today. Only the ones hosted on w3.org went through.

And that. Even though I removed the lines 44-45 and made those 3 annotations.

The issue we are talking here is that if I add an annotation from a spec hosted on specs.webplatform.org doesn’t get sent to the archive at all.

If the archiver worked, we should have got the email from this annotation from that spec on specs.webplatform.org too.

nickstenning commented 9 years ago

Cross-posting from our email thread.

Removing lines 44-45 is exactly the opposite of what you need to do. The problem is almost certainly not the regular expression, but what it's being asked to match.

valid_recipients() takes a list of email addresses, and a URL. Presumably it's supposed to return all the email addresses if the domain in the URL matches the whitelist regex. But at the moment it's checking the whole URL, not just the domain part.

Please see the patch below, which should address this issue.

diff --git a/notes_server/archiver.py b/notes_server/archiver.py
index 530354d..d9f6ec3 100644
--- a/notes_server/archiver.py
+++ b/notes_server/archiver.py
@@ -40,10 +40,10 @@ def reply_to(uri):

 def valid_recipients(unvalidated, uri):
-    url_struct = urlparse(uri)
-    if re.search(r'(\.|^)(w3|webplatform(staging)?)\.org$', uri):
+    domain = urlparse(uri).hostname
+    if re.search(r'(\.|^)(w3|webplatform(staging)?)\.org$', domain):
         return unvalidated
-    domain = re.sub(r'^(www\.)?', '', url_struct.hostname)
+    domain = re.sub(r'^(www\.)?', '', domain)
     domain_re = '@{}$'.format(re.escape(domain))
     return [
         email
tilgovi commented 9 years ago

Let us know when you test this and whether we're good now.

renoirb commented 9 years ago

Hi.

Its deployed now and running on notes.webplatform.org. Also i’m building a VM as notes.webplatformstaging.org with the same version.

renoirb commented 9 years ago

It should work. Let’s wait that @shepazu tries it before closing.

renoirb commented 9 years ago

Fixed.