Summary:
When saving a thread, we weren't properly setting the `participants`
object. Since Sequelize has object properties under getters and setters,
doing `this.participants.push()` did nothing.
Since threads had no `participants`, the related threads widget, which
queries through there, did nothing.
Test Plan: Manually inspect DB and use the related threads widget
Reviewers: halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3938
Summary:
This is the result of auto package.json fixing by lerna. Would be nice to
commit this so you can run script/bootstrap without it making local
changes.
I didn't manually bump any versions.
Test Plan: manual
Reviewers: mark, halla, spang, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3934
Summary:
We don't need to check folders that often while on battery. Check every
5 minutes rather than every 10 seconds.
Test Plan: Run locally, verify the timeout is longer while on battery
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3940
Summary: See title
Test Plan: manual
Reviewers: evan, mark, spang
Reviewed By: spang
Subscribers: mark
Differential Revision: https://phab.nylas.com/D3932
Summary: They obscure the location of our logs in the dev tools.
Test Plan: Run locally, verify that logs link to proper place
Reviewers: evan, spang, juan
Reviewed By: spang, juan
Differential Revision: https://phab.nylas.com/D3939
Summary:
Previously, we would only refresh Google OAuth2 access tokens at the
beginning of the sync loop, and _only_ if the access token had already
expired. This meant that if an access token expired in the middle of a
sync loop iteration, the user would get prompted with the reauth red box
for their account and would have to either go through the oauth flow
again or restart the app for sync to continue.
This diff makes two changes:
1. Adds 5min of padding to the refresh window, so if a token will expire
in <5min, we'll go ahead and refresh the token. This will reduce the
possibility that an access token can expire during a sync loop
iteration.
2. Catches Invalid Credentials IMAPAuthenticationErrors for Gmail
accounts and forces a token refresh on the next sync loop.
These should prevent a user from _ever_ having to reauth their Gmail
account unless the refresh token is revoked, or we encounter some other
permanent error trying to refresh the token.
Fixes T7775 (at least some cases)
Test Plan: manual
Reviewers: khamidou, evan, juan
Reviewed By: juan
Maniphest Tasks: T7775, T7755
Differential Revision: https://phab.nylas.com/D3908
Summary:
Instead of re-implementing exponential backoff, throw the retryable
error so the sync loop handles it and backs-off
Test Plan: manual
Reviewers: spang, halla
Reviewed By: halla
Differential Revision: https://phab.nylas.com/D3914
Summary:
Update Salesforce to use the new search indexer
Depends on D3911
Test Plan: Manually bootup SFDC and ensure it launches and indexes models properly
Reviewers: mark, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3913
Summary: Should help a fair bit with our redis connection pileup.
Test Plan: Tested manually.
Reviewers: mark, spang, juan, evan
Reviewed By: mark, spang, evan
Differential Revision: https://phab.nylas.com/D3916
Summary:
This commit makes it so we always continue retrying syncback tasks as long as they
error with a retryable error. There's really no reason to not continue retrying syncback
tasks after an arbitrary number of retries (especially such a low one) if we
encounter a retryable error. Before this commit, if for example we got 2 random
network errors in a row, we would just mark the task as failed even
though it would eventually succeed in subsequent attempts.
Previously, when N1 synced against the cloud api, we would indefinitely
retry a Task if we continued getting retryable errors. This ensures that
the app can work correctly offline and prevents displaying unecessary errors to
the user, and having actions bouncing back or sending messages without putting them
in the sent folder.
Additionally, this commit ensures that when cleaning up messages without a
folderImapUID we don't delete messages that are currently being added to the sent
folder. This is relevant to this commit because given that we could retry the
EnsureMessageInSentFolder task indefinitely, we might end up deleting that
message because it wont have a uid until the task succeeds.
Depends on D3898
Test Plan: manual
Reviewers: mark, spang, evan, halla
Reviewed By: spang, evan, halla
Differential Revision: https://phab.nylas.com/D3900
Summary:
Since all Nylas Mail actions are thread-based except for sending,
we never use any of these. Make `git grep` less confusing by getting rid
of them.
Test Plan: use the app
Reviewers: evan, halla, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3910
Summary:
Previously, if you were to close the app while you had any tasks queued,
these would be marked as failed the next time you open the app, showing
an annoying error message and reverting any optimisitic actions.
However, we don't need to be so defensive about retrying tasks because
the only tasks we can't retry are the Sending tasks. All of the other
tasks like moving or changing labels are fine to retry (trying move the same
set of uids twice wont cause an error)
This commit adds an extra status to syncback requests, "NOTRETRYABLE".
Only NOTRETRYABLE requests will be marked as failed at the beginning of
the sync loop, and any INPROGRESS tasks will be marked as NEW so they
can be retried
Depends on D3896
Test Plan: manual
Reviewers: mark, evan, spang, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3898
Summary:
Now that we don't run Send tasks outside the sync loop, we don't need
that awful hack wich required passing a `runTask` callback to
`runSyncbackTask` in order to customize how to run the task.
Instead, runSyncbackTask now knows 2 ways to run a task, either via imap, or
via smtp, depending on the resource declared by task to run. So now
SyncbackTasks declare a resource type they need to run, and that will be
passed as their second argument when running.
Depends D3894
Test Plan: manual
Reviewers: mark, halla, spang, evan
Reviewed By: halla, spang, evan
Differential Revision: https://phab.nylas.com/D3896
Summary:
We had previously ripped send tasks outside the sync loop to make them run faster,
but they run fast enough inside the loop.
This commit will also fix the scenario where if you closed the app in the
middle of a send task, the task would just hang forever and never succeed or
fail (T7818); given that it was excluded from the loop, we also had to exclude it
from the cleanup step to mark any INPROGRESS tasks as failed at the beginning
of each loop, which caused send tasks in progress to never get cleaned.
Putting them back inside the loop allows us to fix this without adding more messy
logic, and it cleans up ugly duplicated code. Additionally, it will prevent more
duplicated code for upcoming diffs that will improve syncback task reliability
when the app is closed or window is restarted in the middle of a task.
Depends on D3893
Test Plan:
manually test sending, it still works, it's still fast. Restarted
window in the middle of send task, task fails.
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3894
Summary:
[cloud-api]
Based on the passed in `messageIds`, it finds any existing thread
metadata that might be under a different thread id. If it realizes
there are actually multiple threads that should be the same thread,
(due to getting a missing message link), it reconciles all of them.
[local-sync]
Return `message_ids` in `Thread.toJSON()`
See D3879 for tests
N1 Pairing: D3875
Test Plan: unit tests, local testing soon
Reviewers: juan, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3880
Summary:
Instead of exposing helper functions, make this a class to hold the
shared state of the db, account, and logger required to run any syncback
inside an account sync loop.
Test Plan: manual
Reviewers: mark, spang, halla, evan
Reviewed By: spang, halla, evan
Differential Revision: https://phab.nylas.com/D3893
Summary:
See title.
I really wish we could clean up this error handling a bit better, but I don't
think its super important right now.
Test Plan: manual
Reviewers: spang, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3903
Summary:
I've encountered random imap errors that we mark as permanent, but that
contain try again in the error message. We should check for that
Test Plan: manual
Reviewers: spang, evan, mark
Differential Revision: https://phab.nylas.com/D3899
Summary: There's no file to unlink, we just need to drop the tables.
Test Plan: manual
Reviewers: evan, juan
Reviewed By: evan, juan
Differential Revision: https://phab.nylas.com/D3878
Summary:
If you have no messages in your Gmail Inbox (Yay Inbox Zero!) and you
connect your account and do first sync, then we get an error where we try
and fetch a range from null to -1.
This was due to a logical error in the first sync fetch code.
This diff fixes this bug and renames some variables to make it clearer
what's going on
Fixes T7842
{F11176}
Test Plan:
1. Bring Gmail to Inbox Zero
2. Connect account
3. Verify first sync works
Reviewers: spang, halla, juan
Reviewed By: juan
Maniphest Tasks: T7842
Differential Revision: https://phab.nylas.com/D3889
Summary: See diff title
Test Plan: Run locally, make sure we backoff
Reviewers: juan, spang, evan
Reviewed By: evan
Differential Revision: https://phab.nylas.com/D3886
Summary:
Previously, when processing messages during folder sync, if the message already existed, and it belonged to a thread, we would update the message, but forget to update its thread with any changes that new
message would produce on the thread (e.g. updating the threads folders or labels).
One obvious manifestation of this was when replying to a thread: the EnsureMessageInSentFolderTask would create the new message, and then attempt to sync the sent folder to fetch the newly created message. When processing this message during sync, we would update the message but not update its thread, so the thread would not be associated to the sent folder, and it wouldn't show up in your sent items list in the UI.
Test Plan: manually verify that it works
Reviewers: evan, mark, spang
Reviewed By: mark, spang
Differential Revision: https://phab.nylas.com/D3872
Summary:
Our sentry reporter tries to fetch the nylas identity from the database,
and access properties on it. However, if you are in a state where there
is no identity available (like having logged out, or just starting the
app), and encoutnered an error that would be reported to sentry, we
would throw an error while reporting and that would crash the app
Also, fix lint errors and some really janky code
This fixes T7810
Test Plan: manual
Reviewers: halla, spang, evan
Reviewed By: spang, evan
Maniphest Tasks: T7810
Differential Revision: https://phab.nylas.com/D3867
If the first argument to our local-sync logger is an object
(this is bunyan's api, and it's how we log from isomorphic-core and cloud-* packages
in order to have structured json logs for logstash), make sure we log
the object last and the string that comes as the second argument first.
Summary: Fixes T7649
Test Plan: FML writing unit tests now
Reviewers: evan, mark, juan
Reviewed By: mark, juan
Maniphest Tasks: T7649
Differential Revision: https://phab.nylas.com/D3863
Summary:
This error ocurred, to the best of our knowledge, on iCloud accounts
that had been linked to other clients like Airmail.
On such accounts, node-imap would incorrectly parse the mailbox list
from imap, and return an `Airmail` folder which did not exist, causing
us to try to sync that nonexistent folder and error in the sync loop.
This error is amongst the most frequent we've seen in Sentry and
Support: https://sentry.io/nylas/nylas-mail/issues/213158962/events/4897450600/
The fix es detailed in the PR to node-imap: https://github.com/mscdex/node-imap/pull/594/files
This commit only points the node-imap dependency to our fork for now
Test Plan: manual and unit tests in node-imap
Reviewers: mark, khamidou, spang
Reviewed By: spang
Differential Revision: https://phab.nylas.com/D3860
Summary:
This diff (and the K2 counterpart diff) allow us to run dev-mode Nylas
Mail side-by-side with prod Nylas Mail.
There were 4 things that needed to change:
1. Use different config dir
2. Use different keychain name
3. Use different localhost port
4. Prevent Electron's `app.makeSingleInstance` from killing our app
All of these are activated through `NylasEnv.inDevMode()`.
Test Plan: Manual
Reviewers: halla, mark, spang, khamidou, juan
Reviewed By: juan
Differential Revision: https://phab.nylas.com/D3861
Summary:
In electron, the --enable-logging flag makes it so the main browser
process logs to stdout all of the logs generated from within the renderer
processes.
Unfortunately, the main process will only log out the first argument passed to
`console.log` from within a renderer process (see https://github.com/electron/electron/issues/7061)
This commit makes it so that the local sync logger logs most of the log line in the first
argument passed to `console.log`
Test Plan: manual
Reviewers: evan, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3852
Summary:
When multiple accounts are syncing, it's very hard to scan the local
sync logs because it is unclear to which account the logs belong to,
and it makes debugging hard.
This commit makes it so that all logs from local-sync include the
account info, with the account email prefixed at the beginning of each
log line (this allows filtering), and color coded by account.
Test Plan: manual
Reviewers: mark, spang, khamidou, evan, halla
Reviewed By: evan, halla
Differential Revision: https://phab.nylas.com/D3851
Summary:
On each sync loop, we increment the socketTimeout based on how many times we've
seen socket timeouts in a row. The max socket timeout is 10m
Test Plan: manual
Reviewers: evan, spang, mark
Reviewed By: mark
Differential Revision: https://phab.nylas.com/D3843
Summary:
In most cases (and especially so on Gmail and in the inbox on generic
IMAP), messages with higher UIDs are newer---and even if they aren't the
newest possible messages in other generic IMAP folders, they are the
most recent messages that have been moved to that folder.
Our previous batching strategy unfortunately resulted in us downloading
the lowest UID in each batch first, which was especially confusing when
connecting a new account and having the first message pop up on the
screen be a message from hours or days ago.
This patch changes the batching strategy in three ways:
1. Within a batch, we process downloaded messages from highest UID to
lowest UID.
2. We download batches in order of the ones containing the highest UIDs
first.
3. We group together more UIDs within a single batch by ignoring charset
and transfer-encoding on parts and grouping only by MIME part IDs (which
is the only thing you have to pass to the IMAP FETCH command---no idea
why we included this extraneous part data before, probably just
convenience.)
Example old grouping:
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"UTF-8","mimeType":"text/html"}]'
batch UIDs: [356416,356418,356420,356423,356432,356433,356435,356436,356437,356442,356444]
batch key: '[{"id":"2","transferEncoding":"QUOTED-PRINTABLE","charset":"Windows-1252","mimeType":"text/html"}]'
batch UIDs: [353777]
In the new strategy, all of these messages will be downloaded with the
same FETCH command, reducing IMAP round trips before message processing
begins.
Fixes T7770
Test Plan: manual - connect a new account and see that most recent message downloads first
Reviewers: mark, evan, juan
Reviewed By: juan
Maniphest Tasks: T7770
Differential Revision: https://phab.nylas.com/D3838
Summary:
`isSending` was an artifact from the cloud sync engine that was used to
double check that the sending process for a multi-send draft had been
initiated. I don't believe the intermediate steps are API calls anymore,
and we've had the relevant code commented out for awhile. Time to kill it!
I've revived the double-sending tests in N1 to ensure we still have sufficient
checks against sending the draft again while the first call is still sending.
See D3834
Test Plan: N1 unit tests
Reviewers: juan, evan
Reviewed By: juan, evan
Differential Revision: https://phab.nylas.com/D3835
