diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 8cf6575df..466ea89f1 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -11,7 +11,6 @@ import ( msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/microsoftgraph/msgraph-sdk-go/sites" - "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/m365/graph" @@ -85,13 +84,12 @@ func (c Sites) GetAll(ctx context.Context, errs *fault.Bus) ([]models.Siteable, } err := validateSite(item) - if errors.Is(err, ErrKnownSkippableCase) { - // safe to no-op + if err != nil { + el.AddRecoverable(ctx, graph.Wrap(ctx, err, "validating site")) return true } - if err != nil { - el.AddRecoverable(ctx, graph.Wrap(ctx, err, "validating site")) + if skippableSite(item) { return true } @@ -255,9 +253,26 @@ func (c Sites) GetDefaultDrive( // helpers // --------------------------------------------------------------------------- -var ErrKnownSkippableCase = clues.New("case is known and skippable") +const personalSitePath = "sharepoint.com/personal/" -const PersonalSitePath = "sharepoint.com/personal/" +func skippableSite(item models.Siteable) bool { + wURL := ptr.Val(item.GetWebUrl()) + name := ptr.Val(item.GetDisplayName()) + + // personal (ie: oneDrive) sites have to be filtered out server-side. + if strings.Contains(wURL, personalSitePath) { + return true + } + + // Not checking for a name since it's possible to have sites without a name. + // Do check if it's the search site though since we want to filter that out. + // The built-in site at "https://{tenant-domain}/search" never has a name. + if len(name) == 0 && strings.HasSuffix(wURL, "/search") { + return true + } + + return false +} // validateSite ensures the item is a Siteable, and contains the necessary // identifiers that we handle with all users. @@ -273,21 +288,5 @@ func validateSite(item models.Siteable) error { return clues.New("missing webURL").With("site_id", clues.Hide(id)) } - // TODO(ashmrtn): Personal and search site checks should really be in the - // caller instead of the validation function. - // personal (ie: oneDrive) sites have to be filtered out server-side. - if strings.Contains(wURL, PersonalSitePath) { - return clues.Stack(ErrKnownSkippableCase). - With("site_id", clues.Hide(id), "site_web_url", clues.Hide(wURL)) - } - - // Not checking for a name since it's possible to have sites without a name. - // Do check if it's the search site though since we want to filter that out. - // The built-in site at "https://{tenant-domain}/search" never has a name. - if len(ptr.Val(item.GetDisplayName())) == 0 && strings.HasSuffix(wURL, "/search") { - return clues.Stack(ErrKnownSkippableCase). - With("site_id", clues.Hide(id), "site_web_url", clues.Hide(wURL)) - } - return nil } diff --git a/src/pkg/services/m365/api/sites_test.go b/src/pkg/services/m365/api/sites_test.go index 2f5137914..0cea6f4aa 100644 --- a/src/pkg/services/m365/api/sites_test.go +++ b/src/pkg/services/m365/api/sites_test.go @@ -33,10 +33,9 @@ func (suite *SitesUnitSuite) TestValidateSite() { site.SetId(ptr.To("testID")) tests := []struct { - name string - args models.Siteable - errCheck assert.ErrorAssertionFunc - errIsSkippable bool + name string + args models.Siteable + errCheck assert.ErrorAssertionFunc }{ { name: "No ID", @@ -70,19 +69,17 @@ func (suite *SitesUnitSuite) TestValidateSite() { s.SetWebUrl(ptr.To("sharepoint.com/search")) return s }(), - errCheck: assert.Error, - errIsSkippable: true, + errCheck: assert.NoError, }, { name: "Personal OneDrive", args: func() *models.Site { s := models.NewSite() s.SetId(ptr.To("id")) - s.SetWebUrl(ptr.To("https://" + PersonalSitePath + "/someone's/onedrive")) + s.SetWebUrl(ptr.To("https://" + personalSitePath + "/someone's/onedrive")) return s }(), - errCheck: assert.Error, - errIsSkippable: true, + errCheck: assert.NoError, }, { name: "Valid Site", @@ -96,10 +93,6 @@ func (suite *SitesUnitSuite) TestValidateSite() { err := validateSite(test.args) test.errCheck(t, err, clues.ToCore(err)) - - if test.errIsSkippable { - assert.ErrorIs(t, err, ErrKnownSkippableCase) - } }) } } @@ -134,7 +127,7 @@ func (suite *SitesIntgSuite) TestGetAll() { require.NotZero(t, len(sites), "must have at least one site") for _, site := range sites { - assert.NotContains(t, ptr.Val(site.GetWebUrl()), PersonalSitePath, "must not return onedrive sites") + assert.NotContains(t, ptr.Val(site.GetWebUrl()), personalSitePath, "must not return onedrive sites") assert.NotContains(t, ptr.Val(site.GetWebUrl()), "sharepoint.com/search", "must not return search site") } }