🚀 Imports 2.0 #358

rowanseymour · 2020-09-23T21:58:33Z

No description provided.

codecov · 2020-09-23T22:06:43Z

Codecov Report

Merging #358 into master will increase coverage by 1.01%.
The diff coverage is 68.84%.

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   53.00%   54.01%   +1.01%     
==========================================
  Files         109      111       +2     
  Lines        7856     8010     +154     
==========================================
+ Hits         4164     4327     +163     
+ Misses       3007     2959      -48     
- Partials      685      724      +39

Impacted Files	Coverage Δ
models/msgs.go	`20.80% <0.00%> (ø)`
models/utils.go	`79.31% <ø> (ø)`
tasks/starts/worker.go	`55.68% <0.00%> (ø)`
tasks/contacts/import_contact_batch.go	`25.00% <25.00%> (ø)`
models/imports.go	`67.44% <67.44%> (ø)`
models/contacts.go	`61.02% <70.68%> (+2.92%)`	⬆️
models/events.go	`48.35% <77.77%> (+48.35%)`	⬆️
hooks/session_triggered.go	`81.48% <100.00%> (ø)`
tasks/broadcasts/worker.go	`57.33% <100.00%> (ø)`
web/contact/contact.go	`61.70% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ad2a3c...81e279b. Read the comment docs.

rowanseymour · 2020-09-28T22:25:03Z

models/contacts.go

+// * If URNs exists and belongs to a single contact it returns that contact (other URNs are not assigned to the contact).
+// * If URNs exists and belongs to multiple contacts it will return an error.
+//
+func GetOrCreateContact(ctx context.Context, db *sqlx.DB, oa *OrgAssets, urnz []urns.URN) (*Contact, *flows.Contact, error) {


Contract is a bit different than what we've had in RP - we don't promise that the returned contact owns all of the passed in URNs - instead the import code assigns any missing URNs using modifiers. I like this because:

Unless we added locking - that stuff isn't going to be atomic anyway

We don't duplicate the logic around creating new URNs vs attaching orphaned URNs in that modifier

Using modifiers means there's an event to say that a URN was added to a contact... and one day we'll show all events for all contacts... one day

see nyaruka#360

…ct with URNs

rowanseymour · 2020-09-29T15:30:24Z

models/contacts.go

+// CreateContact creates a new contact for the passed in org with the passed in URNs
+func CreateContact(ctx context.Context, db *sqlx.DB, oa *OrgAssets, userID UserID, name string, language envs.Language, urnz []urns.URN) (*Contact, *flows.Contact, error) {
+	// find current owners of these URNs
+	owners, err := contactIDsFromURNs(ctx, db, oa.OrgID(), urnz)


same check to see if URNs are taken added here to avoid even more tx rollbacks

rowanseymour · 2020-09-29T15:33:37Z

models/contacts.go

+		return contactID, true, nil
+	}
+
+	if dbutil.IsUniqueViolation(err) {


thinking of ways to test this... could have another interface like Queryer that supports transactions (QueryerWithTx).. use that everywhere instead of *sqlx.DB, and make an implementation for testing that we can configure to blow up when needed

Don't know if this is the best, or even a good way, but in Courier we do in goroutines by having a flag we can set on the test to slow things down:
https://github.com/nyaruka/courier/blob/master/backends/rapidpro/backend_test.go#L254
https://github.com/nyaruka/courier/blob/master/backends/rapidpro/contact.go#L180

Interesting - I like the idea of being able to test an actual race - tho I think I'd still want to keep that out of the production code. I.e. could use the db interface as per nyaruka#360 but make one of those calls take an extra second.

Got this now https://github.com/nyaruka/mailroom/pull/358/files#diff-87ee1a97bf30c9671e799d60a8bbd25cR663

nicpottier · 2020-09-30T16:08:33Z

models/contacts.go

 	if err != nil {
-		return nil, errors.Wrapf(err, "error selecting contact ids by uuid")
+		return nil, err


Remember go annoyingly treats no rows as an error.

So the code that it replaces wasn't checking for sql.NoRows because it's only only sued in places where you're expecting get back ids for things that should exist.. so I went back and forth on whether this being a generic utility method should allow that.. and couldn't make up my mind.. but this morning I'm leaning toward it allowing no rows, and the calling code should check that it got as many ids as it expected

nicpottier · 2020-09-30T16:10:22Z

models/contacts.go

 	}
 	defer rows.Close()

 	var id ContactID
 	for rows.Next() {
 		err := rows.Scan(&id)
 		if err != nil {
-			return nil, errors.Wrapf(err, "error scanning contact id")
+			return nil, err


What's your rule for wrapping or not wrapping? I kinda just defer to always wrapping because hey, more info when looking at a trace never seems like a bad thing but totally admit maybe that's overkill. Just wondering what rule you use.

I've generally leaned toward always wrap - not even sure why I didn't here - fine with more wrapping

nicpottier · 2020-09-30T16:27:11Z

models/contacts.go

+		return contactID, true, nil
+	}
+
+	if dbutil.IsUniqueViolation(err) {


Don't know if this is the best, or even a good way, but in Courier we do in goroutines by having a flag we can set on the test to slow things down:
https://github.com/nyaruka/courier/blob/master/backends/rapidpro/backend_test.go#L254
https://github.com/nyaruka/courier/blob/master/backends/rapidpro/contact.go#L180

nicpottier · 2020-09-30T16:28:34Z

models/imports.go

+// import status constants
+const (
+	ContactImportStatusPending    ContactImportStatus = "P"
+	ContactImportStatusProcessing ContactImportStatus = "O"


Would Running be better maybe?

Maybe but these are the same codes that we use in the 3 export models

nicpottier · 2020-09-30T16:29:34Z

models/imports.go

+func (b *ContactImportBatch) Import(ctx context.Context, db *sqlx.DB, orgID OrgID) error {
+	// if any error occurs this batch should be marked as failed
+	if err := b.tryImport(ctx, db, orgID); err != nil {
+		b.markFailed(ctx, db)


Any benefit to logging the error in the task itself too?

I think this is an error for us (it's a a bunch of wrapped err's) rather than the user facing errors what we collect in batch.errors and relate to problems in the user's data, so it's fine if it just goes to sentry.

nicpottier · 2020-09-30T16:31:05Z

models/imports.go

+	}
+
+	// and apply in bulk
+	_, err = ApplyModifiers(ctx, db, nil, oa, modifiersByContact)


May not be worth it but one thing we do in batch starts is if we fail we retry in individual transactions for each contact.. this helps with deadlocks.

So that would be something like reworking HandleAndCommitEvents to catch any error when committing the transaction that wraps ApplyEventPreCommitHooks.. and if it fails, try each scene one by one? That sounds pretty straightforward.. tho inclined to try without first.

nicpottier · 2020-09-30T16:34:32Z

models/imports.go

+	b.FinishedOn = &now
+	_, err = db.ExecContext(ctx,
+		`UPDATE contacts_contactimportbatch SET status = $2, num_created = $3, num_updated = $4, num_errored = $5, errors = $6, finished_on = $7 WHERE id = $1`,
+		b.ID, b.Status, b.NumCreated, b.NumUpdated, b.NumErrored, b.Errors, b.FinishedOn,


Since you have a batch at this point maybe cleaner / less error prone to use NamedQuery?

yeah will update

add is_resend to Msg payload to allow for resending messages manually

WIP

7659543

rowanseymour added 2 commits September 24, 2020 15:54

WIP

2f1e7fe

Allow get or create by multiple URNs (WIP)

c159526

rowanseymour force-pushed the imports2 branch from 6b59a04 to c159526 Compare September 25, 2020 20:17

rowanseymour added 4 commits September 25, 2020 16:59

Merge branch 'master' into imports2

fe8b299

Merge branch 'master' into imports2

68c85f7

Add tests

5d19c90

Fix updating priority for orphaned URNs which are being attached

b37f98b

rowanseymour marked this pull request as ready for review September 28, 2020 22:18

rowanseymour commented Sep 28, 2020

View reviewed changes

CreateContact also should do lookup before trying to create new conta…

daed23a

…ct with URNs

rowanseymour force-pushed the imports2 branch from d524ddd to daed23a Compare September 29, 2020 15:28

rowanseymour requested a review from nicpottier September 29, 2020 15:28

rowanseymour commented Sep 29, 2020

View reviewed changes

rowanseymour added 2 commits September 29, 2020 11:16

Add test for dynamic group re-evaluation of imported contacts

bf2cf51

Add tests for URN normalization during imports

7b904d9

nicpottier approved these changes Sep 30, 2020

View reviewed changes

rowanseymour added 2 commits September 30, 2020 11:57

Merge branch 'master' into imports2

d439565

Updates from code review

81e279b

rowanseymour merged commit 14ec817 into master Sep 30, 2020

rowanseymour deleted the imports2 branch September 30, 2020 19:02

rasoro pushed a commit to Ilhasoft/mailroom that referenced this pull request Mar 10, 2023

Merge pull request rapidpro#358 from nyaruka/resend

93b6eb5

add is_resend to Msg payload to allow for resending messages manually

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 Imports 2.0 #358

🚀 Imports 2.0 #358

rowanseymour commented Sep 23, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading

rowanseymour Sep 28, 2020

rowanseymour Sep 30, 2020

rowanseymour Sep 29, 2020

rowanseymour Sep 29, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

nicpottier Sep 30, 2020

rowanseymour Sep 30, 2020

🚀 Imports 2.0 #358

🚀 Imports 2.0 #358

Conversation

rowanseymour commented Sep 23, 2020 • edited Loading

codecov bot commented Sep 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanseymour commented Sep 23, 2020 •

edited

Loading

codecov bot commented Sep 23, 2020 •

edited

Loading