-
Notifications
You must be signed in to change notification settings - Fork 17
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
Ingest emails from Gmail and send to Airtable #4
Conversation
this may be a pyrric victory over zapier :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A pyrrhic victory over Zapier I think. Thoughts?
} catch (e) { | ||
console.error('Failed to poll gmail. Rescheduling\n %O', e); | ||
} | ||
setTimeout(syncGmail, interval); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any node scheduled task runners you like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. dealers choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok in that case I think maybe leave this as the simplest possible implementation and then think about it when/if the system grows another recurring task?
src/airtable.js
Outdated
* so we would like a single place to map the name to an internal | ||
* symbol: "Email Address" -> "emailAddress" | ||
*/ | ||
airtableBindings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand it didn't feel good duplicating the hardcoded airtable field strings all over the place. On the other hand I feel like there needs to be... a better solution. Ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i feel like hardcoded strings might be our best bet. we need some normalization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll get rid of this complexity and just hardcode some strings. When the app starts dealing with Airtable more comprehensively we can rethink it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is eslint working for you? (i think vscode might still be automagicing for me). The difference that popped out is single quotes instead of double quotes. feel free to change any eslint rules
src/airtable.js
Outdated
* so we would like a single place to map the name to an internal | ||
* symbol: "Email Address" -> "emailAddress" | ||
*/ | ||
airtableBindings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, i feel like hardcoded strings might be our best bet. we need some normalization
} catch (e) { | ||
console.error('Failed to poll gmail. Rescheduling\n %O', e); | ||
} | ||
setTimeout(syncGmail, interval); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. dealers choice
This is a gorgeous pull request. some general context for my thoughts: around monday of this week, I was pushing hard for anything but airtable. mainly for privacy concerns, but also because I knew issues like this would pop up; and also because I wanted to make something that other MA groups could easily deploy. I stopped advocating because I wanted to onboard volunteers fast (airtable flow already exists), and move fast in general (airtable forms are great, schema changes easy). I think I still agree with this. We should work on quickly building the crown heights community instead of a more general approach. of course this may also come at the cost of testing and documentation as well. The dev environment should be as sleek as possible though. we havent had a conversation about this yet, I'd love to hear thoughts
@mab253 what's the situation?
If I understand, I think the answer is all info should be added by an intake volunteer into the airtable. We need to come up with a deletion policy as well.
how's the development environment for zapier? I think if we did go down that route, it should almost definitely be a different repo so we don't need to target another node version.
See the context paragraph I posted above. Think maybe we should document as much as possible, but maybe relax the ease of setup requirement. if you have thoughts on moving out of a monorepo, I'd be interested, but the fewer api keys the better (this coming from someone who just added two api dependencies) |
hiiiii i finally have a chance to look @ all this - awesome, awesome work. a few things:
so: what do we think? is this 1 of the few times we use Zapier (..😱)?! |
TLDR: I agree with using Zapier for the gmail intake. @alexquick I'm so sorry to move back on this awesome PR. We can keep the saveRecords part of airtable.js tho correct?
I think the UI-only actually bothers me the most. If we keep the Zapier work to less-used flows, then could we mirror instructions on setting up in a separate github repo? I think a good goal is easy setup for other groups. But overall I agree with Zapier for less-used flows that have high overhead Related: I think it would be cool if we renamed the repos to what they do. So twilio becomes -> crownheightsaid/request-intake-twilio and gmail could become crownheightsaid/request-intake-gmail. Renaming repos is super easy
Totally agree! I think we're doing a good job of keeping privacy implications in mind |
The best code is code you get to delete! Git never forgets, so if this becomes an issue going forward we can resurrect this. |
Well I'm glad we all really hate zapier, because it was way more work than I thought it was going to be (isn't it always) to link this all together.
General flow
$labelname-tracked
label so that it doesn't end up saving that message again if it's restarted (and to avoid having to use Airtable to de-duplicate data-- though that's definitely an option if we end up having issues).Questions
Resolves: #3
[1] Could easily be a separate process: unset SYNC_GMAIL, add
worker: npm run gmail-sync
to Procfile, and scale the worker up withps:scale
[2] Multipart, embedded content, and text/html are a pain!
[3] It's a bummer. . You need to make a new OAuth client for your account (individually) and then do a song and dance to get a refresh token. Alternatively, we could look into using IMAP instead of the Gmail API. The downside here is the credentials, IMAP will use the password of the gmail account (or if the account has 2FA enabled an app-specific password... but 2FA for 'shared' accounts is difficult..)