Refactor site exclusion logic (#4548)

Pull the logic for excluding personal and search
sites into a separate function instead of overloading
the verify function for a site

---

#### 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

- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

* #4540

merge after:
* #4547

#### Test Plan

- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E

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
 }

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")
 	}
 }