Merge lp:~jonas-drange/account-polld/missing-gmail-notifications into lp:~ubuntu-push-hackers/account-polld/trunk

[Jonas G. Drange]

Drop filter on category, limit the query in time (the same day). Also, fix bug where emails violating rfc2047 would not produce notifications.

A bunch of people (both real and anecdotal) have reported missing gmail notifications, and the cause of this being the “category:personal” filter we use when downloading unread emails.

This change drops that filter, so that we ask for ALL unread email in the inbox. It also specifically asks for email newer than one week. The resulting query, which you can type into your gmail search field, becomes:

    in:inbox is:unread newer_than:7d

compared to the existing one:

    in:inbox is:unread category:personal

The problem “Getting too many email notifications” is easier to understand and address for users, rather than “getting an arbitrary amount of email notifications”. The former is easily configurable in gmail, the latter is less configurable and would require a dedicated UI for this in e.g. System Settings.

Please note that account-polld will keep tabs on what e-mails it has notified you about. In other words, even if the email is unread, is in the inbox and is newer than 7 days, you won't be notified about it multiple times.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:152
http://jenkins.qa.ubuntu.com/job/account-polld-ci/96/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/11
        deb: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-armhf-ci/11/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/account-polld-vivid-i386-ci/11

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/account-polld-ci/96/rebuild

review: Approve (continuous-integration)

[Niklas Wenzel (nikwen)]

I highly support this change. I've wanted this to appear for quite some time. I'll review this as soon as possible. Thanks. :)

[Niklas Wenzel (nikwen)]

Thanks again for the MP! I really like what it does and it works well for me.

However, please see my inline comment down below. In my opinion we should really try maintaining a consistent code style. :)

Thanks again!

Cheers,
Niklas

review: Needs Fixing

[Niklas Wenzel (nikwen)]

One more thing. You'll want to use timeDelta here, not trackDelta.

review: Needs Fixing

[Bill Filler (bfiller)]

I am seeing some problems after reflashing my phone where I'm getting unread messages from many months ago. I'd suggest changing the 7day parameter and simply retreiving from the current time if this is the first time we are checking

review: Needs Fixing

[lp:~jonas-drange/account-polld/missing-gmail-notifications]

153. By Jonas G. Drange on 2016-02-23

addressing nikwen's comments

154. By Jonas G. Drange on 2016-02-23

more nikwen addressing

155. By Jonas G. Drange on 2016-02-24

1d

[Bill Filler (bfiller)]

tested the silo, on reboot I get all my unread messages for last 24hours in the messaging-menu, which I guess is ok. However they are not in the correct order (some older messages are listed first) and some are missing compared to entering this directly in gmail:
is:unread in:inbox newer_than:1d

I thought the plan was to get rid of the historical search and literally change the search to
is:unread in:inbox newer_than:[if freshBoot ? currentTime : lastTimeChecked]

Let me know if you feel differently

review: Needs Fixing

[Jonas G. Drange (jonas-drange)]

Bug 1552449 was found during testing of this MP.

[Niklas Wenzel (nikwen)]

Looking into this now. Thanks for the update. :)

First of all, would you mind updating the commit message, please? Things have changed a bit since it was set. ;)

Secondly, I think that the approach looks good but that there are probably still some issues with the implementation. The first thing which comes to my mind here is time zone stuff. What if we are in a different timezone than the Gmail server and our date is already one day ahead of the Gmail server? This can potentially result in a lot of missed emails, depending on your location!

For that reason, I believe that the old solution using newer_than:1d worked a lot better.

Besides using another selector here and potentially missing emails, I don't see any advancements compared to the older version. We have no way to check for messages which arrived exactly after our last poll, as the server does not accept hours/minutes/seconds in the query.

For that reason, I suggest reverting the last commit and going with the previous version. If I understood him correctly, the only complaints Bill made were the incorrect sorting and not being able to check for mails which arrived *exactly* after the last check, which we have found not to be possible. He seemed to be okay with how it worked.

Let me know what you think. :)

review: Needs Fixing

[Jonas G. Drange (jonas-drange)]

Thanks, Niklas.

> What if we are in a different timezone than the
> Gmail server and our date is already one day ahead of the Gmail server? This
> can potentially result in a lot of missed emails, depending on your location!

The doc [1] is not conclusive, but there are many indications that the API uses the same timezone as the gmail web site, so this should not be an issue.

[1] https://developers.google.com/gmail/api/guides/filtering

[Niklas Wenzel (nikwen)]

Does the Gmail API have as many information about us as the website? Can (and does) it find out our timezone?

As you said, there is no documentation about it, so basically we do not know if the current code works properly or not, right? Shouldn't we go the save way then?

[Niklas Wenzel (nikwen)]

Oh, and thanks for updating the commit message. :)

[Jonas G. Drange (jonas-drange)]

> Does the Gmail API have as many information about us as the website?

Yeah, it knows exactly who is using the API—the logged in gmail user.

> Can (and does) it find out our timezone?

Seems the gmail web site uses some form of algorithm to deduce the timezone, primarily using the device running the browser's tz settings. Try changing your device's tz setting and reload the gmail web site—all times should now reflect your local tz.

> As you said, there is no documentation about it, so basically we do not know
> if the current code works properly or not, right? Shouldn't we go the save way
> then?

I think this is safe, and if not, we have about 20 days to change it. Is that okay for you? I.e. if it doesn't work for most (running rc proposed), we change it to one day.

[Niklas Wenzel (nikwen)]

Ok, as I don't like uninformed guesses about whether something works or not, I tried it.

I changed my time zone from UTC+1 to UTC+9 at about 6:30 pm so that the time changed to 2:30 am of the next day. Then I rebooted and checked whether I would get notifications for e-mails I sent to myself.

I didn't.

Once I switched the time zone back to UTC+1 and restarted account-polld, I received notifications for all the messages that weren't shown before.

In my opinion this shows that we should use the previous solution using the 1d parameter.

review: Needs Fixing

[lp:~jonas-drange/account-polld/missing-gmail-notifications]

156. By Jonas G. Drange on 2016-03-08

bump

=== modified file 'plugins/gmail/gmail.go'
--- plugins/gmail/gmail.go	2015-12-21 11:51:05 +0000
+++ plugins/gmail/gmail.go	2016-03-08 14:11:43 +0000
@@ -56,6 +56,9 @@
 // trackDelta defines how old messages can be before removed from tracking
 var trackDelta = time.Duration(time.Hour * 24 * 7)
 
+// relativeTimeDelta is the same as timeDelta
+var relativeTimeDelta string = "1d"
+
 // regexp for identifying non-ascii characters
 var nonAsciiChars, _ = regexp.Compile("[^\x00-\x7F]")
 
@@ -175,7 +178,10 @@
 			if err != nil {
 				emailAddress = mangledEmail
 			}
-		} else {
+		} else if emailAddress.Name != "" {
+			// We only want the Name if the first ParseAddress
+			// call was successful. I.e. we do not want the name
+			// from a mangled email address.
 			from = emailAddress.Name
 		}
 
@@ -324,10 +330,10 @@
 
 	query := u.Query()
 
-	// only get unread, from the personal category that are in the inbox.
-	// if we want to widen the search scope we need to add more categories
-	// like: '(category:personal or category:updates or category:forums)' ...
-	query.Add("q", "is:unread category:personal in:inbox")
+	// get all unread inbox emails received after
+	// the last time we checked. If this is the first
+	// time we check, get unread emails after timeDelta
+	query.Add("q", fmt.Sprintf("is:unread in:inbox newer_than:%s", relativeTimeDelta))
 	u.RawQuery = query.Encode()
 
 	req, err := http.NewRequest("GET", u.String(), nil)