Mailspring/spec/tasks/task-spec.coffee

221 lines
8.6 KiB
CoffeeScript
Raw Normal View History

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
Actions = require '../../src/flux/actions'
TaskQueue = require '../../src/flux/stores/task-queue'
Task = require '../../src/flux/tasks/task'
{APIError,
OfflineError,
TimeoutError} = require '../../src/flux/errors'
noop = ->
describe "Task", ->
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 ->
class APITestTask extends Task
performLocal: -> Promise.resolve()
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
performRemote: -> 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
@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 ->
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
class OKTask extends Task
performRemote: -> Promise.resolve(Task.Status.Retry)
@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)
class BadTask extends Task
performRemote: -> Promise.resolve('lalal')
@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 ->
spyOn(atom, "emitError")
@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)
expect(atom.emitError).not.toHaveBeenCalled()
describe "when performRemote resolves with Task.Status.Failed and an error", ->
beforeEach ->
spyOn(atom, "emitError")
@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)
expect(atom.emitError).not.toHaveBeenCalled()
describe "when performRemote rejects with Task.Status.Failed", ->
beforeEach ->
spyOn(atom, "emitError")
@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)
expect(atom.emitError).not.toHaveBeenCalled()
describe "when performRemote throws an unknown error", ->
beforeEach ->
spyOn(atom, "emitError")
@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)
expect(atom.emitError).toHaveBeenCalledWith(@error)