After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768820 - No support for mail delivery notifications
No support for mail delivery notifications
Status: RESOLVED INCOMPLETE
Product: geary
Classification: Other
Component: client+engine
0.11.x
Other All
: Normal enhancement
: ---
Assigned To: Geary Maintainers
Geary Maintainers
Depends on: 768263
Blocks: 775570
 
 
Reported: 2016-07-14 20:24 UTC by gszymaszek
Modified: 2019-01-15 06:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Disposition Notification Support - Patch 1/9 (2.63 KB, patch)
2016-12-02 15:39 UTC, Ronan Arraes
needs-work Details | Review
Disposition Notification Support - Patch 2/9 (2.71 KB, patch)
2016-12-02 15:40 UTC, Ronan Arraes
reviewed Details | Review
Disposition Notification Support - Patch 3/9 (10.07 KB, patch)
2016-12-02 15:40 UTC, Ronan Arraes
needs-work Details | Review
Disposition Notification Support - Patch 4/9 (2.82 KB, patch)
2016-12-02 15:40 UTC, Ronan Arraes
reviewed Details | Review
Disposition Notification Support - Patch 5/9 (8.08 KB, patch)
2016-12-02 15:41 UTC, Ronan Arraes
needs-work Details | Review
Disposition Notification Support - Patch 6/9 (10.14 KB, patch)
2016-12-02 15:41 UTC, Ronan Arraes
reviewed Details | Review
Disposition Notification Support - Patch 7/9 (8.54 KB, patch)
2016-12-02 15:41 UTC, Ronan Arraes
reviewed Details | Review
Disposition Notification Support - Patch 8/9 (1.30 KB, patch)
2016-12-02 15:41 UTC, Ronan Arraes
reviewed Details | Review
Disposition Notification Support - Patch 9/9 (5.80 KB, patch)
2016-12-02 15:42 UTC, Ronan Arraes
reviewed Details | Review
Request Read Receipt Option (41.45 KB, image/png)
2016-12-02 15:43 UTC, Ronan Arraes
  Details
Request Read Receipt Option (13.54 KB, image/png)
2016-12-02 15:45 UTC, Ronan Arraes
  Details
Disposition Notification Infobar (6.49 KB, image/png)
2016-12-02 15:45 UTC, Ronan Arraes
  Details

Description gszymaszek 2016-07-14 20:24:58 UTC
Sending an email from (for example) Evolution, with MDN request enabled, causes it to append "Disposition-Notification-To" header to that message. When I receive it in Geary, it should ask me whether I'd like to confirm mail delivery to the sender or not, but it's not asking.

Secondly, I'd like Geary to send MDN requests within new emails, but it seems there's no such option.

There's also "Return-Receipt-To" header (sent by Roundcube), but I'm not aware of any difference between them.
Comment 1 Michael Gratton 2016-07-15 00:43:58 UTC
Another alternative to investigate is DSN - RFC 3464.

I'd consider accepting patches for this, but I am concerned it will not receive widespread use and hence get little testing - thus be prone to breaking as the codebase changes over time. It would need to default to off to avoid being abused by spammers, and I assume demand for it is low since there hasn't been any request for MDN/DSN from users previously. As a result, I'm somewhat doubtful about supporting it. Maybe send an email to the mailing list to see if there is actually more widespread demand?

Any patch accepted would probably need to involve a single, per-account pref turning the whole thing on or off. When enabled, sending reasonable receipts requests by default with perhaps some UI in the composition window for adjusting that, and having something like the show-remote-images InfoBar on messages received to allow the user to send receipts. Care would need to be taken to not send receipts for known spam or other untrusted sources.
Comment 2 Ronan Arraes 2016-11-28 22:28:38 UTC
Hi!

I started to use Geary a while ago to replace Evolution (which I kind of overkill for what I need). However, this feature is precisely the only feature that Geary lacks and that I really need to use (well, and OAuth2 support, but this can be overcame). This is very useful and common in corporate environment.

My suggestion is to take this approach very simple:

If the sender request confirmation, then show a bar telling the user with a button to send the confirmation. Hence, there is no need to worry about spammers and etc. because the user will always choose if he/she wants to send or not the confirmation to that specific sender.

However, I am not sure about the receipt e-mail. I think there isn't a global pattern. Each e-mail client seems to send a different one...
Comment 3 Michael Gratton 2016-11-29 01:27:54 UTC
Using an infobar to prompt the user is probably the only reasonable way to handle sending read receipts. It would need to work like the existing remote images infobar, as in give the user the option to Send Receipt, Always Send For Sender, and Close, with the Always Send... preference being stored in the database if chosen.

We'd probably also want an in-app notification to pop up for a few seconds, allowing the user to cancel sending both manually approved and automatically sent receipts, something like: "Sending message read receipt to Jane Bloggs   [ Don't Send ]"

Further, care would need to be taken to not prompt or automatically send receipts for messages in the Junk folder, even when enabled for the sender. This may require some infrastructure from Bug 768263 to implement, so I'm going to mark this as depending on that for now.
Comment 4 Ronan Arraes 2016-12-02 15:39:15 UTC
Hi guys!

I am posting the 7 patches to implement this feature. Here some considerations:

1) This is not the full implementation of RFC 3798. I only treat the header field `Disposition-Notification-To`, which seems to be what most e-mail clients are doing.

2) If the user send a read receipt, I add a IMAP flag `$MDNSent` according to RFC 3503.

3) There is a TODO: I could not figured out how can I add the aforementioned IMAP flag only when the MDN was actually sent. Now, the flag is added when the used hit the button "Notify Sender".

4) I am attaching 2 snapshots: the infobar to tell the user that the sender wants read confirmation and the pop-up menu with the "Request Read Receipt" option.

5) I had to change somethings related to how Geary send messages. For example, I had to create a new function called `send_rfc822_async` because I had access only to the `send_email_asycn` that uses a ComposedEmail as input. This turned out to be very inconvenient to send the MDN, which has a very specific format. So, I just create a RFC822.Message and send them using the new function. I hope everything is fine :)

6) I tested and compared the output with the one provided by Evolution. Everything seems to be working fine.

Regards,
Ronan Arraes
Comment 5 Ronan Arraes 2016-12-02 15:39:53 UTC
Created attachment 341253 [details] [review]
Disposition Notification Support - Patch 1/9
Comment 6 Ronan Arraes 2016-12-02 15:40:12 UTC
Created attachment 341254 [details] [review]
Disposition Notification Support - Patch 2/9
Comment 7 Ronan Arraes 2016-12-02 15:40:27 UTC
Created attachment 341255 [details] [review]
Disposition Notification Support - Patch 3/9
Comment 8 Ronan Arraes 2016-12-02 15:40:49 UTC
Created attachment 341256 [details] [review]
Disposition Notification Support - Patch 4/9
Comment 9 Ronan Arraes 2016-12-02 15:41:05 UTC
Created attachment 341257 [details] [review]
Disposition Notification Support - Patch 5/9
Comment 10 Ronan Arraes 2016-12-02 15:41:19 UTC
Created attachment 341258 [details] [review]
Disposition Notification Support - Patch 6/9
Comment 11 Ronan Arraes 2016-12-02 15:41:36 UTC
Created attachment 341260 [details] [review]
Disposition Notification Support - Patch 7/9
Comment 12 Ronan Arraes 2016-12-02 15:41:53 UTC
Created attachment 341261 [details] [review]
Disposition Notification Support - Patch 8/9
Comment 13 Ronan Arraes 2016-12-02 15:42:06 UTC
Created attachment 341262 [details] [review]
Disposition Notification Support - Patch 9/9
Comment 14 Ronan Arraes 2016-12-02 15:43:09 UTC
Created attachment 341264 [details]
Request Read Receipt Option
Comment 15 Ronan Arraes 2016-12-02 15:45:28 UTC
Created attachment 341265 [details]
Request Read Receipt Option
Comment 16 Ronan Arraes 2016-12-02 15:45:51 UTC
Created attachment 341266 [details]
Disposition Notification Infobar
Comment 17 Michael Gratton 2016-12-05 14:54:51 UTC
Review of attachment 341253 [details] [review]:

::: src/engine/rfc822/rfc822-message.vala
@@ +103,3 @@
         message.set_date_as_string(this.date.serialize());
         if (message_id != null)
+        {

Style nit: We cuddle our braces, rather than putting them on new lines.
Comment 18 Michael Gratton 2016-12-05 14:54:55 UTC
Review of attachment 341254 [details] [review]:

Probably would have been reasonable to squash this into the last patch, and combine the patch descriptions together.
Comment 19 Michael Gratton 2016-12-05 14:55:00 UTC
Review of attachment 341255 [details] [review]:

::: ui/conversation-message.ui
@@ +590,3 @@
                 </child>
+                <child>
+                  <object class="GtkInfoBar" id="disposition_notification_infobar">

The HIG suggests using "Status message + longer message" for GtkInfoBars: https://developer.gnome.org/hig/stable/info-bars.html.en, so something like this would be good:

> Read receipt requested
> The sender requested to be notified when this message was read

There's some examples of how to implement this this in conversation-message.ui

Bonus points would be awarded for providing a mechanism to display who will receive the MDN. If it's the same as the from/sender address header, then it could be left out, but if not or the D-N-T header contains multiple addresses, maybe having a menu button with a popover containing the list of all of them might be good?
Comment 20 Michael Gratton 2016-12-05 14:55:28 UTC
Review of attachment 341256 [details] [review]:

Looks good.
Comment 21 Michael Gratton 2016-12-05 14:55:40 UTC
Review of attachment 341257 [details] [review]:

::: src/client/application/geary-controller.vala
@@ +2985,3 @@
       */
     private async void on_send_disposition_notification(Geary.RFC822.Message message) {
+        // Disposition Notification message subject.

For both the MDN subject line and human-readable message body below, how do they compare to e.g. what other mailers (Evo, TBird, Gmail, etc) send, do you know?

Do they need to be translatable? If so, do they need to be translated into the same language as the original message, instead of the current desktop login sesssion?

@@ +2987,3 @@
+        // Disposition Notification message subject.
+        string mdn_subject = "Disposition notification to \"" +
+            message.subject.to_string() + "\"";

If nothing else, this might want to read something like the UI terminology, as used in the InfoBar:

"Read receipt for: %s"

@@ +2992,3 @@
+        StringBuilder mdn_body = new StringBuilder();
+        mdn_body.printf(
+            "The message sent on %s to %s with subject \"%s\" has been displayed. " +

Same here, "notified as read" instead of "displayed"?

@@ +3005,3 @@
+                new Geary.RFC822.MailboxAddresses.single(message.disposition_notification_to),
+                mdn_subject, mdn_body.str,
+                GearyApplication.PRGNAME + "/" + GearyApplication.VERSION,

We generate Geary's MUA somewhere else as well, can we reuse that code? If not, both here and there should be reworked so as call the same function, so it's only generated in one place.

::: src/engine/rfc822/rfc822-message.vala
@@ +351,3 @@
+                "Disposition: manual-action/MDN-sent-manually; displayed\r\n" +
+                ""
+                );

Since we're basically writing RFC822 headers here, is there anyway to use existing GMime infrastructure for doing this, rather than just doing string formatting? Just adding the headers to mdn_part.append_header() maybe?

@@ +385,3 @@
+
+            GMime.Object? attachment_part =
+                coalesce_parts(attachment_parts, "mixed");

Note that as-is, this will simply return the mdn_part object - you probably don't need this or the attachment_parts list.

@@ +393,3 @@
+            GMime.Object? main_part = coalesce_parts(
+                main_parts,
+                "report; report-type=disposition-notification;"

Best to avoid encoding params in the string like this, use the GMime.ContentType object instead to set the param from name and value. Maybe generate the multipart/report part manually, since again coalesce_parts() isn't appropriate here.

@@ +397,3 @@
+
+            this.message.set_mime_part(main_part);
+        } catch (Error e) {

Rather than wrapping all this in a huge try block, just simply declare that the method throws an Error and let the caller handle it.
Comment 22 Michael Gratton 2016-12-05 14:56:07 UTC
Review of attachment 341258 [details] [review]:

::: src/client/application/geary-controller.vala
@@ -3003,3 @@
                 new DateTime.now_local(),
                 new Geary.RFC822.MailboxAddresses.single(current_account.information.primary_mailbox),
-                new Geary.RFC822.MailboxAddresses.single(message.disposition_notification_to),

By my reading the RFC is a bit underspecified about this: In the case of D-N-T header having multiple addresses, should they all receive the MDN?

::: src/engine/api/geary-composed-email.vala
@@ +38,3 @@
     public string? body_html { get; set; default = null; }
     public string? mailer { get; set; default = null; }
+    public RFC822.MailboxAddresses? disposition_notification_to { get; set; default = null; }

This would have been good to use `git rebase -i` and squash into patch #2, so I could have avoided making then deleting a comment about the RFC requiring it to be a list. ;)

::: ui/composer-menus.ui
@@ +60,3 @@
+        <attribute name="action">cmp.request-read-receipt</attribute>
+      </item>
+    </section>

These could probably use a tooltip, but I don't think we can specify one using GMenu?
Comment 23 Michael Gratton 2016-12-05 14:56:17 UTC
Review of attachment 341260 [details] [review]:

::: src/client/conversation-viewer/conversation-email.vala
@@ +769,3 @@
+        // XXX We should wait for the email-sent signal to add the flag
+        // MDN_SENT.
+        mark_email(Geary.EmailFlags.MDN_SENT, null);

Should this be done in geary-controller instead then?
Comment 24 Michael Gratton 2016-12-05 14:56:28 UTC
Review of attachment 341261 [details] [review]:

This would have been good to squash into patch #5 using `git rebase -i`, to save having this trivial patch.
Comment 25 Michael Gratton 2016-12-05 14:56:35 UTC
Review of attachment 341262 [details] [review]:

This also would have been good to squash into the appropriate previous patches using `git rebase -i`, again to save having a patch with no functional changes.
Comment 26 Michael Gratton 2016-12-05 15:12:48 UTC
Ronan, thanks for these patches!

A few general themes:

1. The approaches you've taken are in general excellent, nice work!
2. While I very much appreciated having a patch set that took me through the progression of the work, as noted in a few places it could have been a fair bit smaller with some judicious use of `git rebase -i`, saving both you and me a bit of time and effort.
3. This is mostly just a code+ui review for the moment, I haven't had time to completely read the RFC yet and in particular, I want to make sure the conformance & use reqs (section 5) and security concerns (Section 6) are appropriately addressed. For example: the patch set as-is doesn't look for required parameters in the Disposition-Notification-Options header and send an MDN with a disposition of "failed" if any are found.
Comment 27 Ronan Arraes 2016-12-07 20:44:24 UTC
Hi Michael,

I am not sure what should I do now. Unfortunately, as I said in some commit message, this is not a full implementation of that RFC. I simply avoided to look at Disposition-Notificaiton-Options because I don't know any email software that can send messages with it. Hence, I cannot test it :(
Comment 28 Michael Gratton 2016-12-07 23:43:45 UTC
Hey Ronan,

Well, a good start would be to address the issues noted in the reviews above and prep a new patch set.

Regarding testing the D-N-O header, you have a few options. If it were me, I'd generate some RFC822-formatted messages addressed to myself that included different versions of that header, and send them by talking to a friendly SMTP server using telnet. Another option would be to add a temporary kludge to Geary so that it always includes the header, and then you can just use Geary to send yourself some test messages that include the header.

Ideally we would have a unit test framework already in place you could use to test this sort of thing, but that's still on the TODO list (i.e. Bug 713387).
Comment 29 Michael Gratton 2019-01-15 06:24:53 UTC
Hi Ronan, thanks for your interest in this feature. I'm going to close this as incomplete since it's been a while since I have heard from you.

If you are interested picking it back up, please jump in and lodge a merge request over at https://gitlab.gnome.org/GNOME/geary