[Merge] lp:~sergiusens/account-polld/gmail_plugin into lp:account-polld
James Henstridge
james.henstridge at canonical.com
Mon Jul 21 04:22:16 UTC 2014
Review: Needs Fixing
I've left some inline comments about the branch.
I also had a look around the API docs, and wonder if it would make sense to base the updates on the history API:
https://developers.google.com/gmail/api/v1/reference/users/history/list
This would enable requesting incremental changes to the mailbox rather than having to do a full sync each time.
Also, it might be worth using the threads API rather than the messages ones: the threads.list API returns history IDs, and GMail's desktop web interface uses thread IDs in its URLs, so it might be more helpful in forming a URL to send the user to.
Diff comments:
> === added file 'plugins/gmail/api.go'
> --- plugins/gmail/api.go 1970-01-01 00:00:00 +0000
> +++ plugins/gmail/api.go 2014-07-21 00:16:15 +0000
> @@ -0,0 +1,105 @@
> +/*
> + Copyright 2014 Canonical Ltd.
> + Authors: Sergio Schvezov <sergio.schvezov at canonical.com>
> +
> + This program is free software: you can redistribute it and/or modify it
> + under the terms of the GNU General Public License version 3, as published
> + by the Free Software Foundation.
> +
> + This program is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranties of
> + MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
> + PURPOSE. See the GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License along
> + with this program. If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +package gmail
> +
> +import (
> + "fmt"
> +
> + "launchpad.net/account-polld/plugins"
> +)
> +
> +type pushes map[string]plugins.PushMessage
> +
> +// messageList holds a response to call to Users.messages: list
> +// defined in https://developers.google.com/gmail/api/v1/reference/users/messages/list
> +type messageList struct {
> + // Messages holds a list of message.
> + Messages []message `json:"messages"`
> + // NextPageToken is used to retrieve the next page of results in the list.
> + NextPageToken string `json:"nextPageToken"`
> + // ResultSizeEstimage is the estimated total number of results.
> + ResultSizeEstimage uint64 `json:"resultSizeEstimate"`
> +}
> +
> +// message holds a partial response for a Users.messages.
> +// The full definition of a message is defined in
> +// https://developers.google.com/gmail/api/v1/reference/users/messages#resource
> +type message struct {
> + // Id is the immutable ID of the message.
> + Id string `json:"id"`
> + // ThreadId is the ID of the thread the message belongs to.
> + ThreadId string `json:"threadId"`
> + // Snippet is a short part of the message text. This text is
> + // used for the push message summary.
> + Snippet string `json:"snippet"`
> + // Payload represents the message payload.
> + Payload payload `json:"payload"`
> +}
> +
> +func (m message) String() string {
> + return fmt.Sprintf("Id: %d, snippet: '%s'\n", m.Id, m.Snippet[:10])
> +}
> +
> +// ById implements sort.Interface for []message based on
> +// the Id field.
> +type byId []message
> +
> +func (m byId) Len() int { return len(m) }
> +func (m byId) Swap(i, j int) { m[i], m[j] = m[j], m[i] }
> +func (m byId) Less(i, j int) bool { return m[i].Id < m[j].Id }
> +
> +// payload represents the message payload.
> +type payload struct {
> + Headers []messageHeader `json:"headers"`
> +}
> +
> +func (p *payload) mapHeaders() map[string]string {
> + headers := make(map[string]string)
> + for _, hdr := range p.Headers {
> + headers[hdr.Name] = hdr.Value
> + }
> + return headers
> +}
> +
> +// messageHeader represents the message headers.
> +type messageHeader struct {
> + Name string `json:"name"`
> + Value string `json:"value"`
> +}
> +
> +type errorResp struct {
> + Err struct {
> + Code uint64 `json:"code"`
> + Message string `json:"message"`
> + Errors []struct {
> + Domain string `json:"domain"`
> + Reason string `json:"reason"`
> + Message string `json:"message"`
> + } `json:"errors"`
> + } `json:"error"`
> +}
> +
> +func (err *errorResp) Error() string {
> + return fmt.Sprintln("backend response:", err.Err.Message)
Use Sprint() here to avoid the unwanted new line at the end of the string.
> +}
> +
> +const (
> + hdr_DATE = "Date"
> + hdr_SUBJECT = "Subject"
> + hdr_FROM = "From"
The usual Go naming convention here would be hdrDate, hdrSubject and hdrFrom.
> +)
>
> === renamed file 'plugins/gmail/plugin.go' => 'plugins/gmail/gmail.go'
> --- plugins/gmail/plugin.go 2014-07-17 18:35:36 +0000
> +++ plugins/gmail/gmail.go 2014-07-21 00:16:15 +0000
> @@ -18,33 +18,202 @@
> package gmail
>
> import (
> + "encoding/json"
> + "fmt"
> + "net/http"
> + "net/url"
> + "sort"
> +
> "launchpad.net/account-polld/accounts"
> "launchpad.net/account-polld/plugins"
> )
>
> const APP_ID = "com.ubuntu.developer.webapps.webapp-gmail_webapp-gmail"
>
> +var baseUrl, _ = url.Parse("https://www.googleapis.com/gmail/v1/users/me/")
> +
> type GmailPlugin struct {
> + // reportedIds holds the messages that have already been notified. This
> + // approach is taken against timestamps as it avoids needing to call
> + // get on the message.
> + //
> + // TODO determine if persisting the list to avoid renotification on reboot.
> + reportedIds []string
It will probably be more efficient to use a map[string]bool here, since you are treating it like a set. It would let you reduce your reported() method down to "return p.reportedIds[id]"
> }
>
> func New() *GmailPlugin {
> return &GmailPlugin{}
> }
>
> -func (p *GmailPlugin) Poll(authData *accounts.AuthData) ([]plugins.PushMessage, error) {
> - // Stub message
> - n := plugins.PushMessage{
> - Notification: plugins.Notification{
> - Card: &plugins.Card{
> - Summary: "hello",
> - Persist: true,
> - Popup: true,
> - },
> - },
> - }
> - return []plugins.PushMessage{n}, nil
> -}
> -
> func (p *GmailPlugin) ApplicationId() plugins.ApplicationId {
> return plugins.ApplicationId(APP_ID)
> }
> +
> +func (p *GmailPlugin) Poll(authData *accounts.AuthData) ([]plugins.PushMessage, error) {
> + resp, err := p.requestMessageList(authData.AccessToken)
> + if err != nil {
> + return nil, err
> + }
> + messages, err := p.parseMessageListResponse(resp)
> + if err != nil {
> + return nil, err
> + }
> + for i := range messages {
> + resp, err := p.requestMessage(messages[i].Id, authData.AccessToken)
One area where this could be improved would be to use the batching API:
https://developers.google.com/gmail/api/guides/batch
This would let us request the details of every message we're interested in with a single round trip (well, 100 messages, since that is the limit for batching). The standard library mime/multipart package would help with constructing request bodies and parsing responses.
This is decidedly non-trivial though, so probably best to mark as a TODO.
> + if err != nil {
> + return nil, err
> + }
> + messages[i], err = p.parseMessageResponse(resp)
> + if err != nil {
> + return nil, err
> + }
> + }
> + return p.createNotifications(messages)
> +}
> +
> +func (p *GmailPlugin) reported(id string) bool {
> + sort.Strings(p.reportedIds)
> + if i := sort.SearchStrings(p.reportedIds, id); i < len(p.reportedIds) {
> + return p.reportedIds[i] == id
> + }
> + return false
> +}
> +
> +func (p *GmailPlugin) createNotifications(messages []message) ([]plugins.PushMessage, error) {
> + pushMsgMap := make(pushes)
> +
> + for _, msg := range messages {
> + hdr := msg.Payload.mapHeaders()
> + if _, ok := pushMsgMap[msg.ThreadId]; ok {
> + pushMsgMap[msg.ThreadId].Notification.Card.Summary += fmt.Sprintf(", %s", hdr[hdr_FROM])
> + } else {
> + pushMsgMap[msg.ThreadId] = plugins.PushMessage{
> + Notification: plugins.Notification{
> + Card: &plugins.Card{
> + Summary: fmt.Sprintf("Message \"%s\" from %s", hdr[hdr_SUBJECT], hdr[hdr_FROM]),
> + Body: msg.Snippet,
> + // TODO this is a placeholder, Actions aren't fully defined yet and opening
> + // multiple inboxes has issues.
> + Actions: []string{"https://mail.google.com/mail/u/0/?pli=1#inbox/" + msg.ThreadId},
> + Popup: true,
> + Persist: true,
> + },
> + },
> + }
> + }
> + }
> + var pushMsg []plugins.PushMessage
> + for _, v := range pushMsgMap {
> + pushMsg = append(pushMsg, v)
> + }
> + return pushMsg, nil
> +}
> +
> +func (p *GmailPlugin) parseMessageListResponse(resp *http.Response) ([]message, error) {
> + defer resp.Body.Close()
> + decoder := json.NewDecoder(resp.Body)
> +
> + if resp.StatusCode != http.StatusOK {
> + var errResp errorResp
> + if err := decoder.Decode(&errResp); err != nil {
> + return nil, err
> + }
> + return nil, &errResp
> + }
> +
> + var messages messageList
> + if err := decoder.Decode(&messages); err != nil {
> + return nil, err
> + }
> +
> + filteredMsg := p.messageListFilter(messages.Messages)
> +
> + return filteredMsg, nil
> +}
> +
> +// messageListFilter returns a subset of unread messages where the subset
> +// depends on not being in reportedIds. Before returning, reportedIds is
> +// updated with the new list of unread messages.
> +func (p *GmailPlugin) messageListFilter(messages []message) []message {
> + sort.Sort(byId(messages))
> + var reportMsg []message
> + var ids []string
> +
> + for _, msg := range messages {
> + if !p.reported(msg.Id) {
> + reportMsg = append(reportMsg, msg)
> + }
> + ids = append(ids, msg.Id)
> + }
> + p.reportedIds = ids
> + return reportMsg
> +}
> +
> +func (p *GmailPlugin) parseMessageResponse(resp *http.Response) (message, error) {
This function might be clearer with named return variables (no need for "message{}" in the error paths). That's more of a personal choice though, so I'll leave it up to you.
> + defer resp.Body.Close()
> + decoder := json.NewDecoder(resp.Body)
> +
> + if resp.StatusCode != http.StatusOK {
> + var errResp errorResp
> + if err := decoder.Decode(&errResp); err != nil {
> + return message{}, err
> + }
> + return message{}, &errResp
> + }
> +
> + var msg message
> + if err := decoder.Decode(&msg); err != nil {
> + return message{}, err
> + }
> +
> + return msg, nil
> +}
> +
> +func (p *GmailPlugin) requestMessage(id, accessToken string) (*http.Response, error) {
> + u, err := baseUrl.Parse("messages/" + id)
> + if err != nil {
> + return nil, err
> + }
> +
> + query := u.Query()
> + // only request specific fields
> + query.Add("fields", "snippet,threadId,id,payload")
>From the payload, we only seem to be interested in the headers, so consider changing this to "snippet,threadId,id,payload/headers". That will avoid downloading the message body, which could be quite large.
> + // get the full message to get From and Subject from headers
> + query.Add("format", "full")
> + u.RawQuery = query.Encode()
> +
> + req, err := http.NewRequest("GET", u.String(), nil)
> + if err != nil {
> + return nil, err
> + }
> + req.Header.Set("Authorization", "Bearer "+accessToken)
> +
> + client := &http.Client{}
> + return client.Do(req)
Is there any reason why you are creating a new HTTP client here? Just call http.Do or http.DefaultClient.Do, which should let us reuse the connection.
> +}
> +
> +func (p *GmailPlugin) requestMessageList(accessToken string) (*http.Response, error) {
> + u, err := baseUrl.Parse("messages")
> + if err != nil {
> + return nil, err
> + }
> +
> + query := u.Query()
> +
> + // only get unread
> + query.Add("q", "is:unread")
> + // from the INBOX
> + query.Add("labelIds", "INBOX")
> + // from the Personal category.
> + query.Add("labelIds", "CATEGORY_PERSONAL")
> + u.RawQuery = query.Encode()
> +
> + req, err := http.NewRequest("GET", u.String(), nil)
> + if err != nil {
> + return nil, err
> + }
> + req.Header.Set("Authorization", "Bearer "+accessToken)
> +
> + client := &http.Client{}
> + return client.Do(req)
Same comment here about creating a new client.
> +}
>
--
https://code.launchpad.net/~sergiusens/account-polld/gmail_plugin/+merge/227314
Your team Ubuntu Phablet Team is subscribed to branch lp:account-polld.
More information about the Ubuntu-reviews
mailing list