[Bugfix] Fixing getting owner email (#4538)

Owner data might have email and ID, or only one of them, or none. Fixing the code to handle this and adding a unit test. 

---

#### Does this PR need a docs update or release note?

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [x] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* #<issue>

#### Test Plan

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

src/pkg/services/m365/api/mock/site.go

@@ -0,0 +1,52 @@
+package mock
+
+import (
+	"github.com/microsoftgraph/msgraph-sdk-go/models"
+
+	"github.com/alcionai/corso/src/internal/common/ptr"
+)
+
+func UserIdentity(userID string, userEmail string) models.IdentitySetable {
+	user := models.NewIdentitySet()
+	userIdentity := models.NewUserIdentity()
+	userIdentity.SetId(ptr.To(userID))
+
+	if len(userEmail) > 0 {
+		userIdentity.SetAdditionalData(map[string]any{
+			"email": userEmail,
+		})
+	}
+
+	user.SetUser(userIdentity)
+
+	return user
+}
+
+func GroupIdentitySet(groupID string, groupEmail string) models.IdentitySetable {
+	groupData := map[string]any{}
+	if len(groupEmail) > 0 {
+		groupData["email"] = groupEmail
+	}
+
+	if len(groupID) > 0 {
+		groupData["id"] = groupID
+	}
+
+	group := models.NewIdentitySet()
+	group.SetAdditionalData(map[string]any{"group": groupData})
+
+	return group
+}
+
+func DummySite(owner models.IdentitySetable) models.Siteable {
+	site := models.NewSite()
+	site.SetId(ptr.To("id"))
+	site.SetWebUrl(ptr.To("weburl"))
+	site.SetDisplayName(ptr.To("displayname"))
+
+	drive := models.NewDrive()
+	drive.SetOwner(owner)
+	site.SetDrive(drive)
+
+	return site
+}

src/pkg/services/m365/groups.go

@@ -117,7 +117,7 @@ func SitesInGroup(
 	result := make([]*Site, 0, len(sites))
 
 	for _, site := range sites {
-		result = append(result, ParseSite(site))
+		result = append(result, ParseSite(ctx, site))
 	}
 
 	return result, nil

src/pkg/services/m365/sites.go

@@ -13,6 +13,7 @@ import (
 	"github.com/alcionai/corso/src/internal/m365/graph"
 	"github.com/alcionai/corso/src/pkg/account"
 	"github.com/alcionai/corso/src/pkg/fault"
+	"github.com/alcionai/corso/src/pkg/logger"
 	"github.com/alcionai/corso/src/pkg/path"
 	"github.com/alcionai/corso/src/pkg/services/m365/api"
 )
@@ -20,7 +21,7 @@ import (
 type SiteOwnerType string
 
 const (
-	SiteOwnerUnknown SiteOwnerType = ""
+	SiteOwnerUnknown SiteOwnerType = "unknown"
 	SiteOwnerUser    SiteOwnerType = "user"
 	SiteOwnerGroup   SiteOwnerType = "group"
 )
@@ -65,12 +66,21 @@ func SiteByID(
 		Expand: []string{"drive"},
 	}
 
-	s, err := ac.Sites().GetByID(ctx, id, cc)
+	return getSiteByID(ctx, ac.Sites(), id, cc)
+}
+
+func getSiteByID(
+	ctx context.Context,
+	ga api.GetByIDer[models.Siteable],
+	id string,
+	cc api.CallConfig,
+) (*Site, error) {
+	s, err := ga.GetByID(ctx, id, cc)
 	if err != nil {
 		return nil, clues.Stack(err)
 	}
 
-	return ParseSite(s), nil
+	return ParseSite(ctx, s), nil
 }
 
 // Sites returns a list of Sites in a specified M365 tenant
@@ -99,14 +109,14 @@ func getAllSites(
 	ret := make([]*Site, 0, len(sites))
 
 	for _, s := range sites {
-		ret = append(ret, ParseSite(s))
+		ret = append(ret, ParseSite(ctx, s))
 	}
 
 	return ret, nil
 }
 
 // ParseSite extracts the information from `models.Siteable` we care about
-func ParseSite(item models.Siteable) *Site {
+func ParseSite(ctx context.Context, item models.Siteable) *Site {
 	s := &Site{
 		ID:          ptr.Val(item.GetId()),
 		WebURL:      ptr.Val(item.GetWebUrl()),
@@ -145,14 +155,16 @@ func ParseSite(item models.Siteable) *Site {
 				return s
 			}
 
+			// ignore the errors, these might or might not exist
+			// if they don't exist, we'll just have an empty string
 			s.OwnerID, err = str.AnyValueToString("id", group)
 			if err != nil {
-				return s
+				logger.CtxErr(ctx, err).Info("could not parse owner ID")
 			}
 
 			s.OwnerEmail, err = str.AnyValueToString("email", group)
 			if err != nil {
-				return s
+				logger.CtxErr(ctx, err).Info("could not parse owner email")
 			}
 		}
 	}

src/pkg/services/m365/sites_test.go

@@ -18,6 +18,8 @@ import (
 	"github.com/alcionai/corso/src/pkg/account"
 	"github.com/alcionai/corso/src/pkg/credentials"
 	"github.com/alcionai/corso/src/pkg/fault"
+	"github.com/alcionai/corso/src/pkg/services/m365/api"
+	"github.com/alcionai/corso/src/pkg/services/m365/api/mock"
 )
 
 type siteIntegrationSuite struct {
@@ -81,10 +83,7 @@ func (suite *siteIntegrationSuite) TestSites_GetByID() {
 			assert.NotEmpty(t, site.WebURL)
 			assert.NotEmpty(t, site.ID)
 			assert.NotEmpty(t, site.DisplayName)
-			if site.OwnerType != SiteOwnerUnknown {
+			assert.NotEmpty(t, site.OwnerType)
-				assert.NotEmpty(t, site.OwnerID)
-				assert.NotEmpty(t, site.OwnerType)
-			}
 		})
 	}
 }
@@ -156,6 +155,14 @@ func (m mockGASites) GetAll(context.Context, *fault.Bus) ([]models.Siteable, err
 	return m.response, m.err
 }
 
+func (m mockGASites) GetByID(context.Context, string, api.CallConfig) (models.Siteable, error) {
+	if len(m.response) == 0 {
+		return nil, m.err
+	}
+
+	return m.response[0], m.err
+}
+
 func (suite *siteUnitSuite) TestGetAllSites() {
 	table := []struct {
 		name      string
@@ -216,3 +223,93 @@ func (suite *siteUnitSuite) TestGetAllSites() {
 		})
 	}
 }
+
+func (suite *siteUnitSuite) TestGetSites() {
+	table := []struct {
+		name       string
+		mock       func(context.Context) api.GetByIDer[models.Siteable]
+		expectErr  assert.ErrorAssertionFunc
+		expectSite func(*testing.T, *Site)
+	}{
+		{
+			name: "ok - no owner",
+			mock: func(ctx context.Context) api.GetByIDer[models.Siteable] {
+				return mockGASites{[]models.Siteable{
+					mock.DummySite(nil),
+				}, nil}
+			},
+			expectErr: assert.NoError,
+			expectSite: func(t *testing.T, site *Site) {
+				assert.NotEmpty(t, site.ID)
+				assert.NotEmpty(t, site.WebURL)
+				assert.NotEmpty(t, site.DisplayName)
+				assert.Empty(t, site.OwnerID)
+			},
+		},
+		{
+			name: "ok - owner user",
+			mock: func(ctx context.Context) api.GetByIDer[models.Siteable] {
+				return mockGASites{[]models.Siteable{
+					mock.DummySite(mock.UserIdentity("userid", "useremail")),
+				}, nil}
+			},
+			expectErr: assert.NoError,
+			expectSite: func(t *testing.T, site *Site) {
+				assert.NotEmpty(t, site.ID)
+				assert.NotEmpty(t, site.WebURL)
+				assert.NotEmpty(t, site.DisplayName)
+				assert.Equal(t, site.OwnerID, "userid")
+				assert.Equal(t, site.OwnerEmail, "useremail")
+				assert.Equal(t, site.OwnerType, SiteOwnerUser)
+			},
+		},
+		{
+			name: "ok - group user with ID and email",
+			mock: func(ctx context.Context) api.GetByIDer[models.Siteable] {
+				return mockGASites{[]models.Siteable{
+					mock.DummySite(mock.GroupIdentitySet("groupid", "groupemail")),
+				}, nil}
+			},
+			expectErr: assert.NoError,
+			expectSite: func(t *testing.T, site *Site) {
+				assert.NotEmpty(t, site.ID)
+				assert.NotEmpty(t, site.WebURL)
+				assert.NotEmpty(t, site.DisplayName)
+				assert.Equal(t, SiteOwnerGroup, site.OwnerType)
+				assert.Equal(t, "groupid", site.OwnerID)
+				assert.Equal(t, "groupemail", site.OwnerEmail)
+			},
+		},
+		{
+			name: "ok - group user with no ID but email",
+			mock: func(ctx context.Context) api.GetByIDer[models.Siteable] {
+				return mockGASites{[]models.Siteable{
+					mock.DummySite(mock.GroupIdentitySet("", "groupemail")),
+				}, nil}
+			},
+			expectErr: assert.NoError,
+			expectSite: func(t *testing.T, site *Site) {
+				assert.NotEmpty(t, site.ID)
+				assert.NotEmpty(t, site.WebURL)
+				assert.NotEmpty(t, site.DisplayName)
+				assert.Equal(t, SiteOwnerGroup, site.OwnerType)
+				assert.Equal(t, "", site.OwnerID)
+				assert.Equal(t, "groupemail", site.OwnerEmail)
+			},
+		},
+	}
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			t := suite.T()
+
+			ctx, flush := tester.NewContext(t)
+			defer flush()
+
+			gas := test.mock(ctx)
+
+			site, err := getSiteByID(ctx, gas, "id", api.CallConfig{})
+			test.expectSite(t, site)
+			test.expectErr(t, err, clues.ToCore(err))
+		})
+	}
+}