feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766

sairam91 · 2024-10-15T23:22:34Z

Motivation

This PR adds support for developers to prefil submit workflow form by passing in the values through query parameters.

To prefil a submit workflow developers can pass in the following query parameters

sidePanel=submit-new-workflow
template=<template that the workflow should follow>
parameters[key]=value // Here the key would be the parameter for the job and value would be the value of the parameter.
You should provide the keys that are supported by the specified template

Modifications

Updated workflow-list component
Updated submit-workflow-panel.tsx component to default entry point if provided.
Updated workflow-creator.tsx to read default values from query parameters.
The workflow will be prefilled with the parameters that the template supports
Added new shared helper function to parse query parameter of the format ?parameters[key]=value.

Verification

…1214 Signed-off-by: Sairam Arunachalam <[email protected]>

sairam91 · 2024-10-15T23:22:58Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

+                isShown={!!getSidePanel()}
+                onClose={() => {
+                    // Remove any lingering query params
+                    const qParams: {[key: string]: string | null} = {


This is duplicate can be removed.

Could you explain why this is necessary? The navigation.goto() function already has logic to automatically strip out the query parameters if there's been a path change: https://github.com/argoproj/argo-ui/blob/54f36c723d9a0e5d3386689298cbb9c27767fa00/src/components/navigation.ts#L20-L21

If there hasn't been a path change, I think it's fine to leave these query parameters in.

So the workflow this would fix is

Open the submit page prefilled through the URL

Close the submit page

Click on Submit again,

In this case, if the parameters are preserved, we would open the prefilled page. Removing the parameters will ensure the developer can start the page fresh without pre filled data.

Okay, thanks. IMO, it'd be better to remove this, because if a developer wants to start fresh, they can click on the "Workflows" icon on the left bar (or navigate to another page and back again)

Okay, I tried that workflow and the developer would need to go back to the workflows tab and re-trigger a workflow.

Most likely the developer who lands on this page through query parameters would try to Submit it.

Removing the extra added query parameters in this PR, will make the developer experience much better as they can recover quickly.

Okay, that makes sense. I checked and this is the only page using a<SlidingPanel> that calls navigation.goto() on close. Every other page just sets a parameter on close, e.g.

argo-workflows/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx

Line 145 in e09d2f1

<SlidingPanel isShown={!!sidePanel} onClose={() => setSidePanel(null)} isMiddle={true}>

I'm not sure why this page is special in that respect, but it could be a developer experience thing

ui/src/app/shared/get_workflow_params.ts

MasonM · 2024-10-16T01:32:46Z

Good start! A couple initial comments:

This project uses the history library, so I think (but I'm not sure) the useEffect() hooks need to incorporate it as a dependency. That means passing through the history object to the affected components. Ideally, we'd use useQueryParams() too, but when I tried that, the history.listen() callback wouldn't fire. I think that's due to the interdependency with the workflowTemplates/workflowParameters props.
It'd be simpler to push the logic for handling workflow parameters to ui/src/app/workflows/components/submit-workflow-panel.tsx

Here's a commit that does both things: MasonM@d2c11c8

agilgur5

[...] Ideally, we'd use useQueryParams() too, but when I tried that, the history.listen() callback wouldn't fire. I think that's due to the interdependency with the workflowTemplates/workflowParameters props

In this case, I don't think it's necessary, because the URL should not change after the page loads, since the purpose is deeplinking. history.listen() is to make sure the state and query params match.
It could still be used, but may be unnecessary complexity in this case.

It'd be simpler to push the logic for handling workflow parameters to ui/src/app/workflows/components/submit-workflow-panel.tsx

Agree, that was what I thought on first glance of this yesterday too, but hadn't commented yet since it's still draft and wanted to see if you or Sairam would pick that up in iteration 🙂

Signed-off-by: Sairam Arunachalam <[email protected]>

ui/src/app/shared/get_workflow_params.ts

MasonM · 2024-10-16T19:28:09Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

+                isShown={!!getSidePanel()}
+                onClose={() => {
+                    // Remove any lingering query params
+                    const qParams: {[key: string]: string | null} = {


Could you explain why this is necessary? The navigation.goto() function already has logic to automatically strip out the query parameters if there's been a path change: https://github.com/argoproj/argo-ui/blob/54f36c723d9a0e5d3386689298cbb9c27767fa00/src/components/navigation.ts#L20-L21

If there hasn't been a path change, I think it's fine to leave these query parameters in.

ui/src/app/workflows/components/submit-workflow-panel.tsx

ui/src/app/shared/get_workflow_params.ts

MasonM · 2024-10-16T19:44:28Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

+
+        // Add the sidePanel query parameter if it exists
+        params.append('sidePanel', getSidePanel());
+        params.append('template', queryParams.get('template'));


I'm not sure why this component is taking an allow-list approach to query parameters, but you could get rid of this extra code by changing const params = new URLSearchParams() to const params = new URLSearchParams(history.location.search);

I'm not sure what side-effects that'll have, though.

We could preserve all the parameters. We might have to try different scenarios to check for side-effects.

Signed-off-by: Sairam Arunachalam <[email protected]>

MasonM · 2024-10-17T00:28:49Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

@@ -126,7 +126,7 @@ export function WorkflowsList({match, location, history}: RouteComponentProps<an
        }
        storage.setItem('options', options, {} as WorkflowListRenderOptions);

-        const params = new URLSearchParams();
+        const params = new URLSearchParams(history.location.search);


I found one side-effect of this change: if you visit http://localhost:8080/workflows?namespace=argo and refresh the page, it'll append &namespace=argo each time you refresh. That's happening because historyUrl() isn't deduplicating query parameters, which it should IMO. Here's a commit that fixes that: MasonM@8b48d5b

I haven't extensively tested that, so it's possible there's other side-effects

Signed-off-by: Sairam Arunachalam <[email protected]>

MasonM · 2024-10-17T18:26:52Z

ui/src/app/shared/get_workflow_params.test.ts

+        const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2'});
+        history.location.search = path;


This test is failing because window.location.search should only contain the query string, not the path:

Suggested change

const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2'});

history.location.search = path;

history.location.search = '?parameters[key1]=value1&parameters[key2]=value2';

Also, if you haven't already, you should install the Jest extension in the DevContainer so you can debug the tests and set breakpoints:

MasonM · 2024-10-17T18:27:25Z

ui/src/app/shared/get_workflow_params.test.ts

+        const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2&test=123'});
+        history.location.search = path;


Same as above:

Suggested change

const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2&test=123'});

history.location.search = path;

history.location.search = '?parameters[key1]=value1&parameters[key2]=value2&test=123';

Signed-off-by: Sairam Arunachalam <[email protected]>

fix(ui): prefill parameters for workflow submit form. Fixes argoproj#…

fc1c45d

…1214 Signed-off-by: Sairam Arunachalam <[email protected]>

sairam91 commented Oct 15, 2024

View reviewed changes

ui/src/app/shared/get_workflow_params.ts Outdated Show resolved Hide resolved

agilgur5 changed the title ~~fix(ui): prefill parameters for workflow submit form. Fixes #1214~~ feat(ui): prefill parameters for workflow submit form. Fixes #12124 Oct 16, 2024

agilgur5 reviewed Oct 16, 2024

View reviewed changes

fix(ui): refactor code. Fixes argoproj#1214

e8ec0aa

Signed-off-by: Sairam Arunachalam <[email protected]>

MasonM reviewed Oct 16, 2024

View reviewed changes

fix(ui): refactor code. Fixes argoproj#1214

46c3503

Signed-off-by: Sairam Arunachalam <[email protected]>

MasonM reviewed Oct 17, 2024

View reviewed changes

sairam91 added 2 commits October 17, 2024 09:36

feat(ui): fix query parameter issues. Fixes argoproj#1214

c254b7f

Signed-off-by: Sairam Arunachalam <[email protected]>

Merge branch 'main' into issue-12124

2340b21

Signed-off-by: Sairam Arunachalam <[email protected]>

MasonM reviewed Oct 17, 2024

View reviewed changes

feat(ui): fix tests. Fixes argoproj#1214

d34ee68

Signed-off-by: Sairam Arunachalam <[email protected]>

sairam91 marked this pull request as ready for review October 18, 2024 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766

feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766

sairam91 commented Oct 15, 2024 •

edited by agilgur5

Loading

sairam91 Oct 15, 2024

MasonM Oct 16, 2024

sairam91 Oct 16, 2024

MasonM Oct 17, 2024

sairam91 Oct 17, 2024

MasonM Oct 19, 2024

MasonM commented Oct 16, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading

MasonM Oct 16, 2024

MasonM Oct 16, 2024

sairam91 Oct 16, 2024

MasonM Oct 17, 2024

MasonM Oct 17, 2024

MasonM Oct 17, 2024

		const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2'});
		history.location.search = path;

	const path = createPath({pathname: '/workflows', search: '?parameters[key1]=value1&parameters[key2]=value2'});
	history.location.search = path;
	history.location.search = '?parameters[key1]=value1&parameters[key2]=value2';

feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766

Are you sure you want to change the base?

feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766

Conversation

sairam91 commented Oct 15, 2024 • edited by agilgur5 Loading

Motivation

Modifications

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MasonM commented Oct 16, 2024 • edited Loading

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sairam91 commented Oct 15, 2024 •

edited by agilgur5

Loading

MasonM commented Oct 16, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading