Merge pull request #17048 from mvdbeek/prevent_workflow_submission_mi…

…ssing_input_value

[23.1] Prevent workflow submission with missing input values

client/src/components/Common/ButtonSpinner.vue

@@ -14,6 +14,7 @@
         variant="primary"
         class="d-flex flex-nowrap align-items-center text-nowrap"
         :title="tooltip"
+        :disabled="disabled"
         @click="$emit('onClick')">
         <font-awesome-icon icon="play" class="mr-2" />{{ title }}
     </b-button>
@@ -43,6 +44,10 @@ export default {
             type: String,
             default: null,
         },
+        disabled: {
+            type: Boolean,
+            default: false,
+        },
     },
 };
 </script>

client/src/components/Form/FormDisplay.vue

@@ -79,6 +79,10 @@ export default {
             type: Boolean,
             default: false,
         },
+        allowEmptyValueOnRequiredInput: {
+            type: Boolean,
+            default: false,
+        },
     },
     data() {
         return {
@@ -89,7 +93,7 @@ export default {
     },
     computed: {
         validation() {
-            return validateInputs(this.formIndex, this.formData);
+            return validateInputs(this.formIndex, this.formData, this.allowEmptyValueOnRequiredInput);
         },
     },
     watch: {

client/src/components/Form/utilities.js

@@ -104,7 +104,7 @@ export function matchInputs(index, response) {
  * @param{dict}   index     - Index of input elements
  * @param{dict}   values    - Dictionary of parameter values
  */
-export function validateInputs(index, values) {
+export function validateInputs(index, values, allowEmptyValueOnRequiredInput = false) {
     let batchN = -1;
     let batchSrc = null;
     for (const inputId in values) {
@@ -113,8 +113,10 @@ export function validateInputs(index, values) {
         if (!inputDef || inputDef.step_linked) {
             continue;
         }
-        if (inputValue == null && !inputDef.optional && inputDef.type != "hidden") {
-            return [inputId, "Please provide a value for this option."];
+        if (!inputDef.optional && inputDef.type != "hidden") {
+            if (inputValue == null || (allowEmptyValueOnRequiredInput && inputValue === "")) {
+                return [inputId, "Please provide a value for this option."];
+            }
         }
         if (inputDef.wp_linked && inputDef.text_value == inputValue) {
             return [inputId, "Please provide a value for this workflow parameter."];

client/src/components/Workflow/Run/WorkflowRunFormSimple.vue

@@ -1,68 +1,74 @@
 <template>
-    <ConfigProvider v-slot="{ config }">
-        <div>
-            <div class="h4 clearfix mb-3">
-                <b>Workflow: {{ model.name }}</b>
-                <ButtonSpinner id="run-workflow" class="float-right" title="Run Workflow" @onClick="onExecute" />
-                <b-dropdown
-                    v-if="showRuntimeSettings(currentUser)"
-                    id="dropdown-form"
-                    ref="dropdown"
-                    class="workflow-run-settings float-right"
-                    style="margin-right: 10px"
-                    title="Workflow Run Settings"
-                    no-caret>
-                    <template v-slot:button-content>
-                        <span class="fa fa-cog" />
-                    </template>
-                    <b-dropdown-form>
-                        <b-form-checkbox v-model="sendToNewHistory" class="workflow-run-settings-target"
-                            >Send results to a new history</b-form-checkbox
-                        >
-                        <b-form-checkbox
-                            v-if="reuseAllowed(currentUser)"
-                            v-model="useCachedJobs"
-                            title="This may skip executing jobs that you have already run."
-                            >Attempt to re-use jobs with identical parameters?</b-form-checkbox
-                        >
-                        <b-form-checkbox v-if="config.object_store_allows_id_selection" v-model="splitObjectStore"
-                            >Send outputs and intermediate to different object stores?</b-form-checkbox
-                        >
-                        <WorkflowStorageConfiguration
-                            v-if="config.object_store_allows_id_selection"
-                            :split-object-store="splitObjectStore"
-                            :invocation-preferred-object-store-id="preferredObjectStoreId"
-                            :invocation-intermediate-preferred-object-store-id="preferredIntermediateObjectStoreId"
-                            @updated="onStorageUpdate">
-                        </WorkflowStorageConfiguration>
-                    </b-dropdown-form>
-                </b-dropdown>
-            </div>
-            <FormDisplay :inputs="formInputs" @onChange="onChange" />
-            <!-- Options to default one way or the other, disable if admins want, etc.. -->
-            <a href="#" class="workflow-expand-form-link" @click="$emit('showAdvanced')"
-                >Expand to full workflow form.</a
-            >
+    <div>
+        <div v-if="configIsLoaded" class="h4 clearfix mb-3">
+            <b>Workflow: {{ model.name }}</b>
+            <ButtonSpinner
+                id="run-workflow"
+                :wait="waitingForRequest"
+                :disabled="hasValidationErrors"
+                class="float-right"
+                title="Run Workflow"
+                @onClick="onExecute" />
+            <b-dropdown
+                v-if="showRuntimeSettings(currentUser)"
+                id="dropdown-form"
+                ref="dropdown"
+                class="workflow-run-settings float-right"
+                style="margin-right: 10px"
+                title="Workflow Run Settings"
+                no-caret>
+                <template v-slot:button-content>
+                    <span class="fa fa-cog" />
+                </template>
+                <b-dropdown-form>
+                    <b-form-checkbox v-model="sendToNewHistory" class="workflow-run-settings-target"
+                        >Send results to a new history</b-form-checkbox
+                    >
+                    <b-form-checkbox
+                        v-if="reuseAllowed(currentUser)"
+                        v-model="useCachedJobs"
+                        title="This may skip executing jobs that you have already run."
+                        >Attempt to re-use jobs with identical parameters?</b-form-checkbox
+                    >
+                    <b-form-checkbox v-if="config.object_store_allows_id_selection" v-model="splitObjectStore"
+                        >Send outputs and intermediate to different object stores?</b-form-checkbox
+                    >
+                    <WorkflowStorageConfiguration
+                        v-if="config.object_store_allows_id_selection"
+                        :split-object-store="splitObjectStore"
+                        :invocation-preferred-object-store-id="preferredObjectStoreId"
+                        :invocation-intermediate-preferred-object-store-id="preferredIntermediateObjectStoreId"
+                        @updated="onStorageUpdate">
+                    </WorkflowStorageConfiguration>
+                </b-dropdown-form>
+            </b-dropdown>
         </div>
-    </ConfigProvider>
+        <FormDisplay
+            :inputs="formInputs"
+            :allowEmptyValueOnRequiredInput="true"
+            @onChange="onChange"
+            @onValidation="onValidation" />
+        <!-- Options to default one way or the other, disable if admins want, etc.. -->
+        <a href="#" class="workflow-expand-form-link" @click="$emit('showAdvanced')">Expand to full workflow form.</a>
+    </div>
 </template>
 
 <script>
-import ConfigProvider from "components/providers/ConfigProvider";
-import { mapState } from "pinia";
+import { useConfig } from "@/composables/config";
 import { useUserStore } from "@/stores/userStore";
+import { storeToRefs } from "pinia";
 import FormDisplay from "components/Form/FormDisplay";
 import ButtonSpinner from "components/Common/ButtonSpinner";
 import { invokeWorkflow } from "./services";
 import { isWorkflowInput } from "components/Workflow/constants";
 import { errorMessageAsString } from "utils/simple-error";
 import { allowCachedJobs } from "components/Tool/utilities";
 import WorkflowStorageConfiguration from "./WorkflowStorageConfiguration";
+import Vue from "vue";
 
 export default {
     components: {
         ButtonSpinner,
-        ConfigProvider,
         FormDisplay,
         WorkflowStorageConfiguration,
     },
@@ -80,20 +86,26 @@ export default {
             default: false,
         },
     },
+    setup() {
+        const { config, isLoaded: configIsLoaded } = useConfig(true);
+        const { currentUser } = storeToRefs(useUserStore());
+        return { config, configIsLoaded, currentUser };
+    },
     data() {
         const newHistory = this.targetHistory == "new" || this.targetHistory == "prefer_new";
         return {
             formData: {},
             inputTypes: {},
+            stepValidations: {},
             sendToNewHistory: newHistory,
             useCachedJobs: this.useJobCache, // TODO:
             splitObjectStore: false,
             preferredObjectStoreId: null,
             preferredIntermediateObjectStoreId: null,
+            waitingForRequest: false,
         };
     },
     computed: {
-        ...mapState(useUserStore, ["currentUser"]),
         formInputs() {
             const inputs = [];
             // Add workflow parameters.
@@ -121,13 +133,23 @@ export default {
             });
             return inputs;
         },
+        hasValidationErrors() {
+            return Boolean(Object.values(this.stepValidations).find((value) => value !== null && value !== undefined));
+        },
     },
     methods: {
+        onValidation(validation) {
+            if (validation) {
+                Vue.set(this.stepValidations, validation[0], validation[1]);
+            } else {
+                this.stepValidations = {};
+            }
+        },
         reuseAllowed(user) {
-            return allowCachedJobs(user.preferences);
+            return user && allowCachedJobs(user.preferences);
         },
         showRuntimeSettings(user) {
-            return this.targetHistory.indexOf("prefer") >= 0 || this.reuseAllowed(user);
+            return this.targetHistory.indexOf("prefer") >= 0 || (user && this.reuseAllowed(user));
         },
         onChange(data) {
             this.formData = data;
@@ -176,13 +198,15 @@ export default {
                     data.preferred_object_store_id = this.preferredObjectStoreId;
                 }
             }
+            this.waitingForRequest = true;
             invokeWorkflow(this.model.workflowId, data)
                 .then((invocations) => {
                     this.$emit("submissionSuccess", invocations);
                 })
                 .catch((error) => {
                     this.$emit("submissionError", errorMessageAsString(error));
-                });
+                })
+                .finally(() => (this.waitingForRequest = false));
         },
     },
 };

client/src/utils/navigation/navigation.yml

@@ -650,12 +650,16 @@ workflow_run:
     # TODO: put step labels in the DOM ideally
     subworkflow_step_icon: ".portlet-title-icon.fa-sitemap"
     run_workflow: "#run-workflow"
+    run_workflow_disabled: "#run-workflow.disabled"
     validation_error: ".validation-error"
     expand_form_link: '.workflow-expand-form-link'
     expanded_form: '.workflow-expanded-form'
     new_history_target_link: '.workflow-new-history-target-link'
     runtime_setting_button: '.workflow-run-settings'
     runtime_setting_target: '.workflow-run-settings-target'
+    simplified_input:
+      type: xpath
+      selector: //div[@data-label="${label}"]//input
     input_select_field:
       type: xpath
       selector: '//div[@data-label="${label}"]//div[contains(@class, "multiselect")]'

lib/galaxy/workflow/run_request.py

@@ -125,8 +125,9 @@ def _normalize_inputs(
         # but asserting 'optional' is definitely a bool and not a String->Bool or something is a good
         # start to ensure tool state is being preserved and loaded in a type safe way.
         assert isinstance(optional, bool)
-        if not inputs_key and default_value is None and not optional:
-            message = f"Workflow cannot be run because an expected input step '{step.id}' ({step.label}) is not optional and no input."
+        has_input_value = inputs_key and inputs[inputs_key] is not None
+        if not has_input_value and default_value is None and not optional:
+            message = f"Workflow cannot be run because input step '{step.id}' ({step.label}) is not optional and no input provided."
             raise exceptions.MessageException(message)
         if inputs_key:
             normalized_inputs[step.id] = inputs[inputs_key]

lib/galaxy_test/selenium/test_workflow_run.py

@@ -277,6 +277,37 @@ def test_execution_with_null_optional_select_from_data(self):
         )
         self.workflow_populator.wait_for_history_workflows(history_id, expected_invocation_count=1)
 
+    @selenium_test
+    @managed_history
+    def test_workflow_run_button_disabled_when_required_input_missing(self):
+        self.workflow_run_open_workflow(
+            """
+class: GalaxyWorkflow
+inputs:
+  text_param:
+    type: text
+    optional: false
+  data_param:
+    type: data
+  collection_param:
+    type: data_collection
+steps: {}
+"""
+        )
+        workflow_run = self.components.workflow_run
+        # None of the required parameters are present
+        workflow_run.run_workflow_disabled.wait_for_present()
+        # upload datasets and collections
+        history_id = self.current_history_id()
+        self.dataset_populator.new_dataset(history_id, wait=True)
+        self.dataset_collection_populator.create_list_in_history(history_id, wait=True)
+        input_element = workflow_run.simplified_input(label="text_param").wait_for_and_click()
+        input_element.send_keys("3")
+        # Everything is set, workflow is runnable, disabled class should be removed
+        workflow_run.run_workflow_disabled.wait_for_absent()
+        input_element.clear()
+        workflow_run.run_workflow_disabled.wait_for_present()
+
     def _assert_has_3_lines_after_run(self, hid):
         self.workflow_run_wait_for_ok(hid=hid)
         history_id = self.current_history_id()