Merge pull request #588 from mirumee/fix/task-queue

Handle task failure and fix task duplication

CHANGELOG.md

@@ -58,6 +58,7 @@ All notable, unreleased changes to this project will be documented in this file.
 - Handle untracked products - #523 by @dominik-zeglen
 - Display correct error if there were no graphql errors - #525 by @dominik-zeglen
 - Add background task manager - #574 by @dominik-zeglen
+- Handle task failure and fix task duplication - #588 by @dominik-zeglen
 
 ## 2.0.0
 

package-lock.json

@@ -16014,6 +16014,12 @@
         }
       }
     },
+    "mock-apollo-client": {
+      "version": "0.4.0",
+      "resolved": "https://registry.npmjs.org/mock-apollo-client/-/mock-apollo-client-0.4.0.tgz",
+      "integrity": "sha512-cHznpkX8uUClkWWJMpgdDWzEgjacM85xt69S9gPLrssM8Vahas0QmEJkFUycrRQyBkaqxvRe58Bg3a5pOvj2zA==",
+      "dev": true
+    },
     "moment": {
       "version": "2.24.0",
       "resolved": "https://registry.npmjs.org/moment/-/moment-2.24.0.tgz",

package.json

@@ -131,6 +131,7 @@
     "jest": "^24.8.0",
     "jest-file": "^1.0.0",
     "lint-staged": "^9.4.2",
+    "mock-apollo-client": "^0.4.0",
     "prettier": "^1.19.1",
     "react-intl-translations-manager": "^5.0.3",
     "react-test-renderer": "^16.12.0",

src/containers/BackgroundTasks/BackgroundTasksProvider.test.tsx

@@ -1,22 +1,34 @@
 import { renderHook } from "@testing-library/react-hooks";
+import { createMockClient } from "mock-apollo-client";
 
 import {
   backgroundTasksRefreshTime,
   useBackgroundTasks
 } from "./BackgroundTasksProvider";
-import { Task, TaskData } from "./types";
+import { OnCompletedTaskData, Task, TaskData, TaskStatus } from "./types";
 
 jest.useFakeTimers();
 
+function renderBackgroundTasks() {
+  const mockClient = createMockClient();
+  const intl = {
+    formatMessage: ({ defaultMessage }) => defaultMessage
+  };
+
+  return renderHook(() =>
+    useBackgroundTasks(mockClient, jest.fn(), intl as any)
+  );
+}
+
 describe("Background task provider", () => {
   it("can queue a task", done => {
-    const handle = jest.fn<Promise<boolean>, []>(
-      () => new Promise(resolve => resolve(true))
+    const handle = jest.fn<Promise<TaskStatus>, []>(
+      () => new Promise(resolve => resolve(TaskStatus.SUCCESS))
     );
     const onCompleted = jest.fn();
     const onError = jest.fn();
 
-    const { result } = renderHook(useBackgroundTasks);
+    const { result } = renderBackgroundTasks();
 
     const taskId = result.current.queue(Task.CUSTOM, {
       handle,
@@ -24,23 +36,23 @@ describe("Background task provider", () => {
       onError
     });
     expect(taskId).toBe(1);
-    expect(handle).not.toHaveBeenCalled();
-    expect(onCompleted).not.toHaveBeenCalled();
-    expect(onError).not.toHaveBeenCalled();
+    expect(handle).toHaveBeenCalledTimes(0);
+    expect(onCompleted).toHaveBeenCalledTimes(0);
+    expect(onError).toHaveBeenCalledTimes(0);
 
     jest.runOnlyPendingTimers();
 
     setImmediate(() => {
-      expect(handle).toHaveBeenCalled();
-      expect(onCompleted).toHaveBeenCalled();
-      expect(onError).not.toHaveBeenCalled();
+      expect(handle).toHaveBeenCalledTimes(1);
+      expect(onCompleted).toHaveBeenCalledTimes(1);
+      expect(onError).toHaveBeenCalledTimes(0);
 
       done();
     });
   });
 
   it("can handle task error", done => {
-    const handle = jest.fn<Promise<boolean>, []>(
+    const handle = jest.fn<Promise<TaskStatus>, []>(
       () =>
         new Promise(() => {
           throw new Error("dummy error");
@@ -49,7 +61,7 @@ describe("Background task provider", () => {
     const onCompleted = jest.fn();
     const onError = jest.fn();
 
-    const { result } = renderHook(useBackgroundTasks);
+    const { result } = renderBackgroundTasks();
 
     result.current.queue(Task.CUSTOM, {
       handle,
@@ -60,9 +72,36 @@ describe("Background task provider", () => {
     jest.runOnlyPendingTimers();
 
     setImmediate(() => {
-      expect(handle).toHaveBeenCalled();
-      expect(onCompleted).not.toHaveBeenCalled();
-      expect(onError).toHaveBeenCalled();
+      expect(handle).toHaveBeenCalledTimes(1);
+      expect(onCompleted).toHaveBeenCalledTimes(0);
+      expect(onError).toHaveBeenCalledTimes(1);
+
+      done();
+    });
+  });
+
+  it("can handle task failure", done => {
+    const handle = jest.fn<Promise<TaskStatus>, []>(
+      () => new Promise(resolve => resolve(TaskStatus.FAILURE))
+    );
+    const onCompleted = jest.fn((data: OnCompletedTaskData) =>
+      expect(data.status).toBe(TaskStatus.FAILURE)
+    );
+    const onError = jest.fn();
+
+    const { result } = renderBackgroundTasks();
+
+    result.current.queue(Task.CUSTOM, {
+      handle,
+      onCompleted,
+      onError
+    });
+
+    jest.runOnlyPendingTimers();
+
+    setImmediate(() => {
+      expect(handle).toHaveBeenCalledTimes(1);
+      expect(onCompleted).toHaveBeenCalledTimes(1);
 
       done();
     });
@@ -71,10 +110,10 @@ describe("Background task provider", () => {
   it("can cancel task", done => {
     const onCompleted = jest.fn();
 
-    const { result } = renderHook(useBackgroundTasks);
+    const { result } = renderBackgroundTasks();
 
     const taskId = result.current.queue(Task.CUSTOM, {
-      handle: () => new Promise(resolve => resolve(true)),
+      handle: () => new Promise(resolve => resolve(TaskStatus.SUCCESS)),
       onCompleted
     });
 
@@ -85,51 +124,65 @@ describe("Background task provider", () => {
     jest.runOnlyPendingTimers();
 
     setImmediate(() => {
-      expect(onCompleted).not.toHaveBeenCalled();
+      expect(onCompleted).toHaveBeenCalledTimes(0);
 
       done();
     });
   });
 
   it("can queue multiple tasks", done => {
-    const responses: Array<Promise<boolean>> = [
-      new Promise(resolve =>
-        setTimeout(() => resolve(true), backgroundTasksRefreshTime * 1.4)
+    let cycle = 0;
+
+    // Completed in two cycles
+    const shortTask = {
+      handle: jest.fn(() =>
+        Promise.resolve(cycle > 1 ? TaskStatus.SUCCESS : TaskStatus.PENDING)
       ),
-      new Promise(resolve =>
-        setTimeout(() => resolve(true), backgroundTasksRefreshTime * 2.1)
-      )
-    ];
-
-    const tasks: TaskData[] = responses.map(response => ({
-      handle: () => response,
       onCompleted: jest.fn()
-    }));
+    };
 
-    const { result } = renderHook(useBackgroundTasks);
+    // Completed in three cycles
+    const longTask = {
+      handle: jest.fn(() =>
+        Promise.resolve(cycle > 2 ? TaskStatus.SUCCESS : TaskStatus.PENDING)
+      ),
+      onCompleted: jest.fn()
+    };
+    const tasks: TaskData[] = [shortTask, longTask];
+
+    const { result } = renderBackgroundTasks();
 
     tasks.forEach(task => result.current.queue(Task.CUSTOM, task));
 
     // Set time to backgroundTasksRefreshTime
+    cycle += 1;
     jest.advanceTimersByTime(backgroundTasksRefreshTime + 100);
 
     setImmediate(() => {
-      expect(tasks[0].onCompleted).not.toHaveBeenCalled();
-      expect(tasks[1].onCompleted).not.toHaveBeenCalled();
+      expect(shortTask.handle).toHaveBeenCalledTimes(1);
+      expect(longTask.handle).toHaveBeenCalledTimes(1);
+      expect(shortTask.onCompleted).toHaveBeenCalledTimes(0);
+      expect(longTask.onCompleted).toHaveBeenCalledTimes(0);
 
       // Set time to backgroundTasksRefreshTime * 2
+      cycle += 1;
       jest.advanceTimersByTime(backgroundTasksRefreshTime);
 
       setImmediate(() => {
-        expect(tasks[0].onCompleted).toHaveBeenCalled();
-        expect(tasks[1].onCompleted).not.toHaveBeenCalled();
+        expect(shortTask.handle).toHaveBeenCalledTimes(2);
+        expect(longTask.handle).toHaveBeenCalledTimes(2);
+        expect(shortTask.onCompleted).toHaveBeenCalledTimes(1);
+        expect(longTask.onCompleted).toHaveBeenCalledTimes(0);
 
         // Set time to backgroundTasksRefreshTime * 3
+        cycle += 1;
         jest.advanceTimersByTime(backgroundTasksRefreshTime);
 
         setImmediate(() => {
-          expect(tasks[0].onCompleted).toHaveBeenCalled();
-          expect(tasks[1].onCompleted).toHaveBeenCalled();
+          expect(shortTask.handle).toHaveBeenCalledTimes(2);
+          expect(longTask.handle).toHaveBeenCalledTimes(3);
+          expect(shortTask.onCompleted).toHaveBeenCalledTimes(1);
+          expect(longTask.onCompleted).toHaveBeenCalledTimes(1);
 
           done();
         });

src/containers/BackgroundTasks/BackgroundTasksProvider.tsx

@@ -1,4 +1,9 @@
+import { IMessageContext } from "@saleor/components/messages";
+import useNotifier from "@saleor/hooks/useNotifier";
+import ApolloClient from "apollo-client";
 import React from "react";
+import { useApolloClient } from "react-apollo";
+import { IntlShape, useIntl } from "react-intl";
 
 import BackgroundTasksContext from "./context";
 import { handleTask, queueCustom } from "./tasks";
@@ -6,31 +11,35 @@ import { QueuedTask, Task, TaskData, TaskStatus } from "./types";
 
 export const backgroundTasksRefreshTime = 15 * 1000;
 
-export function useBackgroundTasks() {
+// TODO: Remove underscores when working on #575 or similar PR
+export function useBackgroundTasks(
+  _apolloClient: ApolloClient<any>,
+  _notify: IMessageContext,
+  _intl: IntlShape
+) {
   const idCounter = React.useRef(0);
   const tasks = React.useRef<QueuedTask[]>([]);
 
   React.useEffect(() => {
     const intervalId = setInterval(() => {
       const queue = async () => {
-        tasks.current = tasks.current.filter(
-          task => task.status !== TaskStatus.ENDED
-        );
         try {
           await Promise.all(
             tasks.current.map(async task => {
-              let hasFinished: boolean;
+              if (task.status === TaskStatus.PENDING) {
+                let status: TaskStatus;
 
-              try {
-                hasFinished = await handleTask(task);
-              } catch (error) {
-                throw error;
-              }
-              if (hasFinished) {
-                const taskIndex = tasks.current.findIndex(
-                  t => t.id === task.id
-                );
-                tasks.current[taskIndex].status = TaskStatus.ENDED;
+                try {
+                  status = await handleTask(task);
+                } catch (error) {
+                  throw error;
+                }
+                if (status !== TaskStatus.PENDING) {
+                  const taskIndex = tasks.current.findIndex(
+                    t => t.id === task.id
+                  );
+                  tasks.current[taskIndex].status = status;
+                }
               }
             })
           );
@@ -51,10 +60,10 @@ export function useBackgroundTasks() {
 
   function queue(type: Task, data?: TaskData) {
     idCounter.current += 1;
-
     switch (type) {
       case Task.CUSTOM:
         queueCustom(idCounter.current, tasks, data);
+        break;
     }
 
     return idCounter.current;
@@ -67,7 +76,10 @@ export function useBackgroundTasks() {
 }
 
 const BackgroundTasksProvider: React.FC = ({ children }) => {
-  const { cancel, queue } = useBackgroundTasks();
+  const apolloClient = useApolloClient();
+  const notify = useNotifier();
+  const intl = useIntl();
+  const { cancel, queue } = useBackgroundTasks(apolloClient, notify, intl);
 
   return (
     <BackgroundTasksContext.Provider

src/containers/BackgroundTasks/tasks.ts

@@ -1,17 +1,19 @@
 import { QueuedTask, TaskData, TaskStatus } from "./types";
 
-export async function handleTask(task: QueuedTask): Promise<boolean> {
-  let ok = false;
+export async function handleTask(task: QueuedTask): Promise<TaskStatus> {
+  let status = TaskStatus.PENDING;
   try {
-    ok = await task.handle();
-    if (ok) {
-      task.onCompleted();
+    status = await task.handle();
+    if (status !== TaskStatus.PENDING) {
+      task.onCompleted({
+        status
+      });
     }
   } catch (error) {
     task.onError(error);
   }
 
-  return ok;
+  return status;
 }
 
 export function handleError(error: Error) {
@@ -28,7 +30,6 @@ export function queueCustom(
     .forEach(field => {
       throw new Error(`${field} is required when creating custom task`);
     });
-
   tasks.current = [
     ...tasks.current,
     {

src/containers/BackgroundTasks/types.ts

@@ -2,21 +2,28 @@ export enum Task {
   CUSTOM
 }
 export enum TaskStatus {
+  FAILURE,
   PENDING,
-  ENDED
+  SUCCESS
 }
 
+export interface OnCompletedTaskData {
+  status: TaskStatus;
+}
+export type OnCompletedTaskFn = (data: OnCompletedTaskData) => void;
+
 export interface QueuedTask {
   id: number;
-  handle: () => Promise<boolean>;
+  handle: () => Promise<TaskStatus>;
   status: TaskStatus;
-  onCompleted: () => void;
+  onCompleted: OnCompletedTaskFn;
   onError: (error: Error) => void;
 }
 
 export interface TaskData {
-  handle?: () => Promise<boolean>;
-  onCompleted?: () => void;
+  id?: string;
+  handle?: () => Promise<TaskStatus>;
+  onCompleted?: OnCompletedTaskFn;
   onError?: () => void;
 }