Review improvements

billing-api/api/accounts.go

@@ -11,7 +11,12 @@ import (
 	"github.com/weaveworks/service/common/zuora"
 )
 
-// GetBillingStatus returns a single overall summary of the user's account.
+// GetBillingStatus returns the billing status of the user's account, e.g. are they still in-trial, actively paying us, or was there a problem with payment.
+// - TRIAL_ACTIVE - The trial is still active
+// - TRIAL_EXPIRED - The trial has expired
+// - SUBSCRIPTION_INACTIVE - The account has a billing account but they are not actively paying us
+// - PAYMENT_ERROR - There is a problem with payment
+// - ACTIVE - The user is paying us for service and everything is okay
 func GetBillingStatus(ctx context.Context, trialInfo trial.Trial, acct *zuora.Account) (grpc.BillingStatus, string, string) {
 	// Having days left on the trial means we don't have to care about Zuora.
 	if trialInfo.Remaining > 0 {

common/grpc/client.go

@@ -10,7 +10,7 @@ import (
 	"github.com/weaveworks/common/instrument"
 )
 
-func instrumetation(errorKey string, durationCollector *instrument.HistogramCollector) []grpc.DialOption {
+func instrumentation(errorKey string, durationCollector *instrument.HistogramCollector) []grpc.DialOption {
 	interceptors := []grpc.UnaryClientInterceptor{
 		otgrpc.OpenTracingClientInterceptor(opentracing.GlobalTracer()),
 	}
@@ -50,6 +50,6 @@ func dial(urlOrHostPort string, loadBalance bool, opts ...grpc.DialOption) (*grp
 func NewInsecureConn(urlOrHostPort string, loadBalance bool, errorKey string, durationCollector *instrument.HistogramCollector) (*grpc.ClientConn, error) {
 	opts := append(
 		[]grpc.DialOption{grpc.WithInsecure()},
-		instrumetation(errorKey, durationCollector)...)
+		instrumentation(errorKey, durationCollector)...)
 	return dial(urlOrHostPort, loadBalance, opts...)
 }

users-sync/attrsync/org_attrs.go

@@ -42,19 +42,18 @@ func (c *AttributeSyncer) userOrgBillingStatus(ctx context.Context, orgs []*user
 	orgBillingStatusCount := map[string]int{}
 	for statusInt, name := range billing_grpc.BillingStatus_name {
 		status := billing_grpc.BillingStatus(statusInt)
-		attributeName := fmt.Sprintf(orgsAttrPrefix+"status_%s_total", strings.ToLower(name))
+		attributeName := fmt.Sprintf("%sstatus_%s_total", orgsAttrPrefix, strings.ToLower(name))
 		attributeNames[status] = attributeName
 		orgBillingStatusCount[attributeName] = 0
 	}
 	for _, org := range orgs {
 		resp, err := c.billingClient.GetInstanceBillingStatus(
 			ctx, &billing_grpc.InstanceBillingStatusRequest{InternalID: org.ID},
 		)
-		if err == nil {
-			orgBillingStatusCount[attributeNames[resp.BillingStatus]]++
-		} else {
+		if err != nil {
 			return nil, err
 		}
+		orgBillingStatusCount[attributeNames[resp.BillingStatus]]++
 	}
 
 	return orgBillingStatusCount, nil

users/db/db.go

@@ -52,6 +52,8 @@ type DB interface {
 	// Remove a user from an organization. If they do not exist (or are not a member of the org), return success.
 	RemoveUserFromOrganization(ctx context.Context, orgExternalID, email string) error
 
+	// List users who match a filter
+	// NB: page 0 will return all matches. Use page >= 1 for paginated responses
 	ListUsers(ctx context.Context, f filter.User, page uint64) ([]*users.User, error)
 	ListOrganizations(ctx context.Context, f filter.Organization, page uint64) ([]*users.Organization, error)
 	ListAllOrganizations(ctx context.Context, f filter.Organization, page uint64) ([]*users.Organization, error)

users/db/filter/user.go

@@ -67,12 +67,12 @@ func (s LoggedInSince) Where() squirrel.Sqlizer {
 // Squirrel has no NOT filter, so we can't have a general NOT filter either
 type NotLoggedInSince time.Time
 
-// MatchesUser users who have logged in since the given time.
+// MatchesUser users who have not logged in since the given time.
 func (s NotLoggedInSince) MatchesUser(u users.User) bool {
-	return u.LastLoginAt.Before(time.Time(s)) || u.LastLoginAt.Equal(time.Time(s))
+	return time.Time(s).After(u.LastLoginAt)
 }
 
-// Where returns the query to filter for users who have logged in since the given time.
+// Where returns the query to filter for users who have not logged in since the given time.
 func (s NotLoggedInSince) Where() squirrel.Sqlizer {
 	return squirrel.LtOrEq{"last_login_at": time.Time(s)}
 }

users/db/memory/user.go

@@ -218,6 +218,7 @@ func (u usersByCreatedAt) Swap(i, j int)      { u[i], u[j] = u[j], u[i] }
 func (u usersByCreatedAt) Less(i, j int) bool { return u[i].CreatedAt.After(u[j].CreatedAt) }
 
 // ListUsers lists users
+// NB: Does not implement pagination
 func (d *DB) ListUsers(_ context.Context, f filter.User, page uint64) ([]*users.User, error) {
 	d.mtx.Lock()
 	defer d.mtx.Unlock()