[Merge] lp:~sergiusens/account-polld/notif_max into lp:account-polld

[Manuel de la Peña]

Review: Approve

Approving with a small recommendation and with the knowledge that the duplicated logic is merged in a future branch.

Diff comments:

> === modified file 'plugins/twitter/twitter.go'
> --- plugins/twitter/twitter.go	2014-07-25 15:05:33 +0000
> +++ plugins/twitter/twitter.go	2014-07-25 15:05:33 +0000
> @@ -22,6 +22,7 @@
>  	"fmt"
>  	"net/http"
>  	"net/url"
> +	"sort"
>  	"strings"
>  
>  	"launchpad.net/account-polld/accounts"
> @@ -84,23 +85,54 @@
>  	if err := decoder.Decode(&statuses); err != nil {
>  		return nil, err
>  	}
> +
> +	sort.Sort(sort.Reverse(byStatusId(statuses)))
> +	if len(statuses) < 1 {
> +		return nil, nil
> +	}
> +	p.lastMentionId = statuses[0].Id
> +
>  	pushMsg := []plugins.PushMessage{}
> -	latestStatus := p.lastMentionId
>  	for _, s := range statuses {
>  		pushMsg = append(pushMsg, plugins.PushMessage{
>  			Notification: plugins.Notification{
>  				Card: &plugins.Card{
> -					Summary: fmt.Sprintf("Mention from @%s", s.User.ScreenName),
> +					Summary: fmt.Sprintf("@%s mentioned you", s.User.ScreenName),
>  					Body:    s.Text,
> -					Icon:    twitterIcon,
> -				},
> -			},
> -		})
> -		if s.Id > latestStatus {
> -			latestStatus = s.Id
> -		}
> -	}
> -	p.lastMentionId = latestStatus
> +					Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/statuses/%d", s.User.ScreenName, s.Id)},
> +					Icon:    twitterIcon,
> +					Persist: true,
> +					Popup:   true,
> +				},
> +				Sound:   plugins.DefaultSound(),
> +				Vibrate: plugins.DefaultVibration(),
> +			},
> +		})
> +		if len(pushMsg) == 2 {

Can you add a comment on why we do break when len == 2??

> +			break
> +		}
> +	}
> +	// Now we consolidate the remaining statuses
> +	if len(statuses) > len(pushMsg) {
> +		var screennames []string
> +		for _, s := range statuses[2:] {
> +			screennames = append(screennames, s.User.ScreenName)
> +		}
> +		pushMsg = append(pushMsg, plugins.PushMessage{
> +			Notification: plugins.Notification{
> +				Card: &plugins.Card{
> +					Summary: "Multiple more mentions",
> +					Body:    fmt.Sprintf("From %s", strings.Join(screennames, ", ")),
> +					Actions: []string{"http://mobile.twitter.com/i/connect"},
> +					Icon:    twitterIcon,
> +					Persist: true,
> +					Popup:   true,
> +				},
> +				Sound:   plugins.DefaultSound(),
> +				Vibrate: plugins.DefaultVibration(),
> +			},
> +		})
> +	}
>  	return pushMsg, nil
>  }
>  
> @@ -120,27 +152,54 @@
>  	if err := decoder.Decode(&dms); err != nil {
>  		return nil, err
>  	}
> +
> +	sort.Sort(sort.Reverse(byDMId(dms)))
> +	if len(dms) < 1 {
> +		return nil, nil
> +	}
> +	p.lastDirectMessageId = dms[0].Id
> +
>  	pushMsg := []plugins.PushMessage{}
> -	latestDM := p.lastDirectMessageId
>  	for _, m := range dms {
>  		pushMsg = append(pushMsg, plugins.PushMessage{
>  			Notification: plugins.Notification{
>  				Card: &plugins.Card{
> -					Summary: fmt.Sprintf("Direct message from @%s", m.Sender.ScreenName),
> +					Summary: fmt.Sprintf("@%s sent you a DM", m.Sender.ScreenName),
>  					Body:    m.Text,
> -					Icon:    twitterIcon,
> -					Persist: true,
> -					Popup:   true,
> -				},
> -				Sound:   plugins.DefaultSound(),
> -				Vibrate: plugins.DefaultVibration(),
> -			},
> -		})
> -		if m.Id > latestDM {
> -			latestDM = m.Id
> -		}
> -	}
> -	p.lastDirectMessageId = latestDM
> +					Actions: []string{fmt.Sprintf("http://mobile.twitter.com/%s/messages", m.Sender.ScreenName)},
> +					Icon:    twitterIcon,
> +					Persist: true,
> +					Popup:   true,
> +				},
> +				Sound:   plugins.DefaultSound(),
> +				Vibrate: plugins.DefaultVibration(),
> +			},
> +		})
> +		if len(pushMsg) == 2 {

Same as with the above, some context for the if would be nice.

> +			break
> +		}
> +	}
> +	// Now we consolidate the remaining messages
> +	if len(dms) > len(pushMsg) {
> +		var senders []string
> +		for _, m := range dms[2:] {
> +			senders = append(senders, m.Sender.ScreenName)
> +		}
> +		pushMsg = append(pushMsg, plugins.PushMessage{
> +			Notification: plugins.Notification{
> +				Card: &plugins.Card{
> +					Summary: "Multiple direct messages available",
> +					Body:    fmt.Sprintf("From %s", strings.Join(senders, ", ")),
> +					Actions: []string{"http://mobile.twitter.com/messages"},
> +					Icon:    twitterIcon,
> +					Persist: true,
> +					Popup:   true,
> +				},
> +				Sound:   plugins.DefaultSound(),
> +				Vibrate: plugins.DefaultVibration(),
> +			},
> +		})
> +	}
>  	return pushMsg, nil
>  }
>  
> @@ -183,6 +242,14 @@
>  	Text      string `json:"text"`
>  }
>  
> +// ByStatusId implements sort.Interface for []status based on
> +// the Id field.
> +type byStatusId []status
> +
> +func (s byStatusId) Len() int           { return len(s) }
> +func (s byStatusId) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
> +func (s byStatusId) Less(i, j int) bool { return s[i].Id < s[j].Id }
> +
>  // Direct message format is described here:
>  // https://dev.twitter.com/docs/api/1.1/get/direct_messages
>  type directMessage struct {
> @@ -193,6 +260,14 @@
>  	Text      string `json:"text"`
>  }
>  
> +// ByStatusId implements sort.Interface for []status based on
> +// the Id field.
> +type byDMId []directMessage
> +
> +func (s byDMId) Len() int           { return len(s) }
> +func (s byDMId) Swap(i, j int)      { s[i], s[j] = s[j], s[i] }
> +func (s byDMId) Less(i, j int) bool { return s[i].Id < s[j].Id }
> +
>  type user struct {
>  	Id         int64  `json:"id"`
>  	ScreenName string `json:"screen_name"`
> 


-- 
https://code.launchpad.net/~sergiusens/account-polld/notif_max/+merge/228320
Your team Ubuntu Phablet Team is subscribed to branch lp:account-polld.