Mailspring/spec/tasks/task-spec.coffee

214 lines
8.4 KiB
CoffeeScript
Raw Normal View History

Actions = require('../../src/flux/actions').default
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
TaskQueue = require '../../src/flux/stores/task-queue'
Task = require('../../src/flux/tasks/task').default
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
{APIError} = require '../../src/flux/errors'
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
{APITestTask, OKTask, BadTask} = require('../stores/task-subclass')
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
noop = ->
fix(spec): add support for async specs and disable misbehaving ones More spec fixes replace process.nextTick with setTimeout(fn, 0) for specs Also added an unspy in the afterEach Temporarily disable specs fix(spec): start fixing specs Summary: This is the WIP fix to our spec runner. Several tests have been completely commented out that will require substantially more work to fix. These have been added to our sprint backlog. Other tests have been fixed to update to new APIs or to deal with genuine bugs that were introduced without our knowing! The most common non-trivial change relates to observing the `NylasAPI` and `NylasAPIRequest`. We used to observe the arguments to `makeRequest`. Unfortunately `NylasAPIRequest.run` is argumentless. Instead you can do: `NylasAPIRequest.prototype.run.mostRecentCall.object.options` to get the `options` passed into the object. the `.object` property grabs the context of the spy when it was last called. Fixing these tests uncovered several concerning issues with our test runner. I spent a while tracking down why our participant-text-field-spec was failling every so often. I chose that spec because it was the first spec to likely fail, thereby requiring looking at the least number of preceding files. I tried binary searching, turning on and off, several files beforehand only to realize that the failure rate was not determined by a particular preceding test, but rather the existing and quantity of preceding tests, AND the number of console.log statements I had. There is some processor-dependent race condition going on that needs further investigation. I also discovered an issue with the file-download-spec. We were getting errors about it accessing a file, which was very suspicious given the code stubs out all fs access. This was caused due to a spec that called an async function outside ot a `waitsForPromise` block or a `waitsFor` block. The test completed, the spies were cleaned up, but the downstream async chain was still running. By the time the async chain finished the runner was already working on the next spec and the spies had been restored (causing the real fs access to run). Juan had an idea to kill the specs once one fails to prevent cascading failures. I'll implement this in the next diff update Test Plan: npm test Reviewers: juan, halla, jackie Differential Revision: https://phab.nylas.com/D3501 Disable other specs Disable more broken specs All specs turned off till passing state Use async-safe versions of spec functions Add async test spec Remove unused package code Remove canary spec
2016-12-13 04:12:20 +08:00
xdescribe "Task", ->
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
describe "initial state", ->
it "should set up queue state with additional information about local/remote", ->
task = new Task()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
expect(task.queueState).toEqual({ isProcessing : false, localError : null, localComplete : false, remoteError : null, remoteAttempts : 0, remoteComplete : false, status: null, debugStatus: Task.DebugStatus.JustConstructed})
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
describe "runLocal", ->
beforeEach ->
@task = new APITestTask()
describe "when performLocal is not complete", ->
it "should run performLocal", ->
spyOn(@task, 'performLocal').andCallThrough()
@task.runLocal()
expect(@task.performLocal).toHaveBeenCalled()
describe "when performLocal rejects", ->
beforeEach ->
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
spyOn(NylasEnv, "reportError")
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
spyOn(@task, 'performLocal').andCallFake =>
Promise.reject(new Error("Oh no!"))
it "should save the error to the queueState", ->
@task.runLocal().catch(noop)
advanceClock()
expect(@task.performLocal).toHaveBeenCalled()
expect(@task.queueState.localComplete).toBe(false)
expect(@task.queueState.localError.message).toBe("Oh no!")
it "should reject with the error", ->
rejection = null
runs ->
@task.runLocal().catch (err) ->
rejection = err
waitsFor ->
rejection
runs ->
expect(rejection.message).toBe("Oh no!")
describe "when performLocal resolves", ->
beforeEach ->
spyOn(@task, 'performLocal').andCallFake -> Promise.resolve('Hooray')
it "should save that performLocal is complete", ->
@task.runLocal()
advanceClock()
expect(@task.queueState.localComplete).toBe(true)
it "should save that there was no performLocal error", ->
@task.runLocal()
advanceClock()
expect(@task.queueState.localError).toBe(null)
describe "runRemote", ->
beforeEach ->
@task.queueState.localComplete = true
it "should run performRemote", ->
spyOn(@task, 'performRemote').andCallThrough()
@task.runRemote()
advanceClock()
expect(@task.performRemote).toHaveBeenCalled()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
it "it should resolve Continue if it already ran", ->
@task.queueState.remoteComplete = true
waitsForPromise =>
@task.runRemote().then (status) =>
expect(@task.queueState.status).toBe Task.Status.Continue
expect(status).toBe Task.Status.Continue
it "marks as complete if the task 'continue's", ->
spyOn(@task, 'performRemote').andCallFake ->
Promise.resolve(Task.Status.Continue)
@task.runRemote()
advanceClock()
expect(@task.performRemote).toHaveBeenCalled()
expect(@task.queueState.remoteError).toBe(null)
expect(@task.queueState.remoteComplete).toBe(true)
expect(@task.queueState.status).toBe(Task.Status.Continue)
it "marks as failed if the task reverts", ->
spyOn(@task, 'performRemote').andCallFake ->
Promise.resolve(Task.Status.Failed)
@task.runRemote()
advanceClock()
expect(@task.performRemote).toHaveBeenCalled()
expect(@task.queueState.remoteError).toBe(null)
expect(@task.queueState.remoteComplete).toBe(true)
expect(@task.queueState.status).toBe(Task.Status.Failed)
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
describe "when performRemote resolves", ->
beforeEach ->
spyOn(@task, 'performRemote').andCallFake ->
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
Promise.resolve(Task.Status.Success)
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
it "should save that performRemote is complete with no errors", ->
@task.runRemote()
advanceClock()
expect(@task.performRemote).toHaveBeenCalled()
expect(@task.queueState.remoteError).toBe(null)
expect(@task.queueState.remoteComplete).toBe(true)
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
expect(@task.queueState.status).toBe(Task.Status.Success)
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
it "should only allow the performRemote method to return a Task.Status", ->
result = null
err = null
@ok = new OKTask()
@ok.queueState.localComplete = true
@ok.runRemote().then (r) -> result = r
advanceClock()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
expect(@ok.queueState.status).toBe(Task.Status.Retry)
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
expect(result).toBe(Task.Status.Retry)
@bad = new BadTask()
@bad.queueState.localComplete = true
@bad.runRemote().catch (e) -> err = e
advanceClock()
expect(err.message).toBe('performRemote returned lalal, which is not a Task.Status')
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
describe "when performRemote rejects multiple times", ->
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
beforeEach ->
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
spyOn(@task, 'performRemote').andCallFake =>
Promise.resolve(Task.Status.Failed)
feat(offline-mode, undo-redo): Tasks handle network errors better and retry, undo/redo based on tasks Summary: This diff does a couple things: - Undo redo with a new undo/redo store that maintains it's own queue of undo/redo tasks. This queue is separate from the TaskQueue because not all tasks should be considered for undo history! Right now just the AddRemoveTagsTask is undoable. - NylasAPI.makeRequest now returns a promise which resolves with the result or rejects with an error. For things that still need them, there's still `success` and `error` callbacks. I also added `started:(req) ->` which allows you to get the underlying request. - Aborting a NylasAPI request now makes it call it's error callback / promise reject. - You can now run code after perform local has completed using this syntax: ``` task = new AddRemoveTagsTask(focused, ['archive'], ['inbox']) task.waitForPerformLocal().then -> Actions.setFocus(collection: 'thread', item: nextFocus) Actions.setCursorPosition(collection: 'thread', item: nextKeyboard) Actions.queueTask(task) ``` - In specs, you can now use `advanceClock` to get through a Promise.then/catch/finally. Turns out it was using something low level and not using setTimeout(0). - The TaskQueue uses promises better and defers a lot of the complexity around queueState for performLocal/performRemote to a task subclass called APITask. APITask implements "perform" and breaks it into "performLocal" and "performRemote". - All tasks either resolve or reject. They're always removed from the queue, unless they resolve with Task.Status.Retry, which means they internally did a .catch (err) => Promise.resolve(Task.Status.Retry) and they want to be run again later. - API tasks retry until they succeed or receive a NylasAPI.PermanentErrorCode (400,404,500), in which case they revert and finish. - The AddRemoveTags Task can now take more than one thread! This is super cool because you can undo/redo a bulk action and also because we'll probably have a bulk tag modification API endpoint soon. Getting undo / redo working revealed that the thread versioning system we built isn't working because the server was incrementing things by more than 1 at a time. Now we count the number of unresolved "optimistic" changes we've made to a given model, and only accept the server's version of it once the number of optimistic changes is back at zero. Known Issues: - AddRemoveTagsTasks aren't dependent on each other, so if you (undo/redo x lots) and then come back online, all the tasks try to add / remove all the tags at the same time. To fix this we can either allow the tasks to be merged together into a minimal set or make them block on each other. - When Offline, you still get errors in the console for GET requests. Need to catch these and display an offline status bar. - The metadata tasks haven't been updated yet to the new API. Wanted to get it reviewed first! Test Plan: All the tests still pass! Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1694
2015-07-08 01:38:53 +08:00
it "should increment the number of attempts", ->
runs ->
@task.runRemote().catch(noop)
waitsFor ->
@task.queueState.remoteAttempts == 1
runs ->
@task.runRemote().catch(noop)
waitsFor ->
@task.queueState.remoteAttempts == 2
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
describe "when performRemote resolves with Task.Status.Failed", ->
beforeEach ->
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
spyOn(NylasEnv, "reportError")
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
@error = new APIError("Oh no!")
spyOn(@task, 'performRemote').andCallFake =>
Promise.resolve(Task.Status.Failed)
it "Should handle the error as a caught Failure", ->
waitsForPromise =>
@task.runRemote().then ->
throw new Error("Should not resolve")
.catch (err) =>
expect(@task.queueState.remoteError instanceof Error).toBe true
expect(@task.queueState.remoteAttempts).toBe(1)
expect(@task.queueState.status).toBe(Task.Status.Failed)
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
expect(NylasEnv.reportError).not.toHaveBeenCalled()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
describe "when performRemote resolves with Task.Status.Failed and an error", ->
beforeEach ->
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
spyOn(NylasEnv, "reportError")
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
@error = new APIError("Oh no!")
spyOn(@task, 'performRemote').andCallFake =>
Promise.resolve([Task.Status.Failed, @error])
it "Should handle the error as a caught Failure", ->
waitsForPromise =>
@task.runRemote().then ->
throw new Error("Should not resolve")
.catch (err) =>
expect(@task.queueState.remoteError).toBe(@error)
expect(@task.queueState.remoteAttempts).toBe(1)
expect(@task.queueState.status).toBe(Task.Status.Failed)
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
expect(NylasEnv.reportError).not.toHaveBeenCalled()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
describe "when performRemote rejects with Task.Status.Failed", ->
beforeEach ->
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
spyOn(NylasEnv, "reportError")
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
@error = new APIError("Oh no!")
spyOn(@task, 'performRemote').andCallFake =>
Promise.reject([Task.Status.Failed, @error])
it "Should handle the rejection as normal", ->
waitsForPromise =>
@task.runRemote().then ->
throw new Error("Should not resolve")
.catch (err) =>
expect(@task.queueState.remoteError).toBe(@error)
expect(@task.queueState.remoteAttempts).toBe(1)
expect(@task.queueState.status).toBe(Task.Status.Failed)
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
expect(NylasEnv.reportError).not.toHaveBeenCalled()
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
describe "when performRemote throws an unknown error", ->
beforeEach ->
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
spyOn(NylasEnv, "reportError")
fix(tasks): don't continue if dependent task fails Summary: Fixes T4291 If I made a final edit to a pre-existing draft and sent, we'd queue a `SyncbackDraftTask` before a `SendDraftTask`. This is important because since we have a valid draft `server_id`, the `SendDraftTask` will send by server_id, not by POSTing the whole body. If the `SyncbackDraftTask` fails, then we had a very serious issue whereby the `SendDraftTask` would keep on sending. Unfortunately the server never got the latest changes and sent the wrong version of the draft. This incorrect version would show up later when the `/send` endpoint returned the message that got actually sent. The solution was to make any queued `SendDraftTask` fail if a dependent `SyncbackDraftTask` failed. This meant we needed to make the requirements for `shouldWaitForTask` stricter, and block if tasks failed. Unfortunatley there was no infrastructure in place to do this. The first change was to change `shouldWaitForTask` to `isDependentTask`. If we're going to fail when a dependent task fails, I wanted the method name to reflect this. Now, if a dependent task fails, we recursively check the dependency tree (and check for cycles) and `dequeue` anything that needed that to succeed. I chose `dequeue` as the default action because it seemed as though all current uses of `shouldWaitForTask` really should bail if their dependencies fail. It's possible you don't want your task dequeued in this dependency case. You can return the special `Task.DO_NOT_DEQUEUE_ME` constant from the `onDependentTaskError` method. When a task gets dequeued because of the reason above, the `onDependentTaskError` callback gets fired. This gives tasks like the `SendDraftTask` a chance to notify the user that it bailed. Not all tasks need to notify. The next big issue was a better way to determine if a task truely errored to the point that we need to dequeue dependencies. In the Developer Status area we were showing tasks that had errored as "Green" because we caught the error and resolved with `Task.Status.Finished`. This used to be fine since nothing life-or-death cared if a task errored or not. Now that it might cause abortions down the line, we needed a more robust method then this. For one I changed `Task.Status.Finished` to a variety of finish types including `Task.Status.Success`. The way you "error" out is to `throw` or `Promise.reject` an `Error` object from the `performRemote` method. This allows us to propagate API errors up, and acts as a safety net that can catch any malformed code or unexpected responses. The developer bar now shows a much richer set of statuses instead of a binary one, which was REALLY helpful in debugging this. We also record when a Task got dequeued because of the conditions introduced here. Once all this was working we still had an issue of sending old drafts. If after a `SyncbackDraftTask` failed, now we'd block the send and notify the users as such. However, if we tried to send again, there was a separate issue whereby we wouldn't queue another `SyncbackDraftTask` to update the server with the latest information. Since our changes were persisted to the DB, we thought we had no changes, and therefore didn't need to queue a `SyncbackDraftTask`. The fix to this is to always force the creation of a `SyncbackDraftTask` before send regardless of the state of the `DraftStoreProxy`. Test Plan: new tests. Lots of manual testing Reviewers: bengotow Reviewed By: bengotow Subscribers: mg Maniphest Tasks: T4291 Differential Revision: https://phab.nylas.com/D2156
2015-10-22 01:33:43 +08:00
@error = new Error("Oh no!")
spyOn(@task, 'performRemote').andCallFake =>
throw @error
it "Should handle the error as an uncaught error", ->
waitsForPromise =>
@task.runRemote().then ->
throw new Error("Should not resolve")
.catch (err) =>
expect(@task.queueState.remoteError).toBe(@error)
expect(@task.queueState.remoteAttempts).toBe(1)
expect(@task.queueState.status).toBe(Task.Status.Failed)
expect(@task.queueState.debugStatus).toBe(Task.DebugStatus.UncaughtError)
feat(error): improve error reporting. Now `NylasEnv.reportError` Summary: The goal is to let us see what plugins are throwing errors on Sentry. We are using a Sentry `tag` to identify and group plugins and their errors. Along the way, I cleaned up the error catching and reporting system. There was a lot of duplicate error logic (that wasn't always right) and some legacy Atom error handling. Now, if you catch an error that we should report (like when handling extensions), call `NylasEnv.reportError`. This used to be called `emitError` but I changed it to `reportError` to be consistent with the ErrorReporter and be a bit more indicative of what it does. In the production version, the `ErrorLogger` will forward the request to the `nylas-private-error-reporter` which will report to Sentry. The `reportError` function also now inspects the stack to determine which plugin(s) it came from. These are passed along to Sentry. I also cleaned up the `console.log` and `console.error` code. We were logging errors multiple times making the console confusing to read. Worse is that we were logging the `error` object, which would print not the stack of the actual error, but rather the stack of where the console.error was logged from. Printing `error.stack` instead shows much more accurate stack traces. See changes in the Edgehill repo here: https://github.com/nylas/edgehill/commit/8c4a86eb7ee1a06249a9ae35397e2084a09ad1dc Test Plan: Manual Reviewers: juan, bengotow Reviewed By: bengotow Differential Revision: https://phab.nylas.com/D2509
2016-02-04 07:06:52 +08:00
expect(NylasEnv.reportError).toHaveBeenCalledWith(@error)