Merge branch 'modifier_read_fix' into contact_create

goflow/modifiers.go

@@ -0,0 +1,29 @@
+package goflow
+
+import (
+	"encoding/json"
+
+	"github.com/nyaruka/goflow/assets"
+	"github.com/nyaruka/goflow/flows"
+	"github.com/nyaruka/goflow/flows/actions/modifiers"
+
+	"github.com/pkg/errors"
+)
+
+// ReadModifiers reads modifiers from the given JSON
+func ReadModifiers(sa flows.SessionAssets, data []json.RawMessage, allowMissing bool) ([]flows.Modifier, error) {
+	mods := make([]flows.Modifier, 0, len(data))
+	for _, m := range data {
+		mod, err := modifiers.ReadModifier(sa, m, assets.IgnoreMissing)
+
+		// if this modifier turned into a no-op, ignore
+		if err == modifiers.ErrNoModifier && allowMissing {
+			continue
+		}
+		if err != nil {
+			return nil, errors.Wrapf(err, "error reading modifier: %s", string(m))
+		}
+		mods = append(mods, mod)
+	}
+	return mods, nil
+}

goflow/modifiers_test.go

@@ -0,0 +1,63 @@
+package goflow_test
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/nyaruka/mailroom/goflow"
+	"github.com/nyaruka/mailroom/models"
+	"github.com/nyaruka/mailroom/testsuite"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestReadModifiers(t *testing.T) {
+	ctx := testsuite.CTX()
+	db := testsuite.DB()
+
+	oa, err := models.GetOrgAssets(ctx, db, models.Org1)
+	assert.NoError(t, err)
+
+	// can read empty list
+	mods, err := goflow.ReadModifiers(oa.SessionAssets(), []json.RawMessage{}, true)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(mods))
+
+	// can read non-empty list
+	mods, err = goflow.ReadModifiers(oa.SessionAssets(), []json.RawMessage{
+		[]byte(`{"type": "name", "name": "Bob"}`),
+		[]byte(`{"type": "field", "field": {"key": "gender", "name": "Gender"}, "value": "M"}`),
+		[]byte(`{"type": "language", "language": "spa"}`),
+	}, true)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, len(mods))
+	assert.Equal(t, "name", mods[0].Type())
+	assert.Equal(t, "field", mods[1].Type())
+	assert.Equal(t, "language", mods[2].Type())
+
+	// modifier with missing asset can be ignored
+	mods, err = goflow.ReadModifiers(oa.SessionAssets(), []json.RawMessage{
+		[]byte(`{"type": "name", "name": "Bob"}`),
+		[]byte(`{"type": "field", "field": {"key": "blood_type", "name": "Blood Type"}, "value": "O"}`),
+		[]byte(`{"type": "language", "language": "spa"}`),
+	}, true)
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(mods))
+	assert.Equal(t, "name", mods[0].Type())
+	assert.Equal(t, "language", mods[1].Type())
+
+	// modifier with missing asset or an error if allowMissing is false
+	mods, err = goflow.ReadModifiers(oa.SessionAssets(), []json.RawMessage{
+		[]byte(`{"type": "name", "name": "Bob"}`),
+		[]byte(`{"type": "field", "field": {"key": "blood_type", "name": "Blood Type"}, "value": "O"}`),
+		[]byte(`{"type": "language", "language": "spa"}`),
+	}, false)
+	assert.EqualError(t, err, `error reading modifier: {"type": "field", "field": {"key": "blood_type", "name": "Blood Type"}, "value": "O"}: no modifier to return because of missing assets`)
+
+	// error if any modifier structurally invalid
+	mods, err = goflow.ReadModifiers(oa.SessionAssets(), []json.RawMessage{
+		[]byte(`{"type": "field", "value": "O"}`),
+		[]byte(`{"type": "language", "language": "spa"}`),
+	}, false)
+	assert.EqualError(t, err, `error reading modifier: {"type": "field", "value": "O"}: field 'field' is required`)
+}

web/contact/contact.go

@@ -10,8 +10,8 @@ import (
 	"github.com/nyaruka/goflow/contactql"
 	"github.com/nyaruka/goflow/envs"
 	"github.com/nyaruka/goflow/flows"
-	"github.com/nyaruka/goflow/flows/actions/modifiers"
 	"github.com/nyaruka/goflow/utils"
+	"github.com/nyaruka/mailroom/goflow"
 	"github.com/nyaruka/mailroom/models"
 	"github.com/nyaruka/mailroom/web"
 
@@ -369,14 +369,10 @@ func handleModify(ctx context.Context, s *web.Server, r *http.Request) (interfac
 		return nil, http.StatusInternalServerError, errors.Wrapf(err, "unable to clone orgs")
 	}
 
-	// load our modifiers
-	mods := make([]flows.Modifier, len(request.Modifiers))
-	for i, m := range request.Modifiers {
-		mod, err := modifiers.ReadModifier(oa.SessionAssets(), m, assets.IgnoreMissing)
-		if err != nil {
-			return errors.Wrapf(err, "error in modifier: %s", string(m)), http.StatusBadRequest, nil
-		}
-		mods[i] = mod
+	// read the modifiers from the request
+	mods, err := goflow.ReadModifiers(org.SessionAssets(), request.Modifiers, false)
+	if err != nil {
+		return nil, http.StatusBadRequest, err
 	}
 
 	// load our contacts

web/contact/contact_test.go

@@ -207,6 +207,7 @@ func TestModifyContacts(t *testing.T) {
 	db.MustExec(`DELETE FROM contacts_contactgroup_contacts WHERE contact_id = $1`, models.CathyID)
 	db.MustExec(`UPDATE contacts_contacturn SET contact_id = NULL WHERE contact_id = $1`, models.CathyID)
 
+	// because we made changes to a group above, need to make sure we don't use stale org assets
 	models.FlushCache()
 
 	web.RunWebTests(t, "testdata/modify.json")

web/surveyor/surveyor.go

@@ -7,16 +7,20 @@ import (
 
 	"github.com/nyaruka/gocommon/urns"
 	"github.com/nyaruka/goflow/assets"
+<<<<<<< HEAD
 
 	"github.com/nyaruka/goflow/flows/actions/modifiers"
 
+=======
+>>>>>>> modifier_read_fix
 	"github.com/nyaruka/goflow/flows"
 	"github.com/nyaruka/goflow/flows/engine"
 	"github.com/nyaruka/goflow/flows/events"
 	"github.com/nyaruka/goflow/utils"
 	"github.com/nyaruka/mailroom/goflow"
 	"github.com/nyaruka/mailroom/models"
 	"github.com/nyaruka/mailroom/web"
+
 	"github.com/pkg/errors"
 )
 
@@ -85,18 +89,9 @@ func handleSubmit(ctx context.Context, s *web.Server, r *http.Request) (interfac
 	}
 
 	// and our modifiers
-	contactModifiers := make([]flows.Modifier, 0, len(request.Modifiers))
-	for _, m := range request.Modifiers {
-		modifier, err := modifiers.ReadModifier(org.SessionAssets(), m, assets.IgnoreMissing)
-
-		// if this modifier turned into a no-op, ignore
-		if err == modifiers.ErrNoModifier {
-			continue
-		}
-		if err != nil {
-			return nil, http.StatusBadRequest, errors.Wrapf(err, "error unmarshalling modifier: %s", string(m))
-		}
-		contactModifiers = append(contactModifiers, modifier)
+	mods, err := goflow.ReadModifiers(org.SessionAssets(), request.Modifiers, true)
+	if err != nil {
+		return nil, http.StatusBadRequest, err
 	}
 
 	// create / assign our contact
@@ -111,13 +106,13 @@ func handleSubmit(ctx context.Context, s *web.Server, r *http.Request) (interfac
 		return nil, http.StatusInternalServerError, errors.Wrapf(err, "unable to look up contact")
 	}
 
-	modifierEvents := make([]flows.Event, 0, len(contactModifiers))
+	modifierEvents := make([]flows.Event, 0, len(mods))
 	appender := func(e flows.Event) {
 		modifierEvents = append(modifierEvents, e)
 	}
 
 	// run through each contact modifier, applying it to our contact
-	for _, m := range contactModifiers {
+	for _, m := range mods {
 		m.Apply(org.Env(), org.SessionAssets(), flowContact, appender)
 	}
 
@@ -130,7 +125,7 @@ func handleSubmit(ctx context.Context, s *web.Server, r *http.Request) (interfac
 	}
 
 	// create our sprint
-	sprint := engine.NewSprint(contactModifiers, modifierEvents)
+	sprint := engine.NewSprint(mods, modifierEvents)
 
 	// write our session out
 	tx, err := s.DB.BeginTxx(ctx, nil)