From 73202dad6367a58461125240ab58afdc8fbbf1ec Mon Sep 17 00:00:00 2001 From: jules <130390278+juleslasarte@users.noreply.github.com> Date: Wed, 25 Oct 2023 15:38:35 -0700 Subject: [PATCH] [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? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * # #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/pkg/services/m365/api/mock/site.go | 52 ++++++++++++ src/pkg/services/m365/groups.go | 2 +- src/pkg/services/m365/sites.go | 26 ++++-- src/pkg/services/m365/sites_test.go | 105 ++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 12 deletions(-) create mode 100644 src/pkg/services/m365/api/mock/site.go diff --git a/src/pkg/services/m365/api/mock/site.go b/src/pkg/services/m365/api/mock/site.go new file mode 100644 index 000000000..8eefde00a --- /dev/null +++ b/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 +} diff --git a/src/pkg/services/m365/groups.go b/src/pkg/services/m365/groups.go index e520e900d..a9682c56c 100644 --- a/src/pkg/services/m365/groups.go +++ b/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 diff --git a/src/pkg/services/m365/sites.go b/src/pkg/services/m365/sites.go index a7d5e1609..55e830114 100644 --- a/src/pkg/services/m365/sites.go +++ b/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") } } } diff --git a/src/pkg/services/m365/sites_test.go b/src/pkg/services/m365/sites_test.go index b23d84226..af0feeb64 100644 --- a/src/pkg/services/m365/sites_test.go +++ b/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.OwnerID) - assert.NotEmpty(t, site.OwnerType) - } + 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)) + }) + } +}