Refactor user query and return user display name (#1679)

## Description

Move user discovery into a `discovery` package. Also use that to return user display name from `m365.Users()` call.

This will also allow us to remove `Users` from the GC struct going forward.

If this pattern works - will follow up with a similar change for `Sites()`.

## Type of change

<!--- Please check the type of change your PR introduces: --->
- [x] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 CI/Deployment
- [ ] 🐹 Trivial/Minor

## Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

src/internal/connector/discovery/discovery.go

@@ -0,0 +1,83 @@
+package discovery
+
+import (
+	"context"
+
+	msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core"
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	msuser "github.com/microsoftgraph/msgraph-sdk-go/users"
+	"github.com/pkg/errors"
+
+	"github.com/alcionai/corso/src/internal/connector/graph"
+	"github.com/alcionai/corso/src/internal/connector/support"
+)
+
+const (
+	userSelectID            = "id"
+	userSelectPrincipalName = "userPrincipalName"
+	userSelectDisplayName   = "displayName"
+)
+
+func Users(ctx context.Context, gs graph.Service, tenantID string) ([]models.Userable, error) {
+	users := make([]models.Userable, 0)
+
+	options := &msuser.UsersRequestBuilderGetRequestConfiguration{
+		QueryParameters: &msuser.UsersRequestBuilderGetQueryParameters{
+			Select: []string{userSelectID, userSelectPrincipalName, userSelectDisplayName},
+		},
+	}
+
+	response, err := gs.Client().Users().Get(ctx, options)
+	if err != nil {
+		return nil, errors.Wrapf(
+			err,
+			"retrieving resources for tenant %s: %s",
+			tenantID,
+			support.ConnectorStackErrorTrace(err),
+		)
+	}
+
+	iter, err := msgraphgocore.NewPageIterator(response, gs.Adapter(),
+		models.CreateUserCollectionResponseFromDiscriminatorValue)
+	if err != nil {
+		return nil, errors.Wrap(err, support.ConnectorStackErrorTrace(err))
+	}
+
+	var iterErrs error
+
+	callbackFunc := func(item interface{}) bool {
+		u, err := parseUser(item)
+		if err != nil {
+			iterErrs = support.WrapAndAppend("discovering users: ", err, iterErrs)
+			return true
+		}
+
+		users = append(users, u)
+
+		return true
+	}
+
+	if err := iter.Iterate(ctx, callbackFunc); err != nil {
+		return nil, errors.Wrap(err, support.ConnectorStackErrorTrace(err))
+	}
+
+	return users, iterErrs
+}
+
+// parseUser extracts information from `models.Userable` we care about
+func parseUser(item interface{}) (models.Userable, error) {
+	m, ok := item.(models.Userable)
+	if !ok {
+		return nil, errors.New("iteration retrieved non-User item")
+	}
+
+	if m.GetId() == nil {
+		return nil, errors.Errorf("no ID for User")
+	}
+
+	if m.GetUserPrincipalName() == nil {
+		return nil, errors.Errorf("no principal name for User: %s", *m.GetId())
+	}
+
+	return m, nil
+}

src/internal/connector/discovery/discovery_test.go

@@ -0,0 +1,73 @@
+package discovery
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	"github.com/stretchr/testify/suite"
+)
+
+type DiscoverySuite struct {
+	suite.Suite
+}
+
+func TestDiscoverySuite(t *testing.T) {
+	suite.Run(t, new(DiscoverySuite))
+}
+
+func (suite *DiscoverySuite) TestParseUser() {
+	t := suite.T()
+
+	name := "testuser"
+	email := "testuser@foo.com"
+	id := "testID"
+	user := models.NewUser()
+	user.SetUserPrincipalName(&email)
+	user.SetDisplayName(&name)
+	user.SetId(&id)
+
+	tests := []struct {
+		name    string
+		args    interface{}
+		want    models.Userable
+		wantErr bool
+	}{
+		{
+			name:    "Invalid type",
+			args:    string("invalid type"),
+			wantErr: true,
+		},
+		{
+			name:    "No ID",
+			args:    models.NewUser(),
+			wantErr: true,
+		},
+		{
+			name: "No user principal name",
+			args: func() *models.User {
+				u := models.NewUser()
+				u.SetId(&id)
+				return u
+			}(),
+			wantErr: true,
+		},
+		{
+			name: "Valid User",
+			args: user,
+			want: user,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := parseUser(tt.args)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("parseUser() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("parseUser() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

src/internal/connector/exchange/exchange_service_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 	"time"
 
-	absser "github.com/microsoft/kiota-abstractions-go/serialization"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -252,12 +251,6 @@ func (suite *ExchangeServiceSuite) TestGraphQueryFunctions() {
 			name:     "GraphQuery: Get All Folders",
 			function: GetAllFolderNamesForUser,
 		},
-		{
-			name: "GraphQuery: Get All Users",
-			function: func(ctx context.Context, gs graph.Service, toss string) (absser.Parsable, error) {
-				return GetAllUsersForTenant(ctx, gs)
-			},
-		},
 		{
 			name:     "GraphQuery: Get All ContactFolders",
 			function: GetAllContactFolderNamesForUser,

src/internal/connector/exchange/query_options.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	abs "github.com/microsoft/kiota-abstractions-go"
-	msuser "github.com/microsoftgraph/msgraph-sdk-go/users"
 	mscalendars "github.com/microsoftgraph/msgraph-sdk-go/users/item/calendars"
 	mscevents "github.com/microsoftgraph/msgraph-sdk-go/users/item/calendars/item/events"
 	mscontactfolder "github.com/microsoftgraph/msgraph-sdk-go/users/item/contactfolders"
@@ -391,22 +390,6 @@ func optionsForContacts(moreOps []string) (*mscontacts.ContactsRequestBuilderGet
 	return options, nil
 }
 
-func optionsForUsers(moreOps []string) (*msuser.UsersRequestBuilderGetRequestConfiguration, error) {
-	selecting, err := buildOptions(moreOps, users)
-	if err != nil {
-		return nil, err
-	}
-
-	requestParams := &msuser.UsersRequestBuilderGetQueryParameters{
-		Select: selecting,
-	}
-	options := &msuser.UsersRequestBuilderGetRequestConfiguration{
-		QueryParameters: requestParams,
-	}
-
-	return options, nil
-}
-
 // buildOptions - Utility Method for verifying if select options are valid for the m365 object type
 // @return is a pair. The first is a string literal of allowable options based on the object type,
 // the second is an error. An error is returned if an unsupported option or optionIdentifier was used

src/internal/connector/exchange/service_query.go

@@ -74,18 +74,6 @@ func GetAllContactFolderNamesForUser(ctx context.Context, gs graph.Service, user
 	return gs.Client().UsersById(user).ContactFolders().Get(ctx, options)
 }
 
-// GetAllUsersForTenant makes a GraphQuery request retrieving all the users in the tenant.
-func GetAllUsersForTenant(ctx context.Context, gs graph.Service) (absser.Parsable, error) {
-	selecting := []string{"userPrincipalName"}
-
-	options, err := optionsForUsers(selecting)
-	if err != nil {
-		return nil, err
-	}
-
-	return gs.Client().Users().Get(ctx, options)
-}
-
 // GetAllEvents for User. Default returns EventResponseCollection for future events.
 // of the time that the call was made. 'calendar' option must be present to gain
 // access to additional data map in future calls.

src/internal/connector/graph_connector.go

@@ -14,6 +14,7 @@ import (
 	"github.com/microsoftgraph/msgraph-sdk-go/models"
 	"github.com/pkg/errors"
 
+	"github.com/alcionai/corso/src/internal/connector/discovery"
 	"github.com/alcionai/corso/src/internal/connector/exchange"
 	"github.com/alcionai/corso/src/internal/connector/graph"
 	"github.com/alcionai/corso/src/internal/connector/onedrive"
@@ -154,38 +155,20 @@ func (gc *GraphConnector) setTenantUsers(ctx context.Context) error {
 	ctx, end := D.Span(ctx, "gc:setTenantUsers")
 	defer end()
 
-	users, err := getResources(
-		ctx,
-		gc.graphService,
-		gc.tenant,
-		exchange.GetAllUsersForTenant,
-		models.CreateUserCollectionResponseFromDiscriminatorValue,
-		identifyUser,
-	)
+	users, err := discovery.Users(ctx, gc.graphService, gc.tenant)
 	if err != nil {
 		return err
 	}
 
-	gc.Users = users
+	gc.Users = make(map[string]string, len(users))
+
+	for _, u := range users {
+		gc.Users[*u.GetUserPrincipalName()] = *u.GetId()
+	}
 
 	return nil
 }
 
-// Transforms an interface{} into a key,value pair representing
-// userPrincipalName:userID.
-func identifyUser(item any) (string, string, error) {
-	m, ok := item.(models.Userable)
-	if !ok {
-		return "", "", errors.New("iteration retrieved non-User item")
-	}
-
-	if m.GetUserPrincipalName() == nil {
-		return "", "", errors.Errorf("no principal name for User: %s", *m.GetId())
-	}
-
-	return *m.GetUserPrincipalName(), *m.GetId(), nil
-}
-
 // GetUsers returns the email address of users within tenant.
 func (gc *GraphConnector) GetUsers() []string {
 	return buildFromMap(true, gc.Users)

src/pkg/services/m365/m365.go

@@ -3,41 +3,59 @@ package m365
 import (
 	"context"
 
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
 	"github.com/pkg/errors"
 
 	"github.com/alcionai/corso/src/internal/connector"
+	"github.com/alcionai/corso/src/internal/connector/discovery"
 	"github.com/alcionai/corso/src/pkg/account"
 )
 
+type User struct {
+	PrincipalName string
+	ID            string
+	Name          string
+}
+
 // Users returns a list of users in the specified M365 tenant
 // TODO: Implement paging support
-func Users(ctx context.Context, m365Account account.Account) ([]string, error) {
+func Users(ctx context.Context, m365Account account.Account) ([]*User, error) {
 	gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not initialize M365 graph connection")
 	}
 
-	return gc.GetUsers(), nil
+	users, err := discovery.Users(ctx, gc.Service(), m365Account.ID())
+	if err != nil {
+		return nil, err
+	}
+
+	ret := make([]*User, 0, len(users))
+
+	for _, u := range users {
+		pu, err := parseUser(u)
+		if err != nil {
+			return nil, err
+		}
+
+		ret = append(ret, pu)
+	}
+
+	return ret, nil
 }
 
-// UserIDs returns a list of user IDs for the specified M365 tenant
-// TODO: Implement paging support
 func UserIDs(ctx context.Context, m365Account account.Account) ([]string, error) {
-	gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users)
+	users, err := Users(ctx, m365Account)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not initialize M365 graph connection")
+		return nil, err
 	}
 
-	return gc.GetUsersIds(), nil
-}
-
-func GetEmailAndUserID(ctx context.Context, m365Account account.Account) (map[string]string, error) {
-	gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not initialize M365 graph connection")
+	ret := make([]string, 0, len(users))
+	for _, u := range users {
+		ret = append(ret, u.ID)
 	}
 
-	return gc.Users, nil
+	return ret, nil
 }
 
 // Sites returns a list of SharePoint sites in the specified M365 tenant
@@ -50,3 +68,18 @@ func Sites(ctx context.Context, m365Account account.Account) ([]string, error) {
 
 	return gc.GetSites(), nil
 }
+
+// parseUser extracts information from `models.Userable` we care about
+func parseUser(item models.Userable) (*User, error) {
+	if item.GetUserPrincipalName() == nil {
+		return nil, errors.Errorf("no principal name for User: %s", *item.GetId())
+	}
+
+	u := &User{PrincipalName: *item.GetUserPrincipalName(), ID: *item.GetId()}
+
+	if item.GetDisplayName() != nil {
+		u.Name = *item.GetDisplayName()
+	}
+
+	return u, nil
+}

src/pkg/services/m365/m365_test.go

@@ -3,6 +3,7 @@ package m365
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 
@@ -40,30 +41,11 @@ func (suite *M365IntegrationSuite) TestUsers() {
 
 	require.NotNil(suite.T(), users)
 	require.Greater(suite.T(), len(users), 0)
-}
-
-func (suite *M365IntegrationSuite) TestUserIDs() {
-	ctx, flush := tester.NewContext()
-	defer flush()
-
-	acct := tester.NewM365Account(suite.T())
-
-	ids, err := UserIDs(ctx, acct)
-	require.NoError(suite.T(), err)
-
-	require.NotNil(suite.T(), ids)
-	require.Greater(suite.T(), len(ids), 0)
-}
-
-func (suite *M365IntegrationSuite) TestGetEmailAndUserID() {
-	ctx, flush := tester.NewContext()
-	defer flush()
-
-	acct := tester.NewM365Account(suite.T())
-
-	ids, err := GetEmailAndUserID(ctx, acct)
-	require.NoError(suite.T(), err)
-
-	require.NotNil(suite.T(), ids)
-	require.Greater(suite.T(), len(ids), 0)
+
+	for _, u := range users {
+		suite.T().Log(u)
+		assert.NotEmpty(suite.T(), u.ID)
+		assert.NotEmpty(suite.T(), u.PrincipalName)
+		assert.NotEmpty(suite.T(), u.Name)
+	}
 }