Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add job to load messages in from a csv and fix some crucial bugs in twilio.js #1346

Merged
merged 2 commits into from
Jan 2, 2020

Conversation

schuyler1d
Copy link
Collaborator

@schuyler1d schuyler1d commented Dec 31, 2019

Fixes # (issue)

Description

This adds a method to run yarn process-message-csv SOMEMESSAGEFILE.csv on a file and load them message responses into the database all at once -- this is a useful tool when processing messages from outside the twilio api and/or for testing.

In writing this, some critical bugs were found in the current twilio processing that may be causing deadlocks on indexes and other problems. This fixes those minimally, awaiting more fixes in e.g.

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • [na] If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

@schuyler1d schuyler1d added the S-work in progress (WIP) Status: PR label for work that is not yet ready to be reviewed label Dec 31, 2019
@ibrand ibrand temporarily deployed to spoke-review-load-messa-uswxgu December 31, 2019 18:23 Inactive
Copy link
Collaborator Author

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to test this out on production to see if it resolves our issues.

const countResult = await r.getCount(
r.knex("message").where("service_id", messageInstance.service_id)
);
const [countResult] = await r
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A theory about the failure was that this was getting a count rather than a single message -- it therefore deadlocked on two of the same messages coming in, because it didn't need to find just one, but get a full count.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, though it would be good to rename this variable, since it is no longer the count (unless I'm misunderstanding the change).

@@ -56,13 +56,16 @@ async function convertMessagePartsToMessage(messageParts) {
const lastMessage = await getLastMessage({
contactNumber
});
if (!lastMessage) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often SMS spam comes in. If we failed to find a lastMessage, then this failed rather than returning and completing normally -- leaving a db connection open -- and possibly a transaction.

const countResult = await r.getCount(
r.knex("message").where("service_id", messageInstance.service_id)
);
const [countResult] = await r
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, though it would be good to rename this variable, since it is no longer the count (unless I'm misunderstanding the change).

src/server/api/lib/message-sending.js Outdated Show resolved Hide resolved
@ibrand ibrand temporarily deployed to spoke-review-load-messa-uswxgu December 31, 2019 20:57 Inactive
@schuyler1d schuyler1d changed the title [WIP] add job to load messages in from a csv and fix some crucial bugs in twilio.js add job to load messages in from a csv and fix some crucial bugs in twilio.js Jan 2, 2020
@schuyler1d schuyler1d added S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc and removed S-work in progress (WIP) Status: PR label for work that is not yet ready to be reviewed labels Jan 2, 2020
@schuyler1d
Copy link
Collaborator Author

This was tested on MoveOn's production instance and solved a data-race issue for DB changes described in https://github.com/MoveOnOrg/Spoke/pull/1311#issuecomment-568786302

@ibrand ibrand mentioned this pull request Jan 2, 2020
@schuyler1d schuyler1d added the S-qa staging round Status (ADMINS ONLY): PR label for being tested on stage (next step: testing in producgtion) label Jan 2, 2020
@ibrand ibrand merged commit 70b51af into main Jan 2, 2020
@schuyler1d schuyler1d mentioned this pull request Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-qa staging round Status (ADMINS ONLY): PR label for being tested on stage (next step: testing in producgtion) S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants