Merge pull request #2174 from MetRonnie/aotf

AOTF: handle alternate mutation response formats

Author: oliver-sanders

changes.d/2174.fix.md

@@ -0,0 +1 @@
+Fixed spurious error when running a command on multiple workflows.

src/components/core/Alert.vue

@@ -27,40 +27,35 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
   >
     <template v-slot:actions>
       <v-btn
-        icon
+        :icon="mdiClose"
         @click="closeAlert"
         data-cy="snack-close"
-      >
-        <v-icon>{{ $options.icons.mdiClose }}</v-icon>
-      </v-btn>
+      />
     </template>
-    {{ alert.text }}
+    <p>
+      {{ alert.text }}
+    </p>
+    <p
+      v-if="alert.detail"
+      class="mt-2 opacity-80"
+    >
+      {{ alert.detail }}
+    </p>
   </v-snackbar>
 </template>
 
-<script>
+<script setup>
+import { computed } from 'vue'
+import { useStore } from 'vuex'
 import { mdiClose } from '@mdi/js'
-import { mapActions, mapState } from 'vuex'
 
-export default {
-  name: 'Alert',
+const store = useStore()
+const alert = computed(() => store.state.alert)
 
-  computed: {
-    ...mapState(['alert'])
-  },
-
-  methods: {
-    ...mapActions(['setAlert']),
-    /**
-     * Dismisses the alert from the UI, also removing it from the Vuex store.
-     */
-    closeAlert () {
-      this.setAlert(null)
-    }
-  },
-
-  icons: {
-    mdiClose
-  }
+/**
+ * Dismisses the alert from the UI, also removing it from the Vuex store.
+ */
+function closeAlert () {
+  store.dispatch('setAlert', null)
 }
 </script>

src/model/Alert.model.js

@@ -21,10 +21,12 @@ export class Alert {
    * @param {Error|string} err - original thrown error to log if possible, or an error message.
    * @param {string} color - color of the displayed alert.
    * @param {?string} msg - a custom message to display in the alert instead of err.
+   * @param {?string} detail - more details to show.
    */
-  constructor (err, color, msg = null) {
+  constructor (err, color, msg = null, detail = null) {
     this.err = err
     this.text = msg || err
     this.color = color
+    this.detail = detail
   }
 }

src/utils/aotf.js

@@ -52,6 +52,7 @@ import { Alert } from '@/model/Alert.model'
 import { store } from '@/store/index'
 import { Tokens } from '@/utils/uid'
 import { WorkflowState, WorkflowStateNames } from '@/model/WorkflowState.model'
+import { isBoolean } from 'lodash-es'
 
 /** @typedef {import('@apollo/client').ApolloClient} ApolloClient */
 /** @typedef {import('graphql').IntrospectionInputType} IntrospectionInputType  */
@@ -911,10 +912,16 @@ async function _mutateError (mutationName, err, response) {
     console.error('mutation response', response)
   }
 
+  let detail = ''
+  for (const e of err.cause?.result?.errors ?? []) {
+    console.error(e)
+    detail += `${e.message}\n`
+  }
+
   // open a user alert
   await store.dispatch(
     'setAlert',
-    new Alert(err, 'error', `Command failed: ${mutationName} - ${err}`)
+    new Alert(err, 'error', `Command failed: ${mutationName} - ${err}`, detail)
   )
 
   // format a response
@@ -936,14 +943,10 @@ async function _mutateError (mutationName, err, response) {
  */
 export async function mutate (mutation, variables, apolloClient, cylcID) {
   const mutationStr = constructMutation(mutation)
-  let response = null
   // eslint-disable-next-line no-console
-  console.debug([
-    `mutation(${mutation.name})`,
-    mutationStr,
-    variables
-  ])
+  console.debug(mutationStr, variables)
 
+  let response
   try {
     // call the mutation
     response = await apolloClient.mutate({
@@ -961,23 +964,36 @@ export async function mutate (mutation, variables, apolloClient, cylcID) {
   }
 
   try {
-    const { result } = response.data[mutation.name]
-    if (Array.isArray(result) && result.length === 2) {
-      // regular [commandSucceeded, message] format
-      if (result[0] === true) {
-        // success
-        return _mutateSuccess(result[1])
-      }
-      // failure (Cylc error, e.g. could not find workflow <x>)
-      return _mutateError(mutation.name, result[1], response)
-    }
-    // command in a different format (e.g. info command)
-    return _mutateSuccess(result)
+    return handleMutationResponse(mutation, response)
   } catch (error) {
     return _mutateError(mutation.name, 'invalid response', response)
   }
 }
 
+export function handleMutationResponse (mutation, response) {
+  const { result } = response.data[mutation.name]
+  if (Array.isArray(result)) {
+    if (result.length === 2 && isBoolean(result[0])) {
+      // Expected response format
+      const [success, msg] = result
+      if (success) return _mutateSuccess(msg)
+      // failure (Cylc error, e.g. could not find workflow <x>)
+      return _mutateError(mutation.name, msg, response)
+    }
+    // Handle possible nested response formats (https://github.com/cylc/cylc-ui/issues/1851):
+    const results = result.map(
+      (r) => r[mutation.name]?.result?.[0]?.response ?? r.response
+    )
+    for (const [success, msg] of results) {
+      if (!success) return _mutateError(mutation.name, msg, response)
+    }
+    return _mutateSuccess('Command(s) submitted')
+  }
+  console.warn('Unexpected format for mutation response:', result)
+  // But assume success
+  return _mutateSuccess(result)
+}
+
 /**
  * Send a GraphQL query.
  *

tests/unit/services/workflow.service.spec.js

@@ -30,6 +30,15 @@ import WorkflowService from '@/services/workflow.service'
 import ViewState from '@/model/ViewState.model'
 import { TreeCallback, WorkflowCallback } from './testCallback'
 
+vi.mock('@/graphql/index', () => ({
+  createApolloClient: () => ({
+    query: vi.fn(),
+    subscribe: () => ({
+      subscribe: vi.fn()
+    }),
+  })
+}))
+
 const sandbox = sinon.createSandbox()
 
 describe('WorkflowService', () => {
@@ -63,14 +72,6 @@ describe('WorkflowService', () => {
   let subscription
   beforeEach(() => {
     sandbox.stub(console, 'debug')
-    vi.mock('@/graphql/index', () => ({
-      createApolloClient: () => ({
-        query: vi.fn(),
-        subscribe: () => ({
-          subscribe: vi.fn()
-        }),
-      })
-    }))
     subscriptionClient = null
     // TODO: really load some mutations
     sandbox.stub(WorkflowService.prototype, 'loadTypes').returns(

tests/unit/utils/aotf.spec.js

@@ -1,4 +1,4 @@
-/**
+/*
  * Copyright (C) NIWA & British Crown (Met Office) & Contributors.
  *
  * This program is free software: you can redistribute it and/or modify
@@ -15,10 +15,12 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+import { afterAll, beforeAll } from 'vitest'
 import * as aotf from '@/utils/aotf'
 import dedent from 'dedent'
 // need the polyfill as otherwise ApolloClient fails to be imported as it checks for a global fetch object on import...
 import 'cross-fetch/polyfill'
+import sinon from 'sinon'
 
 describe('aotf (Api On The Fly)', () => {
   describe('tokenise', () => {
@@ -751,4 +753,139 @@ describe('aotf (Api On The Fly)', () => {
       ])
     })
   })
+
+  describe('handleMutationResponse()', () => {
+    const mutation = { name: 'breach' }
+    beforeAll(() => {
+      sinon.stub(console, 'error')
+      sinon.stub(console, 'warn')
+    })
+    afterAll(() => {
+      sinon.restore()
+    })
+
+    it.each([
+      {
+        testID: 'original interface',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [true, 'arasaka']
+            }
+          }
+        },
+        expected: {
+          status: 'SUCCEEDED', message: 'arasaka'
+        }
+      },
+
+      {
+        testID: 'original interface with failure',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [false, 'relic malfunction']
+            }
+          }
+        },
+        expected: {
+          status: 'FAILED', message: 'relic malfunction'
+        }
+      },
+
+      {
+        testID: 'nested',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [
+                {
+                  [mutation.name]: {
+                    result: [{ id: 'wflow1', response: [true, 'arasaka'] }]
+                  }
+                },
+                {
+                  [mutation.name]: {
+                    result: [{ id: 'wflow2', response: [true, 'kiroshi'] }]
+                  }
+                }
+              ]
+            }
+          }
+        },
+        expected: {
+          status: 'SUCCEEDED', message: 'Command(s) submitted'
+        }
+      },
+
+      {
+        testID: 'nested with failure',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [
+                {
+                  [mutation.name]: {
+                    result: [{ id: 'wflow1', response: [true, 'arasaka'] }]
+                  }
+                },
+                {
+                  [mutation.name]: {
+                    result: [{ id: 'wflow2', response: [false, 'relic malfunction'] }]
+                  }
+                }
+              ]
+            }
+          }
+        },
+        expected: {
+          status: 'FAILED', message: 'relic malfunction'
+        }
+      },
+
+      {
+        testID: 'other format',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [{ response: [true, 'arasaka'] }]
+            }
+          }
+        },
+        expected: {
+          status: 'SUCCEEDED', message: 'Command(s) submitted'
+        }
+      },
+
+      {
+        testID: 'other format with failure',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: [{ response: [false, 'relic malfunction'] }]
+            }
+          }
+        },
+        expected: {
+          status: 'FAILED', message: 'relic malfunction'
+        }
+      },
+
+      {
+        testID: 'unexpected format - assume success',
+        response: {
+          data: {
+            [mutation.name]: {
+              result: 2077,
+            }
+          }
+        },
+        expected: {
+          status: 'SUCCEEDED', message: 2077
+        }
+      }
+    ])('handles response - $testID', async ({ response, expected, testID }) => {
+      expect(await aotf.handleMutationResponse(mutation, response)).toStrictEqual(expected)
+    })
+  })
 })