Merge pull request #355 from nyaruka/hide_db_groups

Don't include db-trigger maintained status groups in engine assets

core/models/assets.go

@@ -217,16 +217,21 @@ func NewOrgAssets(ctx context.Context, rt *runtime.Runtime, orgID OrgID, prev *O
 	}
 
 	if prev == nil || refresh&RefreshGroups > 0 {
-		oa.groups, err = loadAssetType(ctx, db, orgID, "groups", loadGroups)
+		groups, err := loadAssetType(ctx, db, orgID, "groups", loadGroups)
 		if err != nil {
 			return nil, fmt.Errorf("error loading group assets for org %d: %w", orgID, err)
 		}
-		oa.groupsByID = make(map[GroupID]*Group)
-		oa.groupsByUUID = make(map[assets.GroupUUID]*Group)
-		for _, g := range oa.groups {
+		oa.groups = make([]assets.Group, 0, len(groups))
+		oa.groupsByID = make(map[GroupID]*Group, len(groups))
+		oa.groupsByUUID = make(map[assets.GroupUUID]*Group, len(groups))
+		for _, g := range groups {
 			group := g.(*Group)
 			oa.groupsByID[group.ID()] = group
 			oa.groupsByUUID[group.UUID()] = group
+
+			if group.Visible() {
+				oa.groups = append(oa.groups, g)
+			}
 		}
 	} else {
 		oa.groups = prev.groups

core/models/contacts.go

@@ -165,16 +165,7 @@ func (c *Contact) Stop(ctx context.Context, db DBorTx, oa *OrgAssets) error {
 		return fmt.Errorf("error marking contact as stopped: %w", err)
 	}
 
-	// they are only in the stopped group
-	newGroups := make([]*Group, 0, 1)
-	for _, g := range oa.groups {
-		if g.(*Group).Type() == GroupTypeDBStopped {
-			newGroups = append(newGroups, g.(*Group))
-			break
-		}
-	}
-
-	c.groups = newGroups
+	c.groups = []*Group{} // currently groups always implicitly exclude non-active contacts
 	c.status = ContactStatusStopped
 	return nil
 }
@@ -265,8 +256,8 @@ func (c *Contact) FlowContact(oa *OrgAssets) (*flows.Contact, error) {
 	// convert our groups to a list of references
 	groups := make([]*assets.GroupReference, 0, len(c.groups))
 	for _, g := range c.groups {
-		// exclude the db-trigger based status groups for now
-		if g.Type() == GroupTypeManual || g.Type() == GroupTypeSmart {
+		// exclude the db-trigger based status groups
+		if g.Visible() {
 			groups = append(groups, assets.NewGroupReference(g.UUID(), g.Name()))
 		}
 	}
@@ -349,11 +340,11 @@ func LoadContacts(ctx context.Context, db Queryer, oa *OrgAssets, ids []ContactI
 			currentFlowID: e.CurrentFlowID,
 		}
 
-		// load our real groups
+		// load our real groups (exclude status groups)
 		groups := make([]*Group, 0, len(e.GroupIDs))
 		for _, g := range e.GroupIDs {
 			group := oa.GroupByID(g)
-			if group != nil {
+			if group != nil && group.Visible() {
 				groups = append(groups, group)
 			}
 		}
@@ -560,15 +551,10 @@ SELECT ROW_TO_JSON(r) FROM (SELECT
 FROM
 	contacts_contact c
 LEFT JOIN (
-	SELECT 
-		contact_id, 
-		ARRAY_AGG(contactgroup_id) AS groups 
-	FROM 
-		contacts_contactgroup_contacts g
-	WHERE
-		g.contact_id = ANY($1)		
-	GROUP BY 
-		contact_id
+	SELECT contact_id, ARRAY_AGG(contactgroup_id) AS groups 
+	FROM contacts_contactgroup_contacts g
+	WHERE g.contact_id = ANY($1)		
+	GROUP BY contact_id
 ) g ON c.id = g.contact_id
 LEFT JOIN (
 	SELECT 

core/models/contacts_test.go

@@ -478,9 +478,7 @@ func TestContactStop(t *testing.T) {
 	err := contact.Stop(ctx, rt.DB, oa)
 	assert.NoError(t, err)
 	assert.Equal(t, models.ContactStatusStopped, contact.Status())
-	if assert.Len(t, contact.Groups(), 1) {
-		assert.Equal(t, "Stopped", contact.Groups()[0].Name())
-	}
+	assert.Len(t, contact.Groups(), 0)
 
 	// verify that matches the database state
 	assertdb.Query(t, rt.DB, `SELECT status FROM contacts_contact WHERE id = $1`, testdata.Cathy.ID).Returns("S")

core/models/fields_test.go

@@ -17,6 +17,13 @@ func TestFields(t *testing.T) {
 	oa, err := models.GetOrgAssetsWithRefresh(ctx, rt, testdata.Org1.ID, models.RefreshFields)
 	require.NoError(t, err)
 
+	fields, err := oa.Fields()
+	require.NoError(t, err)
+	assert.Len(t, fields, 6) // excludes the proxy fields
+	assert.Equal(t, "age", fields[0].Key())
+	assert.Equal(t, "Age", fields[0].Name())
+	assert.Equal(t, assets.FieldTypeNumber, fields[0].Type())
+
 	expectedFields := []struct {
 		field     testdata.Field
 		key       string

core/models/groups.go

@@ -27,9 +27,12 @@ const (
 type GroupType string
 
 const (
-	GroupTypeManual    = GroupType("M")
-	GroupTypeSmart     = GroupType("Q")
-	GroupTypeDBStopped = GroupType("S")
+	GroupTypeDBActive   = GroupType("A")
+	GroupTypeDBBlocked  = GroupType("B")
+	GroupTypeDBStopped  = GroupType("S")
+	GroupTypeDBArchived = GroupType("V")
+	GroupTypeManual     = GroupType("M")
+	GroupTypeSmart      = GroupType("Q")
 )
 
 // Group is our mailroom type for contact groups
@@ -60,6 +63,9 @@ func (g *Group) Status() GroupStatus { return g.Status_ }
 // Type returns the type of this group
 func (g *Group) Type() GroupType { return g.Type_ }
 
+// Visible returns whether this group is visible to the engine (status groups are not)
+func (g *Group) Visible() bool { return g.Type_ == GroupTypeManual || g.Type_ == GroupTypeSmart }
+
 // loads the groups for the passed in org
 func loadGroups(ctx context.Context, db *sql.DB, orgID OrgID) ([]assets.Group, error) {
 	rows, err := db.QueryContext(ctx, sqlSelectGroupsByOrg, orgID)

core/models/groups_test.go

@@ -3,7 +3,6 @@ package models_test
 import (
 	"testing"
 
-	"github.com/nyaruka/goflow/assets"
 	"github.com/nyaruka/mailroom/core/models"
 	"github.com/nyaruka/mailroom/testsuite"
 	"github.com/nyaruka/mailroom/testsuite/testdata"
@@ -19,29 +18,26 @@ func TestLoadGroups(t *testing.T) {
 
 	groups, err := oa.Groups()
 	require.NoError(t, err)
+	assert.Len(t, groups, 3) // excludes the status groups
+	assert.Equal(t, testdata.DoctorsGroup.UUID, groups[0].UUID())
+	assert.Equal(t, "Doctors", groups[0].Name())
 
 	tcs := []struct {
-		id            models.GroupID
-		uuid          assets.GroupUUID
+		group         *testdata.Group
 		name          string
 		query         string
 		expectedCount int
 	}{
-		{testdata.ActiveGroup.ID, testdata.ActiveGroup.UUID, "Active", "", 124},
-		{testdata.ArchivedGroup.ID, testdata.ArchivedGroup.UUID, "Archived", "", 0},
-		{testdata.BlockedGroup.ID, testdata.BlockedGroup.UUID, "Blocked", "", 0},
-		{testdata.DoctorsGroup.ID, testdata.DoctorsGroup.UUID, "Doctors", "", 121},
-		{testdata.OpenTicketsGroup.ID, testdata.OpenTicketsGroup.UUID, "Open Tickets", "tickets > 0", 0},
-		{testdata.StoppedGroup.ID, testdata.StoppedGroup.UUID, "Stopped", "", 0},
-		{testdata.TestersGroup.ID, testdata.TestersGroup.UUID, "Testers", "", 10},
+		{testdata.ActiveGroup, "Active", "", 124},
+		{testdata.BlockedGroup, "Blocked", "", 0},
+		{testdata.DoctorsGroup, "Doctors", "", 121},
+		{testdata.OpenTicketsGroup, "Open Tickets", "tickets > 0", 0},
 	}
 
-	assert.Equal(t, 7, len(groups))
-
-	for i, tc := range tcs {
-		group := groups[i].(*models.Group)
-		assert.Equal(t, tc.uuid, group.UUID())
-		assert.Equal(t, tc.id, group.ID())
+	for _, tc := range tcs {
+		group := oa.GroupByUUID(tc.group.UUID)
+		assert.Equal(t, tc.group.UUID, group.UUID())
+		assert.Equal(t, tc.group.ID, group.ID())
 		assert.Equal(t, tc.name, group.Name())
 		assert.Equal(t, tc.query, group.Query())
 

core/search/search.go

@@ -86,7 +86,14 @@ func GetContactTotal(ctx context.Context, rt *runtime.Runtime, oa *models.OrgAss
 		}
 	}
 
-	eq := BuildElasticQuery(oa, group, models.NilContactStatus, nil, parsed)
+	// if group is a status group, Elastic won't know about it so search by status instead
+	status := models.NilContactStatus
+	if group != nil && !group.Visible() {
+		status = models.ContactStatus(group.Type())
+		group = nil
+	}
+
+	eq := BuildElasticQuery(oa, group, status, nil, parsed)
 	src := map[string]any{"query": eq}
 
 	count, err := rt.ES.Count().Index(rt.Config.ElasticContactsIndex).Routing(oa.OrgID().String()).Raw(bytes.NewReader(jsonx.MustMarshal(src))).Do(ctx)
@@ -116,7 +123,14 @@ func GetContactIDsForQueryPage(ctx context.Context, rt *runtime.Runtime, oa *mod
 		}
 	}
 
-	eq := BuildElasticQuery(oa, group, models.NilContactStatus, excludeIDs, parsed)
+	// if group is a status group, Elastic won't know about it so search by status instead
+	status := models.NilContactStatus
+	if group != nil && !group.Visible() {
+		status = models.ContactStatus(group.Type())
+		group = nil
+	}
+
+	eq := BuildElasticQuery(oa, group, status, excludeIDs, parsed)
 
 	fieldSort, err := es.ToElasticSort(sort, oa.SessionAssets())
 	if err != nil {
@@ -164,6 +178,12 @@ func GetContactIDsForQuery(ctx context.Context, rt *runtime.Runtime, oa *models.
 		}
 	}
 
+	// if group is a status group, Elastic won't know about it so search by status instead
+	if group != nil && !group.Visible() {
+		status = models.ContactStatus(group.Type())
+		group = nil
+	}
+
 	eq := BuildElasticQuery(oa, group, status, nil, parsed)
 	sort := elastic.SortBy("id", true)
 	ids := make([]models.ContactID, 0, 100)